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

Persisters should error if you try to save something which is gone now, but used to be persisted. #867

Merged
merged 15 commits into from
Oct 15, 2021
Merged
2 changes: 1 addition & 1 deletion lib/valkyrie/persistence/fedora/persister.rb
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ def initialize(adapter:)
end

# (see Valkyrie::Persistence::Memory::Persister#save)
def save(resource:)
def save(resource:, external_resource: false)
initialize_repository
internal_resource = resource.dup
internal_resource.created_at ||= Time.current
Expand Down
7 changes: 5 additions & 2 deletions lib/valkyrie/persistence/memory/persister.rb
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,15 @@ def initialize(adapter)

# Save a single resource.
# @param resource [Valkyrie::Resource] The resource to save.
# @param external_resource [Boolean] Whether the resource to be saved comes
# from a different metadata store. Allows a resource to be saved even if
# it's not already in the store.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add example use case, "e.g. when building a new solr index"

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or migrating to a new data store?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tpendragon add an example use case?

# @return [Valkyrie::Resource] The resource with an `#id` value generated by the
# persistence backend.
# @raise [Valkyrie::Persistence::StaleObjectError]
def save(resource:)
def save(resource:, external_resource: false)
raise Valkyrie::Persistence::StaleObjectError, "The object #{resource.id} has been updated by another process." unless valid_lock?(resource)
raise Valkyrie::Persistence::ObjectNotFoundError, "The object #{resource.id} is previously persisted but not found at save time." unless valid_for_save?(resource)
raise Valkyrie::Persistence::ObjectNotFoundError, "The object #{resource.id} is previously persisted but not found at save time." unless external_resource || valid_for_save?(resource)

# duplicate the resource so we are not creating side effects on the caller's resource
internal_resource = resource.dup
Expand Down
4 changes: 2 additions & 2 deletions lib/valkyrie/persistence/postgres/persister.rb
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,10 @@ def initialize(adapter:)
# @return [Valkyrie::Resource] the persisted/updated resource
# @raise [Valkyrie::Persistence::StaleObjectError] raised if the resource
# was modified in the database between been read into memory and persisted
def save(resource:)
def save(resource:, external_resource: false)
orm_object = resource_factory.from_resource(resource: resource)
orm_object.transaction do
raise Valkyrie::Persistence::ObjectNotFoundError, "The object #{resource.id} is previously persisted but not found at save time." if resource.persisted? && !orm_object.persisted?
raise Valkyrie::Persistence::ObjectNotFoundError, "The object #{resource.id} is previously persisted but not found at save time." if !external_resource && resource.persisted? && !orm_object.persisted?
orm_object.save!
if resource.id && resource.id.to_s != orm_object.id
raise Valkyrie::Persistence::UnsupportedDatatype,
Expand Down
4 changes: 2 additions & 2 deletions lib/valkyrie/persistence/solr/persister.rb
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,11 @@ def initialize(adapter:)

# (see Valkyrie::Persistence::Memory::Persister#save)
# @return [Boolean] If write_only, whether saving succeeded.
def save(resource:)
raise Valkyrie::Persistence::ObjectNotFoundError, "The object #{resource.id} is previously persisted but not found at save time." unless write_only? || valid_for_save?(resource)
def save(resource:, external_resource: false)
if write_only?
repository([resource]).persist
else
raise Valkyrie::Persistence::ObjectNotFoundError, "The object #{resource.id} is previously persisted but not found at save time." unless external_resource || valid_for_save?(resource)
repository([resource]).persist.first
end
end
Expand Down
6 changes: 6 additions & 0 deletions lib/valkyrie/specs/shared_specs/persister.rb
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,12 @@ class CustomResource < Valkyrie::Resource

expect { persister.save(resource: saved) }.to raise_error(Valkyrie::Persistence::ObjectNotFoundError)
end
it "is okay if it's from another persister" do
memory_adapter = Valkyrie::Persistence::Memory::MetadataAdapter.new
saved = memory_adapter.persister.save(resource: resource)

expect { persister.save(resource: saved, external_resource: true) }.not_to raise_error
end
end

it "can persist single values" do
Expand Down