Add player object

[TheNullicorn]

• Provides weak encapsulation around player response JSON in the form of the Player object

  • Comes with methods for common operations, like getting the player's name, rank, experience, etc
  • Inherits from UnstableHypixelObject, which itself wraps the actual response JSON
  • Getters accept a simplified version of JsonPath (dot-notation; supports escaping)
  • Provides a layer of null-safety when accessing deeply-nested properties (whose parents may be null)
  • The original object can still be retrieved via getRaw()

• Adds an optional API for filtering unwanted/unneeded properties from objects (based on MongoDB projections)

  • Wanted keys can be marked for inclusion via PropertyFilter#include(String[]) and the static constructor PropertyFilter#including(String[])
  • Filtered objects will only contain those properties whose names are explicitly included
  • All UnstableHypixelObject support filtering via their filter(PropertyFilter) method
  • Filtered overloads for all player getters are added to HypixelAPI
  • e.g. HypixelAPI#getPlayerByUuid(UUID, PropertyFilter)

• Updates GetPlayerExample to reflect these changes

[TheNullicorn]

Just gonna make this a draft until I'm sure it's good. Are there any other methods you think would be helpful on a player object? Also, thoughts on getUuid returning a java.util.UUID instead of a string?

[HydroPage]

I was wondering if this was being done. That's a LOT of work for you, man, good luck (I'm a little under the impression that you're going to map most if not all of the Player JsonObject? Probably overkill)

[TheNullicorn]

I was wondering if this was being done. That's a LOT of work for you, man, good luck (I'm a little under the impression that you're going to map most if not all of the Player JsonObject? Probably overkill)

Sorry to disappoint, but that wasn't my plan for the PR. This is mainly for providing a layer of null-safety over the player structure, making it easier to access deeply nested fields which could potentially be missing. Player objects have no defined structure and are constantly changing, so mapping out the entire thing would be unrealistic. I did provided some utility methods for common fields though, such as those related to rank, experience, etc.

[TheNullicorn]

I've resolved the requested changes, so it should be ready to merge.

Right now I've got an isOnline() method that uses lastLogin and lastLogout, but I've marked it as deprecated to prefer the status endpoint. Is that fine as-is, or should we not be adding deprecated methods into the API?

[mdashlw]

Right now I've got an isOnline() method that uses lastLogin and lastLogout, but I've marked it as deprecated to prefer the status endpoint. Is that fine as-is, or should we not be adding deprecated methods into the API?

using lastLogin and lastLogout is not deperecated in any way

[TheNullicorn]

Right now I've got an isOnline() method that uses lastLogin and lastLogout, but I've marked it as deprecated to prefer the status endpoint. Is that fine as-is, or should we not be adding deprecated methods into the API?

using lastLogin and lastLogout is not deperecated in any way

Referring to the method, not the fields themselves. I considered it unconventional when there's a proper endpoint for it, but I'd be fine removing the annotation.

[HydroPage]

Hahaha

On Tue, May 11, 2021 at 9:00 PM Brynn @.***> wrote:

@TheNullicorn commented on this pull request.

In Java/src/main/java/net/hypixel/api/util/Utilities.java https://github.com/HypixelDev/PublicAPI/pull/260#discussion_r630665325:

• return UUID.fromString(
• uuidStr.substring(0, 8) + "-" + uuidStr.substring(8, 12) + "-" + uuidStr
• .substring(12, 16) + "-" + uuidStr.substring(16, 20) + "-" + uuidStr
• .substring(20, 32));

Must've accidentally ran the formatter on that file, but sure why not

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/HypixelDev/PublicAPI/pull/260#discussion_r630665325, or unsubscribe https://github.com/notifications/unsubscribe-auth/AHQUR4QLHQASAC23L77OFCDTNHOL7ANCNFSM4N4ULBGQ .

[HydroPage]

@TheNullicorn To be honest with you here, I understand why we're providing a weak encapsulation on Player, since we can't wrap it all in practice. But don't you think it's a little weird for Player to be (I think) the only object we have which provides access to the internals this much? I feel like we should make Player an abstracted structure just like everything else, but perhaps add a RawPlayer class containing all these safe raw property getter methods. Player#getRaw() would return a RawPlayer view of the Player's JSON, and RawPlayer could have a toJson() if someone needs to reach the lowest level. This is just an outline. What do you think?

EDIT: It is a bit iffy, I understand, but something just doesn't feel right about keeping the property accessors in Player itself either

[TheNullicorn]

@TheNullicorn To be honest with you here, I understand why we're providing a weak encapsulation on Player, since we can't wrap it all in practice. But don't you think it's a little weird for Player to be (I think) the only object we have which provides access to the internals this much? I feel like we should make Player an abstracted structure just like everything else, but perhaps add a RawPlayer class containing all these safe raw property getter methods. Player#getRaw() would return a RawPlayer view of the Player's JSON, and RawPlayer could have a toJson() if someone needs to reach the lowest level. This is just an outline. What do you think?

EDIT: It is a bit iffy, I understand, but something just doesn't feel right about keeping the property accessors in Player itself either

An alternative I had thought of was moving the property methods to an abstract ComplexHypixelObject class (or something along those lines) that Player could inherit from. That way there would be some context for why Player would contain those methods directly, as well as allowing them to be reused in some of the SkyBlock endpoints that are lacking proper deserialization.

[HydroPage]

An alternative I had thought of...

I like that idea. In the end, it's in our best interest to get those methods out of Player itself. Because those methods have nothing to do with players, but with things we can't fully wrap

[HydroPage]

Hey, Nullicorn, how about we somehow provide the option to not wrap the JsonObject somehow? I actually wrapped the player's JsonObject in my own immutable Player class in my application, and my application is one where I have to cache hundreds of Players' data; problem was, though, JsonObjects are gigantic, and not efficient at all for caching, given simply how much garbage there is in a Player's data. It's just way too much to matter. After wrapping it, and only storing what I cared about, my memory usage was decreased a hundred fold. I feel like our clients would appreciate an option like this. We could make the JsonObject null in ComplexHypixelObject given a flag argument in the request for a Player and make ComplexHypixelObject throw an IllegalStateException if you attempt to use its methods while it's null

[ConnorLinfoot]

You could also look at adding projections to the methods, these would of course only be client sided projections but it could mean data not needed can be discarded within the ComplexHypixelObject

[HydroPage]

Could you elaborate? I haven't heard of such a technique

[ConnorLinfoot]

@HydroPage It's basically a way of stating which fields you want, let's say you only care about Bed Wars, you can do a projection that includes only stats.Bedwars.

An easy example that I'm familiar with is MongoDB java driver, so you can do something like

Projections.include("uuid", "rank", "stats.Bedwars")

to only include those fields, or even something like

Projections.exclude("stats.Bedwars", "quests")

to instead exclude only those fields.

I'm not sure the best approach to implement this myself, but the TLDR is basically a whitelist/blacklist for which fields to keep in the json element

[HydroPage]

Great input. We can make ComplexHypixelObject take a projection in its constructor, and have it consider said Projection while deserializing the JsonObject passed. Though this will have to leak into an overload of HypixelAPI#getPlayerByXXX taking a UUID/String and a Projection optionally

[TheNullicorn]

I like that idea, and I'll try to do what I can with it over the weekend.

The only thing I'm not too sure about is exclusions. I think they have more of a use in the context of Mongo with small documents or sensitive fields, but player objects don't fit either of those. I'd be happy to add them if anyone wants to comment on that, but I just don't see it being nearly as useful as inclusions.

[HydroPage]

Yes, I say inclusions are the only thing necessary. Players are just way too big for exclusion to be that practical

[TheNullicorn]

Alright there we go,

• Any keys not explicitly included in a filter will be stripped from filtered objects.
  • Keys are in dot-notation.
  • I don't believe Mongo supports keys with dots (.) in them, but as a safety I made them escapable via double-backslash (\\.).
• To keep filters and objects decoupled I went with an applyTo(...) method for filtering objects instead of passing to the constructor.
  • There's also similar methods that accept Iterable and arrays of objects for batch filtering.

[TheNullicorn]

Thanks for all the help. Wasn't able to get to a few of your reviews today, but hopefully by tomorrow. If you spot anything else in the changes, or if I prematurely closed one of the conversations, let me know.

[TheNullicorn]

Apologies, I think I pressed re-review twice by mistake. I've left a few of the conversations open for you to look over, but I'm pretty confident the rest are addressed.

[TheNullicorn]

I've made the example a bit more friendly for new users, and cleared up some vague docs. I think I'm happy with this now, but reviews are still welcome.

[HydroPage]

It was fun helping you, nullicorn. GG on the merge, time to implement this in my applications

[TheNullicorn]

It was fun helping you, nullicorn. GG on the merge, time to implement this in my applications

Thanks for all the helpful comments! Not used to that sort of review, so it was a really nice learning experience