Skip to content

Commit

Permalink
avoid n+1 query on file_metadata delete
Browse files Browse the repository at this point in the history
Hyrax should avoid looping over ids when it could use a custom query. e.g. we
should never be doing `my_resource.member_ids.each` and looking up members instead doing
`query_service.find_members(resource: my_resource)`. doing this creates two
problems:

  - it prevents adapters from optimizing n+1 query problems for large sets of
  ids.
  - it locks hyrax into modelling details (relationship directionality) that
  applications may want to avoid.

there are probably other places this problem exists, but i'm aware of this one
and wanted to get a patch in right away.

note that this proposes unceremoniously removing the `property:` step
argument. if we need to keep this, we could choose to make in `nil` by default
and fall back to the old code if a value is provided. i can't see any reason a
caller would want to change the property of this step though, and i strongly
doubt anyone downstream is doing so.
  • Loading branch information
tamsin woo authored and tamsin woo committed Mar 26, 2024
1 parent 5f5f306 commit 56c25f0
Showing 1 changed file with 4 additions and 5 deletions.
9 changes: 4 additions & 5 deletions lib/hyrax/transactions/steps/delete_all_file_metadata.rb
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,7 @@ class DeleteAllFileMetadata

##
# @params [#save] persister
def initialize(property: :file_ids, query_service: Hyrax.query_service, persister: Hyrax.persister, publisher: Hyrax.publisher)
@property = property
def initialize(query_service: Hyrax.query_service, persister: Hyrax.persister, publisher: Hyrax.publisher)
@persister = persister
@query_service = query_service
@publisher = publisher
Expand All @@ -29,10 +28,10 @@ def initialize(property: :file_ids, query_service: Hyrax.query_service, persiste
def call(resource)
return Failure(:resource_not_persisted) unless resource.persisted?

resource[@property].each do |file_id|
return Failure[:failed_to_delete_file_metadata, file_id] unless
@query_service.custom_queries.find_files(file_set: resource).each do |file_metadata|
return Failure[:failed_to_delete_file_metadata, file_metadata.id] unless
Hyrax::Transactions::Container['file_metadata.destroy']
.call(@query_service.custom_queries.find_file_metadata_by(id: file_id))
.call(file_metadata)
.success?
rescue ::Ldp::Gone
nil
Expand Down

0 comments on commit 56c25f0

Please sign in to comment.