Add custom_features container to Persisters?

[tpendragon]

custom_queries has proven to be a great point for extension. We've struggled on how to implement, use, and make clear adapter-specific features like Transactions (see #582). My proposal is this:

  postgres_adapter.persister.custom_features.transaction do |transaction_adapter|
    transaction_adapter.persister.save(resource: resource)
  end # => Runs the save inside a transaction

  solr_adapter.persister.custom_features.transaction do
  end # => Raises Valkyrie::Persistence::FeatureNotImplemented with a message along the lines of "you've attempted to call the `transaction` custom feature, but it is not registered for this adapter. You will need to create or find an implementation of it and register it to this adapter in order to use this."

[jeremyf]

I like the syntax, and the articulated behavior. It looks relatively clear. From a Law of Demeter standpoint, I'm a little confused by having transaction_adapter.persister.save(resource: resource) when I'd that the yielded transaction_adapter would expose #save instead of chaining one level deeper.

My naive assumption would be (removed the chained persister call within the block):

 postgres_adapter.persister.custom_features.transaction do |transaction_adapter|
    transaction_adapter.save(resource: resource)
  end # => Runs the save inside a transaction

[tpendragon]

@jeremyf Yeah, it might be that really adapters need custom_features too, which would be like

postgres_adapter.custom_features.transaction do |transaction_adapter|
  transaction_adapter.persister.save(resource: resource)
end

Law of Demeter's always going to problematic in Valkyrie because of the expected separation of persister and query service.

Edit: The reasoning for returning an adapter instead of a persister is some implementations (fedora really) will need to be able to query the transaction.

[botimer]

Since the purpose here is to provide a scope for the lifetime of the transaction, I wonder about passing specific proxies bound to the transaction. Something like:

postgres_adapter.custom_features.transaction do |persister, <whatever>, transaction|
  transaction.on_rollback do |status|
    # log or clean up anything that isn't implicit
  end
  transaction.on_commit do |status|
    # cake and tea!
  end
  persister.save(resource: resource)
end

Something nice about this approach and ordering is that the most common case is likely to be a transaction for a basic multiple-item update. The additional parameters could be omitted when not needed.

I'm not really sure what other collaborators might need to be bound/proxied, but a transaction_adapter as a scoped service locator doesn't quite feel right. It seems that there should be a little more definition to the extension point than that.

There is also almost certain to be isolation, commit, or rollback behavior to treat on a per-transaction basis, so I don't think you can get away from a way to interact with the transaction itself. In the Valkyrie context, "adapter" is used more as a handle to an abstract, single service than for isolated, new instances of a type. It might be a bit of a Java-ism, but for stuff that relates to how transactions (outside of any given one) should be configured/interrogated/executed, an app-wide "manager" seems closer to the semantics here, while the transaction itself operates in the block. Maybe.

[jeremyf]

@tpendragon thanks; understanding that my understanding (and is something I'd love to see carried forward in the inline documentation). The "violation" makes sense as a short-cut and is quite reasonable. I do like @botimer's yielded parameters, but I think that is an implementation feature of each custom_feature.

[carolyncole]

@tpendragon This seems to allow for any number of adaptations including custom queries, since we are creating an adapter. I wonder if instead of having two different interfaces, that the custom queries be rolled into this same pattern.

That might make this line https://github.com/psu-libraries/cho/blob/fa03e78d2a42ecbf7f9b9254d53277f5a3982975/config/initializers/valkyrie.rb#L54 Valkyrie.config.metadata_adapter.query_service.custom_feature.register(FindUsing) And the call be query_service.custom_feature.find_using

[no-reply]

one tricky thing about transactions as a custom adapter feature is that code that intends to support multiple adapters (e.g. Hyrax) won't be able to use them without some top-level, guaranteed transaction semantics.

even if those semantics are something like #supports_transactions?, that would give engines and gem code a way to dispatch calls appropriately.

better might be a generic interface with a null implementation. e.g. a #transaction(&block) that just executes the given block unmodified if transactions are unsupported. in this case it's important that there be some way for code to determine, at runtime, what transaction semantics are being applied (#atomic?, etc...). doing this really well would admittedly take significant consideration, but even a minimal implementation would do a lot to make code using transactions portable.

[botimer]

No matter what we come up with, we should study how Spring handles transactions. It has been hard-earned ground over 15 years. Obviously, the XML mappings and annotations have limited application here, but the semantics for all of the tricky cases are thoroughly developed and well documented.

https://docs.spring.io/spring-framework/docs/current/spring-framework-reference/data-access.html#transaction