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

CompatibleFieldSerializer but duplicate field name will be accepted #187

Closed
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@bohrqiu

bohrqiu commented Jan 26, 2014

Features like
com.esotericsoftware.kryo.serializers.CompatibleFieldSerializer,but The
situation where a super class has a field with the same name as a
subclass will be accepted.

bohrqiu added some commits Jan 26, 2014

CompatibleFieldSerializer but duplicate field name will be accepted
Features like
com.esotericsoftware.kryo.serializers.CompatibleFieldSerializer,but The
situation where a super class has a field with the same name as a
subclass will be accepted.
@NathanSweet

This comment has been minimized.

Show comment
Hide comment
@NathanSweet

NathanSweet Jan 26, 2014

Member

DuplicateFieldNameAcceptedCompatibleFieldSerializer is a terrible class name. :) Maybe this should go in CompatibleFieldSerializer?

Member

NathanSweet commented Jan 26, 2014

DuplicateFieldNameAcceptedCompatibleFieldSerializer is a terrible class name. :) Maybe this should go in CompatibleFieldSerializer?

@bohrqiu

This comment has been minimized.

Show comment
Hide comment
@bohrqiu

bohrqiu Jan 26, 2014

yes, It is very terrible.If it is merge in CompatibleFieldSerializer,It will be more elegant. :) thank you accept it.

bohrqiu commented Jan 26, 2014

yes, It is very terrible.If it is merge in CompatibleFieldSerializer,It will be more elegant. :) thank you accept it.

@bohrqiu

This comment has been minimized.

Show comment
Hide comment
@bohrqiu

bohrqiu Jan 27, 2014

Can I merge this in CompatibleFieldSerializer ?

bohrqiu commented Jan 27, 2014

Can I merge this in CompatibleFieldSerializer ?

@NathanSweet

This comment has been minimized.

Show comment
Hide comment
@NathanSweet

NathanSweet Jan 27, 2014

Member

I don't have time to look more closely, but yes it seems like it fits better there. If you update the PR or create a new one with the functionality in CompatibelFieldSerializer it is more likely to be merged.

Member

NathanSweet commented Jan 27, 2014

I don't have time to look more closely, but yes it seems like it fits better there. If you update the PR or create a new one with the functionality in CompatibelFieldSerializer it is more likely to be merged.

@bohrqiu

This comment has been minimized.

Show comment
Hide comment
@bohrqiu

bohrqiu Jan 28, 2014

hi,NathanSweet,I have merge the functionality in to CompatibelFieldSerializer,and passed all the testcases.

bohrqiu commented Jan 28, 2014

hi,NathanSweet,I have merge the functionality in to CompatibelFieldSerializer,and passed all the testcases.

@romix

This comment has been minimized.

Show comment
Hide comment
@romix

romix Jan 28, 2014

Contributor

@bohrqiu Have you removed the new test cases from your latest pull request or have I just overlooked it? It should demonstrate the difference between old and new behavior.

Also, I don't see a clear statement in JavaDocs about name conflicts between child and parent classes and how they are solved. This should be stated in a very explicit way so that people know what to expect.

Contributor

romix commented Jan 28, 2014

@bohrqiu Have you removed the new test cases from your latest pull request or have I just overlooked it? It should demonstrate the difference between old and new behavior.

Also, I don't see a clear statement in JavaDocs about name conflicts between child and parent classes and how they are solved. This should be stated in a very explicit way so that people know what to expect.

@bohrqiu

This comment has been minimized.

Show comment
Hide comment
@bohrqiu

bohrqiu Jan 28, 2014

hi @romix DuplicateFieldNameAcceptedCompatibleFieldSerializerTest is used for test the new behavior.and name conflicts functionality had been merged in CompatibleFieldSerializer.
thanks your advice,I will add some comments in JavaDocs

bohrqiu commented Jan 28, 2014

hi @romix DuplicateFieldNameAcceptedCompatibleFieldSerializerTest is used for test the new behavior.and name conflicts functionality had been merged in CompatibleFieldSerializer.
thanks your advice,I will add some comments in JavaDocs

@NathanSweet

This comment has been minimized.

Show comment
Hide comment
@NathanSweet

NathanSweet Mar 22, 2014

Member

The PR needs to be formatted with the Eclipse source formatter. It makes the diff hard to read otherwise.

Does this handle a class hierarchy with multiple levels? Eg, classes A, B, C where class A and B both have fields with the same name? Or all three have fields with the same names?

Just ignoring the super class field isn't great, it would be better to serialize both fields. This means we'd need to write more than just the field name to the serialized data, which is a pretty significant change unless it can be done without affecting the current CompatibleFieldSerializer behavior and serialized size much. Maybe one byte extra per field along with the field names in the header, indicating how many super class's up in the hierarchy the field is for.

I'd rather not merge just ignoring the super class field. Maybe we can at least detect the problem and throw an exception until a full solution can be implemented.

Member

NathanSweet commented Mar 22, 2014

The PR needs to be formatted with the Eclipse source formatter. It makes the diff hard to read otherwise.

Does this handle a class hierarchy with multiple levels? Eg, classes A, B, C where class A and B both have fields with the same name? Or all three have fields with the same names?

Just ignoring the super class field isn't great, it would be better to serialize both fields. This means we'd need to write more than just the field name to the serialized data, which is a pretty significant change unless it can be done without affecting the current CompatibleFieldSerializer behavior and serialized size much. Maybe one byte extra per field along with the field names in the header, indicating how many super class's up in the hierarchy the field is for.

I'd rather not merge just ignoring the super class field. Maybe we can at least detect the problem and throw an exception until a full solution can be implemented.

@bohrqiu

This comment has been minimized.

Show comment
Hide comment
@bohrqiu

bohrqiu Apr 17, 2014

yes, every class has a CompatibleFieldSerializer instance,so classes A, B, C where class A and B both have fields with the same name can't affect with each other.

we serialize POJOs for transmitting data at most of the scene. we don't need care super class field .

I think we need add some warning logs for duplicate field name in com.esotericsoftware.kryo.serializers.FieldSerializer#initializeCachedFields before this PR be merged.

bohrqiu commented Apr 17, 2014

yes, every class has a CompatibleFieldSerializer instance,so classes A, B, C where class A and B both have fields with the same name can't affect with each other.

we serialize POJOs for transmitting data at most of the scene. we don't need care super class field .

I think we need add some warning logs for duplicate field name in com.esotericsoftware.kryo.serializers.FieldSerializer#initializeCachedFields before this PR be merged.

@NathanSweet

This comment has been minimized.

Show comment
Hide comment
@NathanSweet

NathanSweet Apr 17, 2014

Member

Not sure I follow you. Consider:

class A {
    public String moo;
}
class B extends A {
    public String moo;
}
B b = new B();
((A)b).moo = "a";
b.moo = "b";
System.out.println(((A)b).moo); // a
System.out.println(b.moo); // b

If I serialize the instance of B stored in b, after deserialization I expect the two print statements to print "a" and "b" as they do above. It seems your code would ignore the A#moo field, so would print "null" and "b", which means data was lost.

Member

NathanSweet commented Apr 17, 2014

Not sure I follow you. Consider:

class A {
    public String moo;
}
class B extends A {
    public String moo;
}
B b = new B();
((A)b).moo = "a";
b.moo = "b";
System.out.println(((A)b).moo); // a
System.out.println(b.moo); // b

If I serialize the instance of B stored in b, after deserialization I expect the two print statements to print "a" and "b" as they do above. It seems your code would ignore the A#moo field, so would print "null" and "b", which means data was lost.

@bohrqiu

This comment has been minimized.

Show comment
Hide comment
@bohrqiu

bohrqiu Apr 21, 2014

yes,B extend A ,A's moo field will be null.but we read field by getter method generally.so, we can avoid the inconsistent.

bohrqiu commented Apr 21, 2014

yes,B extend A ,A's moo field will be null.but we read field by getter method generally.so, we can avoid the inconsistent.

@xfrendx

This comment has been minimized.

Show comment
Hide comment
@xfrendx

xfrendx May 3, 2016

I've the same problem now (using Kryo 3.0.3). "null" from super class attributes overrides same named attribute values. What's the status of this "bug" please?

xfrendx commented May 3, 2016

I've the same problem now (using Kryo 3.0.3). "null" from super class attributes overrides same named attribute values. What's the status of this "bug" please?

@magro

This comment has been minimized.

Show comment
Hide comment
@magro

magro May 3, 2016

Member

@xfrendx The status is what you can read here. AFAICS the proposed solution replaces the existing issue with another one (if the superclass works on the field directly instead of using the getter), so it's not a real solution. Perhaps we should introduce a setting that allows to activate the new behavior, so that users have to actively choose this.
Apart from this the code needs some cleanup as it was mentioned.
If you want to push this forward go ahead, this would be great!

Member

magro commented May 3, 2016

@xfrendx The status is what you can read here. AFAICS the proposed solution replaces the existing issue with another one (if the superclass works on the field directly instead of using the getter), so it's not a real solution. Perhaps we should introduce a setting that allows to activate the new behavior, so that users have to actively choose this.
Apart from this the code needs some cleanup as it was mentioned.
If you want to push this forward go ahead, this would be great!

xfrendx pushed a commit to xfrendx/kryo that referenced this pull request May 13, 2016

xfrendx pushed a commit to xfrendx/kryo that referenced this pull request May 15, 2016

xfrendx pushed a commit to xfrendx/kryo that referenced this pull request May 15, 2016

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

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

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

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

phamrak
Support for same named fields in class hierarchy
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: #424
See also: #187

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

phamrak
Support for same named fields in class hierarchy
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: #424
See also: #187

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

phamrak
Support for same named fields in class hierarchy
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: #424
See also: #187
@magro

This comment has been minimized.

Show comment
Hide comment
@magro

magro Jun 10, 2016

Member

Resolved with #424.

Member

magro commented Jun 10, 2016

Resolved with #424.

@magro magro closed this Jun 10, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment