Once more, GenericJson.clone() method blows up when trying to clone Google reads

[jean-philippe-martin]

This is related to #293, but #293's fix doesn't work for this problem.

From the Hellbender project (broadinstitute/hellbender#650):

When we copy a Google read (ie., a com.google.api.services.genomics.model.Read), we currently call into the GenericJson.clone() method, which uses reflection to deep copy the fields. However it seems to run into difficulty with com.google.common.collect.Lists$TransformingRandomAccessList.

Here's the stack trace:

Error:   (e0086ea6f7eea4e6): java.lang.IllegalArgumentException: unable to create new instance of class com.google.common.collect.Lists$TransformingRandomAccessList possibly because it is not public
    at com.google.api.client.util.Types.handleExceptionForNewInstance(Types.java:165)
    at com.google.api.client.util.Types.newInstance(Types.java:120)
    at com.google.api.client.util.Data.clone(Data.java:229)
    at com.google.api.client.util.Data.deepCopy(Data.java:302)
    at com.google.api.client.util.GenericData.clone(GenericData.java:172)
    at com.google.api.client.json.GenericJson.clone(GenericJson.java:90)
    at com.google.api.services.genomics.model.LinearAlignment.clone(LinearAlignment.java:128)
    at com.google.api.services.genomics.model.LinearAlignment.clone(LinearAlignment.java:34)
    at com.google.api.client.util.Data.clone(Data.java:212)
    at com.google.api.client.util.Data.deepCopy(Data.java:302)
    at com.google.api.client.util.GenericData.clone(GenericData.java:172)
    at com.google.api.client.json.GenericJson.clone(GenericJson.java:90)
    at com.google.api.services.genomics.model.Read.clone(Read.java:548)
    at org.broadinstitute.hellbender.utils.read.GoogleGenomicsReadToGATKReadAdapter.copy(GoogleGenomicsReadToGATKReadAdapter.java:637)
    at org.broadinstitute.hellbender.tools.dataflow.transforms.bqsr.ApplyBQSRTransform$ApplyBQSR.processElement(ApplyBQSRTransform.java:65)

[droazen]

It would be good to get a general solution to this problem of instantiating private types, instead of special-casing each such type we encounter -- ie., can we use reflection to getDeclaredConstructor() and then constructor.setAccessible(true)?

[ejona86]

Where are you getting this Read object from? It seems like it could be considered a bug in that code. Trying to make clone() work for arbitrary collections seems to be a lost cause.

I looked at com.google.cloud.genomics.utils.ReadUtils and it seems to use "normal" types for alignedQuality and info (as seen in workAroundHttpClientCloneBug()). However, it does look like it could be the cause of this bug with cigar.

(And I got to ReadUtils in the first place trying to locate where Read was constructed.)

As I say in 8d2d6a: "Arrays$ArrayList does not have a zero-arg constructor." And Arrays$ArrayList uses a final field.

The problem is identical here:

  private static class TransformingRandomAccessList<F, T> extends AbstractList<T>
      implements RandomAccess, Serializable {
    final List<F> fromList;
    final Function<? super F, ? extends T> function;

    TransformingRandomAccessList(List<F> fromList, Function<? super F, ? extends T> function) {

So a generic solution that does a precise clone does not seem possible. The only alternative I see is to fall back to creating an ArrayList if the specific list type can't be created. That isn't guaranteed to work either though, and is hard to predict all the repercussions. It seems this is only remotely possible with List; other collection types would be impossible.

The best solution seems to be to change ReadUtils.

[droazen]

Creating a new ArrayList when an uninstantiable List is encountered in clone() seems like a good option -- it would enable clone() to succeed in many cases in which it currently does not.

The problem with patching ReadUtils is that this is not the only place where Reads are manipulated. A large amount of code across multiple projects mutates Reads, and auditing all of these to ensure that no uninstantiable Lists are passed in would be like playing whack-a-mole.

[jean-philippe-martin]

Eric, you seem to be referring to some assumptions made in the implementation of clone(). Where are those assumptions documented? All I could find was the text on GenericData https://developers.google.com/api-client-library/java/google-http-java-client/reference/1.20.0/com/google/api/client/util/GenericData which spells out restrictions that apply only to fields of the original objects that have the Key annotation. It says nothing about constructors, though, or of limitations on the contents of those fields.

On Thu, Aug 20, 2015 at 8:32 AM, droazen notifications@github.com wrote:

Creating a new ArrayList when an uninstantiable List is encountered in clone() seems like a good option -- it would enable clone() to succeed in many cases in which it currently does not.

The problem with patching ReadUtils is that this is not the only place where Reads are manipulated. A large amount of code across multiple projects mutates Reads, and auditing all of these to ensure that no uninstantiable Lists are passed in would be like playing whack-a-mole.

— Reply to this email directly or view it on GitHub https://github.com/google/google-http-java-client/issues/297#issuecomment-133052664 .

[ejona86]

GenericData/Data/DataUtil has been around since at least 2010, in basically the same state it is today. Sometime in 2010-2011 it got array support. And then in 2015 it received Arrays$asList support.

As I mentioned, even if I use an ArrayList (and assuming that doesn't have its own problems, which I'm not 100% certain of), that doesn't solve the problem for maps. And you already know that you have to do the same workaround for maps (as I can see in workAroundHttpClientCloneBug()).

[droazen]

Even if a general solution is not possible, fixing this just for uninstantiable Lists and Maps (by wrapping within ArrayList/HashMap as necessary) would alleviate the problem quite a bit (and maybe resolve our issues in the hellbender project completely).

[cypressf]

I encountered the same issue with java.util.Collections$SingletonList when trying to clone a com.google.api.services.webmasters.model.SearchAnalyticsQueryRequest. It'd also make my life a lot easier to use ArrayList in the clone where uninstantiable lists were used in the original object.

[elharo]

I looked at this and it seems to me that marking this class Cloneable was a mistake. Its graph can contain objects of arbitrary types and there's no guaranatee any of them can be cloned.