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

[24.0] /api/users/ does not include deleted #18300

Closed
bernt-matthias opened this issue Jun 2, 2024 · 5 comments
Closed

[24.0] /api/users/ does not include deleted #18300

bernt-matthias opened this issue Jun 2, 2024 · 5 comments

Comments

@bernt-matthias
Copy link
Contributor

bernt-matthias commented Jun 2, 2024

Describe the bug

Apparently it did until 23.x galaxyproject/bioblend#487

Not sure if a bug, since deleted is a parameter and therefor the info is redundant.
But the example in https://usegalaxy.eu/api/docs#/users/get_users_api_users_get
also shows it in the response.

Edit: The code suggests that only id (and maybe email and username) are included.

Galaxy Version and/or server at which you observed the bug
Galaxy Version: 24.0

Expected behavior

Not sure if the key should be in the response. But at least the docs should be consistent.

@davelopez
Copy link
Contributor

I'm also not sure about when it has to be present, but regarding the docs, the response contains a Union of models List[Union[UserModel, LimitedUserModel]]. The example value in the doc renders the first one it encounters, but if you want to check other possible models you can check the schema:

image

@bernt-matthias
Copy link
Contributor Author

What is the return type of user.to_dict()?

My guess would be that the LimitedUserModel might come due to this

This might explain why galaxyproject/bioblend#487 returns only limited info (need to check if the test is done as admin and the query is for another user), but I noticed it in an application where the user was an admin.

@davelopez
Copy link
Contributor

Hmmm now that I look, it might actually be because of this

if trans.app.config.expose_user_name:

If any of these options (expose_user_name or expose_user_email) are set to false the UserModel will fail validation because it expects username and email to be non-null and FastAPI might choose to return the limited version then 🤔

@davelopez
Copy link
Contributor

I've just compared the admin response for http://127.0.0.1:8080/api/users between 23.1 and 24.0 and can confirm that there are unexpected changes. I'll add some integration tests a prepare a fix 👍

23.1

[
  {
    "model_class": "User",
    "id": "f2db41e1fa331b3e",
    "email": "[email protected]",
    "username": "admin",
    "deleted": false,
    "active": true,
    "last_password_change": "2024-02-06T13:04:23.454162"
  },
  {
    "model_class": "User",
    "id": "1cd8e2f6b131e891",
    "email": "[email protected]",
    "username": "user02",
    "deleted": false,
    "active": true,
    "last_password_change": "2024-02-06T13:04:23.638815"
  },
  {
    "model_class": "User",
    "id": "f597429621d6eb2b",
    "email": "[email protected]",
    "username": "user01",
    "deleted": false,
    "active": true,
    "last_password_change": "2024-02-06T13:04:23.558743"
  }
]

24.0

[
  {
    "id": "ebfb8f50c6abde6d",
    "username": "user03",
    "email": "[email protected]"
  },
  {
    "id": "f597429621d6eb2b",
    "username": "user01",
    "email": "[email protected]"
  },
  {
    "id": "f2db41e1fa331b3e",
    "username": "admin",
    "email": "[email protected]"
  }
]

@davelopez
Copy link
Contributor

This should be fixed in #18329

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants