Fix for CompatibleFieldSerializer inheritance issue #424

xfrendx · 2016-05-13T20:56:54Z

My try to fix the PR #187. This solution here doesn't ignore "duplicate" fields, because they are not duplicate at all. They just belongs to different classes...

Added a central method FieldSerializer#getFieldName(CachedField) that returns per default the field name. This behaviour is overriden in CompatibleFieldSerialzer and can be driven by the "extendedFieldNaming" boolean. If extendedFieldNaming is true, then Class simple name is added to the field name... and this helps to find the right field when deserializing. It's not perfect, but don't want to have it too verbose.

Any suggestions will be welcome!

magro · 2016-05-14T22:38:59Z

Intersting approach, great idea!

Suggestions:

I'd prefer to have the test in the existing CompatibleFieldSerializerTest.
The extendedFieldNaming config setting should go into a CompatibleFieldSerializerConfig which should also be added to Kryo, see also TaggedFieldSerializerConfig.
Maybe we should also mention this setting in the README?

@NathanSweet WDYT?

xfrendx · 2016-05-15T21:28:21Z

I did some changes:

test now in the existing CompatibleFieldSerializerTest
added CachedFieldNameStrategy with two implementations DEFAULT AND EXTENDED
per default, the DEFAULT strategy is activated
you can switch with kryo.getFieldSerializerConfig().setCachedFieldNameStrategy(FieldSerializer.CachedFieldNameStrategy.EXTENDED);
README will be extended after this solutions passes the kryo technical QA

magro · 2016-05-16T09:03:19Z

I wonder if it makes any sense for FieldSerializer to use anything else than default - perhaps only allow to override getFieldName and move/keep the strategy close to the CompatibleFieldSerializer in its own config. WDYT?

xfrendx · 2016-05-16T09:22:44Z

There are some methods in FieldSerializer that uses the CacheField name:

getField
removeField
compare

... all this methods would need to be overriden in CompatibleFieldSerializer, otherwise they won't work correctly (evt. you can be confused by getField...what CachedField you get when multiple same named fields are present).

... but I got your point. I don't like to attack the FieldSerializer code

xfrendx · 2016-05-25T07:08:15Z

Hi @magro,

would like to go ahead with this PR, but I'm confused somehow. The getField(String) and removeField(String) in FieldSerializer can't work correctly with the current/default approach. They get/remove only the first field with same name... but if there are more fields with same name (but different class), they give wrong result.

I can do now the fix only for CompatibleFieldSerializer...but need then override the getField/removeField methods.... To fix it in FieldSerializer already seems to be much better for me.

WDYT?

magro · 2016-05-25T23:19:23Z

Sounds reasonable to fix FieldSerializer, can you do this in a separate commit or even separate PR? Adding a test for this as well would be super awesome :-)

xfrendx · 2016-06-05T20:38:04Z

Hi @magro,

the new PR would have identical commits like this one ;) If you think it's neccessary, I'll do it. But pls. explain me how it should help us?

I'll try to sum up the new features/bugfixes that are comming with this PR:

added CachedFieldNameStrategy that can be set to FieldSerializerConfig
strategy influences how CachedFields are recognized in:
- FieldSerializer.getField(String)
- FieldSerializer.removeField(String)
- FieldSerializer.compare(CachedField, CachedField)
- CompatibleFieldSerializer.read(...)
DEFAULT strategy uses just field name
EXTENDED strategy uses Class simple name and field name. That helps in better identification in case that there are same named fields in class inheritance path.
- E.g. you can't get the right field via FieldSerializer.getField method without
- CompatibleFieldSerializer doesn't work correctly...you'll not having the same data back after storing to Kryo

magro · 2016-06-05T21:17:20Z

A PR with a bugfix and a test usually can be merged fairly fast. Therefore I mentioned this option. You can also add the commit to this PR. Please add a test that shows the issue for FieldSerializer as well. Thanks!

magro · 2016-06-07T16:51:06Z

@xfrendx We're going to have a release soon, what do you think when you can finalize this?

xfrendx · 2016-06-07T18:15:19Z

Hi @magro,

I added the TCs right now, pls. check it. Thx

magro · 2016-06-07T23:08:33Z

test/com/esotericsoftware/kryo/FieldSerializerTest.java

@@ -621,7 +621,71 @@ public void testMultipleTimesAnnotatedMapFields () {

 		assertFalse("Exception was expected", true);
 	}
-
+


If you don't mind you could put this into a FieldSerializerInheritanceTest - sorry that I've suggested to add this to this test, I wasn't aware that it's already that big...

magro · 2016-06-07T23:22:57Z

@xfrendx Thanks, I added minor/cosmetic comments. Once they're resolved please squash the changes to a single commit and change the commit message to mention both FieldSerializer and CompatibleFieldSerializer. I'll then merge the PR.

xfrendx · 2016-06-08T07:48:06Z

@magro your suggestions have been added and changes suqashed to one commit. Let me know if there is something left...

magro · 2016-06-09T22:45:54Z

test/com/esotericsoftware/kryo/FieldSerializerInheritanceTest.java

@@ -0,0 +1,129 @@
+package com.esotericsoftware.kryo;


Please add the license header.

magro · 2016-06-09T22:53:23Z

@xfrendx Great, thanks! In the new test only the license header is missing. Please change the commit message to s.th. like "Support 'duplicate'/overwritten fields in (Compatible)FieldSerializer" and add a description with the what and why of the change. This would be perfect :-)

…e how CachedFields are recognized in FieldSerieliazer and CompatibleFieldSerializer. It is an improvement for FieldSerializer (influences getField/removeField methods) and nearly a must have in CompatibleFieldSerialzer in case your class hierarchy contains same named fields (e.g. class A has a field named "a", and class B extends A and also contains a field named "a"). Per default, both mentioned serializers behave like before (DEFAULT strategy used). To change it, use `kryo.getFieldSerializerConfig().setCachedFieldNameStrategy(FieldSerializer.CachedFieldNameStrategy.EXTENDED);`. At this moment, class simple name is added to CachedField name. This helps in CompatibleFieldSerializer write the right value to right class field. Without EXTENDED, all same named values are written just to the first found field.

xfrendx · 2016-06-10T06:42:03Z

@magro done

magro · 2016-06-10T07:20:57Z

@xfrendx Following general rules on commit messages they should have a short title/subject identifying the change. The description should be separated from the title with an empty line. See also http://chris.beams.io/posts/git-commit/

Added an option/strategy to influence how CachedFields are recognized in FieldSerieliazer and CompatibleFieldSerializer. It is an improvement for FieldSerializer (influences getField/removeField methods) and nearly a must have in CompatibleFieldSerialzer in case your class hierarchy contains same named fields (e.g. class A has a field named "a", and class B extends A and also contains a field named "a"). Per default, both mentioned serializers behave like before (DEFAULT strategy used). To change it, use `kryo.getFieldSerializerConfig().setCachedFieldNameStrategy(FieldSerializer.CachedFieldNameStrategy.EXTENDED);`. At this moment, class simple name is added to CachedField name. This helps in CompatibleFieldSerializer write the right value to right class field. Without EXTENDED, all same named values are written just to the first found field. Resolves: EsotericSoftware#424 See also: EsotericSoftware#187

xfrendx · 2016-06-10T07:32:52Z

@magro thx for you patience & done

Added an option/strategy to influence how CachedFields are recognized in FieldSerieliazer and CompatibleFieldSerializer. It is an improvement for FieldSerializer (influences getField/removeField methods) and nearly a must have in CompatibleFieldSerialzer in case your class hierarchy contains same named fields (e.g. class A has a field named "a", and class B extends A and also contains a field named "a"). Per default, both mentioned serializers behave like before (DEFAULT strategy used). To change it, use `kryo.getFieldSerializerConfig().setCachedFieldNameStrategy(FieldSerializer.CachedFieldNameStrategy.EXTENDED);`. At this moment, class simple name is added to CachedField name. This helps in CompatibleFieldSerializer write the right value to right class field. Without EXTENDED, all same named values are written just to the first found field. Resolves: EsotericSoftware#424 See also: EsotericSoftware#187

Added an option/strategy to influence how CachedFields are recognized in FieldSerieliazer and CompatibleFieldSerializer. It is an improvement for FieldSerializer (influences getField/removeField methods) and nearly a must have in CompatibleFieldSerialzer in case your class hierarchy contains same named fields (e.g. class A has a field named "a", and class B extends A and also contains a field named "a"). Per default, both mentioned serializers behave like before (DEFAULT strategy used). To change it, use `kryo.getFieldSerializerConfig().setCachedFieldNameStrategy( FieldSerializer.CachedFieldNameStrategy.EXTENDED);`. At this moment, class simple name is added to CachedField name. This helps in CompatibleFieldSerializer write the right value to right class field. Without EXTENDED, all same named values are written just to the first found field. Resolves: EsotericSoftware#424 See also: EsotericSoftware#187

magro · 2016-06-10T07:54:05Z

@xfrendx Awesome, thanks! :-)

magro reviewed Jun 7, 2016
View reviewed changes

xfrendx force-pushed the master branch from f45cdea to 4ee2d32 Compare June 8, 2016 07:45

xfrendx pushed a commit to xfrendx/kryo that referenced this pull request Jun 8, 2016

PR EsotericSoftware#424 Fixes

4ee2d32

magro reviewed Jun 9, 2016
View reviewed changes

xfrendx force-pushed the master branch from 4ee2d32 to 6f22af8 Compare June 10, 2016 06:40

xfrendx force-pushed the master branch from 6f22af8 to 654a6be Compare June 10, 2016 07:32

xfrendx force-pushed the master branch from 654a6be to f45e27b Compare June 10, 2016 07:34

xfrendx force-pushed the master branch from f45e27b to 5a7b7c5 Compare June 10, 2016 07:41

magro merged commit 3520bc4 into EsotericSoftware:master Jun 10, 2016

magro mentioned this pull request Jun 10, 2016

CompatibleFieldSerializer but duplicate field name will be accepted #187

Closed

magro mentioned this pull request Jun 9, 2018

CompatibleFieldSerializer does not support fields with the same name in sub classes #261

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix for CompatibleFieldSerializer inheritance issue #424

Fix for CompatibleFieldSerializer inheritance issue #424

xfrendx commented May 13, 2016 •

edited by magro

Loading

magro commented May 14, 2016

xfrendx commented May 15, 2016

magro commented May 16, 2016

xfrendx commented May 16, 2016

xfrendx commented May 25, 2016

magro commented May 25, 2016

xfrendx commented Jun 5, 2016

magro commented Jun 5, 2016

magro commented Jun 7, 2016

xfrendx commented Jun 7, 2016

magro Jun 7, 2016

magro commented Jun 7, 2016

xfrendx commented Jun 8, 2016 •

edited

Loading

magro Jun 9, 2016

magro commented Jun 9, 2016

xfrendx commented Jun 10, 2016

magro commented Jun 10, 2016

xfrendx commented Jun 10, 2016

magro commented Jun 10, 2016

		@@ -621,7 +621,71 @@ public void testMultipleTimesAnnotatedMapFields () {

		assertFalse("Exception was expected", true);
		}

Fix for CompatibleFieldSerializer inheritance issue #424

Fix for CompatibleFieldSerializer inheritance issue #424

Conversation

xfrendx commented May 13, 2016 • edited by magro Loading

magro commented May 14, 2016

xfrendx commented May 15, 2016

magro commented May 16, 2016

xfrendx commented May 16, 2016

xfrendx commented May 25, 2016

magro commented May 25, 2016

xfrendx commented Jun 5, 2016

magro commented Jun 5, 2016

magro commented Jun 7, 2016

xfrendx commented Jun 7, 2016

magro Jun 7, 2016

Choose a reason for hiding this comment

magro commented Jun 7, 2016

xfrendx commented Jun 8, 2016 • edited Loading

magro Jun 9, 2016

Choose a reason for hiding this comment

magro commented Jun 9, 2016

xfrendx commented Jun 10, 2016

magro commented Jun 10, 2016

xfrendx commented Jun 10, 2016

magro commented Jun 10, 2016

xfrendx commented May 13, 2016 •

edited by magro

Loading

xfrendx commented Jun 8, 2016 •

edited

Loading