#744 CompatibleFieldSerializer and fields declared as super-types

[theigl]

This PR attempts to resolve #744.

The original fix pushed by @NathanSweet in fb10f37 only works for fields that have the same type as the object stored in it. It fails for fields declared as super-types and interfaces.

This PR adds a check for primitive types originally proposed by @isaki in #670 and uses the concrete value class in all other instances.

[theigl]

@isaki: I'm not 100% sure my solution achieves the same as yours. You mentioned a failing unit test due to primitive boxing in #670. Was it an existing test or one you added locally to verify your fix?

[isaki]

It was something failing in my test applications. I basically had multiple JARs built with different implementations of the same class to trigger the JVM crash. My final PR had all the required fixes.

[theigl]

Thanks for the info @isaki!

My implementation also prevents boxing, but it still sets the field's valueClass. I have to think about the potential consequences some more. If I'm not 100% sure that my solution works in all cases, I'll replace it with the conditional from your original PR tomorrow.

[theigl]

I was able to reproduce the test failures you mentioned @isaki. I never ran the tests before I added my condition to prevent boxing. When I remove the condition, a bunch of tests fail with "Read type is incompatible with the field type: int -> Integer" etc.

So I guess we are good to go and can apply this PR.

@magro: Please take a look.

[isaki]

I actually think this might run into issues; in my original investigation setting value class even to the primitive type caused problems. Based on the "Files changed" tab you are assigning valueClass in all cases and then invoking the set method on the cachedField instance. I suspect that you have reintroduced the issues my checks intended to prevent. I could be wrong; it's been awhile since I looked at this.

Unfortunately I no longer have the project that I used to generate these issues, however, it would not take me long to recreate. I shall endeavor to do so later today/tomorrow. I will test both my approach and yours to see if the things work correctly. If it turns out my paranoia wasn't required, all the better!

[theigl]

Unfortunately I no longer have the project that I used to generate these issues, however, it would not take me long to recreate. I shall endeavor to do so later today/tomorrow. I will test both my approach and yours to see if the things work correctly. If it turns out my paranoia wasn't required, all the better!

That would be great! We can add the condition from your PR, but I would feel much more comfortable doing it because of a failing test-case.

[theigl]

I actually think this might run into issues; in my original investigation setting value class even to the primitive type caused problems. Based on the "Files changed" tab you are assigning valueClass in all cases and then invoking the set method on the cachedField instance. I suspect that you have reintroduced the issues my checks intended to prevent. I could be wrong; it's been awhile since I looked at this.

It is possible that this causes other issues, but not very likely because the fix applied by Nate also sets the valueClass in all cases. Do you remember if you verified your tests against Nate's fixes? If so, we should be relatively safe.

[isaki]

I did not test against Nate's fixes, no; I had assumed he had.

[isaki]

This is very time consuming, but the current release version has at least one bug. My test setup involved commenting in and out various lines to create 3 different jars. One used a String field, one used a Long field, and one used a long field; all three had the same class and name.

I've pushed my work in progress code to here: https://github.com/isaki/kryo-jvm-test

Kryo 5.0.0-RC7 Release

Source	Target	Result	Notes
String	String	Success	N/A
String	Long	com.esotericsoftware.kryo.KryoException: Read type is incompatible with the field type: String -> Long (io.isaki.kryojvmtest.Payload#id)	N/A
String	long	com.esotericsoftware.kryo.KryoException: Read type is incompatible with the field type: String -> long (io.isaki.kryojvmtest.Payload#id)	N/A
Long	String	com.esotericsoftware.kryo.KryoException: Read type is incompatible with the field type: long -> String (io.isaki.kryojvmtest.Payload#id)	N/A
Long	Long	com.esotericsoftware.kryo.KryoException: Read type is incompatible with the field type: long -> Long (io.isaki.kryojvmtest.Payload#id)	BUG, likely in serialization given the exception text.
Long	long	io.isaki.kryojvmtest.Payload@210366b4 (19000)	The primitive based JAR read the boxed value no problem
long	String	com.esotericsoftware.kryo.KryoException: Read type is incompatible with the field type: long -> String (io.isaki.kryojvmtest.Payload#id)	N/A
long	Long	com.esotericsoftware.kryo.KryoException: Read type is incompatible with the field type: long -> Long (io.isaki.kryojvmtest.Payload#id)	This could probably be made to work.
long	long	io.isaki.kryojvmtest.Payload@210366b4 (18000)	The primitive based JAR read the primitive value no problem

I believe that the current implementation is broken in that it is serializing a boxed value as a primitive. I double checked my build and ensured I was working with a boxed value.

EDIT: This was simply to establish a baseline for the current released version. This testing is time consuming and I will test the proposed PR as well as my initial change tomorrow.

[theigl]

@isaki: Thank you so much for your work!

I can reproduce the bug you found, but I don't fully understand it yet. The value is clearly written as a Long but read as a long for some reason. The same thing happens on my branch and the condition from your PR does not work either.

I think we have to add a check for primitives and their wrapper types instead of simply checking if the classes are assignable. This would also support changes from long to Long and vice versa.

[theigl]

I just pushed a better solution for this issue in 6e61f77

I completely removed the condition that checks for primitive classes when serializing and made the assignable check aware of primitive types and their wrappers. I also added a test that should guard against these issues in the future.

[isaki]

I'll build your patch and run my tests later this afternoon.

[isaki]

My testing results show things look good except for one small case.

Source	Target	Result	Notes
String	String	io.isaki.kryojvmtest.Payload@67784306 (stringid)	N/A
String	Long	com.esotericsoftware.kryo.KryoException: Read type is incompatible with the field type: String -> Long (io.isaki.kryojvmtest.Payload#id)	N/A
String	long	com.esotericsoftware.kryo.KryoException: Read type is incompatible with the field type: String -> long (io.isaki.kryojvmtest.Payload#id)	N/A
Long	String	com.esotericsoftware.kryo.KryoException: Read type is incompatible with the field type: long -> String (io.isaki.kryojvmtest.Payload#id)	BUG? That should read Long - > String
Long	Long	io.isaki.kryojvmtest.Payload@eec5a4a (19000)	N/A
Long	long	io.isaki.kryojvmtest.Payload@67784306 (19000)	Nice!
long	String	com.esotericsoftware.kryo.KryoException: Read type is incompatible with the field type: long -> String (io.isaki.kryojvmtest.Payload#id)	N/A
long	Long	io.isaki.kryojvmtest.Payload@eec5a4a (18000)	Nice!
long	long	io.isaki.kryojvmtest.Payload@67784306 (18000)	N/A

[theigl]

@isaki: Thanks again! I would consider the remaining issue more a problem of wording than a real bug. I don't think it has any consequences other than a slightly incorrect error message.

@magro: Please review. This fixes #744 and resolves a number of hidden issues with CompatibleFieldSerializer.

[jgiannuzzi]

Thanks a lot for your work on fixing these issues, @theigl and @isaki!

We are also hitting #744 and it prevents us from upgrading to the latest Kryo, even though it contains other fixes that we need. Could you please review this PR, @magro and @NathanSweet? Thanks!

[magro]

Many thanks for your really great work here, @theigl and @isaki!