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.

Sign up for GitHub

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

Jump to bottom

Add JsonNode.optional(String name) and JsonNode.optional(int index) to be able to get an Optional JsonNode #2145

Closed
burdoto opened this issue Sep 26, 2018 · 12 comments · Fixed by #4866
Closed

Add JsonNode.optional(String name) and JsonNode.optional(int index) to be able to get an Optional JsonNode #2145

burdoto opened this issue Sep 26, 2018 · 12 comments · Fixed by #4866
Assignees
@cowtowncoder
Labels
2.19 Issues planned at 2.19 or later

Comments

@burdoto
Copy link

burdoto commented Sep 26, 2018 •
edited by cowtowncoder
Loading

NOTE: considered part of JSTEP-3

So I had this idea of how nice it would be to have a getter method that returns an optional JsonNode, like #path(String name) does, but when returning an Optional, it would be simpler to only use a node when its actually present, or handle cases that its not present in.

Current Solution:

this.guild = data.has("guild_id") ? discord.getServer(data.get("guild_id").asLong()) : null;

With optional nodes:

this.guild = data.getOptional("guild_id").map(node -> discord.getServer(node.asLong())).orElse(null);

In this scenario, the difference is counter intuitive, because I can use a ternary-if in this case.
See this scenario:
Without the addition:

        long afkChannelId = data.path("afk_channel_id").asLong(-1);
        if (afkChannel == null || afkChannel.getId() != afkChannelId) {
            afkChannel = afkChannelId == -1 ? null : discord.getChannelCache()
                    .getOrRequest(afkChannelId, afkChannelId);
            traits.add(AFK_CHANNEL);
        }

With the addition:

        Optional<JsonNode> node = data.getOptional("afk_channel_id");
        if (afkChannel == null || node.map(idN -> idN.asLong() == afkChannel.getId()).orElse(false)) {
            afkChannel = node.map(json -> discord.getChannelCache().getOrRequest(json.asLong(), json.asLong())).orElse(null);
        }

I think this scenario could show why the addition could be helpful.
If this idea gets approved and it is wished for; I could work on a PR myself.
@cowtowncoder
Copy link
Member

You may not be familiar with MissingNode, which serves purpose similar to Optional.
It is produce by various calls like:

JsonNode maybeValue = objectNode.path("tree").path("branch");
JsonNode otherMaybe = objectNode.at("/tree/branch/value/3");

in cases where there is no matching node.

You can call usual methods on MissingNode (which coerces to default values), or check for "missing"ness (node.isMissingNode()).

So. Although I am not totally opposed to some level of compatibility with JDK Optional for Jackson 3.0 (2.x it can not be done as that's Java 8 feature), I want to make sure there is real need.

@burdoto
Copy link
Author

burdoto commented Oct 2, 2018 •
edited
Loading

I am already aware of the missingnode technique, I just feel that my optional-variant is missing.
What if you're trying to get an optional int node, and set it to null if it's not there?
You can't pass null to the #asInt(int) method, as primitives cannot be really "null" in that kind of sense
yet you might want to have it be null if it's unavailable.
With the optional variant you could do node.optionalInt("uses").orElse(null)
Where with the current version, the smallest solution to this is to use a ternary with node.has("uses"),which also means you have to either store the node of "uses" or access it at least twice

@burdoto
Copy link
Author

burdoto commented Oct 2, 2018 •
edited
Loading

Parallel to that, what about a method asIntOrNull() that either gets the actual value of the node or null?

In that case, we would lose the option of using the Optional#map method, though, which would be a huge advantage to have

@cowtowncoder
Copy link
Member

I don't think I want asIntOrNull() since that opens doors to combinatorial explosion (for different types).
Back to optional: there are asInt(defaultValue) and similar variants that should cover use cases.

But perhaps adding simple optional(key) and optional(index) would make sense for inter-operability: there are many Java APIs (streaming most notably) that would benefit, and adding that would be trivial.

@cowtowncoder cowtowncoder added the 3.x Issues to be only tackled for Jackson 3.x, not 2.x label Oct 2, 2018
@burdoto burdoto changed the title [idea] Add a method #getOptional(String name) to be able to get an Optional JsonNode from a field. Add method #optional(String name) to be able to get an Optional JsonNode Oct 3, 2018
@burdoto burdoto mentioned this issue Oct 3, 2018
Closed
@cowtowncoder cowtowncoder removed the 3.x Issues to be only tackled for Jackson 3.x, not 2.x label Dec 22, 2024
@cowtowncoder cowtowncoder changed the title Add method #optional(String name) to be able to get an Optional JsonNode Add JsonNode.optional(String name) and JsonNode.optional(int index) to be able to get an Optional JsonNode Dec 22, 2024
@cowtowncoder cowtowncoder added the 2.19 Issues planned at 2.19 or later label Dec 22, 2024
@cowtowncoder cowtowncoder self-assigned this Dec 22, 2024
@JooHyukKim
Copy link
Member

Lemme start this one if no one has started?
Seems like a good start for picking up JSTEP-3

Wrt implementation details, would be one PR seems okay? Including following tasks

  • Add JsonNode.optional(String name) and JsonNode.optional(int index)
  • Add appropriate implementations in subclasses
  • Write optional() tests in the one isolated class. We may try find default-test-class for each JsonNode-subclasses but idk.

@JooHyukKim JooHyukKim mentioned this issue Dec 25, 2024
Merged
@cowtowncoder
Copy link
Member

Sounds good. The only (?) open question -- which I should have noted here -- is the naming.
While optional() is a reasonable possibility, I think there are 2 possible alternatives to consider:

  • opt() -- shorter, if that matters, comparable to get() and path() (which this new method is similar to)
  • getOpt() -- same length, but would be grouped next to get() in Javadocs (and IDE auto-completion), which might be desireable.

WDYT @JooHyukKim, @pjfanning, @yawkat ?

@JooHyukKim
Copy link
Member

Hmmm I was actually 50/50 on 'getOptional()' vs 'optional()'. Atm I am more concerned about users complaining about lack of intuitiveness ("what does opt mean?") rather than method name being too long. But I'm open to other suggestions as well.

But with the current PR I chose to follow the title.

@cowtowncoder
Copy link
Member

cowtowncoder commented Dec 27, 2024 •
edited
Loading

Makes sense. Let's see what others think.

Also creating some more issues for simple work:

@burdoto
Copy link
Author

burdoto commented Dec 27, 2024

it should be put into consideration that a method name getOptional would be relevant for property scanners and property based DSLs, such as groovy

@JooHyukKim
Copy link
Member

@burdoto Good point, traditional get-ter names are too easily exposed literally anything...
+1 on not adding getXxx prefix.

@JooHyukKim
Copy link
Member

Also I just realized this change is going into 2.19 as well... meaning coming up with best name is quite important.

@cowtowncoder
Copy link
Member

Ok yes, I think going with optional() makes sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@cowtowncoder cowtowncoder

Labels
2.19 Issues planned at 2.19 or later
Projects
None yet
Milestone
No milestone
3 participants
@cowtowncoder @burdoto @JooHyukKim