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

Turn JsonbComponentInstanceCreator into a service provider interface #260

Closed
mkouba opened this issue Apr 12, 2019 · 13 comments · Fixed by #261
Closed

Turn JsonbComponentInstanceCreator into a service provider interface #260

mkouba opened this issue Apr 12, 2019 · 13 comments · Fixed by #261

Comments

@mkouba
Copy link
Contributor

mkouba commented Apr 12, 2019

So that an integrator could provide a custom implementation of this interface. JsonbComponentInstanceCreatorFactory currently contains some reflection magic to detect the presence of a CDI container. In fact, the presence of the CDI API is enough.

I believe it would be reasonable to define this SPI. Quarkus integration would definitely make use of it because our DI solution is compatible with CDI but we don't support all "portable extensions related" SPIs (they do not fit into our build time metadata processing approach) and so the invocation of the BeanManagerInstanceCreator#getOrCreateComponent() fails.

@gsmet
Copy link

gsmet commented Apr 26, 2019

Any thought on this? This is rather blocking for the Yasson Quarkus integration and we have quite a few users asking for it.

@aguibert
Copy link
Member

Can you elaborate on what needs to happen in order to turn this into an SPI? Are you asking that we make a META-INF/service/ entry for this? Or perhaps just moving it out of the Yasson .internal package so it is exported by our JPMS module?

@mkouba
Copy link
Contributor Author

mkouba commented Apr 29, 2019

Just use the ServiceLoader to obtain the JsonbComponentInstanceCreator instance so that an integrator could provide its own implementation. The logic from JsonbComponentInstanceCreatorFactory should be extracted to a default impl (e.g. DefaultJsonbComponentInstanceCreator) and either moved to a separate artifact with META-INF/service config, or alternatively JsonbComponentInstanceCreator could define a priority (e.g. default int getPriority() { return 1; }) and the instance with the highest priority is used.

@aguibert
Copy link
Member

Sounds like a reasonable change -- do you have cycles to propose a PR?

@mkouba
Copy link
Contributor Author

mkouba commented Apr 30, 2019

Yes, I'll try to prepare something.

mkouba added a commit to mkouba/yasson that referenced this issue Apr 30, 2019
- if there is no service provider use the default implementations
- resolves eclipse-ee4j#260
mkouba added a commit to mkouba/yasson that referenced this issue Apr 30, 2019
- if there is no service provider use the default implementations
- resolves eclipse-ee4j#260

Signed-off-by: Martin Kouba <mkouba@redhat.com>
@geoand
Copy link

geoand commented May 1, 2019 via email

mkouba added a commit to mkouba/yasson that referenced this issue May 2, 2019
- if there is no service provider use the default implementations
- resolves eclipse-ee4j#260

Signed-off-by: Martin Kouba <mkouba@redhat.com>
bravehorsie pushed a commit that referenced this issue May 2, 2019
#261)

- if there is no service provider use the default implementations
- resolves #260

Signed-off-by: Martin Kouba <mkouba@redhat.com>
@gsmet
Copy link

gsmet commented May 13, 2019

@bravehorsie @aguibert is the next release already scheduled? Asking that so that we don't miss it and do the appropriate work in Quarkus as soon as it is released.

Thanks!

@bravehorsie
Copy link
Contributor

@gsmet @aguibert Lets review current PRs and do the release.

@masini
Copy link

masini commented May 14, 2019

Thank you for fixing this issue, is a blocking one for our porting to JSONB.

@gsmet
Copy link

gsmet commented May 31, 2019

@bravehorsie @aguibert any news on the release front?

Thanks!

@aguibert
Copy link
Member

aguibert commented Jun 1, 2019

@bravehorsie I added a code review to #262 (your big JSON-P interop PR) and I think we should merge it and then we should be good to cut a new release.

@bravehorsie
Copy link
Contributor

Yasson 1.0.4 has been released to central, which includes the fix.

@gsmet
Copy link

gsmet commented Jun 10, 2019

Thanks @bravehorsie !

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

Successfully merging a pull request may close this issue.

6 participants