Revert "Extract FacetSearchBuilder"

[jrochkind]

This would revert #3597

PR created for discussion/demo, to see if it still reverts fairly cleanly and green CI. it should probably not be merged without disucssoin with original author and reviewer (@jcoyne and @barmintor).

My argument:

SearchBuilder is generated into local application becuase it's an intended site of local customization, a "seam". Historically, it's where many plugins that want to interact with the search process have injected themselves.

With the split to a separate FacetSearchBuilder -- any customization in SearchBuilder (or many, and any I can think of?) used in a given controller, needs to be duplicated in the corresponding FacetSearchBuilder on that same controller. If it is not, when you press 'more' to get the 'more facets' dialog/page will display the wrong facets, not those corresponding to the search you just came from. (This was pointed out by @cbeer on original PR too in June).

So this is a very jagged edge, waiting to trip people up. Any plugins that are used to injecting into SearchBuilder I guess have to inject into both places, or you'll get a potentially hard-to-notice bug in the facets displayed in more-facets.

In real world apps making extensive use of this (intended, provided) seam for local customization, it can be pretty cumbersome. In my BL8 app, I have a:

• SearchBuilder (with customization)
• CollectionSearchBuilder < SearchBuilder. (same app-wide customization from search builder, plus more)
• FeaturedTopicSearchBuilder < SearchBuilder (similar)

In the move to a BL9 with the current "Extract FacetSearchBuilder' architecture, if I want to keep it DRY between the places that need to match to avoid faulty behavior, I need to introduce a bunch of modules, as well as all the paired search builder classes.

• LocalSearchBuilderBehavior (concern)
• SearchBuilder (include LocalSearchBuilderBehavior)
• FacetSearchBuilder (include LocalSearchBuilderBehavior)

_

• LocalCollectionSearchBuilderBehavior (concern)
• CollectionSearchBuilder (include LocalCollectionSearchBuilderBehavior) < SearchBuilder
• CollectionFacetSearchBuilder (include LocalCollectionSearchBuilderBehavior) < FacetSearchBuilder

_

• LocalFeaturedTopicSearchBuilderBehavior (concern)
• FeaturedTopicSearchBuilder (include LocalFeaturedTopicSearchBuilderBehavior) < SearchBuilder
• FeaturedTopicFacetSearchBuilder (include LocalFeaturedTopicSearchBuilderBehavior) < FacetSearchBuilder

i've gone from three local classes/modules to 9, with a much more confusing relationship between them. And every place I set (eg) blacklight_config.search_builder_class = CollectionSearchBuilder, I need to make sure to also set blacklight_config.facet_search_builder_class = CollectionFacetSearchBuilder -- or I'll have faulty behavior. For each pair.

I think I get (more or less) the philosophical issue with single-responsibility principle here... but prinicples like that are meant to serve the cleanliness and maintainability of our code, right?

I'd like to suggest reverting this for now, until/unless the existing architecture is causing real pain and/or a solution can be found to "SRP" that provides a better developer DX, without the jagged edges and gotchas and need for boilerplate code.

[jrochkind]

@jcoyne writes:

We needed it to be separate because we only wanted to customize the facet search behavior. https://github.com/sul-dlss/SearchWorks/blob/1d08a420685ac91590022ae7d4901d8ac62a8df1/app/models/custom_facet_search_builder.rb#L4

[jrochkind]

For discussion, @cbeer @jcoyne @barmintor @seanaery , et al?

Can anyone think of a solution that meets all needs? The current one seems to me to add extraordinary complexity and gotcha for what was meant to be a recommended place of customization in Blacklight -- logic in the local SearchBuilder.

[jrochkind]

Just for discussion/consideration/exploration on @jcoyne's use case:

https://github.com/sul-dlss/SearchWorks/blob/1d08a420685ac91590022ae7d4901d8ac62a8df1/app/models/custom_facet_search_builder.rb#L4

One way, I think, to do this in "old" architecture (without extracted SearchBuilder) would be, in your custom SearchBuilder:

  self.default_processor_chain += [:force_facet_method_for_searching]
  
  # We use facet.method=uif for normal faceting (presumably because it improves performance or
  # resource usage), but uif isn't compatible with facet.contains based searching.
  def force_facet_method_for_searching(solr_params)  
    return if facet.blank? || facet_suggestion_query.blank?

    facet_config = blacklight_config.facet_fields[facet]
    solr_params[:"f.#{facet_config.field}.facet.method"] = 'fc'
  end

Only actual difference to local code (other than location) is change of first line of method from return if facet.blank? to return if facet.blank? || facet_suggestion_query.blank?. (and actually you may not need that, the existing facet.blank? might be enough? And not technically required in the FacetSearchBuilder branch?)

Now, I totally agree that this does feel un-tidy and mixed up -- it doesn't feel great that we're having to mix methods only used for facet (suggestion) use case in with methods used for any searching; it doesn't feel great to have to duplicate that magic if facet.blank? || facet_suggestion_query.blank? code (for that latter, we could just add a method facet_suggestion_happening? or something.

On the other hand, the actual complexity/maintenance cost, I would suggest, is less than what the "extract FacetSearchBuilder" refactor is requiring of many use cases, as in my description, which also feels very not right and not good DX/maintainability. This one (without refacotr) feels dirty, but it's actually not that big a deal or that big of a maintainance problem, I submit it's not unworkable?

It's possible with more time a clever architecture could be devised that keeps all of the cases tidy and maintainable -- at the cost of some architectural complexity maybe (I guess the FacetSearchBuilder has to somehow use the base SearchBuilder?). But for now, I suggest a revert may be acceptable before 9.0.0 final, without requiring too much violence to existing 9.0.0.beta code (which was still beta after all).

[barmintor]

I am against reverting #3597 based solely on this report - introducing a service that searches for facets as entities rather than documents is an appropriate change for a major revision, and it seems like this kind of change would, for example, facilitate making the range limit plugin's controller and builder behaviors less complicated.

I would prefer to document the problem you are having and try to minimize upgrade work than to undo that work and punt to BL10. Can you link the app you are trying to upgrade so that we can look at the search builder hierarchy?

[jrochkind]

@jcoyne writes:

Okay, I think I have a concrete example of this written up: sul-dlss/SearchWorks#6279

I think that demo's the need to copy many/most things (anything that effects fq or q) from a SearchBuilder to FacetSearchBuilder, in order to avoid faulty logic?

As well as the "gotcha" where it's easy to miss, as it was here. It is not obvious that any customization to SearchBuilder that touches fq or q needs to be copied to a parallel FacetSearchBuilder, which needs to be configured next to any SearchBuilder, or you will get faulty logic.

For instance anyone upgrading to BL9 would have to know they have to make new FacetSearchBuilders and copy logic to them and configure them, to avoid faulty logic as above. Also any plugin which injects behavior into a local SearchBuilder needs to be rewritten to do equivalent behavior in a matched FacetSearchBuilder.

While some kind of refactor may benefit in the ways you say @barmintor without this flaw, what if we revert for now for 9.0.0 until such a future refactor is devised? I think releasing 9.0.0 like this is may lead to lots of apps without faulty behavior, as in SearchWorks above.

[jrochkind]

@barmintor Here are the SearchBuilders in my app:

SearchBuilder

WithinCollectionSearchBuilder (supports per-collection landing pages that let you search within a collection)

WithinFeaturedTopicSearchBuilder (supports landing pages that let you search within constructed "featured topics", defined by a list of subject headings, inclusive).

In the current architecture, I don't think there's any way around each one needing a paired FacetSearchBuilder, with identical logic, which is always configured next to it?

[ebenenglish]

We're not on BL9 yet at my institution, and I don't have a BL9 test app spun up to experiment with, so apologies if any of this seems uninformed.

I agree with @jrochkind that this is a very disruptive change, and the default behavior that it creates would be seen by most users as a bug. @jcoyne's writeup here (sul-dlss/SearchWorks#6279) is a very clear demonstration of the problem.

This will also likely trip up a lot of folks during the upgrade process.

I also have a lot of custom SearchBuilder classes in my app, and the idea of having to create an accompanying FacetSearchBuilder for all of them sounds painful.

I'm wondering if the goal that #3597 was intended to address (allowing customization of facet "more" modal behavior) is a widely shared need that outweighs the likely disruption. I don't see any issues linked from the PR, so I'm not sure.

@barmintor says, "It did/does seem to me to simplify things for apps that do a lot of customization around facet searching" in his comment here (#3597 (comment)) but how many of those apps are there? The evident need for this change is not clear to me.

I understand that this a major version change and there are going to be bumps, but it seems like this change was not fully thought out before being merged, since we are seeing even major Blacklight committers being caught off guard.

On the topic of mitigating the impact (and this is where my lack of BL9 familiarity comes into play), would it be possible to alleviate the need for a "paired" SearchBuilder/FacetSearchBuilder via the blacklight_config by using a single SearchBuilder for both, as in:

blacklight_config.search_builder_class = MyCustomSearchBuilder
blacklight_config.facet_search_builder_class = MyCustomSearchBuilder

And then in MyCustomSearchBuilder. making sure that the Blacklight::Solr::FacetSearchBuilderBehavior module is included? As in:

class MyCustomSearchBuilder < Blacklight::SearchBuilder
  include Blacklight::Solr::FacetSearchBuilderBehavior
  
  # my custom `self.default_processor_chain` stuff
end

Though I can see that won't work out of the box, since there are some methods in Blacklight::FacetSearchBuilder that would not get included. Could the two methods from Blacklight::FacetSearchBuilder could be moved into Blacklight::Solr::FacetSearchBuilderBehavior perhaps?

Thanks!

[jcoyne]

I just want to add context, that this was added to support ElasticSearch, which could potentially allow us to do hybrid searching with embeddings. The FacetSearchBuilder part of this is something that I don't believe ElasticSearch can support, so it makes sense to have one module that can be turned off when not using solr.

[barmintor]

(This was pointed out by cbeer on original PR too in June)

Chris made his observation in July, after the change was merged.

I understand that this a major version change and there are going to be bumps, but it seems like this change was not fully thought out before being merged, since we are seeing even major Blacklight committers being caught off guard.

A major Blacklight committer opened #3597, and it took 40 33 days for another committer to review the change. It was merged almost 5 months ago.

[barmintor]

@barmintor Here are the SearchBuilders in my app:

SearchBuilder

WithinCollectionSearchBuilder (supports per-collection landing pages that let you search within a collection)

WithinFeaturedTopicSearchBuilder (supports landing pages that let you search within constructed "featured topics", defined by a list of subject headings, inclusive).

In the current architecture, I don't think there's any way around each one needing a paired FacetSearchBuilder, with identical logic, which is always configured next to it?

As I mentioned earlier, I would have handled this situation by having the relevant controllers configure the extra step that controller requires in the processing chain. Handling it via subclasses is not illegitimate - There's More Than One Way To Do It - but the maintainers need to decide in this case what type of design they want to optimize for. Searching within facets is a frequently requested feature here, so the design direction of #3597 was valuable to me. We also don't subclass SearchBuilder. And the general direction of supporting ElasticSearch is very valuable to me, as I think the vector support is much better (as is the migration path to OpenSearch).

[barmintor]

I am completely on-board with refining the merged work to facilitate migration. I am suspicious of reverting a change on a major revision because it is a breaking change for some implementation patterns. Maybe that's the right thing to do, but I'm hearing more cause for refinement based on a desired API that has not been articulated. The requirement shouldn't be "no change for a v8 app" unless this is a feature release on 8.x.

[jrochkind]

The FacetSearchBuilder part of this is something that I don't believe ElasticSearch can support, so it makes sense to have one module that can be turned off when not using solr.

If understand right, turning off "more facet" display seems like something that can be implemented by config without this doubling of the SearchBuilderClass, config that various parts of BL are going to have to consult anyway to not display the various UI portions. (I guess under the "extract" refactor, the config to be checked is facet_search_builder == nil? It does not seem to add that much complexity to check show_facet_list == false instead?)

I am feeling frustrated becuase I don't feel like we've been shown any concrete situations that are made actually more feasible/maintainable with the extraction. I think all the things can be very easily and concisely and maintainably done with just the SearchBuilder, if in a way that is philosophically or aesthetically pleasing.

And this is against actual jagged edge "gotchas" for developers in actual use cases (leading to bugs only discovered in this discussion process, made accidentally and undiscovered even by expert BL developers), and actual increased maintenance labor and code complexity for actually existing BL apps. I don't understand why a sense of aesthetics of philosophy would beat actual increased challenge for local BL installations.

Does the community have a process for resolving disagreements like this at which consensus seems unlikely to be reached?

[jrochkind]

I also find it quite rude that, while I intentionally set this to "draft" knowing it should not simply be merged with one approver when I know it is a matter of debate -- others have proceeded to merge PR's on top that are dependent on this decision, with one approver, making this PR now in conflict when it merged simply when I made it.

[barmintor]

@jcoyne @body-clock #3766 undoes one of the mechanisms that I am arguing makes #3597 acceptable, I agree that we should revert that change while this PR is under discussion.

[ebenenglish]

I think it's worth noting that subclassing SearchBuilder has been a pattern described in the Blacklight wiki for quite some time:
https://github.com/projectblacklight/blacklight/wiki/Extending-or-Modifying-Blacklight-Search-Behavior#extending-the-searchbuilder

Whether people find it elegant or not, it's likely to have been used by many, many implementations at this point. I personally find this pattern very straightforward to use, and it gets the job done (setting Solr params for specific controllers/views) with a minimal amount of code. Or maybe I'm just used to it.

What would be helpful to me in this discussion is an idea of whether there are some changes that could make this less painful, which I think is what @barmintor means by "refining the merged work to facilitate migration"?

I'm totally on board with supporting ElasticSearch, are there other potential ways to do this that are less disruptive? Are suggestions like the one @jrochkind makes here #3762 (comment) feasible?

[jrochkind]

For having some concrete code for discussion, this is what I did to get my app green for BL9:

sciencehistory/scihist_digicoll#3182

it was kind of confusing juggling all these classes when doing this, I had bad memories come back of early 2000s Java development and FactoryFactoryFactories. I am not certain about all the naming/namespaceing choices I made, this gave me a lot more.

It does work, so certainly my app has a way forward if necessary.

I feel like it imposes an ongoing maintenance and complexity burden on my app though. I prioritize trying to leave all code in my local app in place that a future replacement, imagining an "advanced junior/intermediate Rails developer", wouldn't have too much trouble wrapping their head around the code, but I don't feel good about this stuff.

I am not following what code would become complex without the extraction refactor, that couldn't be handled by having a single SearchBuilder with "chain" items that are just disabled except when doing a "facet search". But I'm interested in seeing/considering the ones that exist that I am not currently following!

[barmintor]

I think it's worth noting that subclassing SearchBuilder has been a pattern described in the Blacklight wiki for quite some time:
https://github.com/projectblacklight/blacklight/wiki/Extending-or-Modifying-Blacklight-Search-Behavior#extending-the-searchbuilder

@ebenenglish that documentation (introduced in 2015 during the v6.0.0.pre3 cycle and last edited in 2021 during the v7.21.0 cycle) doesn't describe subclassing the generated SearchBuilder, it describes adding methods to the generated SearchBuilder - the amount of overhead in migrating such an app vs one that has a number of SearchBuilder subclasses is one of the issues here.

Subclassing the generated SearchBuilder might be inferred to be a customization strategy from the presence the configuration property for search_builder_class, but that attribute was introduced for v5.10.0 in this commit so that different repository implementations could have appropriate mappings of BL query params to the backend query, whatever it may be, as indicated by the documenting comment in Blacklight::Configuration "class for converting Blacklight parameters to request parameters for the repository_class".

[ebenenglish]

@ebenenglish that documentation (introduced in 2015 during the v6.0.0.pre3 cycle and last edited in 2021 during the v7.21.0 cycle) doesn't describe subclassing the generated SearchBuilder, it describes adding methods to the generated SearchBuilder - the amount of overhead in migrating such an app vs one that has a number of SearchBuilder subclasses is one of the issues here.

I take your point. I should have been more specific: subclassing Blacklight::SearchBuilder (which is what the generated app/models/search_builder.rb does) seemed like an acceptable customization strategy based on the docs.

In my app I have a bunch of different classes that subclass Blacklight::SearchBuilder, but I define all of the methods used in my *SearchBuilder classes in a module that can be mixed in. Here's an example of the module, as mixed in to Blacklight::SearchBuilder subclass.

I think this strategy could ease the pain of migration -- an implementation could keep things reasonably DRY even if there need to be paired SearchBuilder/FacetSearchBuilder classes? Some code, like the default_processor_chain would need to be repeated, I guess.

[body-clock]

It might also be helpful not to mark MRs "ready for review" when they are dependent on other MRs that haven't been approved yet. Perhaps there's another status that's appropriate for that case, such as "on hold" or something like that.

When I see an MR that says "remove unused code" I assume that it is, in fact, unused - and that makes it a trivial approval. I will admit, though, that I should take more time to understand what code exactly is being removed, and how that impacts the rest of the BL ecosystem outside of core.

[seanaery]

I don't have a strong opinion about how best to resolve this situation, but I will say I think @jrochkind & @ebenenglish raised some valid concerns about the added maintainability/complexity that will be felt downstream by some implementers when SearchBuilder and FacetSearchBuilder are separate, parallel classes.

From my own institution's perspective, we have a mix of BL7/BL8 (no 9 yet), and we have one active / to-be-upgraded Blacklight application (out of four) that uses the strategy of subclassing SearchBuilder in order to add/subtract filters from the default_processor_chain in certain contexts. It's our digital repository application (code / UI).

If indeed the community decides to proceed with the current code (with a FacetSearchBuilder in the mix), I'm OK with the refactoring this will require when we upgrade to BL9, but the steps should be made very clear in the https://github.com/projectblacklight/blacklight/wiki/Upgrading-to-Blacklight-9 page and release notes.

I have checked the ArcLight engine; it doesn't subclass SearchBuilder itself (nor does our local arclight app). ArcLight has a few other minor revisions needed in order to work correctly on the latest BL9 beta, but those are unrelated to SearchBuilder changing.

[seanaery]

@jrochkind After further discussion at the 11/19/2025 Blacklight Community meeting, attendees' decision was to proceed with reverting this FacetSearchBuilder extraction for now as you've proposed. This will help facilitate releasing BL9 very soon, while giving more time for community consideration about optimal post-9.0 seams for features like Elasticsearch support.

If you resolve the merge conflicts on this PR and mark it ready for review we'll proceed with the revert...

[jrochkind]

Hi I've merged main in to resolve conflicts -- I think all the conflicts were structural or not relevant with a revert, but happy to have someone check my work.

(Rebasing got more complicated than I wanted, the merge was easier).

I'm not sure what's going on with CI. it seems to be not actually running the rspec tests at present?

https://github.com/projectblacklight/blacklight/actions/runs/19513636221

I think the github workflow file, not touched in this file, has an erorr in it? Github workflow may be broken in main? If someone wants to fix it, I can merge in again? Or anyone know what's up?

I ran CI locally but just on default build, not multiple jobs for multiple rubies/config; it passed.

[seanaery]

Thanks; merging #3779 will address the CI issues here...

[seanaery]

This looks right to me--approved. I took the liberty of removing "For discussion: " from the PR title.