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

Support specify serializer for 3rd classes #18

Merged
merged 8 commits into from
Nov 5, 2024

Conversation

chaokunyang
Copy link
Contributor

@chaokunyang chaokunyang commented Nov 5, 2024

Support specify serializer for 3rd classes:

@FurySerialization(targetClass = ThirdPartyBar.class, serializer = ThridPartyBarSerializer.class)
public class ThirdPartyFooConfig {
}

@chaokunyang chaokunyang requested a review from a team as a code owner November 5, 2024 10:19
@chaokunyang
Copy link
Contributor Author

@zhfeng Seems ThirdPartyFooConfig won't be captured by quarkus, does this means if a class is not visited by application main entrance, it won't be identified by quarkus at deployment?

@zhfeng
Copy link
Contributor

zhfeng commented Nov 5, 2024

@chaokunyang Hmm, I don't understand here. What is the class not visisted?

@chaokunyang
Copy link
Contributor Author

I congifured ThirdPartyBar with ThridPartyBarSerializer.class. But it seems ThirdPartyFooConfig annotated configuration class is not scanned by quarkus:

@FurySerialization(targetClass = ThirdPartyBar.class, serializer = ThridPartyBarSerializer.class)
public class ThirdPartyFooConfig {
}

@zhfeng
Copy link
Contributor

zhfeng commented Nov 5, 2024

Maybe you can take a look at RegisterForReflection in Quarkus which is very similar with what you want to achive.

@chaokunyang
Copy link
Contributor Author

It's not about the annotation itself, it's about how to make the class annotated by this annotation got identified by Quarkus

* Specify which classes the provided serialized is used for.
* If not specified, the current annotated class is used.
*/
Class<?> targetClass() default CurrentAnnotatedTypeStub.class;
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe better use

Class<?>[] targetClass() default {};

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated to this in the new commit. Actually it was my earliest implementation, I thought it may introduce confusion with classId since we can't configure multiple class id but we can configure multiple classes. I added a classId check in the new commit. If multiple classes are specified, no class id are allowed in the annotation config

@zhfeng zhfeng force-pushed the support-specify_serializer_for_3rd_classes branch from 44378ae to 8e551cd Compare November 5, 2024 12:27
int classed = annotation.classId();
Class<? extends Serializer> serializer = annotation.serializer();
if (classes.length > 1) {
Preconditions.checkArgument(classed == -1,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think serializer need to check as well since I suppose serializer should only to a specfic target class?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not exactly, multiple classes can share same serializer. This is common, users may write a serializer for an abstract parent class, which can be used for all subclasses serialization

@zhfeng zhfeng merged commit 91d89e3 into main Nov 5, 2024
1 check passed
@zhfeng zhfeng deleted the support-specify_serializer_for_3rd_classes branch November 5, 2024 22:55
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.

2 participants