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

recursive_on_duplicate_key_update and unique validations #841

Open
kalsan opened this issue Aug 2, 2024 · 14 comments
Open

recursive_on_duplicate_key_update and unique validations #841

kalsan opened this issue Aug 2, 2024 · 14 comments

Comments

@kalsan
Copy link

kalsan commented Aug 2, 2024

Hey there and thank you for this awesome gem!

I'm currently using it to import a compount key in the following form where Customer has_many :subscriptions:

import_result = Customer.import(
        customers,
        # validate_uniqueness: false, # TODO: why does this not help?
        validate: false, # TODO: why is this needed?
        recursive:                         true,
        on_duplicate_key_update:           {
          conflict_target: %i[imported_by_id imported_id],
          columns:         %i[
            foo
            bar
          ]
        },
        recursive_on_duplicate_key_update: {
          subscriptions: {
            # validate_uniqueness: false, # TODO: why does this not help?
            conflict_target: %i[imported_by_id imported_id],
            columns:         %i[
              morefoo
              morebar
            ]
          }
        }
      )

As you can see in the "TODO" marked parts above, I need to pass validate: false for the record to sucessfully import. Otherwise, the following validation in Subscription fails:

validates :imported_id, uniqueness: { scope: :imported_by_id }, allow_blank: true

I'm wondering whether this is a problem of understanding on my side or a bug.

Thanks a lot in advance!

Best,
Kalsan

@kaliara
Copy link

kaliara commented Aug 22, 2024

@kalsan did you happen to make any progress on this? Dealing with the same issue.

@jkowens
Copy link
Collaborator

jkowens commented Aug 23, 2024

@kalsan and @kaliara Rails 7.2 validation issues should be resolved now in the master branch. Would you be able to point to that branch and test to make sure it is good?

@kaliara
Copy link

kaliara commented Aug 23, 2024

That fixed it @jkowens, thank you very much!

@jkowens
Copy link
Collaborator

jkowens commented Aug 23, 2024

That's great news. Just released fix in v1.8.0 🎉

@jkowens jkowens closed this as completed Aug 23, 2024
@kalsan
Copy link
Author

kalsan commented Aug 24, 2024

Hi @jkowens and thanks a lot for the update! Unfortunately, this does not help in my case (even with validate_uniqueness: false in both the parent and child import config.
I guess this is a different problem from what @kaliara is experiencing, as I am still on Rails 7.1.

@jkowens jkowens reopened this Aug 24, 2024
@jkowens
Copy link
Collaborator

jkowens commented Aug 24, 2024

@kaliara thanks for following up. I'm wondering if the issue is due to the scoped uniqueness validation, will need to investigate.

@kalsan
Copy link
Author

kalsan commented Aug 24, 2024

Let me know if I can provide any further information :-)

@jkowens
Copy link
Collaborator

jkowens commented Aug 24, 2024

@kalsan I'm curious, does the Customer and Subscription import work with validations if you don't do a recursive import?

@kalsan
Copy link
Author

kalsan commented Aug 25, 2024

@jkowens good point. It does not. The used code:

import_result = Customer.import(
        customers,
        on_duplicate_key_update:           {
          conflict_target: %i[imported_by_id imported_id],
          columns:         %i[
            first_name
            last_name
            contact_person
            address_extra
            street_and_number
            zip_code
            town
            country
            birth_date
            bad_payer
            notes
          ]
        }
      )

Experiments:

  • works when validate: false
  • does not work if just validate_uniqueness: false
  • does not work if both statements are omitted.

@jkowens
Copy link
Collaborator

jkowens commented Aug 28, 2024

@kalsan can you provide a simple application that reproduces this error? I seem to be having trouble doing so.

@kalsan
Copy link
Author

kalsan commented Aug 29, 2024

@jkowens here you go :-)

fooimport.zip

Steps required:

  • Run postgres (easiest method ist to quickly fire up a docker container and bind it to the host network or forward the port)
  • bundle
  • rails c
  • CustomerImport.run
  • CustomerImport.run again -> failure

Let me know if I can help with anything :-)

@jkowens
Copy link
Collaborator

jkowens commented Sep 6, 2024

Thanks @kalsan this was helpful. So I've determined there is an inherit issue with the way validations are handled for activerecord-import. We jump thru hoops to prevent uniqueness validations from running for a model, but we didn't account for the fact that associations are validated for a record as well. We'll have to think this over a bit.

@kalsan
Copy link
Author

kalsan commented Sep 6, 2024

@jkowens thanks for following up :-) It's awesome that you take the time and commitment to make the Gem even better!

@jacob-carlborg-apoex
Copy link
Contributor

@kalsan I strongly recommend against using Rails to validate uniqueness, regardless of using Activerecord-Import or not. The way Rails validates uniqueness is by executing a query in the database to see if there's already a row with an existing, in your case, imported_id. That means:

  • It's slow. Because an extra query is performed
  • It cause a race condition. After Rails has performed the query that checks if a value is unique, another query might have inserted a new row with the same value

Instead, make sure you have uniqueness constrains in the database and let the database take care of the validation. Then handle the exception that is raised. You need to do this anyway due to the above mentioned race condition (if you uniqueness constrains). Or you can use this gem [1] that handles this automatically.

[1] https://rubygems.org/gems/record_not_unique

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

No branches or pull requests

4 participants