[PROPOSAL] Allow custom `to_constraints` for Adapters
15 views
Skip to first unread message
Peter Mueller
Jun 20, 2024, 10:24:24 PMJun 20
to elixir-ecto
Hello folks!
This proposal is about adding a mechanism for users of ecto_sql to be able to handle custom "uncalled" exceptions in the adapter with the `to_constraints` callback. This would allow for things such as MyXQL and the matching adapter to be able to opt-in to support handling check constraint exceptions.
Background:
I was recently working on a feature to add a non-overlapping date range constraint in MySQL, which resulted in me basically using a trigger to return a `signal` of `ER_DUP_ENTRY` if there was an existing overlap. I would've preferred a custom SQLSTATE/error but it was close enough. You can see the thread and code on the elixirforum if you're interested (https://elixirforum.com/t/handling-a-custom-sql-signal-similar-to-changeset-constraint-functions/64303)
That got me thinking though, and remembered that we recently had to add some custom SQL in a migration to support a table check constraint in our MySQL database.
MyXQL has `:extra_error_codes` as a way to handle them at that layer, but I noticed that there's no way to handle them higher in `ecto_sql`
So I'd like to propose a way that we could configure at compile-time an error handler mfa that could allow extension of the `Ecto.Adapters.SQL.Connection` `to_constraint` callback.
To me this would be the ideal place to accommodate new error handling features into the adapters without having to convince people to try RCs, as presumably they'd have already built the new handler themselves, we'd just be bringing it into the adapters.
It doesn't "seem" like it would be particularly difficult to implement (and I'm happy to take a pass at it) but I'd like to know people's appetite for this, and what people think the right interface would be. I'm aware they are behaviours but as the adapter and the connection are both behaviours it seems like it would be difficult to inject a wrapper connection module from config.
Let me know what people think, and thank you for your time reading my rambling :D
This proposal is about adding a mechanism for users of ecto_sql to be able to handle custom "uncalled" exceptions in the adapter with the `to_constraints` callback. This would allow for things such as MyXQL and the matching adapter to be able to opt-in to support handling check constraint exceptions.
Background:
I was recently working on a feature to add a non-overlapping date range constraint in MySQL, which resulted in me basically using a trigger to return a `signal` of `ER_DUP_ENTRY` if there was an existing overlap. I would've preferred a custom SQLSTATE/error but it was close enough. You can see the thread and code on the elixirforum if you're interested (https://elixirforum.com/t/handling-a-custom-sql-signal-similar-to-changeset-constraint-functions/64303)
That got me thinking though, and remembered that we recently had to add some custom SQL in a migration to support a table check constraint in our MySQL database.
MyXQL has `:extra_error_codes` as a way to handle them at that layer, but I noticed that there's no way to handle them higher in `ecto_sql`
So I'd like to propose a way that we could configure at compile-time an error handler mfa that could allow extension of the `Ecto.Adapters.SQL.Connection` `to_constraint` callback.
To me this would be the ideal place to accommodate new error handling features into the adapters without having to convince people to try RCs, as presumably they'd have already built the new handler themselves, we'd just be bringing it into the adapters.
It doesn't "seem" like it would be particularly difficult to implement (and I'm happy to take a pass at it) but I'd like to know people's appetite for this, and what people think the right interface would be. I'm aware they are behaviours but as the adapter and the connection are both behaviours it seems like it would be difficult to inject a wrapper connection module from config.
Let me know what people think, and thank you for your time reading my rambling :D
José Valim
Jun 21, 2024, 2:33:18 AMJun 21
to elixi...@googlegroups.com
The idea sounds good to me, the biggest question is indeed how we would implement it. So please give it a try in a PR, although there is no guarantee we will merge it. Thanks for the write up!
--
You received this message because you are subscribed to the Google Groups "elixir-ecto" group.
To unsubscribe from this group and stop receiving emails from it, send an email to elixir-ecto...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/elixir-ecto/cbe48d7a-6b19-4539-a9bb-b78b96073e83n%40googlegroups.com.
Peter Mueller
Jul 1, 2024, 12:46:45 AMJul 1
to elixir-ecto
TL;DR I added PRs for check constraint handling in MyXQL, and I'm going to take a crack at Approach A from below, but I'm unsure of the impact on downstream adapter libraries.
I presume further discussion would probably benefit from a working example PR either way, so could maybe be discussed there? Idk.
To start, I've submitted 2 PRs to add check constraint handling for MyXQL as it's now supported as of 8.0.16:
As to the original proposal, I've thought over this for the last week, and I think there's a few approaches.
To start, I've submitted 2 PRs to add check constraint handling for MyXQL as it's now supported as of 8.0.16:
As to the original proposal, I've thought over this for the last week, and I think there's a few approaches.
A. Extract `to_constraints` to an adapter level behaviour and allow overriding in the `init` / config / `use` / etc (maybe `Ecto.Adapters.SQL.Constraint.from_error` or `Ecto.Adapters.SQL.ErrorHandler.to_constraints` ?)
- risks issues w/ non elixir-ecto adapters, although it "should" just be declaring an extra `(at)behaviour` and maybe setting a default module name to call if it's nil?
- easier for users of `ecto_sql` to write custom handlers than wrapping an existing adapter and defdelegating everything
- might require a `to_constraints/3` on Connection that receives the adapter_meta, depending on how the handler was injected (e.g. use Ecto.Adapters.SQL vs compile_env somewhere? idk)
B. Do nothing to `ecto_sql` and establish convention that this is handled in the libraries, similar to `tds`
- risks reintroducing auto-generated error handling in libraries adding thousands of atoms to systems, e.g. like what was removed in https://github.com/elixir-ecto/myxql/pull/134 or burdening those libraries w/more config
- inconsistent experience :/
- tds link for reference: https://github.com/elixir-ecto/tds/blob/9e49f36434467b6c4eda1ccc8ad1471ead6c0d51/lib/tds/error.ex#L76
C. Add documentation for `ecto_sql` for how to handle custom error handling by wrapping the existing adapter and defdelegating everything
- Could contain an example of a MySQL "exclusion"-esque custom SQLSTATEs/errors, similar to my forum post, but with SQLSTATE 45000 and a custom message
- Looks like it would probably be okay for the `elixir-ecto` libraries (myxql, tds, postgrex), but I'm unsure about downstream
- Feels a little... messy?
- I'd rather write these style docs but have it use Approach A for the custom part.
I'm going to make a PR for Approach A and see how bad it is.
If people want to weigh in here, that's fine, otherwise chatting/discussing on the PR would be fine too.
If people want to weigh in here, that's fine, otherwise chatting/discussing on the PR would be fine too.
It's mostly just figuring out where all the points for config injection are that don't change the APIs much, or add lookup overhead.
Reply all
Reply to author
Forward
0 new messages