Module architecture used incorrectly

[atz]

This is symptomatic of all the Hydra ruby stack, but we need to stop abusing the module pattern. That means more classes and more OO, less chains of include.

A module is designed to be a set of methods that get mixed-in to a consuming class without knowing anything about that class.

Ways to recognize a badly formed module:

• Is only included once anywhere. Premature optimization: use concerning or similar for structure.
• Makes assumptions about consumers: contains calls to things not guaranteed to exist in an otherwise empty class.
• Directly defines Class-level variables: That is the domain of the class.
• Directly defines Instance-level variables: Again, the domain of the class.
• Untraceable logic due to dozens of modules-including-modules.

I realize rails is all-in on module magic, adding capabilities directly to Module to do things like define class-and-instance level accessors. These are also terrible for our purposes.

[mjgiarlo]

@atz does it make more sense to start working on this from AF up, or not really? I'm wondering what we can do to put this in action. Do we need a more detailed analysis that can, e.g., list a top ten or twenty modules to concentrate work ("bang-for-buck" analysis)? I want folks to take this on, and I'm not sure we have a shared sense of what done looks like yet. Thanks for writing this up and starting to list characteristics of antipatterns for modules!

[atz]

First, I just want to stop propagating a broken pattern where development is happening, i.e. here. Yes, the task is more difficult because the patterns seem to be already set by dependencies, but it is also more necessary. If you want me to file similar tickets on core gems, I can do that, but pretty much everything can buck-pass to something underneath it. Rails, devise, blacklight, BL access controls, Hydra access controls, certainly ActiveFedora, etc. At some point I have to accept those as problematic and just work on the layer that I'm working on.

IMO, it would be counterproductive to say "we'll only fix it here when we get new major versions from AF, BL, etc.". Working in parallel is fine, but with a stack this deep, I doubt I will still be working on this project by the time changes bubbled up from the bottom, by which point a whole lot more Hyrax and Hyku code will have been written, likely abusing modules as before if nothing prevents it.

[mjgiarlo]

@atz Yes, I'm inclined as you are. My primary concern here is that we make this actionable. It doesn't much matter to me whether we take a top-down or bottom-up approach.

[tpendragon]

I'm largely voting for top-down. I have questions about the architectural future of AF, and Hyrax has plenty of examples where we could just be okay with large classes (and refactor with delegation).

[jcoyne]

I don't think this belongs as a ticket as it's never likely to be closed. Yes, we all know this isn't great situation. However, writing a ticket about it, isn't going to change anything because nobody knows where to get started. If you want to make progress on this, we ought to open a ticket for each class where this is a problem and then figure out which ones are the priority to fix. As this code is already working, this is just going to be "technical debt" and will never receive a very high priority.

[atz]

"Working" as in "failing creatively", being untestable and causing us hundreds of hours of debugging cost later. And since it is an established "working pattern", it is repeated. It already metastasized across every level of our stack: that's how we got here. I was hoping for a declaration of "That ends here".

Starting from top/bottom doesn't matter to me. I think it is easier to start at the top, and anywhere we are forced down that path because of a dependency, root down and eradicate it in the dependency. Try to keep one thing clean.

[jcoyne]

failing creatively

I don't understand what this means. Can you provide some concrete examples.

, it is repeated. It already metastasized across every level of our stack: that's how we got here. I was hoping for a declaration of "That ends here".

The problem is that this pattern is in Rails itself, we aren't going to be able to excise that: https://github.com/rails/rails/blob/master/activerecord/lib/active_record/base.rb#L291-L323

So no matter how much effort we throw at this, we'll never be successful at completely removing this pattern.

[atz]

@jcoyne ActiveRecord:Base shows the use of modules (one might argue, overuse), but it is NOT what I detailed above. As I've said in a similar conversation, if a rails Module made assumptions about its includers, I expect the rails maintainers would regard it as a bug. They simply are not doing what we are.

As for failing creatively, look at the way SearchBuilders are built on top of Hydra::Access::Controls and a bunch of Blacklight modules. Now, if we were to try to use some of those modules without the others (and we did), as should be possible, if they are modular, we find all manner of assumptions being made by the module about what exists at the time its code is executing (or even being included!). Also, any point that a module overrides another module's method (and therefore, the order of inclusion is semantically important) is a tricky point of intersection. It would be better for the "outer" module to inherit from the other and have consumers replace using one with the other. Our chain-of-engines architecture works against us there, making it harder to swap dependencies because of generators within generators and tight coupling at lower levels.

We have comparable problems with intermingled dependencies and execution-time assumptions in dor-services concerns.

[atz]

Here's Blacklight::Solr::SearchBuilderBehavior, so a basic example is:

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.

And even if I add default_processor_chain= to survive include-time, later NINE different methods expect to have runtime access to blacklight_config. Is that part of ActiveSupport::Concern? No? Why does SearchBuilderBehavior have any right to expect that it exists then? You can see the difference between this and ActiveRecord:Base, right?

[jcoyne]

In ActiveRecord::Base assumptions about it's includers all over the place: https://github.com/rails/rails/blob/master/activerecord/lib/active_record/callbacks.rb#L329-L345 (calls super) https://github.com/rails/rails/blob/master/activerecord/lib/active_record/persistence.rb#L68 (calls attributes_builder, defined here: https://github.com/rails/rails/blob/e8f170cec11873bfaab68d8b24737adb7b9331c6/activerecord/lib/active_record/model_schema.rb#L330) included here: https://github.com/rails/rails/blob/29b3b5dd8eeb5d202bbd6987535539e1f26ae8b0/activerecord/lib/active_record/base.rb#L294

I think you'd be hard pressed to use a Module include without making assumptions about the includer.

Now I'm not suggesting this is good, or easy to reason about, it isn't. However the code does work right now. The magnitude of a potential fix is huge. Can we even enumerate the number of occurrences we need to fix? This is just not something we have the will or resources to tackle now. The best we can do is to stop perpetuating the pattern (as you suggest) and fix it one occurrence at a time. My prime objection here is not that we shouldn't do this, but that this is not something for a ticket. A ticket should be actionable. We should know when a ticket is done and be able to close it. This ticket isn't written that way. This is written as a style document or something. That's fine, but lets not put it in a ticket.

[atz]

We have epic tickets. If this is an epic ticket, then we can tag it as such.

[jcoyne]

@atz can you please open a ticket about the issue with:

SearchBuilders are built on top of Hydra::Access::Controls

I'd like to explore that more.

[jcoyne]

@atz an epic ticket should have actionable sub tickets. Even an epic ticket should have completion criteria.

[atz]

@jcoyne, without intending to belabor the point, I think your reading of the examples you cite is inaccurate:

• ActiveRecord::Callbacks does not call super, it passes a block containing super to the _run_*_callbacks methods.
• attributes_builder is called off an object (klass.attributes_builder), so is not relevant to whether the module expects methods in its own namespace.

[atz]

See related: https://github.com/samvera/hydra-head/issues/371

[no-reply]

This is true, and something we will work on gradually, but not an actionable ticket. I'm closing it for that reason.