helios-testing shaded artifact is broken

[mattnworb]

#1137 added a "shaded" artifact for helios-testing to attempt to relocate Guava classes to a new package, so that users of helios-testing are not impacted whenever helios updates the Guava version it uses (which tends to cause a chain of headaches because of version incompatibilities).

The shaded helios-testing jar appears to be broken and unusable, at least for users of HeliosSoloDeployment: a NoSuchMethodError is thrown in a simple test case like

private static final HeliosDeploymentResource soloResource =
      new HeliosDeploymentResource(HeliosSoloDeployment.fromEnv().build());

java.lang.NoSuchMethodError: com.spotify.docker.client.messages.NetworkSettings.ports()Lcom/spotify/helios/testing/shaded/com/google/common/collect/ImmutableMap;
  at com.spotify.helios.testing.HeliosSoloDeployment.getHostPort(HeliosSoloDeployment.java:462)
  at com.spotify.helios.testing.HeliosSoloDeployment.<init>(HeliosSoloDeployment.java:141)
  at com.spotify.helios.testing.HeliosSoloDeployment$Builder.build(HeliosSoloDeployment.java:879)
  at com.spotify.foobar.ServiceTestIT.<clinit>(ServiceTestIT.java:27)

This looks to be due to the fact that the maven-shade-plugin is rewriting the bytecode for the getHostPort method in com.spotify.helios.testing.HeliosSoloDeployment to look like the following (this is from the 0.9.146 JAR):

private java.lang.String getHostPort(java.lang.String, int) throws com.spotify.helios.testing.HeliosDeploymentException;
  descriptor: (Ljava/lang/String;I)Ljava/lang/String;
  flags: ACC_PRIVATE
  Code:
    stack=7, locals=7, args_size=3
      ...
      35: invokevirtual #733                // Method
      38: invokevirtual #737                // Method com/spotify/helios/testing/shaded/com/google/common/collect/ImmutableMap.entrySet:()Lcom/spotify/helios/testing/shaded/com/google/common/collect/ImmutableSet;
      41: invokevirtual #742                // Method com/spotify/helios/testing/shaded/com/google/common/collect/ImmutableSet.iterator:()Lcom/spotify/helios/testing/shaded/com/google/common/collect/UnmodifiableIterator;

This last 3 lines are the problem - it is trying to invoke a method named com/spotify/docker/client/messages/NetworkSettings.ports with no arguments (()) which is expected to return com/spotify/helios/testing/shaded/com/google/common/collect/ImmutableMap.

It seems as if the plugin does not realize that in code like this

final NetworkSettings settings = dockerClient.inspectContainer(containerId).networkSettings();
for (final Map.Entry<String, List<PortBinding>> entry : settings.ports().entrySet()) {

the invokevirtual operations that invoke the com.google.common.collect.ImmutableMap NetworkSettings.ports() method should not be rewritten to return com.spotify.helios.testing.shaded.com.google.common.collect.ImmutableMap since this class is supplied from another library and will never actually return this rewritten type.

In looking at this, I noticed that we are using version 2.4.3 of maven-shade-plugin and that 3.0.0 exists, I'm not sure yet if upgrading there would fix this.

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 (#1138 or #1141) should also fix this.

But in general simply relocating Guava classes is not as simple as thought and might have to be scrapped as unworkable.

[mattnworb]

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;

[davidxia]

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

[mattnworb]

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.

[davidxia]

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();
}

[mattnworb]

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

[mattnworb]

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.

[davidxia]

Makes sense. Should we change the return types to regular collections? Did you have a branch somewhere?

[mattnworb]

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

[davidxia]

Yea we can bump the major version. Seems like solving the relocation problem might be worth it to some users. I would’ve never followed the autovalue examples of returning immutable types if I knew it was going to be such a headache later on.

[stale]

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.