Skip to content

Commit

Permalink
🐛 Remove subtle timing error in ActorStack coercion
Browse files Browse the repository at this point in the history
Prior to this commit, the order of operations was:

1. Coerce the curation_concern to a valkyrie_resource
2. Save the curation concern

The subtle bug is that the curation_concern's valkyrie resource would
have an out of sync access control.  Why, because we're relying on logic
elsewhere (e.g. Hyrax::ModelConverter) to assign the permissions.

In v5.0.0 and prior we're copying the current curation concern's
permissions to the valkyrie permission; this happens before we save.
[See implementation][1].

However, this can create issues depending on metadata adapter
configuration; in particular when we update the ACL object for a record
without updating the record; something that we see happening in the
Frigg and Freyja adapters, but is also not limited to those adapters.

Also, consider that we likely want to remove a timing of parameters;
because when reading method call, we (or at least I) mentally treat the
parameters we pass as order independent.  Put another way, the
parameters we pass should likely not change state of each other.

[1]: bcf3ab0
  • Loading branch information
jeremyf committed Feb 6, 2024
1 parent 0ee1b47 commit 0de7df6
Showing 1 changed file with 6 additions and 3 deletions.
9 changes: 6 additions & 3 deletions app/actors/hyrax/actors/base_actor.rb
Original file line number Diff line number Diff line change
Expand Up @@ -69,10 +69,13 @@ def apply_deposit_date(env)
end

def save(env, use_valkyrie: false)
return env.curation_concern.save unless use_valkyrie
# NOTE: You must call env.curation_concern.save before you attempt to coerce the curation
# concern into a valkyrie resource.
is_valid = env.curation_concern.save
return is_valid unless use_valkyrie

# don't run validations again on the converted object if they've already passed
resource = valkyrie_save(resource: env.curation_concern.valkyrie_resource, is_valid: env.curation_concern.save)
resource = valkyrie_save(resource: env.curation_concern.valkyrie_resource, is_valid: is_valid)

# we need to manually set the id and reload, because the actor stack requires
# `env.curation_concern` to be the exact same instance throughout.
Expand All @@ -83,7 +86,7 @@ def save(env, use_valkyrie: false)
# for now, just hit the validation error again
# later we should capture the _err.obj and pass it back
# through the environment
env.curation_concern.save
is_valid
end

def apply_save_data_to_curation_concern(env)
Expand Down

0 comments on commit 0de7df6

Please sign in to comment.