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

Refactors /dois faceting and adds support for on-demand facets #1299

Merged
merged 4 commits into from
Dec 12, 2024

Conversation

codycooperross
Copy link
Contributor

@codycooperross codycooperross commented Dec 6, 2024

Purpose

Refactors approach to OpenSearch aggregations and serialized faceting at /dois to reduce calls to OpenSearch for unused facets. Implements new facets= URL param that allows the requestor to select from the possible facets.

closes: https://github.com/datacite/product-backlog/issues/127 #1301

Approach

Builds the aggregations based on the requested facets, default or otherwise, before assembling the OpenSearch query. Accepts comma-separated values via a new facets= URL param that selects from the possible facets. Facets are serialized based on supported and available aggregations in the OpenSearch response.

Open Questions and Pre-Merge TODOs

Learning

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

  • New feature (non-breaking change which adds functionality)

  • Breaking change (fix or feature that would cause existing functionality to change)

Reviewer, please remember our guidelines:

  • Be humble in the language and feedback you give, ask don't tell.
  • Consider using positive language as opposed to neutral when offering feedback. This is to avoid the negative bias that can occur with neutral language appearing negative.
  • Offer suggestions on how to improve code e.g. simplification or expanding clarity.
  • Ensure you give reasons for the changes you are proposing.

_key: "desc",
},
min_doc_count: 1,
def self.all_doi_aggregations
Copy link
Contributor

Choose a reason for hiding this comment

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

At this point it could be a constant.

AGGREGATION_DEFINITIONS = {...
...
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jrhoads Do you have thoughts on where the constant should be defined?

Copy link
Contributor

Choose a reason for hiding this comment

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

It can be a constant within the model.

def self.query_aggregations(disable_facets: false, facets: nil)
return {} if disable_facets

requested_facets = facets.respond_to?(:split) ? facets.split(",").map(&:strip).map(&:underscore).map(&:to_sym).uniq : default_doi_query_facets
Copy link
Contributor

Choose a reason for hiding this comment

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

This assumes that facets is only allowed to be a string. It could be useful to allow arrays of symbols to be passed in. And treat the string as a special case.

return {} if disable_facets

requested_facets = facets.respond_to?(:split) ? facets.split(",").map(&:strip).map(&:underscore).map(&:to_sym).uniq : default_doi_query_facets
requested_facets.index_with { |facet| all_doi_aggregations.dig(facet) }.compact
Copy link
Contributor

@jrhoads jrhoads Dec 11, 2024

Choose a reason for hiding this comment

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

I think this is similar to the slice method, available on hashes.
so this would become
all_doi_aggregations.slice(*requested_facets)
or
AGGREGATION_DEFINITIONS .slice(*requested_facets) if using the constant

keys that are not in the original hash are discarded.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is great work. Some time in the future we may want to refactor this out in to some sort of DoiAggregationSerializer.

…_aggregations to be callable with an array of symbols
@codycooperross codycooperross merged commit 5ad6dfb into master Dec 12, 2024
19 checks passed
@codycooperross codycooperross deleted the customized-facets branch December 12, 2024 16:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants