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

Migrate ItemTags API to FastAPI #17064

Merged
merged 4 commits into from
Nov 27, 2023

Conversation

arash77
Copy link
Collaborator

@arash77 arash77 commented Nov 22, 2023

What did you do?

  • Include an API test under lib/galaxy_test/api/
  • Refactor the legacy ItemTags API logic so all the logic is contained in the ItemTagsManager class
  • Create pydantic models for the responses and payloads
  • Create the FastAPI routes that will replace the old ones

related to #10889

Why did you make this change?

This is part of the efforts to migrate the full Galaxy API to FastAPI see #10889.

How to test the changes?

License

  • I agree to license these and all my past contributions to the core galaxy codebase under the MIT license.

@arash77 arash77 force-pushed the migrate_item_tags_fastapi branch from a2ff3b4 to a10f574 Compare November 22, 2023 21:17
Copy link
Contributor

@davelopez davelopez left a comment

Choose a reason for hiding this comment

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

Very cool indeed! 🚀

Just some minor comments from my side below.

Also, the OpenAPI linting failures seem relevant, and I guess they are somewhat legitimate:

validating _schema.yaml...
[1] _schema.yaml:17120:7 at #/paths/~1api~1histories~1{history_id}~1contents~1{id}~1tags/get/parameters

The operation does not define the path parameter `{history_id}` expected by path `/api/histories/{history_id}/contents/{id}/tags`.

17118 | 'get':
17119 |   'operationId': 'index_api_histories__history_id__contents__id__tags_get'
17120 |   'parameters':
17121 |   - 'description': 'history_content_id'
17122 |     'in': 'path'

Error was generated by the path-parameters-defined rule.

Maybe we can do some kind of trick/workaround to fix this using https://fastapi.tiangolo.com/advanced/path-operation-advanced-configuration/#custom-openapi-path-operation-schema? 🤔

lib/galaxy/webapps/galaxy/api/item_tags.py Outdated Show resolved Hide resolved
lib/galaxy/webapps/galaxy/api/item_tags.py Outdated Show resolved Hide resolved
test/integration/test_item_tags.py Outdated Show resolved Hide resolved
lib/galaxy/webapps/galaxy/api/item_tags.py Outdated Show resolved Hide resolved
Relocate ItemTags API test
@arash77 arash77 marked this pull request as ready for review November 24, 2023 19:15
@github-actions github-actions bot added this to the 23.2 milestone Nov 24, 2023
Copy link
Contributor

@davelopez davelopez left a comment

Choose a reason for hiding this comment

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

Looks pretty good to me!

Thanks again for addressing all comments diligently!

lib/galaxy_test/api/test_item_tags.py Outdated Show resolved Hide resolved
@dannon dannon merged commit 6d51f48 into galaxyproject:dev Nov 27, 2023
51 checks passed
Copy link

This PR was merged without a "kind/" label, please correct.

@nsoranzo nsoranzo added kind/enhancement kind/refactoring cleanup or refactoring of existing code, no functional changes labels Nov 27, 2023
@arash77 arash77 deleted the migrate_item_tags_fastapi branch November 27, 2023 15:55
nsoranzo added a commit to nsoranzo/galaxy that referenced this pull request Dec 7, 2023
Before galaxyproject#17064 it was
possible to create a tag by simply making a POST request like
`/api/histories/911dde3ddb677bcd/tags/tag1` , this restores the
previous behaviour.

Fix BioBlend test failure:

https://github.com/galaxyproject/bioblend/actions/runs/7094150509/job/19308900670#step:10:3029
nsoranzo added a commit to nsoranzo/galaxy that referenced this pull request Dec 7, 2023
Before galaxyproject#17064 it was
possible to create a tag by simply making a POST request like
`/api/histories/911dde3ddb677bcd/tags/tag1` , this restores the
previous behaviour.

Fix BioBlend test failure:

https://github.com/galaxyproject/bioblend/actions/runs/7094150509/job/19308900670#step:10:3029
@mvdbeek mvdbeek modified the milestones: 23.2, 24.0 Dec 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/API area/testing/integration area/testing kind/enhancement kind/refactoring cleanup or refactoring of existing code, no functional changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants