Skip to content

[Java] Use ReflectionUtils.getCtrHandle() in ExternalizableSerializer#1044

Merged
chaokunyang merged 5 commits intoapache:mainfrom
knutwannheden:externalizable-accessible
Oct 30, 2023
Merged

[Java] Use ReflectionUtils.getCtrHandle() in ExternalizableSerializer#1044
chaokunyang merged 5 commits intoapache:mainfrom
knutwannheden:externalizable-accessible

Conversation

@knutwannheden
Copy link
Contributor

@knutwannheden knutwannheden commented Oct 30, 2023 •
edited
Loading

When deserializing an Externalizable object, it may end up throwing an exception, as the class may be declared as non-public in another package and thus isn't accessible from the serializer. Fix this by using the ReflectionUtils.getCtrHandle() utility.

@knutwannheden
[Java] Call setAccessible(true) on Externalizable constructor
2d61576 
When deserializing an `Externalizable` object, it may be necessary to call `setAccessible(true)` on its constructor first, as the class may be declared as non-public in another package.
@knutwannheden knutwannheden changed the title [Java] Call setAccessible(true) on Externalizable constructor [Java] Use ReflectionUtils.getCtrHandle() in ExternalizableSerializer Oct 30, 2023
@knutwannheden @chaokunyang
Update java/fury-core/src/main/java/io/fury/serializer/Externalizable…
1b44a45 
…Serializer.java

Co-authored-by: Shawn <shawn.ck.yang@gmail.com>
@knutwannheden
Copy link
Contributor Author

knutwannheden commented Oct 30, 2023

@chaokunyang I think to support the combination of Externalizable and writeReplace() / readResolve() I will have to hand over to you. I am not sure how that would best be done, as I see that you already have some support for the latter. Feel free to close this PR, if you think you want to fix that together with this change.

@chaokunyang
Copy link
Collaborator

chaokunyang commented Oct 30, 2023

@chaokunyang I think to support the combination of Externalizable and writeReplace() / readResolve() I will have to hand over to you. I am not sure how that would best be done, as I see that you already have some support for the latter. Feel free to close this PR, if you think you want to fix that together with this change.

writeReplace() / readResolve() has higher priority than Externalizable, currently we ignored that. This PR can be merged first, the support for combination can be put in a new PR

chaokunyang
chaokunyang approved these changes Oct 30, 2023
View reviewed changes
Copy link
Collaborator

@chaokunyang chaokunyang left a comment

Choose a reason for hiding this comment

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

LGTM

@chaokunyang chaokunyang merged commit 3ee57da into apache:main Oct 30, 2023
@knutwannheden knutwannheden deleted the externalizable-accessible branch October 30, 2023 09:30
@knutwannheden
Copy link
Contributor Author

knutwannheden commented Oct 30, 2023

@chaokunyang Thanks for merging this fix! I hope that I will soon be able to successfully serialize and deserialize our object graphs. Do you want me to create a separate issue for the combination of Externalizable and writeReplace() / readResolve()?

@knutwannheden
Copy link
Contributor Author

knutwannheden commented Oct 30, 2023

Here it is in any case: #1045

@chaokunyang
Copy link
Collaborator

chaokunyang commented Oct 30, 2023

@knutwannheden Thanks for creating new issue, I'll fix it soon

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

Reviewers

@chaokunyang chaokunyang chaokunyang approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@knutwannheden @chaokunyang