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

[cowtowncoder]

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]

/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]

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]

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]

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().

[pjfanning]

The existing impl is:

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

what's wrong with this?

[cowtowncoder]

@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]

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]

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]

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]

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]

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.