Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix SQLite3 support by checking the Ecto adapter name #443

Draft
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

igorgue
Copy link

@igorgue igorgue commented Jul 3, 2024

To be specific, it fixes the :index and :show actions that were broken with the following error:

[error] GenServer #PID<0.1375.0> terminating
** (Ecto.QueryError) DISTINCT with multiple columns is not supported by SQLite3 in query:

from m0 in Ns.Chat.Message,
  as: :message,
  left_join: u1 in Ns.Accounts.User,
  as: :user,
  on: m0.user_id == u1.id,
  where: m0.id == ^...,
  distinct: [asc: m0.id],
  select: m0,
  preload: [:user]

    (ecto_sqlite3 0.16.0) lib/ecto/adapters/sqlite3/connection.ex:851: Ecto.Adapters.SQLite3.Connection.distinct/3
    (ecto_sqlite3 0.16.0) lib/ecto/adapters/sqlite3/connection.ex:859: Ecto.Adapters.SQLite3.Connection.select/2
    (ecto_sqlite3 0.16.0) lib/ecto/adapters/sqlite3/connection.ex:178: Ecto.Adapters.SQLite3.Connection.all/2
    (ecto_sqlite3 0.16.0) lib/ecto/adapters/sqlite3.ex:187: Ecto.Adapters.SQLite3.prepare/2
    (ecto 3.11.2) lib/ecto/query/planner.ex:182: Ecto.Query.Planner.query_without_cache/4
    (ecto 3.11.2) lib/ecto/query/planner.ex:152: Ecto.Query.Planner.query_prepare/6
    (ecto 3.11.2) lib/ecto/query/planner.ex:127: Ecto.Query.Planner.query_with_cache/8
    (ecto 3.11.2) lib/ecto/repo/queryable.ex:214: Ecto.Repo.Queryable.execute/4
    (ecto 3.11.2) lib/ecto/repo/queryable.ex:19: Ecto.Repo.Queryable.all/3
    (ecto 3.11.2) lib/ecto/repo/queryable.ex:162: Ecto.Repo.Queryable.one!/3
    (ns 0.1.0) lib/ns_web/live/admin/message_live.ex:5: NsWeb.Live.Admin.MessageLive.apply_action/2
    (ns 0.1.0) lib/ns_web/live/admin/message_live.ex:5: NsWeb.Live.Admin.MessageLive.handle_params/3
    (phoenix_live_view 1.0.0-rc.6) lib/phoenix_live_view/utils.ex:451: anonymous fn/5 in Phoenix.LiveView.Utils.call_handle_params!/5
    (telemetry 1.2.1) /home/igor/Code/nerdystreamer/deps/telemetry/src/telemetry.erl:321: :telemetry.span/3
    (phoenix_live_view 1.0.0-rc.6) lib/phoenix_live_view/channel.ex:909: Phoenix.LiveView.Channel.sync_handle_params_with_live_redirect/5
    (stdlib 6.0) gen_server.erl:2173: :gen_server.try_handle_info/3
    (stdlib 6.0) gen_server.erl:2261: :gen_server.handle_msg/6
    (stdlib 6.0) proc_lib.erl:329: :proc_lib.init_p_do_apply/3

Notes

  • Calls to_string to not depend on the module in the project, this is why I need to use the fully qualified name as well.
  • I'm unsure if MySQL needs similar handling.
  • Actually, I'm not sure if this is the most elegant way to handle this either.
  • Since this is the only distinct in the project does this fix all SQLite3 issues?

…g to not depend on the module in the project
@igorgue
Copy link
Author

igorgue commented Jul 3, 2024

Another thing I was thinking over my lunch break was that maybe we just want to do order_by instead?

@Flo0807
Copy link
Collaborator

Flo0807 commented Jul 12, 2024

If distinct is the only issue, I think we should try to remove the distinct altogether. I don't know if we even need it here 🤔 @igorgue

@igorgue
Copy link
Author

igorgue commented Jul 12, 2024

@Flo0807 I also fixed search locally, the problem is that SQLite doesn't support ilike, so it's a bit more work. Or maybe we could pass an option to make it call like instead of ilike

@Flo0807
Copy link
Collaborator

Flo0807 commented Jul 17, 2024

@Flo0807 I also fixed search locally, the problem is that SQLite doesn't support ilike, so it's a bit more work. Or maybe we could pass an option to make it call like instead of ilike

I would opt for more work rather than changing the behavior based on the database.

@igorgue
Copy link
Author

igorgue commented Jul 17, 2024

@Flo0807 I agree I'll update the PR with a better proposal

@Flo0807 Flo0807 marked this pull request as draft September 3, 2024 17:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants