Add `:constraint_handler` and `Ecto.Adapters.SQL.Constraint`

[petermueller]

Adds a new :constraint_handler to allow customizing adapter error handling, that can be passed as a shared option to repo functions, e.g. MyRepo.insert(..., constraint_handler: {MyCustomHandler, :to_constraints, []}), but also passed as repository config, similar to the :prefix option.

[petermueller]

This is a proof of concept PR, from the original thread on the Google Group.

I have not yet swapped the Postgres and Tds implementations, but it should be a matter of a few minutes to do so.

The main things I could use some insights and feedback on are:

1. where should people be allowed to set the :constraint_handler option?
2. how should the "default" constraint handler be set for the built-in adapters?
3. how should Ecto.Adapter.SQL.Connection.to_constraints/2 be deprecated?
4. what do you think of also pulling the Ecto.Adapters.SQL.to_constraints/4 out and making it be a default implementation of doing something like use Ecto.Adapters.SQL.Constraint?

All 4 of these items are somewhat interconnected, and affect the possible options of others.

e.g. doing # 4 would be neat for allowing downstream libraries an escape hatch in case they structure their config differently, but it might make more sense to just put it in the __using__ of Ecto.Adapters.SQL, but that may make it harder to not break downstream things, or hard-deprecate the old behaviour callbacks.

As it is currently written, it should work for downstream libraries without breaking them, but it does not provide a clear path, primarily because I did not want to go down a particular direction without input.

Let me know what you think 🙂 I know this solution would let us more aggressively enforce new/consistent DB controls on a legacy app we're migrating away from while still having a nice developer experience when working w/ Ecto.

[petermueller]

The mysql tests are reliant on an :extra_error_codes value of [{1644, :ER_SIGNAL_EXCEPTION}], and I wasn't quite sure how we'd want to add that in these tests.

I can also just open a PR on myxql to add it, as it's a fairly useful, and likely the error code people would use to write custom errors from within procedures.

[petermueller]

I checked through the dependent libraries I could find from hex and ecto_ch (Clickhouse) seemed to be the only one implementing Ecto.Adapters.SQL.Connection, and theirs just raises as "not implemented" currently.

I didn't really see many others that were sticking to the typical patterns from the built-in adapters, but maybe I just missed some.

But I think if we just removed the Connection.to_constraints/2 callback, I don't know that it would cause any issue in downstream adapter libraries

EDIT: I don't know how I missed the SQLite adapter. They'd be affected probably

[josevalim]

Given it is a single function, can we make the contract a MFArgs instead of a module? This way we don't need to create new modules for every new adapter. We can just point to the existing function. Thank you.

[warmwaffles]

I don't know how I missed the SQLite adapter. They'd be affected probably

Don't worry I'm watching this 😈

[josevalim]

Ping! :)

[petermueller]

Ping! :)

Pong 😬 My bad, been poking it once a week or so.

I was trying to think of an example of when a custom constraint handler would be useful in Postgres, but most of the constraints I could think of could be handled by pg's existing "box" model for exclusion constraints, or they didn't feel like a realistic use-case.

For now though, I think I'm just going to do a simple check that would otherwise be some business-logic you'd need to add to an update/insert, or use a Multi.

If you can think of anything off the top of your head, let me know. I was mainly thinking of this for docs, but also potentially just for the tests.

[petermueller]

TODO

• [x] allows setting a :constraint_handler option in locations similar to :prefix
  • [x] config
  • [x] start_link
  • [x] per operation
• [x] extracts c:Ecto.Adapters.SQL.Connection.to_constraints/2 to new Ecto.Adapters.SQL.Constraint
• [x] add @behaviour Ecto.Adapters.SQL.Constraint to built-in adapters' connections
• defaults to the connection's to_constraints/2
• [ ] Adds tests for the built-in adapters verifying custom constraint handlers work
  • [x] MyXQL
    • [x] 5.7
    • [x] 8.0
  • [x] Postgres
    • [x] 9.5
    • [x] 9.6
    • [x] 11.11
    • [x] 16.2
  • [ ] Tds
    • [ ] 2019
    • [ ] 2022
• [ ] Adds Ecto.Adapters.SQL.to_constraints/4 and macros for MyRepo.to_constraints/3 and docs
  • [x] Example for Postgres
  • [x] Example for MySQL
  • [ ] Example for Tds
• [ ] Adds general documentation for new :constraint_handler shared option, and when/how to use it
• [ ] Adds general documentation for new :constraint_handler config option and when/how to use it along w/ the new Ecto.Adapters.SQL.Constraint behaviour

[petermueller]

I'm stuck on the TDS test. Since we use OUTPUT without INTO, any of the triggers I could use will cause SQL Server to complain.

Anyone have any ideas for how to test this? I haven't done TDS since 2015 in Ruby. I don't actually care that it's a trigger that's emulating a constraint, it's mainly just something that a user might do to validate data, and have the DB throw an error, that they'd want to have an escape hatch for surfacing as an application error, instead of having rescue in their code.

I saw we could modify the returning function of the TDS connection module, but then we'd have to always "know" the columns and pass them into a temp table for pretty much all statements. That seems like a massive change just to support an escape hatch

[josevalim]

My suggestion is to add a unit test, without focusing on the database, since the logic does not seem to be database specific, and then having a smoke test either via PG or MySQL would be fine. I also don't think we need a MFA or a behaviour, we could have constraint_handler receive a regular function?

[petermueller]

Sounds good.

My thought was that a module or mfargs was mainly so we could support compile-time config, similar to the :prefix option so they don't have to pass it in everywhere.

I like the function idea, especially for ad-hoc use cases, but I think people who'd use this would still want compile-time support.

The behaviour is really just for someone to be able to read the docs and see "oh I just need to implement [this] and have it fallback to the adapter's connection, then stick it in config.exs"

[josevalim]

Our default_options or prepare_options in the repo should still allow you to set it "globally", right? It doesn't feel like this is a config though (or something that deserves to be far away in a config file).

[petermueller]

Ah, I missed default_options.

I'll remove the behaviour and change stuff around to just expect a function