Skip to content

WebHostLib: Properly format IDs in API responses #4944

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

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

qwint
Copy link
Collaborator

@qwint qwint commented Apr 29, 2025

What is this fixing or adding?

update the id formatter to use staticmethods to not fake the unused self arg, and then use the formatter for the user session endpoints

found testing #4887
the user session responses have unusable ids that cause the id parser on webhost to error out with a ValueError: bytes is not a 16-char string
this also refactors the formatter class to use the staticmethod decorator like it's already being used

How was this tested?

import requests

s = requests.Session()

# grab the session cookie for my localhost which has existing data
s.get("http://localhost/session/a66f6a05-5c23-4473-8cc0-f9b2c70c27bb")
api_base = "http://localhost/api"

get_rooms_response = s.get(f"{api_base}/get_rooms")
assert get_rooms_response.status_code == 200, "get_rooms failed"
rooms = get_rooms_response.json()
first_room = rooms[1]["room_id"]
room_status_response = s.get(f"{api_base}/room_status/{first_room}")
assert room_status_response.status_code == 200, "room_status failed"
room_data = room_status_response.json()
print(room_data)

and manually pinging the different apis and using returned ids in more api calls in browser

If this makes graphical changes, please attach screenshots.

…elf arg, and then use the formatter for the user session endpoints
@github-actions github-actions bot added affects: webhost Issues/PRs that touch webhost and may need additional validation. waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. labels Apr 29, 2025
Copy link
Collaborator

@alwaysintreble alwaysintreble left a comment

Choose a reason for hiding this comment

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

I'm not really sure why they were instance methods in the first place, but this all looks correct. I'm not really sure why our api endpoints were all returning the encoded versions instead of the url safe decoded since those are unusable without the client requesting the information doing the same decoding the webhost already has to do in other places, so definitely support returning all of them as decoded.

Co-authored-by: Aaron Wagener <[email protected]>
@Berserker66
Copy link
Member

This breaks pattern with the parent class:
image

@Berserker66
Copy link
Member

I'm not really sure why they were instance methods in the first place,[...]

It's rather simple, really. I looked at the implementation of other Convertors.

@qwint
Copy link
Collaborator Author

qwint commented Apr 30, 2025

I can pull those functions out to standalone helpers if you really want, but i really hope their code isn't depending on Class.to_python(None, value)...

@alwaysintreble
Copy link
Collaborator

If we really want to follow that pattern then we're better off instantiating it

@Exempt-Medic Exempt-Medic added the is: bug/fix Issues that are reporting bugs or pull requests that are fixing bugs. label May 3, 2025
@qwint
Copy link
Collaborator Author

qwint commented May 6, 2025

updated with new helpers to be used by other calls and reverting Converter class to BaseConverter style

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects: webhost Issues/PRs that touch webhost and may need additional validation. is: bug/fix Issues that are reporting bugs or pull requests that are fixing bugs. waiting-on: peer-review Issue/PR has not been reviewed by enough people yet.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants