Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

🐛 Restore qt: 'standard' to SolrService #6677

Merged
merged 4 commits into from
Feb 8, 2024
Merged

Conversation

jeremyf
Copy link
Contributor

@jeremyf jeremyf commented Feb 7, 2024

tl;dr Can anyone explain why we don't want qt: 'standard' for Valkyrie connections?

In ActiveFedora::SolrService we always assign the qt: 'standard'.1

However, with this old commit we dropped passing the qt: 'standard' when we had use_valkyrie of true.

We discovered this bug when upgrading Hyku and Bulkrax to Hyrax v3.x and some.

Closes #6676

Related to:

@samvera/hyrax-code-reviewers

@jeremyf jeremyf force-pushed the fix-hyrax-solr-service branch 2 times, most recently from 3462159 to 190550a Compare February 7, 2024 20:28
In `ActiveFedora::SolrService` we always assign the `qt: 'standard'`.[1]

However, with [this old commit][1] we dropped passing the `qt:
'standard'` when we had `use_valkyrie` of true.

We discovered this bug when upgrading Hyku and Bulkrax to Hyrax v3.x and
some.

Closes #6676

Related to:

- #3857

[1]:https://github.com/samvera/active_fedora/blob/8e7d365a087974b4ff9b9217f792c0c049789de6/lib/active_fedora/solr_service.rb#L40-L48
[2]:be6104f
@dlpierce
Copy link
Contributor

dlpierce commented Feb 7, 2024

Perhaps it was to allow the application more flexibility when making solr queries?
I realize it was many moons ago, but perhaps @hackartisan has some idea?

@jeremyf
Copy link
Contributor Author

jeremyf commented Feb 8, 2024

@dlpierce The main problem is that it means ActiveFedora::SolrService and Hyrax::SolrService are not interchangeable; which I assume will mean that even though the Hyrax::SolrService might fall back to the ActiveFedora::SolrService it won't be the same types of queries. Thus introducing breaks for those that are migrating their Hyrax application.

I amended the class to expose a configuration option. I did favor the configuration option that will not break existing implementations of Bulkrax and Hyku (e.g. by including qt: 'standard').

My assumption is that folks using valkyrie will likely have specified a qt: in their calls, and thus the merge will favor the explicit specification.

@jeremyf jeremyf requested a review from dlpierce February 8, 2024 15:04
Copy link
Contributor

@dlpierce dlpierce left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I love it!

@dlpierce dlpierce merged commit b09be88 into main Feb 8, 2024
6 checks passed
@dlpierce dlpierce deleted the fix-hyrax-solr-service branch February 8, 2024 16:19
@dlpierce dlpierce added the notes-minor Release Notes: Non-breaking features label Feb 8, 2024
@hackartisan
Copy link
Contributor

Yeah, sorry I have specific memory about that param!

jeremyf added a commit to scientist-softserv/iiif_print that referenced this pull request Feb 15, 2024
This relates to the fact that `Hyrax::SolrService` does not behave the
same way (for earlier versions of Hyrax).

Related to:

- samvera/hyrax#6677
jeremyf added a commit to scientist-softserv/iiif_print that referenced this pull request Feb 23, 2024
This relates to the fact that `Hyrax::SolrService` does not behave the
same way (for earlier versions of Hyrax).

Related to:

- samvera/hyrax#6677
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
notes-minor Release Notes: Non-breaking features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Discrepancy between Hyrax::SolrService#query and ActiveFedora::SolrService#query
3 participants