Skip to content

Add controlled keywords to score set search filter options.#702

Open
EstelleDa wants to merge 8 commits intorelease-2026.1.3from
feature/estelle/693/addControlledKeywordsToSearchFilterOptions
Open

Add controlled keywords to score set search filter options.#702
EstelleDa wants to merge 8 commits intorelease-2026.1.3from
feature/estelle/693/addControlledKeywordsToSearchFilterOptions

Conversation

@EstelleDa
Copy link
Copy Markdown
Member

No description provided.

@EstelleDa EstelleDa requested a review from bencap April 10, 2026 07:42
@EstelleDa EstelleDa linked an issue Apr 10, 2026 that may be closed by this pull request
Copy link
Copy Markdown
Collaborator

@bencap bencap left a comment

Choose a reason for hiding this comment

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

Looks great, thanks Estelle.

Comment on lines +219 to +230
if search.controlled_keywords:
for label in search.controlled_keywords:
query = query.filter(
ScoreSet.experiment.has(
Experiment.keyword_objs.any(
ExperimentControlledKeywordAssociation.controlled_keyword.has(
ControlledKeyword.label == label
)
)
)
)
)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Hmm, actually one thing that I noticed based on the front end. If a user searches for 'Other' (or any value which is ambiguous between keyword categories), all results will show up regardless of the intended class being searched. We may need to reqork the way that the controlled keyword search works to accept both a value and a key, similar to how you return both a value and a key. This would let us more precisely match the users' search request.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Thanks for finding this! It's a serious problem.

journals: Optional[list[str]] = None
publication_identifiers: Optional[list[str]] = None
keywords: Optional[list[str]] = None
controlled_keywords: Optional[list[ControlledKeywordSearch]] = None
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for the fix and the new shape. My one suggestion is that API consumers who don't know about this new change are going to be confused with what we did if they try to search by the old keywords.

What I would suggest is adding a model validator and an issue to the backlog to remove it in 6 months or something:

    # TODO#XXX - Remove validator after consumers have had a chance to update
    @model_validator(mode="before")
    @classmethod
    def reject_deprecated_keywords(cls, data):
        if isinstance(data, dict) and ("keywords" in data or "Keywords" in data):
            raise ValueError(
                "'keywords' is no longer supported. Use 'controlled_keywords' with "
                "a list of {key, label} objects to filter by specific keyword groups."
            )
        return data

Now if a user tries to make a request and use the old keywords parameter, they get a really clear error about why it doesn't work and what they need to do to fix it, rather than just a generic error that keywords isn't a valid property.

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.

Add Controlled Keywords to Score Set Search Filter Options

2 participants