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

public static void setInstance(RestClientBuilderResolver resolver) is an implementation leak and should be dropped from the API #103

Open
rmannibucau opened this issue May 29, 2018 · 11 comments

Comments

@rmannibucau
Copy link

See eclipse/microprofile-open-api#224, this kind of method don't belong to the API and is only issues promises so should disappear from any coming release

@andymc12
Copy link
Contributor

Per discussion in eclipse/microprofile-open-api#224 the resolution of this issue should be to put the RestClientBuilderResolver (and any other SPI code) into a separate SPI artifact - so that vendors can implement this spec without allowing end-users to be able to call SPI methods.

@andymc12 andymc12 added this to the Future milestone Jul 11, 2018
@rmannibucau
Copy link
Author

Any rational to keep this method since multiple threads qhowed osgi and other envs dont need it? Others are ok to extract even if pby overkill.

@andymc12
Copy link
Contributor

CXF does not use the setInstance method, so from that implementor's standpoint, it could be removed. It would be good to know whether other implementors use it or not.

We'd probably need to deprecate it first.

@OndroMih
Copy link
Contributor

It's often the only way how to set a custom resolver in an OSGi environment. I think that @Emily-Jiang has some opinion about this. In Payara, the resolver is provided via the service loader mechanism.

@rmannibucau
Copy link
Author

@OndrejM I know it is what @Emily-Jiang keeps saying but this is not true and there are OSGi solution nowdays which avoid to break the API introducing some vendor specific implementations, please have a look to eclipse/microprofile#33 or geronimo which does it since almost 20 years without any need of API tweak.

@OndroMih
Copy link
Contributor

I believe, @rmannibucau. I even think that the API doesn't need to support the service loader pattern, which is also impl-specific. The SPI package exists only to make it easier to create implementations. Maybe a better solution is to provide an SPI to provide an impl-specific extension, which could provide implementations in any way

@rmannibucau
Copy link
Author

Agree, if you think 2s about here is concrete status of that particular point:

  1. JavaEE always used SPI so MP "copied" that pattern but (point 2)
  2. MP is CDI centric so the way to have a SPI is to use CDI beans. MP API would provide an interface and the implementation would add a bean for that type. it works for 90% of the cases but the config one which is needed before the container is started but that's really all.

Sounds like a trivial fix, no?

@OndroMih
Copy link
Contributor

It sounds OK to me to use CDI for providing implementations for specs that can rely on CDI. Most of them should except the config one you mentioned.

@OndroMih
Copy link
Contributor

It is obvious from the discussion in eclipse/microprofile-open-api#224 that this pattern is used in many other specs (config, REST client, openapi). This should then be discussed on a higher level at the architecture board. I raised an issue here: eclipse/microprofile#43

@Emily-Jiang
Copy link
Member

close this as it has been resolved. Please reopen if you think otherwise.

@rmannibucau
Copy link
Author

Hmm, AFAIK there is no change in the API and the buggy method is still surfacing so this issue is still valid since:

  • in terms of design it creates inconsistencies with CDI depending the environment (in OSGi-CDI for ex the classloading is not sufficient so if it would use CDI there wouldn't be any issue but there it is random)
  • it is not generally working since it can be called any time and a consumer code can get the unexpected resolver
  • as originally mentionned it is a design bug of a few spec but since MP is CDI centric there is no real reason to keep this broken pattern in the spec so the method should be deprecated and implemented as a noop when possible (to not break compilation immediately) then dropped in a few versions IMHO

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

No branches or pull requests

4 participants