Inconsistencies between return types on different Client methods

[jmkd3v]

Roblox endpoints that take in a single ID, like users.roblox.com/v1/users/{userId}, usually raise errors when the passed ID is invalid or does not exist. For example, /v1/users/{userId} returns the following data and a 404 error when an invalid user ID is passed:

{
  "errors": [
    {
      "code": 3,
      "message": "The user id is invalid.",
      "userFacingMessage": "Something went wrong"
    }
  ]
}

In ro.py, this means we can raise a NotFound error:

roblox.utilities.exceptions.NotFound: 404 Not Found: https://users.roblox.com/v1/users/4.

Errors:
	3: The user id is invalid.
		User-facing message: Something went wrong

Endpoints that take in multiple user IDs, like games.roblox.com/v1/games/multiget-place-details, tend to ignore invalid IDs and don't raise errors if none of the IDs are valid. In ro.py, all of our get_PLURAL functions like get_places and get_universes maintain this behavior and do not raise errors for invalid IDs.

The issue is that not all of our get_SINGULAR functions use endpoints that only take in one ID. multiget-place-details is the only endpoint ro.py uses for places. Because it does not raise an error, get_place returns None if the place is invalid and doesn't raise an exception. Multiple other methods also have this behavior.

This causes an inconsistency between methods where some methods (the ones that send requests that take just one ID) raise errors with invalid inputs, but other methods (the ones that take multiple IDs but only pass one) don't raise errors.

The following Client methods all return None with invalid IDs:

get_user_by_username()
get_universe()
get_place()
get_plugin()

All other singular methods raise exceptions for invalid IDs.
This causes an inconsistency between methods. What should we do?

We have a couple options here:

• Make the other methods also return None instead of raising exceptions
  • Not only is this a large breaking change, but it's also not proper when compared to API responses which do raise errors. I feel better about raising errors manually than I do about replacing the API's error behavior.
• Raise fake NotFound errors on these methods in particular
  • I don't like this because we're only supposed to raise NotFound for actual 404 errors.
• Create custom errors, like UserNotFound that are raised in place of NotFound on get_user and get_user_by_username, and do the same for other methods
  • This means you can't do a try-except for NotFound on get_user anymore, which is breaking.
  • Making it a subclass of HTTPException kind of breaks the rules because it's really only for 4xx/5xx HTTP status codes, not for other purposes.
  • Making it a subclass of RobloxException means we're not raising the right exception for get_user anymore. That's an actual HTTP exception, so I think we should keep that there.
  • Only raising it for get_user_by_username and not get_user is a bit weird for the developer because there's an exception called UserNotFound that isn't raised on the method used to get users but is raised on the method used to get users by username.
• Live with the inconsistencies.
  • Obviously not ideal. 😔
• Completely get rid of get_user_by_username, get_universe, get_place and get_plugin and force developers to use their plural counterparts: get_users_by_username, get_universes, get_places and get_plugins.
  • This makes the methods consistent now but breaks existing code and is not really ideal.

[NihatSul]

Well it should give an error when the ID of the user is invalid or does not exists

[jmkd3v]

I edited this issue with proper information - sorry for posting it too early!

[GlowingUmbreon]

Completely get rid of get_user_by_username, get_universe, get_place and get_plugin and force developers to use their plural counterparts: get_users_by_username, get_universes, get_places and get_plugins.

This solution probably isn't ideal since it probably is slower than the other events, but if you was to go ahead with it you could maintain backwards compatibility by doing something like

def get_user_by_username(username):
   return get_users_by_username(username)[0]

I personally thing the 3rd one (Create custom errors) will probably be the ideal solution, but make sure to mark the release as a major release so people know it has breaking changes.

[jmkd3v]

This solution probably isn't ideal since it probably is slower than the other events,

There is no speed factor here. get_place always just returned index 0 or None from get_places - it was always a shortcut.

def get_user_by_username(username):
   return get_users_by_username(username)[0]

This wouldn't help because it just creates another issue - now we have a method that raises an IndexError when the ID is invalid rather than returning None, which would break compatibility regardless. Retaining compatibility is not possible here.

[Boegie19]

Why don't you just error if get_user_by_username has no values in it and then just give a custom not found error

[jmkd3v]

Why don't you just error if get_user_by_username has no values in it and then just give a custom not found error

It already raises an error when no value is passed. I outlined the issues with a custom NotFound error in the first post.

[jmkd3v]

This has been fixed in 4ad3e3d and 5922eb1. Thanks everyone!