Implement Structure.dump_cmd/3

[maennchen]

See https://github.com/elixir-ecto/ecto_sql/pull/423

• [x] TODO: Change mix.exs back to released versions.

[maennchen]

I think the CI failure is not caused by this change.

[warmwaffles]

Thank you @maennchen I saw the PR for the other adapters. I'll get this rectified and this should be good to go.

[warmwaffles]

@maennchen try updating / rebasing this branch off of main. This was recently merged https://github.com/elixir-sqlite/ecto_sqlite3/pull/80

[greg-rychlewski]

@warmwaffles There is another issue. An insert_all integration test was added that uses select statements for values and it doesn't seem like your adapter supports that. The insert_select tag was missed but it's been fixed and merged into Ecto: https://github.com/elixir-ecto/ecto/pull/3942.

[greg-rychlewski]

There are two other failures:

• Stacktraces were added to logging and the test didn't take into account that adapters like yours will have their integration tests nested under the deps directory. This has been fixed and merged https://github.com/elixir-ecto/ecto_sql/pull/426.
• There is a failure for a new json test that was added after your version of Ecto: https://github.com/elixir-ecto/ecto/blame/master/integration_test/cases/type.exs#L321. What's happening is the values inside the meta map are not typed and when the driver returns 1 for meta["enabled"], your loaders don't detect that this is actually a boolean field and needs to be casted to true.

For the second issue, the only thing that comes to mind is to document that users should explicitly set the type in these situations:

TestRepo.one(from o in Order, select: type(o.meta["enabled"], :boolean))

instead of

TestRepo.one(from o in Order, select: o.meta["enabled"])

and then I guess there are two options for the tests:

1. You skip all the json_extract_path tests and implement custom versions of them for your adapter
2. Ecto has 2 versions of these tests, one of them using type/2 and the other one not. And the one that doesn't has a tag for you to skip.

Probably number 1 is better, because this is a super specific thing for sqlite? But let me know if you have a different idea.

[maennchen]

@warmwaffles I've rebased onto main. There's however still a few problems left as @greg-rychlewski noticed.

[warmwaffles]

Probably number 1 is better, because this is a super specific thing for sqlite? But let me know if you have a different idea.

This is probably a better idea to do.

It's probably okay to merge this into main and then follow up with tests. I am unsure why this PR doesn't have github actions running for it.

[warmwaffles]

I haven't forgotten about this, just haven't had time to dig in more.

[greg-rychlewski]

I think the actions are only running the first time the PR is opened because of this

pull_request:
    types:
      - opened

I think you need the synchronize activity for it to trigger on new commits.

[warmwaffles]

@maennchen would you mind rebasing off of the latest main branch? I want to see if the workflow change I made gets picked up and kicks off.

[maennchen]

@warmwaffles Rebased. Tests seem to be running :+1:

[warmwaffles]

Excellent, thanks @greg-rychlewski for pointing out that I had that types section.

[greg-rychlewski]

btw failed integration tests 1 and 3 can be fixed by updating to the latest commit on ecto/ecto_sql:

https://github.com/elixir-ecto/ecto/pull/3942 https://github.com/elixir-ecto/ecto_sql/pull/426

[maennchen]

@greg-rychlewski Updated

[greg-rychlewski]

@maennchen I sent a PR to your fork to fix the broken unit tests. the planner now returns the cast and dump version of the parameters and it was causing a match error.

[greg-rychlewski]

@maennchen if you rebase on master the tests should all pass now

[maennchen]

@greg-rychlewski Still seems to error, unfortunately.

[greg-rychlewski]

@maennchen oh it looks like the PR I sent you was reverted: https://github.com/maennchen/ecto_sqlite3/pull/1/files. if that change is re-implemented it should all pass

[maennchen]

@greg-rychlewski Oops, I rebased away your commit. I've brought it back.

[maennchen]

After the update, the following test breaks:

== Compilation error in file integration_test/json_test.exs ==
** (KeyError) key :meta not found
    expanding struct: Ecto.Integration.Order.__struct__/1
    integration_test/json_test.exs:10: Ecto.Integration.JsonTest."test json_extract_path with primitive values"/1

EDIT

The error above was fixed with https://github.com/elixir-sqlite/ecto_sqlite3/pull/79/commits/3d8d9cacdd506a3aae879b5ba181aa0f95b0a9f1

[maennchen]

After the fix 4 tests still fail with similar errors:

  * test query select selected_as/2 with having (3.9ms) [L#1608]

  1) test query select selected_as/2 with having (Ecto.Integration.RepoTest)
     deps/ecto/integration_test/cases/repo.exs:1608
     ** (Ecto.QueryError) unsupported expression :min_visits in query:
     
     from p0 in Ecto.Integration.Post,
       group_by: [p0.posted],
       having: selected_as(:min_visits) > 0,
       or_having: not (selected_as(:min_visits) > 0),
       order_by: [asc: p0.posted],
       select: %{posted: p0.posted, min_visits: selected_as(min(coalesce(p0.visits, 0)), :min_visits)}
     
     code: |> TestRepo.all()
     stacktrace:
       (ecto_sqlite3 0.8.1) lib/ecto/adapters/sqlite3/connection.ex:1403: Ecto.Adapters.SQLite3.Connection.expr/3
       (ecto_sqlite3 0.8.1) lib/ecto/adapters/sqlite3/connection.ex:1806: Ecto.Adapters.SQLite3.Connection.intersperse_map/4
       (ecto_sqlite3 0.8.1) lib/ecto/adapters/sqlite3/connection.ex:1356: Ecto.Adapters.SQLite3.Connection.expr/3
       (ecto_sqlite3 0.8.1) lib/ecto/adapters/sqlite3/connection.ex:1353: Ecto.Adapters.SQLite3.Connection.expr/3
       (ecto_sqlite3 0.8.1) lib/ecto/adapters/sqlite3/connection.ex:1172: Ecto.Adapters.SQLite3.Connection.paren_expr/3
       (ecto_sqlite3 0.8.1) lib/ecto/adapters/sqlite3/connection.ex:1143: Ecto.Adapters.SQLite3.Connection.boolean/4
       (ecto_sqlite3 0.8.1) lib/ecto/adapters/sqlite3/connection.ex:171: Ecto.Adapters.SQLite3.Connection.all/2
       (ecto_sqlite3 0.8.1) lib/ecto/adapters/sqlite3.ex:176: Ecto.Adapters.SQLite3.prepare/2
       (ecto 3.9.0) lib/ecto/query/planner.ex:182: Ecto.Query.Planner.query_without_cache/4
       (ecto 3.9.0) lib/ecto/query/planner.ex:152: Ecto.Query.Planner.query_prepare/6
       (ecto 3.9.0) lib/ecto/query/planner.ex:127: Ecto.Query.Planner.query_with_cache/8
       (ecto 3.9.0) lib/ecto/repo/queryable.ex:211: Ecto.Repo.Queryable.execute/4
       (ecto 3.9.0) lib/ecto/repo/queryable.ex:19: Ecto.Repo.Queryable.all/3
       deps/ecto/integration_test/cases/repo.exs:1624: (test)

[greg-rychlewski]

@maennchen You can add these tags to the ignore list for the integration tests:

:selected_as_with_group_by :selected_as_with_order_by :selected_as_with_order_by_expression :selected_as_with_having

[warmwaffles]

@maennchen I believe if you drop the changes made to the mix.lock the conflict should be resolved.

[maennchen]

@warmwaffles I've rebased and it seems the tests are failing again.

I've spent more time now rebasing this PR than writing it. I would welcome it if a maintainer makes sure that this PR is incorporated in a way that actually works.

[warmwaffles]

@maennchen I'll see what I can do and try to retain your original authorship. Definitely want to keep your contributions attached.

[warmwaffles]

Thanks a ton @maennchen for implementing this. It's been merged and deployed under v0.9.0