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.

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

Add Stream-friendly alternative to JsoneNode.fields(): Set<Map.Entry<String, JsonNode>> properties() #3809

Closed
cowtowncoder opened this issue Mar 5, 2023 · 12 comments

Comments

@cowtowncoder
Copy link
Member

cowtowncoder commented Mar 5, 2023

Currently with Jackson 2.x the only way to traverse entries of ObjectNode is using fields() method.
This has the problem that it returns Iterator (of Map.Entry<String, JsonNode>) which is awkward to use (with Java 8 and above).

Similarly we can consider adding methods to replace:

  • public Iterator iterator();
  • public Iterator elements();

for "values" stream.

Naming-wise I am thinking of

  • JsonNode.entryStream() -- but could also consider entries() or properties()?
  • JsonNode.valueStream() -- but could consider just values()?

EDIT: At this point, favoring

  • JsonNode.values() for single-value Stream (Array elements, Map entry values)
  • JsonNode.properties() for Map.Entry<String, JsonNode> valued stream

backed by simple implementations of:

  • ArrayList.stream() (ArrayNode), Map.values().stream() (ObjectNode)
  • Map.entrySet().stream()
@cowtowncoder cowtowncoder added to-evaluate Issue that has been received but not yet evaluated 2.15 labels Mar 5, 2023
@cowtowncoder
Copy link
Member Author

/cc @yawkat @pjfanning -- this seems straight-forward and I hope to do PR soon (maybe tonight). But wanted to see if you had suggestions, opinions. This is based on realizing how awkward use of Iterator-based alternatives currently is for ObjectNode in particular.

@yawkat
Copy link
Member

yawkat commented Mar 8, 2023

having a stream is nice but i think in most cases an Iterable would be preferable so that you can use a foreach loop. So basically an Iterable that behaves like fields().

@carterkozak
Copy link
Contributor

I tend to avoid using streams in hot code paths because performance leaves a bit to be desired and is harder to reason about.

I wonder if there's an intermediate option in which Collection<T> is exposed as opposed to Iterator<T> or Iterable<T>, allowing us to interact with nodes using for-each loops:

for (JsonNode fieldValue : node.values()) {

as well as streams:

node.values()
    .stream()
    ./*etc*/

@cowtowncoder
Copy link
Member Author

Yeah perhaps exposing Iterable makes sense: I'd prefer not exposing actual underlying List (or Collection) for ArrayNode or Map etc for ObjectNode.

Since JsonNode already implements Iterable<JsonNode>, that part is actually covered, just leaving replacement of fields().

So perhaps I should only add Iterable<String, JsonNode> properties() method in JsonNode, with default impl of returning emptyList(); and ObjectNode overriding to returning actualMap.entrySet().

@cowtowncoder cowtowncoder removed the to-evaluate Issue that has been received but not yet evaluated label Mar 8, 2023
@cowtowncoder cowtowncoder changed the title Add Stream-returning alternative to ObjectNode.fields() Add Stream-friendly alternative to ObjectNode.fields() (ObjectNode.properties() returning Iterable<String, JsonNode>?) Mar 8, 2023
@pjfanning
Copy link
Member

The existing impl is:

    @Override
    public Iterator<Map.Entry<String, JsonNode>> fields() {
        return _children.entrySet().iterator();
    }

what's wrong with this?

@cowtowncoder
Copy link
Member Author

@pjfanning It is cumbersome to go from Iterator into stream, cannot use foreach. So what'd be better would have been to return Iterable instead.
At least based on when I have tried to write basic iteration code most examples have ~5 lines (or require use of helper libraries).

Not the biggest thing in the world but one of ergonomic pet peeves.

@yawkat
Copy link
Member

yawkat commented Mar 9, 2023

going from Iterable to stream is also annoying. You can go through the spliterator but it's not easy.

I think that actually, returning a collection like @carterkozak suggested would be fine. If you look at AbstractCollection, it actually only requires two methods for the most basic implementation, size and iterator. So size is the only additional exposed value if you return a Collection instead of an Iterable.

We don't have to implement the mutation methods, though eg remove will work by default if we just use .entrySet().iterator(), because that iterator allows remove(). Might want to use an unmodifiable iterator to avoid that.

So a Collection backed by an unmodifiable iterator would be no additional api exposure, it would allow easy enhanced for (like Iterable), and it has a stream method (Iterable does not).

@cowtowncoder
Copy link
Member Author

Ah. So Iterable alone won't help; Collection does.

My initial concern return Collection is not so much removal via Iterator, but direct modification of entries. But I guess that is actually no different from Iterable because both returning backing Map (with full mutability) but Map.Entry<>; and that's what Iterator already does.

So I think the simplest and (no less) safe (than before) way to go is to just add:

public Set<Map.Entry<String, JsonNode>> properties() {
    return _children.entrySet();
}

and be done with it?

No need to construct Iterators or custom Collections... I mean, if there was no existing fields() method, protection against modifications would be sensible but since we are not opening more holes, adding more protection won't help. Unless we retrofitted fields(); and doing that might even break existing "working" usage -- would not be surprised if some developers considered mutability via Iterator a Feature, not bug. :)

@pjfanning
Copy link
Member

I like immutable myself but Java users can't be weaned off mutable side effect laden coding. Any chance that wrapping the collection as immutable could be considered?

@carterkozak
Copy link
Contributor

I would bias toward immutability, this opens the possibility of deprecating and eventually deleting the mutable iterator methods. It's always easier to make an immutable result mutable later on. It's possible, however unlikely, that wrapping the collection allocates enough wrappers that performance is impacted, but I suspect it won't be measurable, and it's not painful to reverse course.

@cowtowncoder
Copy link
Member Author

My point is this tho: while we could wrap Collection or Set in immutable, I do not see much value -- ObjectNode itself is mutable, and existing Iterable<...> fields() is likewise exposing mutable entries. Latter could be changed, but potentially breaking existing usage.

Or maybe formulate this different way: given the situation as of Jackson 2.14, ability to change entries exists, perhaps as a feature, so keeping things consistent, Collection/Set of properties probably should be mutable.

We could definitely change this for Jackson 3.0, but even there the question is... what is the main goal? To design actually immutable JsonNode-like API one has to start from ground up (I have thought about this over past 10 years since I, too, like Immutability a lot -- and would love to have Immutable Tree Model).
Just blocking mutability of ITerators/iterables will not gets us to immutability.
If it is for preventing modifications via secondary methods, is that to allow potentially changing internal storage aspects? Fine, but are there plans to change it.

@cowtowncoder
Copy link
Member Author

I like immutable myself but Java users can't be weaned off mutable side effect laden coding. Any chance that wrapping the collection as immutable could be considered?

Possible, but as per my comments above, what would be the benefit? (keeping in mind that ObjectNode contents are mutable via its API)

@cowtowncoder cowtowncoder changed the title Add Stream-friendly alternative to ObjectNode.fields() (ObjectNode.properties() returning Iterable<String, JsonNode>?) Add Stream-friendly alternative to ObjectNode.fields(): Set<Map.Entry<String, JsonNode>> properties() Mar 18, 2023
@cowtowncoder cowtowncoder changed the title Add Stream-friendly alternative to ObjectNode.fields(): Set<Map.Entry<String, JsonNode>> properties() Add Stream-friendly alternative to JsoneNode.fields(): Set<Map.Entry<String, JsonNode>> properties() May 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants