Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ClassTypeInformation computes incorrect TypeVariable mappings for recursive generics [DATACMNS-896] #1354

Closed
spring-projects-issues opened this issue Aug 18, 2016 · 8 comments
Assignees
Labels
in: core type: bug

Comments

@spring-projects-issues
Copy link

@spring-projects-issues spring-projects-issues commented Aug 18, 2016

Ryan Gustafson opened DATACMNS-896 and commented

When using a class with type variables in the class hierarchy, which contains variables referencing other classes also using common parts of the same class hierarchy, the type variables type mappings for the referenced classes are incorrectly mixed into the original classes type variables.

See the attached JUnit test which can demonstrate the problem. It may require several runs for it appear, but it is consistently failing for us.

I believe this is due the essentially random ordering of the HashSet.entrySet().iterator() on line 130 and the putAll() on line 136:

https://github.com/spring-projects/spring-data-commons/blob/master/src/main/java/org/springframework/data/util/ClassTypeInformation.java#L130
https://github.com/spring-projects/spring-data-commons/blob/master/src/main/java/org/springframework/data/util/ClassTypeInformation.java#L136

Depending on the sequencing, you either get your own type variable value, or some other related classes. Last one wins.

We've tried modifying this class to removed the putAll (line 136), and it fixes this test, and does not appear to cause problems for 100s of other Spring Data Repository based tests we have. It's not clear why the type variable mappings from other classes that are not direct superclasses or interfaces are included at all, it seems very wrong.

It took us 4 days to track down this as the ultimate source of a random startup failures of a Spring Data JPA Repository while parsing of dynamic query methods on a particular part of our domain model. It was very hard to reproduce, but one developers box seemed to want to fail rather consistently, which gave the opportunity to narrow it down. My hope, for now, is to work around it by mapping the query method to a custom implementation


Affects: 1.11.4 (Gosling SR4), 1.12.1 (Hopper SR1), 1.13 M1 (Ingalls)

Attachments:

Referenced from: commits 8cf4aae, 4fa84fc, 50026d4

Backported to: 1.12.3 (Hopper SR3), 1.11.5 (Gosling SR5)

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Aug 18, 2016

Oliver Drotbohm commented

I don't think we can remove that line. The code was introduced to fix DATACMNS-594 and looking at the test case I think this is a very valid construct. I ran your test case quite a couple of times but never actually got it show the expected error.

Given that you've already spend quite a bit of time on this one, a quick question: do you think it might help if we ourselves switched from a plain HashMap to a LinkedHashMap? I am not quite sure how the TypeVariables could even override each other because, even if they have the same name (T in out test case) they're actually different instances with different has codes so that we're basically just creating a flat map from a nested structure and the keys should never ever overlap?

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Aug 19, 2016

Ryan Gustafson commented

The reason you're not seeing the failure, is the same reason this was such a pain to track down. The ordering of the HashMap is not predictable. For our original problem, we got lucky with one developer's box reliably processing in an ordering that manifested the problem. Numerous other environments would never manifest, or with low probability, and not once would it manifest when run in debug mode.

These are not different TypeVariables they are in fact the same variable bound to different actual classes by each subclass. To attempt to put these actual classes into a common Map indexed by the TypeVariable, gives the wrong impression of the original class. In the original test case, the ZZZ TypeVariable on CommonClass was mapped 4 different times to separate class values. I'll attach an updated Test case to demonstrate the commonality of the TypeVariable, and that each class maps T to a different type. I've also added more type variables and different compositing classes, all in the hopes of increasing the chance of a failure.

Using a LinkedHashMap here would only give a consistent runtime ordering, but only if GenericTypeResolver.getTypeVariableMap(...) used one and itself processed in a predictable fashion. Even if you did this, it would not be useful, as a consistent ordering will just make the problem always manifest or never manifest, for a given class arrangement.

If you still need to have the putAll in there, then I think the asked for classes TypeVariables should be added last to the map, and not interleaved with the putAll contents

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Aug 19, 2016

Ryan Gustafson commented

Additionally, here's some of the things we noticed would cause the problem to manifest or not:

  • Running in debug.
  • Adding @AfterClass to a test.
  • In the test class provided, removing the wilcard (?) usages in the composite subclasses.
  • Running tests in Eclipse vs. Gradle.
  • Running on different machines.

None of these are related to the problem, but maybe they will help you find a means to trigger it

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Aug 19, 2016

Ryan Gustafson commented

Locally, I'm using the following temporary modification to the ClassTypeInformation.getTypeVariableMap looping logic, in order shake out places in our Repositories we need to avoid Spring Data JPA dynamic queries. Toggle the incorrect flag to get desired behavior.

boolean incorrect = true;
if (incorrect) {
    // Put mine first
    for (Entry<TypeVariable, Type> entry : source.entrySet()) {
        map.put(entry.getKey(), entry.getValue());
    }
}
// Put others
for (Entry<TypeVariable, Type> entry : source.entrySet()) {
    Type value = entry.getValue();
    if (value instanceof Class) {
        map.putAll(getTypeVariableMap((Class<?>) value, visited));
    }
}
if (!incorrect) {
    // Put mine last
    for (Entry<TypeVariable, Type> entry : source.entrySet()) {
        map.put(entry.getKey(), entry.getValue());
    }
}

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Aug 19, 2016

Oliver Drotbohm commented

Thanks for the detailed analysis, Ryan. The new example you provide fails constantly, so that's a good thing. I can see the issue being the same generic type being used on nested generics declarations, sort of like this (for reference):

class GenericType<T> {}

class Nested extends GenericType<String> {}

class Concrete extends GenericType<Nested> {}

Our traversal of the generics discovers T bound to String in Nested and as that is extending GenericType as well, the T detected here is indeed the same instance as the T within the map discovered for Concrete. As we add the nested types after we add the original entry, the nested discovery overwrites the more local one. That means, we can fix this by making sure that nested type variable mappings never replace an already existing one.

With that fix in place I see your newly provided test sort of succeed. It still fails but due to the fact that you expect a raw List to be equivalent to List<?>, but I guess the gist of it is that the right type is discovered. I could see my sample fail before the fix and succeed after it has been applied.

I deployed the fix for master and the maintenance branches for Hopper and Gosling. Would you mind giving the snapshots a spin?

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Aug 19, 2016

Ryan Gustafson commented

The SNAPSHOT allows the test to pass, which should fix it for our application usage. Thank you very much!

Sorry, I forgot to give you the test variant that had the wildcards in the composite types removed, I noticed the failures upon corrected behavior after I uploaded the file.

I'd like to point out in nested cases like this, the contents of the Map are predictable only for the top level type. For the nested types, it's not always clear what value will be in the map. In this arrangement, for the Concrete class, consider what is in the map for the Comparable<T> type variable

class GenericType<T,U> {}
class Nested extends GenericType<String, Long> {}
class Concrete extends GenericType<Nested, Integer> {}

Is it String, Long, or Integer ? Also notice that Comparable<T> has nothing to do with the top level type directly, yet it is in the map.

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Aug 19, 2016

Oliver Drotbohm commented

No worries, the test case really helped to get to the root of the problem, and you already invested such a lot of time. It's me who has to thank you! :)

I don't get the issue regarding Comparable<T> though. It's not a type variable, it's a parameterized type. For the types you gave, on Concrete, T evaluates to Nested, U evaluates to Integer (i.e. the top-level bound generics). That's correct I think. Also the type variable map for Concrete doesn't contain Comparable — again, as it's a type and not a type variable.

{T=class java.lang.Integer, U=class java.lang.Integer, T=class org.springframework.data.util.ClassTypeInformationUnitTests$OtherNested}

You see the first T here resolves to Integer but that's bound to Integer itself (coming from the Comparable<T> implementation). I totally see that this additional variable might not be needed in this particular case, but — again — as the test cases for DATACMNS-594 show, they might be needed in other, also quite complex generics scenarios

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Aug 19, 2016

Ryan Gustafson commented

Sorry for the confusion, I used Comparable<<T>> as short hand for the T type variable defined on Comparable. You are correct, I shouldn't do that, as it is the same notation for a parameterized type.

With that cleared up, the T on Comparable contained in the map could be the T=Integer you listed. It could just as well as been T=String or T=Long too. Again, it's a common type variable, specifically the T on Comparable. getTypeVariableMap is a recursive called on each type parameter encountered that is a Class. So it's going to be called explicitly on Concrete the top class, and then on either Nested or Integer, and ultimately recursively onto String and Long. The processing order is not clear, so which value for T from Comparable gets into the map is not deterministic.

To make this practical, I get in one run:

{T=class java.lang.Integer, U=class java.lang.Integer, T=class SpringDataClassTypeInformationTypeVariableToTypeMappingBugTest$Nested}

Next, when run in debug mode:

{T=class java.lang.String, T=class SpringDataClassTypeInformationTypeVariableToTypeMappingBugTest$Nested, U=class java.lang.Integer}

In one case, T=Integer, in the next T=String

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: core type: bug
Projects
None yet
Development

No branches or pull requests

2 participants