[#416] Add support for Stream and Iterable to @ProtoField

[ryanemerson]

Closes #416

[pruivo]

LGTM!

[pruivo]

@wburns do you wanna take a look too?

[wburns]

It looks good to me in general. My only main concern is I don't know if I like the idea of Stream being marshalled. A Stream instance is inherently meant to only be used once, so I worry this can lead to misuse in some way.

But if we want to keep it knowing that, I won't say no.

[ryanemerson]

It looks good to me in general. My only main concern is I don't know if I like the idea of Stream being marshalled. A Stream instance is inherently meant to only be used once, so I worry this can lead to misuse in some way.

My thinking was that the Stream would only be consumed once when unmarshalling either by the setter or @ProtoFactory implementation. On the marshalling side the @ProtoField should create a new Stream per method call. It's a bit trickier for fields, but in general people probably shouldn't be storing Stream instances as fields. If we think this is a concern, we could prevent @ProtoField stream when defined on a field?

[pruivo]

It adds value supporting Stream for methods annotated with @ProtoField when we need to transform the sent data. We have a couple of places in Infinispan/Keycloak where this comes in handy to avoid copying everything to a list before sending.

I don't expect users to use Stream as a field 😄

[wburns]

I don't expect users to use Stream as a field 😄

Never know, if you allow it, some may 😢

But maybe just document that users shouldn't would be sufficient to me.

[wburns]

Also one thing to note regarding Stream when I checked the generated code is on the unmarshalling side we still create an ArrayList and write into it and then pass the Stream which many times still copies the value into another ArrayList. So while it may save space on the marshalling side it can incur additional overhead on the unmarshalling side.

[ryanemerson]

Also one thing to note regarding Stream when I checked the generated code is on the unmarshalling side we still create an ArrayList and write into it and then pass the Stream which many times still copies the value into another ArrayList

Do you mean when a user explicitly creates another list when consuming the stream, or an internal op of the stream?

The primary use-case for consuming a Stream like this in Infinispan is for when we're either returning some entries which first need tranformation before they can be marshalled, or Arrays. With the latter, we can't avoid converting from a List to an array due to the way the marshaller reads repeated fields individually from the InputStream and us not knowing the number of entries, but having the Stream allows the consuming @ProtoFactory to have more control.

[pruivo]

Also one thing to note regarding Stream when I checked the generated code is on the unmarshalling side we still create an ArrayList and write into it and then pass the Stream which many times still copies the value into another ArrayList. So while it may save space on the marshalling side it can incur additional overhead on the unmarshalling side.

can we make the generator code to check collectionImplementation and if set use it instead of Stream

@wburns I'm expecting that Stream allows us to do stuff like this (I know it is a very simple example)

      public List<UUID> uuids;

      @ProtoField(1)
      public Stream<String> getStrings() {
         return uuids.stream().map(UUID::toString);
      }

      public void setStrings(Stream<String> strings) {
         this.uuids = strings.map(UUID::fromString).toList();
      }

A more real-world example is to get rid of this "not so great" code:

https://github.com/infinispan/infinispan/blob/016970bb6ff16b4b92f787136bc3325140a80fbb/core/src/main/java/org/infinispan/security/impl/SubjectAdapter.java#L21-L33

[wburns]

Okay, I think I understand. My problem was I was looking at the test classes that are using Iterable and Stream and they are not good usages of Stream marshalling. I wonder if we should just tweak the tests to be more indicative of the desired use case?

The only other thing I would recommend is to use a SpinedBuffer to store the elements in the case of Iterable and Stream. SpinedBuffer prevents resize operations for larger amount of values. Other "types" should be fine as the goal is to store them as the impl type.

[wburns]

Actually Stream.builder() would be better than SpinedBuffer, as we don't need to maintain it ourselves.

[wburns]

I guess we can integrate it as is. We can always change as needed later.