#1448 Create AuthMePlayer to get player data from API with#2000
#1448 Create AuthMePlayer to get player data from API with#2000
Conversation
ljacqu
changed the title
ljacqu
changed the title
sgdc3
left a comment
sgdc3
left a comment
There was a problem hiding this comment.
I think we should use optionals in our API
| * @return AuthMe player info, or null if not available | ||
| */ | ||
| public String getRegistrationIp(String playerName) { | ||
| public AuthMePlayer getPlayerInfo(String playerName) { |
There was a problem hiding this comment.
Api v4, deprecate api v3
No.
This is just a method addition and shouldn't warrant a new API version. Unless you mean the removed method, but that's one that's two days old and wasn't part of any release (even beta).
There was a problem hiding this comment.
Fair, but i would like to change that in the future, with proper versioning, maybe by splitting the api into a separate maven module.
| * | ||
| * @return player uuid, or null if not available | ||
| */ | ||
| UUID getUuid(); |
| * | ||
| * @return player's email or null | ||
| */ | ||
| String getEmail(); |
| /** | ||
| * @return the registration date of the player's account - never null | ||
| */ | ||
| Instant getRegistrationDate(); |
There was a problem hiding this comment.
One comment would have been enough :P
| * | ||
| * @return the ip address used during the registration of the account, or null | ||
| */ | ||
| String getRegistrationIpAddress(); |
| * | ||
| * @return date the player last logged in successfully, or null if not applicable | ||
| */ | ||
| Instant getLastLoginDate(); |
| * | ||
| * @return ip address the player last logged in with successfully, or null if not applicable | ||
| */ | ||
| String getLastLoginIpAddress(); |
TuxCoding
left a comment
TuxCoding
left a comment
There was a problem hiding this comment.
The Optional syntax would make the nullable values obvious, but it's a new concept that some might not understand. We could also use the @Nullable annotations. Personally I like Optionals more, because they provide great mapping functionality.
| import java.util.UUID; | ||
|
|
||
| /** | ||
| * Read-only player info exposed in the AuthMe API. |
There was a problem hiding this comment.
With the current implementation, we should maybe also mention that this object is a copy that won't update after the API method is called. So it could be outdated later without any notification.
| } | ||
|
|
||
| @Override | ||
| public String getLastLoginIpAddress() { |
There was a problem hiding this comment.
Is there a reason we don't use InetAddresses? I know they perform a DNS request, but only on DNS names not IP addresses.
There was a problem hiding this comment.
Most cases I'm thinking of would compare the IP address with another string IP from elsewhere, or store it somewhere, no? I'm worried it will be a more annoying type than helpful as I'd anticipate a lot of uses having to unwrap it. But if you think otherwise I'm happy to change it.
(Also I'm lazy because constructing InetAddresses might throw UnknownHostException, which should never happen, but we'd still have to handle them properly 😆). It's not an excuse to not do it and as mentioned, I'm happy to do it if you really want.
Another alternative is offering both String and InetAddress but I think we should be careful with our interface(s), i.e. choose an approach and kind of stick with it. Say, if we added more fields to the AuthMePlayer it would quickly become confusing.
There was a problem hiding this comment.
Also I'm lazy because constructing InetAddresses might throw UnknownHostException,
Well if it's correct IP address we could ignore it. If not, it doesn't make sense to return this invalid string either.
Most cases I'm thinking of would compare the IP address with another string IP from elsewhere, or store it somewhere, no? I'm worried it will be a more annoying type than helpful as I'd anticipate a lot of uses having to unwrap it. But if you think otherwise I'm happy to change it.
Good point, then we should keep the String representation, because I think the methods of InetAddress are most likely not relevant over the mentioned String use cases (in comparison to objects like Instant).
|
Happy to use |
- Use Optional for all values that may be null
3 participants
Introduces an API method to get a lot of player info with one load from the database. Unit tests will still follow but I open this PR early to get some feedback on the naming (and anything else). Ideally we won't have to remove or rename anything in the future since this is an API so it's really worth looking at our API methods carefully.
For the naming of AuthMePlayer: "ApiPlayer" would make sense for us in our codebase but not for anyone that is integrating our API. I also chose "Player" over "User" as I guess that's really the more used terminology in Minecraft (you have players on the server; it's players who register themselves in AuthMe).
cc: @RikoDEV @YellowZaki