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: add endpoint for getting collection metadata #424

Merged
merged 30 commits into from Apr 25, 2024

Conversation

yolile
Copy link
Member

@yolile yolile commented Apr 17, 2024

closes #421

@yolile yolile requested a review from jpmckinney April 17, 2024 15:45
Copy link
Member

@jpmckinney jpmckinney left a comment

Choose a reason for hiding this comment

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

Would it be possible to make this part of the CollectionViewSet in api.py instead? We try to use Django REST Framework where possible. I recently added http://127.0.0.1:8000/swagger-ui/ which automatically documents Django REST Framework APIs. In Pelican Backend's views.py, we made all methods use Django REST Framework, but dataset_filter_items and dataset_distinct_values were too unusual to fit into that framework. Not sure if this one is too unusual or not.

@jpmckinney
Copy link
Member

Noting that the 3 current endpoints could be changed to REST framework: #360

@yolile
Copy link
Member Author

yolile commented Apr 17, 2024

Not sure if this one is too unusual or not.

What is usual? The queries and the results of the queries we are performing here are very specific, e.g. there is not a model with the resulting fields, etc.

@jpmckinney
Copy link
Member

jpmckinney commented Apr 17, 2024

I think this one is not too unusual. If you look at https://github.com/open-contracting/pelican-frontend/blob/main/backend/api/views.py some responses are just Response(python_dict) like metadata.

The two that are unusual have more request parameters (in the path, query string and/or request body). In principle, they can probably be aligned, I suppose.

@jpmckinney
Copy link
Member

I see precommit makes some fixes. Make sure you run pre-commit install locally to catch these early.

@jpmckinney
Copy link
Member

Since you're converting the old endpoints, we'll need a PR in the data-registry as well.

Another option is to keep views.py and the urls.py entries and deploy Process, then update and deploy the Registry, and finally remove views.py in a later PR.

@yolile
Copy link
Member Author

yolile commented Apr 18, 2024

Since you're converting the old endpoints, we'll need a PR in the data-registry as well.

We will need that PR to start using the new endpoint, so I will do both things in the same one

@yolile
Copy link
Member Author

yolile commented Apr 18, 2024

@jpmckinney any idea why the open api test if failing with django_filters.RemovedInDjangoFilter25Warning: Built-in schema generation is deprecated. Use drf-spectacular. ?

@jpmckinney
Copy link
Member

I will do both things in the same one

Cool - you can implement the new API in the registry, and update the others once this PR is reviewed.

any idea why the open api test if failing

I'll take a look.

process/api.py Outdated Show resolved Hide resolved
process/api.py Outdated Show resolved Hide resolved
process/api.py Outdated Show resolved Hide resolved
process/api.py Outdated Show resolved Hide resolved
process/api.py Outdated Show resolved Hide resolved
process/api.py Outdated Show resolved Hide resolved
@jpmckinney
Copy link
Member

any idea why the open api test if failing

I'll take a look.

Fixed (had to switch to drf-spectacular, like in message)

…nd until fa0ad29. The help text had not been updated to reflect this change.
…e API and Kingfisher Collect extension field docs. Align errors in create API and checker command.
…er can take the source's compiledRelease object, which might not have deleted null fields

- Calculate the ocid_prefix in SQL
- Extract dictfetchone() method

I compared runtime for the old and new queries, and the new (fewer) queries are not noticeably slower (if at all).
jpmckinney
jpmckinney previously approved these changes Apr 20, 2024
@yolile
Copy link
Member Author

yolile commented Apr 22, 2024

Thanks, I will prepare the PRs for Collect and Pelican, test them locally, and then merge and deploy the 3 projects together

@jpmckinney
Copy link
Member

You mean the data registry? I don't think Pelican needs changes to be made in sync.

@yolile
Copy link
Member Author

yolile commented Apr 22, 2024

@yolile yolile merged commit f224f16 into main Apr 25, 2024
9 checks passed
@yolile yolile deleted the 421-data-registry-endpoint branch April 25, 2024 17:21
Copy link

sentry-io bot commented Apr 26, 2024

Suspect Issues

This pull request was deployed and Sentry observed the following issues:

  • ‼️ DatabaseError: Save with update_fields did not affect any rows. /api/collections/{pk}/close/ View Issue

Did you find this useful? React with a 👍 or 👎

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 API endpoint for data registry integration for collection metadata
2 participants