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

Add optimistic locking to Fedora #519

Merged
merged 1 commit into from
Aug 6, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions .rubocop_todo.yml
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
Metrics/ClassLength:
Exclude:
- 'lib/valkyrie/persistence/postgres/query_service.rb'

Metrics/MethodLength:
Exclude:
- 'lib/valkyrie/persistence/fedora/persister.rb'
5 changes: 5 additions & 0 deletions lib/valkyrie/persistence/fedora/permissive_schema.rb
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,11 @@ def self.valkyrie_time
uri_for(:valkyrie_time)
end

# @return [RDF::URI]
def self.optimistic_lock_token
uri_for(:optimistic_lock_token)
end

# Cast the property to a URI in the namespace
# @param property [Symbol]
# @return [RDF::URI]
Expand Down
44 changes: 37 additions & 7 deletions lib/valkyrie/persistence/fedora/persister.rb
Original file line number Diff line number Diff line change
Expand Up @@ -13,13 +13,16 @@ def initialize(adapter:)
# (see Valkyrie::Persistence::Memory::Persister#save)
def save(resource:)
initialize_repository
resource.created_at ||= Time.current
resource.updated_at ||= Time.current
orm = resource_factory.from_resource(resource: resource)
alternate_resources = find_or_create_alternate_ids(resource)

if !orm.new? || resource.id
cleanup_alternate_resources(resource) if alternate_resources
internal_resource = resource.dup
internal_resource.created_at ||= Time.current
internal_resource.updated_at ||= Time.current
validate_lock_token(internal_resource)
generate_lock_token(internal_resource)
orm = resource_factory.from_resource(resource: internal_resource)
alternate_resources = find_or_create_alternate_ids(internal_resource)

if !orm.new? || internal_resource.id
cleanup_alternate_resources(internal_resource) if alternate_resources
orm.update { |req| req.headers["Prefer"] = "handling=lenient; received=\"minimal\"" }
else
orm.create
Expand All @@ -34,6 +37,9 @@ def save_all(resources:)
resources.map do |resource|
save(resource: resource)
end
rescue Valkyrie::Persistence::StaleObjectError
# blank out the message / id
raise Valkyrie::Persistence::StaleObjectError
end

# (see Valkyrie::Persistence::Memory::Persister#delete)
Expand Down Expand Up @@ -99,5 +105,29 @@ def save_reference_to_resource(resource, alternate_resources)

resource
end

# @note Fedora's last modified response is not granular enough to produce an effective lock token
# therefore, we use the same implementation as the memory adapter. This could fail to lock a
# resource if Fedora updated this resource between the time it was saved and Valkyrie created
# the token.
Copy link
Contributor

Choose a reason for hiding this comment

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

I appreciate the need for finer-grained locking than Fedora's HTTP-date based 1-second granularity. But maybe we should fall back on that, so we had at least that much protection against updates from non-Valkyrie clients?

Copy link
Contributor

Choose a reason for hiding this comment

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

8/6/18: We decided that the current implementation is good enough to close this ticket, and created a separate ticket for using Fedora's If-Unmodified-Since header (#522)

def generate_lock_token(resource)
return unless resource.optimistic_locking_enabled?
token = Valkyrie::Persistence::OptimisticLockToken.new(adapter_id: adapter.id, token: Time.now.to_r)
resource.send("#{Valkyrie::Persistence::Attributes::OPTIMISTIC_LOCK}=", token)
end

def validate_lock_token(resource)
return unless resource.optimistic_locking_enabled?
return if resource.id.blank?

current_lock_token = resource[Valkyrie::Persistence::Attributes::OPTIMISTIC_LOCK].find { |lock_token| lock_token.adapter_id == adapter.id }
return if current_lock_token.blank?

retrieved_lock_tokens = adapter.query_service.find_by(id: resource.id)[Valkyrie::Persistence::Attributes::OPTIMISTIC_LOCK]
retrieved_lock_token = retrieved_lock_tokens.find { |lock_token| lock_token.adapter_id == adapter.id }
return if retrieved_lock_token.blank?

raise Valkyrie::Persistence::StaleObjectError, resource.id.to_s unless current_lock_token.serialize == retrieved_lock_token.serialize
end
end
end
12 changes: 12 additions & 0 deletions lib/valkyrie/persistence/fedora/persister/orm_converter.rb
Original file line number Diff line number Diff line change
Expand Up @@ -237,6 +237,18 @@ def result
end
end

class ValkyrieOptimisticLockToken < ::Valkyrie::ValueMapper
FedoraValue.register(self)
def self.handles?(value)
value.statement.object.is_a?(RDF::Literal) && value.statement.object.datatype == PermissiveSchema.optimistic_lock_token
end

def result
value.statement.object = Valkyrie::Persistence::OptimisticLockToken.new(adapter_id: value.adapter.id, token: value.statement.object.to_s)
calling_mapper.for(Property.new(statement: value.statement, scope: value.scope, adapter: value.adapter)).result
end
end

class InternalURI < ::Valkyrie::ValueMapper
FedoraValue.register(self)
def self.handles?(value)
Expand Down
6 changes: 6 additions & 0 deletions spec/valkyrie/persistence/fedora/permissive_schema_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -13,4 +13,10 @@
expect(described_class.alternate_ids).to eq RDF::URI("http://example.com/predicate/alternate_ids")
end
end

describe ".optimistic_lock_token" do
it "returns the expected temporary URI" do
expect(described_class.optimistic_lock_token).to eq RDF::URI("http://example.com/predicate/optimistic_lock_token")
end
end
end
2 changes: 2 additions & 0 deletions spec/valkyrie/persistence/fedora/persister_spec.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
# frozen_string_literal: true
require 'spec_helper'
require 'valkyrie/specs/shared_specs'
require 'valkyrie/specs/shared_specs/locking_persister'

RSpec.describe Valkyrie::Persistence::Fedora::Persister do
let(:adapter) do
Expand All @@ -13,6 +14,7 @@
let(:persister) { adapter.persister }
let(:query_service) { adapter.query_service }
it_behaves_like "a Valkyrie::Persister"
it_behaves_like "a Valkyrie locking persister"

context "when given an id containing a slash" do
before do
Expand Down