Architecture: modules not modular

[atz]

There are likely hundreds of examples, but we have a problem every time module code invokes a method (or attribute, class variable, etc.) that:

• it has no certainty even exists (i.e. nothing inside or included by the module defines it);
• does not check whether exists (i.e no fallback);
• does not validate the return value if, by luck, it does exist at runtime.

This demonstrates a failure to separate concerns. The module is not modular enough to be used by a thin wrapper class without throwing exceptions. This should be made obvious by basic tests for the modules that utilize such a thin wrapper class. In reality the modules are still married to conglomerate classes they originated from (worse still: other modules; worst: other modules that only meet the dependent code in some generated class/module).

Example

https://github.com/projectblacklight/blacklight/blob/master/lib/blacklight/solr/search_builder_behavior.rb#L168

  def add_sorting_to_solr(solr_parameters)
    solr_parameters[:sort] = sort unless sort.blank?
  end

In this case, the sort method is not defined. https://github.com/projectblacklight/blacklight/blob/v6.7.2/lib/blacklight/search_builder.rb#L224-L241

It lives in the Blacklight::SearchBuilder class that a generated SearchBuilder extends.

Actions

• Affirm that Blacklight intends to use ruby Modules conventionally
• Build basic tests w/ thin wrapper classes to find breakdowns, and
• Fix them

Alternatively...

• Declare Blacklight intends to use Modules unconventionally
• Documentation: make implicit requirements explicit, possibly adding include-time checks or fallbacks
• Live with it.

[jcoyne]

In https://github.com/projectblacklight/blacklight/blob/master/lib/blacklight/solr/search_builder_behavior.rb#L168 you are looking at the Solr specific implementations. It references sort, only within the abstract version here: https://github.com/projectblacklight/blacklight/blob/dee8d794125306ec8d4ab834a6a45bcf9671c791/lib/blacklight/search_builder.rb#L217 There is some guarantee that you will only mix one concrete version of SearchBuilderBehavior (either Solr, ElasticSearch, DPLA-api, etc) onto the SearchBuilder. This could be arranged differently, but we don't want the ::SearchBuilder to be aware of what implementation is being used. Any ideas?

[atz]

I understand the motivation. The code is still wrong, as far as modularity and conventionality goes.

The conventional approach would be to have each concrete module include the abstract one and the consumer still mix in just one of the concrete modules. Each concrete module is thereby guaranteed the abstract module's features (and can even override them). Right? This should make everything easier for the consumer to reason about and is no more burdensome than the current implementation.

[atz]

Example from: https://github.com/samvera-labs/hyrax/issues/799

Blacklight::Solr::SearchBuilderBehavior, basic example:

class MyClass
  include Blacklight::Solr::SearchBuilderBehavior
end

If you do that in console, you won't even get to do MyClass.new. Instead:

NoMethodError: undefined method `default_processor_chain=' for MyClass:Class

I get why, but this is an abuse of modules. This module would kill my app at load/autoload time. A module should not be able to crash the ruby process just by being included in an empty class.

And even if I add default_processor_chain= to survive include-time, later NINE different methods expect to have runtime access to blacklight_config. This is not modularity and not OO.

[jcoyne]

Blacklight::Solr::SearchBuilderBehavior is only intended to be included on an ancestor of Blacklight::SearchBuilder

[jcoyne]

This is the mechanism by which we isolate Solr behavior vs ES vs Europeana API calls.

[atz]

@jcoyne Right, that intent (or foreknowledge) is the anti-pattern. The module is still wedded to the class. I appreciate the contextualization, but I am not interpreting that as excusing the fundamental problem. There are OO patterns for handling this correctly.

To clarify: you say ancestor, but I think we're talking about descendants. (Obviously, we are not expecting to monkey-patch Object to include Blacklight::Solr::SearchBuilderBehavior, for example, because it would fail the same way.)

[barmintor]

Marking as 8.x as implicated refactoring is not in scope for 7.0 (and implies API issues).