Clarify Equality

[no-reply]

Equality is currently based on the default Dry::Struct implementation, which compares all ResourceClass.__attributes__ . This includes ::Valkyrie::Persistence::OptimisticLockToken, which should likely be excluded.

We may want to consider excluding other reserved attributes.

class ResourceWithLock < Valkyrie::Resource)
  enable_optimistic_locking
end

resource1 = ResourceWithLock(id: 'blah', Valkyrie::Persistence::Attributes::OPTIMISTIC_LOCK => Valkyrie::Persistence::OptimisticLockToken.deserialize('lock_token:test:1'))
resource2 = ResourceWithLock(id: 'blah', Valkyrie::Persistence::Attributes::OPTIMISTIC_LOCK => Valkyrie::Persistence::OptimisticLockToken.deserialize('lock_token:test:2'))

resource1 == resource2 # => false

[no-reply]

One approach to this issue is to move the optimistic lock token out of #attributes.

That probably constitutes a major release, and might involve a bit of work since there's some use of the Dry::Types, but my intuition is that it's the best approach.

[escowles]

It looks like both Memory and Fedora are using the current Time of the object's instantiation to create the lock tokens:

• https://github.com/samvera/valkyrie/blob/master/lib/valkyrie/persistence/memory/persister.rb#L88
• https://github.com/samvera/valkyrie/blob/master/lib/valkyrie/persistence/fedora/persister.rb#L138

[tpendragon]

The other option is we just change equality to be something like AR, where if it's persisted it == if the two share an ID/class (thanks @jrochkind for digging that up)

[no-reply]

i'll express a preference for Dry::Struct-style equality. it seems so easy to do ID equality as a client that using #== for it seems wasteful. resource.id == other.id

for reference: include Dry::Equalizer(:id) ought to be all it takes to implement #== as AR-style ID equality; including the class check.

[tpendragon]

I'm having a hard time figuring out how we can move this forward. I think I need some use cases where I want two things to be equal to one another if they have different lock tokens.

[no-reply]

@tpendragon: if i recall, the motivating need here was Sets. for example, i think the behavior of the following code varies depending on otherwise irrelevant details of the adapters optimistic locking behavior:

class Permission < Valkyrie::Resource
  enable_optimistic_locking
  attribute :mode
  attribute :user
end

class ResourceWithPermissions < Valkyrie::Resource
  enable_optimistic_locking
  attribute :permissions, Valkyrie::Types::Set.of(Permission)
end

p = Permission.new(mode: :read, user: 'user_key')
p = Valkyrie.config.metadata_adapter.persister.save(resource: p)

resource = ResourceWithPermissions.new(permissions: [p])

p2 = Valkyrie.config.metadata_adapter.query_service.find_by(id: p.id)
resource.permissions << p2

a practical example exists in Hyrax: https://github.com/samvera/hyrax/blob/f8706bc91d1caa3a4cdd73401cc7d081ec158437/app/models/hyrax/permission.rb#L13

but honestly, i think it would be desirable as a matter of principle for Valkyrie.config.metadata_adapter.query.find_by(id: 'some_id') == Valkyrie.config.metadata_adapter.query.find_by(id: 'some_id') to be true for all adapters.