What do we do when a project wants a query that's specialized?

[tpendragon]

The scenario:

I have a special query which is necessary for my repository to work, but Valkyrie doesn't currently provide it. IE, "Find everything which is in the collection Y" or "Everything with the checksum Z"

Possible solutions:

1. Add a variable and more complicated query interface to Valkyrie, to likely support these use cases.
2. Decide on a pattern to enable customizing the query service.
   a. Maybe this is just a recommendation to write their own query service if they need new queries, and know if they switch they'll have to solve that same interface?
   b. Maybe this is a way to register an alternative query service in an adapter, to fall back to if the base adapter doesn't support it?
   c. For either A or B we'd have to formalize the ResourceFactory pattern again.
3. Add every possible query (IE find_by_checksum and find_by_collection_uri) to Valkyrie as it comes up.

[tpendragon]

Possible query extension patterns:

Build a new extend_with feature to append new methods.

      class QueryExtension
        def self.queries
          [:fake_find_by]
        end

        attr_reader :parent_query_service, :resource_factory
        def initialize(resource_factory:, parent_query_service:)
          @parent_query_service = parent_query_service
          @resource_factory = resource_factory
        end

        def fake_find_by(nothing:)
          1
        end
      end

      query_service.extend_with(QueryExtension)
      expect(query_service.fake_find_by(nothing: 1)).to eq 1
    end

Extend with a module

      module QueryExtension
        def fake_find_by_2(nothing:)
          1
        end
      end
      query_service.class.include QueryExtension
      expect(query_service.fake_find_by_2(nothing: 1)).to eq 1

Sub-class

      class NewQueryService < Valkyrie::Persistence::Memory::QueryService
        def fake_find_by_3(nothing:)
          1
        end
      end
      new_query_service = NewQueryService.new(adapter: query_service.adapter)
      expect(new_query_service.fake_find_by_3(nothing: 1)).to eq 1
    end

[tpendragon]

What I want to avoid is building out a feature that's basically just slower funnier-looking monkey-patching. Part of me likes option 1, but I can't put a finger on what benefit it gives really - other than some level of improved testability, I guess? But the sub-class option offers that too.

[escowles]

Subclassing seems like the most straightforward way to do this to me — and would make it easier for overriding the default query implementations if you wanted to for some reason.

[tpendragon]

Subclassing seems fine, but its a shame you have to sub class two things and keep an eye on them (both the adapter and query service)

[botimer]

I'm thinking it would be really beneficial to not extend the interface of the query service with application-specific methods. An alternative here is to allow a "special queries" object/factory to be registered with the query service -- I'll call this app_queries for discussion.

The advantage, in terms of maintenance, is that it is very explicit which application code uses the Valkyrie query service and which uses the "homegrown" extensions. If the "registration" provides a way for app_queries to invoke the underlying query service, there would be a convenient place for transformations or filters. I see overriding a standard query as a different kind of concern -- buyer-beware kind of stuff.

So in pseudo-code:

# somewhere in your app's lib/
class AppQueryFactory
  def instance(query_service)
    CrazyAppQueries.new(query_service)
  end
end

class CrazyAppQueries
  attr_reader :query_service
  def initialize(query_service)
    @query_service = query_service
  end

  def find_cool_things
    ids = query_service.some_standard_query.map {|item| item.id }
    query_service.connection.raw_query "some crazy native query (?)", ids
  end

  def find_something_from_totally_different_backend
    other_backend.query "something totally different"
  end
end

# some config
Valkyrie::QueryService.register_app_extensions(AppQueryFactory.new)

# some use case / service object
class AwesomeUseCase
  def do_cool_stuff
    do_something_awesome_with query_service.app_queries.find_cool_things
  end
end

[tpendragon]

@botimer I think that's the same thing as option #1 above, especially since

Valkyrie::QueryService.register_app_extensions(AppQueryFactory.new)

would have to be changed to be specific to a single adapter (unlikely extensions will be adapter agnostic.)

It's nice that it doesn't allow overriding queries, but again - it's a Valkyrie specific solution for something ruby can just solve, and I want to make sure it's worth it. I really do prefer it, but it's more of a gut feeling that I can't really pin down with words, which seems like a problem.

[botimer]

@tpendragon Hmmm.. I guess I read option 1 as very different... as in the semantics/syntax of the underlying tech poking through a generic interface. Something like .query being able to take a string. That's the kind of "I dunno, just smash a string in here" interface that scares me -- the stuff that enables the pathological "build a solr predicate in javascript and pass it to a controller" kind of thing.

I saw my example as closer to 2a/b. I still do see it as different, though. The main issue in my mind is: is it clear from the calling code context whether something is core or an extension? If it is not (as by grep/static analysis), it takes serious forensics to track down the customizations. That feels like a creepy thing to do in a project geared around clearer architecture and a strictly defined API you can count on.

Also, I agree that they are almost certain to be adapter-specific (a bug in the pseudocode). On the other hand, if you were, implementing a new query for all of the stock backends, you could have the component consult lower-level config, or just require that the user add a bit of adapter-specific config to match their infrastructure. I think that'd be an edge case -- someone willing to do that much work would probably be in touch and it would be worth considering adding to the core interface (option 3, possibly in a "contrib/javax/labs/probationary" area).

In general, I think it is pretty expected that someone doing customization like this will be doing it for their infrastructure -- they have already chosen, so putting that choice into some config is not a problem.

[tpendragon]

@botimer I think the big difference between my #1 and your suggestion is that yours sits behind another background collaborating object - which I don't think is a bad idea (although, it's going to violate Demeter all over the place.)

Here's an example of what the implementation would look like:

https://github.com/samvera-labs/valkyrie/compare/extendable_query_services

[tpendragon]

What I like most about this pattern is we can pass these query customizations around between us and not have to deal with any inheritance hurdles.

[tpendragon]

If we use @botimer's option, with the above branch, #find_by_depositor looks like this:

dda730b

I like that pretty okay. The only thing that bothers me is that registration has to happen in that initializer, or it doesn't seem to trigger.

[botimer]

Yeah, I don't mind the Demeter violations here -- the return on clarity and maintenance effort seems worth it.

The "my base query service has different behavior than yours" is almost certain to cause a lot of confusion across apps and time, whereas clearly having a cave means we can say "there be dragons". I think I prefer that.

An implementation-level thing, the method_missing approach kinda-sorta feels good, but it also feels a little weird. I suppose there is a temptation to plan for a union of custom additions -- not sure whether to push back on that or not. It's really the same concern, one level deeper. Do you locate an object to call a known method on or send a message to a broker who passes it on to a collaborator?

As long as there aren't runtime extensions to the base API we've been trying to define clearly, I think these are sort of 50-50 trades "in the cave".