Migrate a part of the users API to Fast API

[heisner-tillman]

This is a part of #10889.

Routes

• Get
  • /api/users/{id}/custom_builds --> /api/users/{user_id}/custom_builds
  • /api/users/deleted --> /api/users/deleted
  • /api/users/deleted/{id} --> /api/users/deleted/{user_id}
  • /api/users --> /api/users
  • /api/users/{id} --> /api/users/{user_id}
• Put
  • /api/users/{id}/custom_builds/{key} --> /api/users/{user_id}/custom_builds/{key}
  • /api/users/{id}/favorites/{object_type} --> /api/users/{user_id}/favorites/{object_type}
  • /api/users/{id}/theme/{theme} --> /api/users/{user_id}/theme/{theme}
  • /api/users/{id} --> /api/users/{user_id}
• Post
  • /api/users/deleted/{id}/undelete --> /api/users/deleted/{user_id}/undelete
  • /api/users
• Delete
  • /api/users/{id}/custom_builds/{key} --> /api/users/{user_id}/custom_builds/{key}
  • /api/users/{id}/favorites/{object_type}/{object_id} --> /api/users/{user_id}/favorites/{object_type}/{object_id}
  • /api/users/{id} --> /api/users/{user_id}

Summary

• Add pydantic models
• Operations now return pydanctic models
• Removed the mapping to the legacy route and add FastAPI routes

How to test the changes?

• I've included appropriate automated tests.
• This is a refactoring of components with existing test coverage.
• Instructions for manual testing are as follows:
  You can find the interactive API documentation here: http://127.0.0.1:8080/api/docs#/users

License

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

[heisner-tillman]

@mvdbeek
You suggested that I migrate the following routes:

But the show operation also handles the /api/users/deleted/{id}, so I migrated that one as well.

Also it is hard to remove the legacy routes, because there are multiple routes handled by the following legacy route:

All of the following routes are handeled by it:

So should migrate them all?

[mvdbeek]

Yes, that would be great!

[davelopez]

Great work!

Some minor comments, just around consistency, all the rest looks pretty good!

[heisner-tillman]

galaxy/lib/galaxy/webapps/galaxy/api/users.py

Line 700 in 3d258db

def create(self, trans: GalaxyWebTransaction, payload: dict, **kwd):

This endpoint uses the following method:

galaxy/lib/galaxy/webapps/base/webapp.py

Line 757 in 7842619

def get_or_create_remote_user(self, remote_user_email):

The method uses the environ attribute of the GalaxyWebTransaction class two times as follows:

if "webapp" not in self.environ or self.environ["webapp"] != "tool_shed":

I do not know how to access the environ attribute outside of the GalaxyWebTransaction class.
Are these if-conditions necessary?
Because if I can ignore them I could copy the method to UserService or Manager and then migrate this endpoint to FastAPI.

Could anyone help me with this one?

[mvdbeek]

if "webapp" not in self.environ or self.environ["webapp"] != "tool_shed":

is a weird way to ensure we're in Galaxy

Are these if-conditions necessary?

I suppose you could check if the associated app or request is Galaxy-specific. This brings me to something important though, it would be good that we don't duplicate logic between the tool shed and Galaxy.
One thing you could try is move get_or_create_remote_user into the UserManager class. The user manager has everything available that you need, minus environ. I'd just add a app_type="galaxy" default argument to the UserManager's __init__ and then turn if "webapp" not in self.environ or self.environ["webapp"] != "tool_shed"" into if self.app_type == "galaxy":.

UserManager is also used in the tool shed, so there you'd do something like

diff --git a/lib/tool_shed/webapp/app.py b/lib/tool_shed/webapp/app.py
index 3a9e0fe273..3f27d5bcca 100644
--- a/lib/tool_shed/webapp/app.py
+++ b/lib/tool_shed/webapp/app.py
@@ -80,7 +80,7 @@ class UniverseApplication(ToolShedApp, SentryClientMixin, HaltableContainer):
         self._register_singleton(SharedModelMapping, model)
         self._register_singleton(mapping.ToolShedModelMapping, model)
         self._register_singleton(scoped_session, self.model.context)
-        self.user_manager = self._register_singleton(UserManager, UserManager)
+        self.user_manager = self._register_singleton(UserManager, UserManager(app_type="tool_shed"))
         self.api_keys_manager = self._register_singleton(ApiKeyManager)
         # initialize the Tool Shed tag handler.
         self.tag_handler = CommunityTagHandler(self)

(also self.user_manager = self._register_singleton(UserManager, UserManager) looks like a bug, _register_singleton takes an instance as second argument)

Or you could create a app-specific class hierarchy for UserManager and eliminate the if completely. That seems like more work for marginal gains, I'll let you decide on that.

[heisner-tillman]

One thing you could try is move get_or_create_remote_user into the UserManager class.

I have done it this way.

In addition to the app_type i also passed self to the UserManager:

galaxy/lib/tool_shed/webapp/app.py

Line 83 in a947282

self.user_manager = self._register_singleton(UserManager, UserManager(self, app_type="tool_shed"))

However, I am not sure how to create a test that uses the get_or_create_remote_user method, so I just tested it manually. Can somebody write a test for this?

[mvdbeek]

I think this might be the only required field when use_remote_user is set in the Galaxy config, see https://bioblend.readthedocs.io/en/latest/api_docs/galaxy/all.html#bioblend.galaxy.users.UserClient.create_local_user and https://bioblend.readthedocs.io/en/latest/api_docs/galaxy/all.html#bioblend.galaxy.users.UserClient.create_remote_user

I would separate this into two alternative models and then create a union for valid payloads.

[heisner-tillman]

I have made the suggested changes.

[mvdbeek]

Do you mind pulling in e775edf ? That should fix the client build tests.

[bgruening]

Great work @heisner-tillman!

[mvdbeek]

Can you make this an enum of possible values, I think that's file, fasta and text.

[heisner-tillman]

I have made the suggested changes.

[mvdbeek]

Excellent work, thanks so much @heisner-tillman! Are you interested in taking this work and updating the client to use the new schema models you've created here ? Or are you looking for a next route to port ?

[bgruening]

Congratulations @heisner-tillman! :)

[heisner-tillman]

Right now I am working on writing my thesis, so I won't have any time or capacity to do something else in the next month. It was fun learning new things and I felt that I contributed to something important, which also feels good :) I don't know what the future will bring, but if I can find the time, I am interested to keep contributing to the project.

Are you interested in taking this work and updating the client to use the new schema models you've created here ?

That sounds interesting and I am always eager to learn something new :)

Or are you looking for a next route to port?

As I had fun porting those routes, that is another thing I am interested in.

I would also like to thank @davelopez for always being available to answer my questions and for helping me out when I was stuck.

[mvdbeek]

That sounds interesting and I am always eager to learn something new :)

Fortunately that's easy enough if you just want to get a feeling for it (and how a good python client generator should work ...).

diff --git a/client/src/stores/users/queries.ts b/client/src/stores/users/queries.ts
index 693bb36b57..1f53617731 100644
--- a/client/src/stores/users/queries.ts
+++ b/client/src/stores/users/queries.ts
@@ -3,9 +3,12 @@ import axios from "axios";
 import { prependPath } from "@/utils/redirect";
 import { rethrowSimple } from "@/utils/simple-error";
 
+import { fetcher } from "@/schema";
+
+const getUser = fetcher.path("/api/users/{user_id}").method("get").create();
+
 export async function getCurrentUser() {
-    const url = prependPath("/api/users/current");
-    const response = await axios.get(url);
+    const response = await getUser({user_id: "current"})
     if (response.status != 200) {
         throw new Error("Failed to get current user");
     }

which means you'll get nice types everywhere: