sea-orm icon indicating copy to clipboard operation
sea-orm copied to clipboard

Support on conflict

Open SebastienGllmt opened this issue 3 years ago • 5 comments

PR Info

This PR exposes the ON CONFLICT support that was added in sea-query

Adds

Option for a new insert_or that allows you to specify an on-conflict behavior

Public API Rationale

I think there were 3 ways this could be exposed from a public API point of view:

  1. Modify the existing insert function to add a new parameter
  2. Keep the insert function as-is, but introduce some state in the ActiveModel to handle conflicts
  3. Create a new function called insert_or
  • I didn't pick (1) because this feels like too big of a breaking change.
  • I didn't pick (2) because to be, handling conflict is something you do in a per-query basis and not in the model. For example, in some cases you may want to do nothing on a conflict, some cases you may want to error, some cases you may want to override.
  • (3) seemed like the best solution

Handling of return value

One of the conflict options is ON CONFLICT DO NOTHING. This doesn't work with the current API where the insert function is assumed to always have a return value (do nothing returns an empty SQL result if a conflict is found)

There are two ways this can be solved:

  1. When using ON CONFLICT DO NOTHING, check if nothing was returned and then do a find to find the entry that caused the conflict and return that as the result of the insert
  2. Make that when using insert_or, we have an optional return instead of a mandatory return

I picked (2) seems (1) probably has performance implications

TODO

  • [ ] When upgrading to sea-query 0.24.0 (which introduces the on-conflict support), I had to add window: None to a few functions. Is this correct? (see https://github.com/SeaQL/sea-query/pull/271)
  • [ ] When using do nothing, we can't call after_save since nothing was saved. Is this okay?
  • [ ] I'm not sure what to do about testing since it seems the insert functions are not directly tested either. Should it be done as an integration test in tests/relational_tests?
  • [ ] Add documentation for insert_or

SebastienGllmt avatar Apr 13 '22 18:04 SebastienGllmt

I rebased on top of #673 to fix the bug with the 0.24.0 upgrade

SebastienGllmt avatar Apr 19 '22 10:04 SebastienGllmt

@billy1624 I'm concerned that the performance implications of doing this probably aren't desired for cases where ON CONFLICT DO NOTHING is used (the only case where nothing is returned)

Rationale

ON CONFLICT can only be used on unique constraints. Although there are some tables with multiple unique constraints, most of the time your unique constraint is the primary key (possibly a composite key)

Example: A database with 3 tables:

  • Persons table
  • Jobs table
  • Some table that maps the many-many relation of <PersonId, JobId>

If you want to insert a new <PersonId, JobId> pair, you can just insert with an ON CONFLICT DO NOTHING because if row already exists, you already know all the data in the row so you don't care about the return value. Having sea-orm do a 2nd query to get the row provides no additional information and double the number of database roundtrips required

Potential compromise solution

Instead of implementing insert_or as a member function of ActiveModel, we could decide to force users to use the Entity::insert() function instead that way the user has to explicitly pick exec or exec_with_returning so they can pick which they want.

If we really want to provide insert_or as a member function, we could add in the documentation to encourage users to use the Entity function if they explicitly want to ignore the return value

SebastienGllmt avatar Apr 21 '22 06:04 SebastienGllmt

If you want to insert a new <PersonId, JobId> pair, you can just insert with an ON CONFLICT DO NOTHING because if row already exists, you already know all the data in the row so you don't care about the return value. Having sea-orm do a 2nd query to get the row provides no additional information and double the number of database roundtrips required

Make sense. I think the return type of insert_or is confusing, Result<Option<Model>, ...>, as you have said the return values doesn't matter, we could just return Result<(), ...>. However, one can't access the inserted ActiveModel anymore as it has been moved because the signature of insert_or<'a, C>(self, ...).

async fn insert_or<'a, C>(self, db: &'a C, on_conflict: &OnConflict) -> Result<Option<<Self::Entity as EntityTrait>::Model>, DbErr>

billy1624 avatar Apr 21 '22 07:04 billy1624

Sorry for the delay but this seems like a complicated topic. Hopefully I will be able to provide some meaningful input soon

tyt2y3 avatar May 09 '22 14:05 tyt2y3

Hey @SebastienGllmt, sorry for the delay.

The ON CONFLICT features has been added to SeaORM on another PR:

  • https://github.com/SeaQL/sea-orm/pull/791

Thank you so much for the contributions. And do let us know if you have idea on further improving it :)

billy1624 avatar Jul 12 '22 05:07 billy1624