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 library folder contents API to FastAPI #14223

Merged

Conversation

davelopez
Copy link
Contributor

I'm trying to refactor and simplify the folder_contents API as part of a different PR and then see if I can optimize the index endpoint when trying to search on folders containing a large number of elements.

I've extracted these changes that focus only on migrating the current code to FastAPI for easier review.

How to test the changes?

  • This is a refactoring of components with existing test coverage.

License

@davelopez davelopez added kind/refactoring cleanup or refactoring of existing code, no functional changes area/API area/libraries Data libraries labels Jun 29, 2022
@github-actions github-actions bot added this to the 22.09 milestone Jun 29, 2022
@mvdbeek
Copy link
Member

mvdbeek commented Jun 29, 2022

Let's use DecodedDatabaseIdField where we can ... it might involve some more changes deeper in the stack, but this should be worth doing.

@davelopez
Copy link
Contributor Author

Let's use DecodedDatabaseIdField where we can ... it might involve some more changes deeper in the stack, but this should be worth doing.

Sure! Do we want to replace all EncodedDatabaseIdField with DecodedDatabaseIdField at some point? or is worth using EncodedDatabaseIdField in some situations?

@mvdbeek
Copy link
Member

mvdbeek commented Jun 29, 2022

I think changing all occurrences is going to be a big project (see my stale PR #13598), but when converting or adding new routes I think we should always prefer DecodedDatabaseIdField

Co-authored-by: Marius van den Beek <m.vandenbeek@gmail.com>
Comment on lines +31 to +32
FolderIdPathParam: DecodedDatabaseIdField = Path(
..., title="Folder ID", description="The encoded identifier of the library folder."
Copy link
Member

Choose a reason for hiding this comment

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

I don't know/understand much of the pydantic stuff, but I'd expect to find EncodedDatabaseIdField at the API layer like here, and DecodedDatabaseIdField in services/managers/controllers. Here it looks confusing when you compare the type and description.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I got a bit confused too at the beginning, that's why I kept using EncodedDatabaseIdField, but using DecodedDatabaseIdField is waaaay cleaner as Marius suggested, we can avoid any further explicit decoding down the road.
I guess we can just rename it to DatabaseIdField or something similar to reduce the confusion after we get rid of the existing EncodedDatabaseIdFields.

@davelopez davelopez marked this pull request as draft June 30, 2022 08:22
@davelopez davelopez marked this pull request as ready for review June 30, 2022 09:30
@mvdbeek mvdbeek merged commit b87a89b into galaxyproject:dev Jun 30, 2022
@davelopez davelopez deleted the fastapi_migrate_library_folder_contents branch June 30, 2022 18:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/API area/libraries Data libraries kind/refactoring cleanup or refactoring of existing code, no functional changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants