`BulkInsertSafe` fails its callback checks when included in models with `has_many` associations.

Summary

If BulkInsertSafe is included in a model that has a has_many relation, its callback check will fail:

Not allowed to call `set_callback(save, [:before, :before_save_collection_association, {}])`

Steps to reproduce

Choose a model with a has_many relationship like x509_issuer.rb:

diff --git a/app/models/x509_issuer.rb b/app/models/x509_issuer.rb
index 4b75e38bbde..164acc01c2a 100644
--- a/app/models/x509_issuer.rb
+++ b/app/models/x509_issuer.rb
@@ -1,6 +1,8 @@
 # frozen_string_literal: true

 class X509Issuer < ApplicationRecord
+  include BulkInsertSafe 
+
   has_many :x509_certificates, inverse_of: 'x509_issuer'

   # rfc 5280 - 4.2.1.1  Authority Key Identifier

And then try to start the console.

added typebug label

changed the description

added database tooling (archive) labels

cc @gitlab-org/database-team

mentioned in merge request !41714 (merged)

cc @mkaeppler

Could you expand a bit more what the intention is? Are you trying to bulk-insert X509Issuers, or X509Certificates? If you want to bulk-insert the latter via the former, you need to include BulkInsertableAssociations.

See https://docs.gitlab.com/ee/development/insert_into_tables_in_batches.html#insert-has_many-associations-in-bulk

@mkaeppler In this example it's the former. I chose X509Issuer as an example in this issue because it's a simple model. But my real-life case is actually DesignManagement::Design !41714 (comment 410901814). I want to bulk insert data into design_management_designs and I can with ::Gitlab::Database.bulk_insert but not by using BulkInsertSafe because of this error.

It looks like a has_many relation adds a before_save to the model. This is the callback.

You can also see the error if you run bulk_safe_insert_spec.rb with this patch:

diff --git a/spec/models/concerns/bulk_insert_safe_spec.rb b/spec/models/concerns/bulk_insert_safe_spec.rb
index 82b0c00b396..072be2079fd 100644
--- a/spec/models/concerns/bulk_insert_safe_spec.rb
+++ b/spec/models/concerns/bulk_insert_safe_spec.rb
@@ -32,6 +32,7 @@ RSpec.describe BulkInsertSafe do
       include ShaAttribute

       validates :name, :enum_value, :secret_value, :sha_value, :jsonb_value, presence: true
+      has_many :issues

       sha_attribute :sha_value

Right, I see now. Yes this indeed is not supported currently because we'd have to recursively walk the tree of relations to find out whether all of them are also BulkInsertSafe, then bulk-insert those too.

I don't think this is a bug; it is a missing feature. We should prioritize accordingly.

has_many as per the current design is only supported when the association is bulk-inserted through its owner explicitly via bulk_insert_associations! (as per the docs.)

Thanks, @mkaeppler.

Yes this indeed is not supported currently because we'd have to recursively walk the tree of relations to find out whether all of them are also BulkInsertSafe, then bulk-insert those too.

In the situation of Design at least, I don't care about inserting associations data at all, just the data that should go into design_management_designs. So in this case it seems like it should not matter whether the associations are considered safe or not.

Yes, I see, it's an interesting point. I need to think about it more. It sounds like in this case we should indeed allow the call, but there are other cases where not inserting dependencies might cause integrity issues.

I see from the MR you linked that you're not currently blocked on this correct? I don't think I'll get to this issue anytime soon since our team is pretty swamped with the image scaling work currently.

Thanks, @mkaeppler. I really appreciate you looking and thinking about this 🙏.

I'm not blocked and can use the deprecated ::Gitlab::Database.bulk_insert. It means that this issue would be a blocker to #221032, so I'll mark it as such.

/cc @syasonik This is a duplicate of the error you were seeing today.

For context, we have a similar situation that @.luke had. We have an existing model that had BulkInsertSafe defined and we are trying to add a new has_many association.

Like @.luke, I don't think we have plans to bulk insert data for the has_many association through it's parent.

@syasonik You may be able to use the workaround like Luke did in copy_service.rb.

@mkaeppler Is there any reason why we can't allow the auto-generated Rails callbacks (similar to the already allowed autosave_associated_records_for_ one) for has_many associations, provided there is no autosave: true or accepts_nested_attributes_for defined in the class?

No reason other than we didn't consider it when we wrote the first version of this, because it wasn't necessary at the time.

The main rule we should follow is that we cannot bulk insert something that itself holds references to something that isn't bulk-insertable. Even if autosave is false, doesn't that create integrity issues, because I now have records in the database that hold references to non-persisted associations? 🤔

Even if autosave is false, doesn't that create integrity issues, because I now have records in the database that hold references to non-persisted associations?

In the case of the has_many the foreign keys/references are on the associated tables, and not the table we are discussing bulk inserting into.

I imagine a typical workflow might be:

1. Bulk insert some models
2. Fire off a job for business logic + insert for data for the has_many

It looks like we already have a workflow for inserting both the record + has_many associations at once. https://docs.gitlab.com/ee/development/insert_into_tables_in_batches.html#insert-has_many-associations-in-bulk

@minac @ifrenkel @mallocke Do you have any input on this? I've seen in multiple instances where this has caused confusion during development, or where we have chosen to deliberately bypass this validation by changing include ordering.

Examples:

1. !114304 (comment 1323346833)
2. !108600 (comment 1247889309)
3. !109398 (comment 1247474063)

There are cases where it's safe to ignore these associations: Either because they are nullable, or because we are bulk inserting the association ids as well. Since BulkInsertSafe does model validation, shouldn't that be able to catch such issues? If an association is required and the foreign key for that association is not present, the model validation would fail.

I don't really know why we even check the callbacks. To me, BulkInsertSafe is just a wrapper around ActiveRecord::InsertAll with an additional feature of running the validations. If we are worried about data integrity issues, we should have already created the foreign key constraints and model layer presence validations(not for the association as it causes N+1 query issues).

For the record, as most of the discussion here is around has_many, a similar issue occurs with has_one ... :through where active record tries to define an after_create callback. I haven't confirmed whether it occurs for has_one without :through but I think it will.

BulkInsertSafe::MethodNotAllowedError:
  Not allowed to call `set_callback(create, [:after, :autosave_associated_records_for_author

I think these callbacks could be potentially added for any of has_one, belongs_to, has_many or has_and_belongs_to_many if :autosave is added to the association, via https://github.com/rails/rails/blob/2cef3ac45bd85e1642b1270ab4fa31629044c962/activerecord/lib/active_record/autosave_association.rb

For has_one, and possibly other association types, :autosave is enabled implicitly:

Note that <tt>autosave: false</tt> is not same as not declaring <tt>:autosave</tt>.
When the <tt>:autosave</tt> option is not present then new association records are
saved but the updated association records are not saved.

marked this issue as related to #221032

mentioned in merge request !44047 (closed)

mentioned in merge request !45027 (merged)

mentioned in merge request !46596 (merged)

added vintage label

Hi @.luke 👋,

Thanks for raising this bug.

Contributions like this are vital to help make GitLab a better product.

We would be grateful for your help in verifying whether your bug report requires further attention from the team. If you think this bug still exists, and is reproducible with the latest stable version of GitLab, please comment on this issue.

This issue has been inactive for more than 12 months now and based on the policy for inactive bugs, will be closed in 7 days.

Thanks for your contributions to make GitLab better!

I believe this problem still exists.

added stale label

removed stale label

removed tooling (archive) label

changed milestone to %Backlog

added [deprecated] Accepting merge requests label

added groupdatabase label

added devopsdata stores sectioncore platform labels

added backend label

Setting label(s) Category:Database based on groupdatabase.

added Category:Database label

Hi @.luke 👋,

Thanks for raising this bug.

Contributions like this are vital to help make GitLab a better product.

We would be grateful for your help in verifying whether your bug report requires further attention from the team. If you think this bug still exists, and is reproducible with the latest stable version of GitLab, please comment on this issue.

This issue has been inactive for more than 12 months now and based on the policy for inactive bugs, will be closed in 7 days.

Thanks for your contributions to make GitLab better!

This is my yearly pre-Christmas bot check-in on this 🎄 😅.

The patch here #247718 (comment 411541756) shows the problem still exists.

added stale label

removed stale label

mentioned in issue gitlab-org/database-team/team-tasks#308 (closed)

added severity3 label

mentioned in merge request !114304 (merged)

More examples:

• https://gitlab.com/gitlab-org/gitlab/-/blob/19c8e45f7b02ea78da87dcf4187f28e0b6fb01a8/ee/app/models/vulnerabilities/identifier.rb#L45
• !108600 (comment 1256011395)
• !114304 (comment 1323346833)

I am facing the same issue with my MR - !117149 (merged)

Similar to what Sean mentioned, the model I am changing IssueEmailParticipant already had this concern included.

I added a has_many association to it with Note model and added belongs_to to the Note model.

I tried using this patch but this fails on validation as id that it sets is null. Is there any workaround for this?

mentioned in issue #226995 (closed)

mentioned in issue gitlab-org/monitor/respond#230 (closed)

added SLOMissed label

mentioned in merge request !145881 (merged)

marked this issue as related to #382240

I came across this issue in !145881 (comment 1798741796). As many have already noted, the current workaround is to put include BulkInsertSafe after the has_many declaration. We see this enacted in multiple places in the codebase:

• ee/app/models/package_metadata/advisory.rb:12
• ee/app/models/package_metadata/license.rb:7
• ee/app/models/package_metadata/package.rb:21
• ee/app/models/vulnerabilities/identifier.rb:51

Since this is a common use case, perhaps the BulkInsertSafe module should be updated to support it.

Related to the discussion here.

added devopsdata_access label and removed devopsdata stores label

added groupdatabase frameworks label and removed groupdatabase [DEPRECATED] label