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

Allow Kryo serializer registration #441

Closed
wants to merge 1 commit into from

Conversation

EdwardvanRaak
Copy link
Collaborator

@EdwardvanRaak EdwardvanRaak commented Sep 29, 2022

Need to discuss this solution.

This would allow implementing applications to use KryoUtil.addSerializer(URI::class.java, URISerializer()) for example.

Which solves this crash when attempting to clone anything with a DV_MULTIMEDIA on Android (where java.net.URI can be from Java 8 which has no default empty constructor)

com.esotericsoftware.kryo.kryo5.KryoException: Class cannot be created (missing no-arg constructor): java.net.URI
Serialization trace:
value (com.nedap.archie.rm.datavalues.DvURI)

Note: I grabbed a serializer to test this out from https://github.com/magro/kryo-serializers

Example from Android side (Kotlin)

Screenshot from 2022-09-29 17-38-15

@codecov
Copy link

codecov bot commented Sep 29, 2022

Codecov Report

Base: 71.34% // Head: 71.34% // Decreases project coverage by -0.00% ⚠️

Coverage data is based on head (db183bf) compared to base (868963d).
Patch coverage: 50.00% of modified lines in pull request are covered.

Additional details and impacted files
@@             Coverage Diff              @@
##             master     #441      +/-   ##
============================================
- Coverage     71.34%   71.34%   -0.01%     
  Complexity     6723     6723              
============================================
  Files           657      657              
  Lines         22254    22258       +4     
  Branches       3573     3573              
============================================
+ Hits          15877    15879       +2     
- Misses         4680     4682       +2     
  Partials       1697     1697              
Impacted Files Coverage Δ
.../src/main/java/com/nedap/archie/util/KryoUtil.java 76.92% <50.00%> (-11.97%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@EdwardvanRaak
Copy link
Collaborator Author

EdwardvanRaak commented Sep 29, 2022

Another solution would be to add this line in KryoUtil

kryo.addDefaultSerializer(URL.class, URLSerializer.class);

See EsotericSoftware/kryo#885 (comment)

But that would mean it applies to everyone using Archie 😛 (which maybe is a good thing?)

@pieterbos
Copy link
Collaborator

I think registering the default serializer is a better solution. This Kryo instance is only intended for cloning, not for serializing and deserializing, so backwards compatibility should not be an issue. The benefit is that more of Archie just works in more cases.

Copy link
Collaborator

@J3173 J3173 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think registering the default serializer is a better solution.

I agree. The current solution could also give strange behavior if the pool is already used before addSerializer is called.

Comment on lines +17 to +18
@SuppressWarnings("rawtypes")
private static final HashMap<Class, Serializer> serializers = new HashMap<>();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
@SuppressWarnings("rawtypes")
private static final HashMap<Class, Serializer> serializers = new HashMap<>();
private static final HashMap<Class<?>, Serializer<?>> serializers = new HashMap<>();

This fixes the warning without suppressing it.

Comment on lines +42 to +45
@SuppressWarnings("rawtypes")
public static void addSerializer(Class clazz, Serializer serializer) {
serializers.put(clazz, serializer);
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
@SuppressWarnings("rawtypes")
public static void addSerializer(Class clazz, Serializer serializer) {
serializers.put(clazz, serializer);
}
public static <T> void addSerializer(Class<T> clazz, Serializer<T> serializer) {
serializers.put(clazz, serializer);
}

This fixes the warning without suppressing it.

@EdwardvanRaak
Copy link
Collaborator Author

Closing this for the alternative solution

@EdwardvanRaak EdwardvanRaak deleted the ed/allow-kryo-serializer-registration branch September 30, 2022 11:12
@EdwardvanRaak EdwardvanRaak mentioned this pull request Sep 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants