helios-testing shaded artifact is broken about helios HOT 10 CLOSED

Confirmed that this is fixed in #1141 (if merged):

        35: invokevirtual #733                // Method com/spotify/docker/client/messages/NetworkSettings.ports:()Lcom/spotify/docker/client/shaded/com/google/common/collect/ImmutableMap;
        38: invokevirtual #739                // Method com/spotify/docker/client/shaded/com/google/common/collect/ImmutableMap.entrySet:()Lcom/spotify/docker/client/shaded/com/google/common/collect/ImmutableSet;
        41: invokevirtual #744                // Method com/spotify/docker/client/shaded/com/google/common/collect/ImmutableSet.iterator:()Lcom/spotify/docker/client/shaded/com/google/common/collect/UnmodifiableIterator;

from helios.

On the other hand, upgrading docker-client so that the NetworkSettings.ports() method does not return a type from the com.google.common.collect package should also fix this.

We can return the built in Collection types, but AutoValue docs recommend returning the Immutable types. See https://github.com/google/auto/blob/master/value/userguide/howto.md#mutable_property

from helios.

I think they mean that as a general principle and not something directly related to using auto-value.

I started on a branch to reduce our usage of Guava in the public API of docker-client, but it might break a lot of methods on classes in the 'messages' package that use the immutable Builders or offer varargs setters.

from helios.

Can I close this issue since the original problem is now fixed?

I think they mean that as a general principle and not something directly related to using auto-value.

Are you sure? The example they give here has ImmutableList as the return type of the getter.

@AutoValue
public abstract class ListExample {
  public static ListExample create(String[] mutableNames) {
    return new AutoValue_ListExample(ImmutableList.copyOf(mutableNames));
  }

  public abstract ImmutableList<String> names();
}

from helios.

@davidxia my point was more that it is a recommendation and not a requirement when using auto-value, and we don't have to follow it if it leads to a problematic design like in this case.

I'm not sure if this is still a problem or not, I haven't tried building a new project that uses helios-testing:shaded since #1141 was done - there might be other problems.

from helios.

To elaborate a little bit:

Returning Guava types (ImmutableList, ListenableFuture, etc.) from the public API surface of helios-testing or docker-client means that the users of these libraries have to be aware of Guava too. This has caused a lot of pain internally for projects that use a number of clients that depend on differing Guava versions, since there have been a number of incompatibilities between recent Guava versions recently.

Trying to alleviate this by shading and relocating the Guava classes in helios-testing or docker-client would seem like an obvious solution, but if the public API is returning Guava types, then these uses get relocated too. This means that someone using the normal helios-testing passes in a com.google.guava.whatever.ImmutableList to a docker-client/helios-testing method, but if they want to use helios-testing:shaded then now they have to change their code to pass in a com.spotify.helios.relocated.com.google.guava.whatever.ImmutableList instance. This makes it harder for someone to switch to using the shaded artifact as they have to update their code and not just the coordinates of the artifact in their pom. It gets more complicated if helios-testing (regular) is to use docker-client:shaded. So in the end the relocation is still painful to users, as the use of Guava is not purely internal to the library.

For a value class like NetworkSettings I think it is a bad idea (from the lessons we've learned with all the Guava upgrade pains internally) to return Guava collection types as opposed to the core types like Set or List for just this reason. It also makes it harder to ever decide to remove Guava from the library, as now that is a breaking API change for users.

Should note that if you decide to not expose fields as Immutable/Guava types, you can still use them internally in the auto-value static factory - accept a List and wrap it in a ImmutableList.copyOf() - without exposing this to users.

Also for a class like NetworkSettings I don't think there is any reasonable expectation that a user could add elements to a field like List ports(), especially since I don't think the client ever accepts a user-supplied NetworkSettings as an input to an operation. I would be a bigger fan of communicating immutability with javadocs or annotations rather than using the Guava type.

from helios.

from helios.

No, I hadn't started on that. That type of change might be considered breaking, since code like the following would fail to compile:

ImmutableList<?> ports = networkSettings.ports() // if ports() now returns java.util.List

from helios.

from helios.

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

from helios.