-
Notifications
You must be signed in to change notification settings - Fork 124
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
Use actual solr field names instead of index_field_mapper.set_field #3831
Conversation
817ceaf
to
eb0c7ff
Compare
@@ -7,7 +7,7 @@ class AdminSetIndexer < ActiveFedora::IndexingService | |||
def generate_solr_document | |||
super.tap do |solr_doc| | |||
# Makes Admin Sets show under the "Admin Sets" tab | |||
Hyrax.config.index_field_mapper.set_field(solr_doc, 'generic_type', 'Admin Set', :facetable) | |||
solr_doc['generic_type_sim'] = ['Admin Set'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about setting these in a lookup table? Like Hyrax.config.solr_fields = { type: 'generic_type_sim' }
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe. I think you might start working your way back to something like solrizer, though, because there will often be more than one solr field for a given model property. I'm advocating to move toward "just let everyone use the solr field they want to use". The dynamic fields are pretty well documented in the solr config. And I think this is a direction the community has been more or less comfortable with for a while.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's walk through that "just let everyone use the solr field they want to use" scenario. What if I want to use primary_type_sim
instead of generic_type_sim
. How would I go about that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Write an indexer that adds the field you want. And then either stop using the one you don't want or have your indexer remove it I guess.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jcoyne are you trying to say that this field in particular is uniquely in need of being overridden downstream? Could you flesh out the use case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, so the plan is "write your own (or extend the default indexers)". I can live with that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 these are already hard coded in many places, including adjacent to changes in this diff. It seems fine to just normalize on "you customize index keys by changing the indexer to use different keys".
We're going to get something to that effect into the release notes.
@@ -7,7 +7,7 @@ class AdminSetIndexer < ActiveFedora::IndexingService | |||
def generate_solr_document | |||
super.tap do |solr_doc| | |||
# Makes Admin Sets show under the "Admin Sets" tab | |||
Hyrax.config.index_field_mapper.set_field(solr_doc, 'generic_type', 'Admin Set', :facetable) | |||
solr_doc['generic_type_sim'] = ['Admin Set'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 these are already hard coded in many places, including adjacent to changes in this diff. It seems fine to just normalize on "you customize index keys by changing the indexer to use different keys".
We're going to get something to that effect into the release notes.
advances #3794