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

feat: handle multiple tags on datasets and topics apis #3204

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

abulte
Copy link
Contributor

@abulte abulte commented Nov 21, 2024

Multiple tags filter is now supported and datasets and topics list APIs.

The filter is a AND, i.e. ?tag=tag1&tag=tag2 will return objects that have both the tag1 and tag2 tags.

@abulte abulte changed the title feat: handle multiple tags on datasets api feat: handle multiple tags on datasets and topics apis Nov 21, 2024
@abulte abulte marked this pull request as ready for review November 21, 2024 16:37
@@ -73,7 +73,7 @@
log = logging.getLogger(__name__)

ns = apiv2.namespace("datasets", "Dataset related operations")
search_parser = DatasetSearch.as_request_parser()
search_parser = DatasetSearch.as_request_parser(store_missing=False)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We use store_missing=False because we don't want to send filters set to None to the search service (Mongo does not care).

Copy link
Contributor

@magopian magopian left a comment

Choose a reason for hiding this comment

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

Excellent work! Very well executed 🙏
I'm slightly confused as it seems we don't have the multi-tags implemented for the topics on the udata-search-service side, am I missing something?

try:
return search.query(Dataset, **multi_to_dict(request.args))
return search.query(Dataset, **args)
Copy link
Contributor

Choose a reason for hiding this comment

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

Makes more sense indeed.

Note for self: we have the same pattern elsewhere

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Those could be refactored when multi-tags is introduced elsewhere (except for Organizations that don't have tags AFAIK).

udata/search/query.py Show resolved Hide resolved
url = url + f"&{name}={value}"
return url

# FIXME: unused?
Copy link
Contributor

Choose a reason for hiding this comment

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

Note to self: check, and remove in another cleanup PR if indeed unused.

udata/tests/apiv2/test_topics.py Show resolved Hide resolved
Copy link
Contributor

@maudetes maudetes left a comment

Choose a reason for hiding this comment

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

Thank you a lot for this PR 🚀
I think we can add a changelog entry and we're all clear!

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