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

Drop redundant legacy API endpoints #13582

Merged
merged 69 commits into from Mar 23, 2022

Conversation

mvdbeek
Copy link
Member

@mvdbeek mvdbeek commented Mar 20, 2022

Builds on #13579 to identify what needs to be dropped from buildapp.py.

How to test the changes?

(Select all options that apply)

  • I've included appropriate automated tests.
  • This is a refactoring of components with existing test coverage.
  • Instructions for manual testing are as follows:
    1. [add testing steps and prerequisites here if you didn't write automated tests covering all your changes]

License

@davelopez
Copy link
Contributor

It seems there are some legacy endpoints that bypass the FastAPI ones. The lib/galaxy_test/api/test_api_batch.py::ApiBatchTestCase::test_querystring_params bite me too, I think BatchMiddleware is still somewhat using the old routes

@mvdbeek
Copy link
Member Author

mvdbeek commented Mar 21, 2022

Oh, nice catch, yes, we can probably drop the middleware

I can't find a trace of this being used in the client. We should
re-implement it as a ASGI middleware if we've identified a need for it,
but I don't love how this makes the call a potentially very long
blocking call. To fetch many items in batch we might be better off using
Server Sent Events, which can trickle out intermediate updates.
and drop require_admin, users can have permission to update libraries
(see also dropped legacy endpoint that did not enfore this).
@mvdbeek mvdbeek force-pushed the drop_redundant_routes branch 2 times, most recently from 70b2901 to cfa51ef Compare March 21, 2022 12:16
@mvdbeek mvdbeek marked this pull request as ready for review March 21, 2022 12:17
@mvdbeek mvdbeek mentioned this pull request Mar 21, 2022
32 tasks
The ConfigProvider isn't strictly necessary, passing the store to
mounted component would have been sufficient to make
`getGalaxyInstance().config` work.
@github-actions github-actions bot added this to the 22.05 milestone Mar 21, 2022
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.

Everything looks good! Only found a missing route that may be an alias.

Thank you so much also for the fixes and cleaning of unused routes!

lib/galaxy/webapps/galaxy/api/__init__.py Outdated Show resolved Hide resolved
lib/galaxy/webapps/galaxy/buildapp.py Show resolved Hide resolved
@mvdbeek mvdbeek merged commit 248d96d into galaxyproject:dev Mar 23, 2022
@github-actions
Copy link

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

@nsoranzo nsoranzo added the kind/refactoring cleanup or refactoring of existing code, no functional changes label Mar 23, 2022
@nsoranzo nsoranzo deleted the drop_redundant_routes branch March 23, 2022 13:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants