eclipse / microprofile-open-api

Microprofile open api
Apache License 2.0
132 stars 82 forks source link

Consider deprecating/removing the `spi` package in the `api` module #576

Open MikeEdgar opened 8 months ago

MikeEdgar commented 8 months ago

The spi module contains an out-of-date single class that is also present in the api module. Maybe we can:

Azquelt commented 8 months ago

Hmm, I'm now not sure if we might have done this the wrong way around. The intent in #224 was to move that class into an SPI jar, so I think it may have been the API copy that was left by mistake.

MikeEdgar commented 8 months ago

Ah, I never saw that. I'll prepare a PR to get it the right way then.

MikeEdgar commented 8 months ago

Thinking about a way to do this - we may actually need to move the version of the class from api into spi and (temporarily?) add spi as a dependency of api. Otherwise the class will be duplicated on the path. Am I missing something?

Azquelt commented 8 months ago

Yes, we should move the version from api into spi.

Assuming we're still on board with the goals of #224, I think we might want a provided scope dependency from api to spi? The OASFactoryResolver class shouldn't be visible to users of the api jar, but there is implementation code in api which calls it so we need it at compile time.

Any implementation should depend on both api and spi.

MikeEdgar commented 8 months ago

Moving the class introduces a dependency cycle between the modules (OASFactoryResolver requires Constructible in api). This might be solved by dropping the extends Constructible from the signature of OASFactoryResolver#createObject, but that's not a great solution in my opinion.

I question the utility of #224. This pattern is (still) used by MP REST and MP Config (despite eclipse/microprofile#43) and it also is present in APIs like CDI [1]. Even if there is a separate spi moduile with provided scope, wouldn't that be visible (and needed to invoke methods on OASFactory) to the users of api at runtime?

[1] https://github.com/jakartaee/cdi/blob/45b911d8e0b5a0e2ec86021dfb6d4ffdb4e99111/api/src/main/java/jakarta/enterprise/inject/spi/CDI.java

Azquelt commented 8 months ago

Hmm, and the reason this wasn't a problem before is that nothing was actually using the class in the spi jar?

MikeEdgar commented 8 months ago

Right - the SPI module depended on the API module, but the OASFactoryResolver wasn't removed from API.

Azquelt commented 8 months ago

Yeah, I can see a few options here but none of them are great.

  1. Remove the setInstance method and have the API load the SPI using ServiceLoader

    This defeats the objective of keeping setInstance for use in an OSGi environment so an alternative solution would be needed there. SPI Fly's bytecode weaving to replace the ServiceLoader call with an OSGi service lookup is one option, but another would be to include a static implementation of the SPI which delegates to an instance looked up as an OSGi service in a fragment which attaches to the API bundle.

  2. Do some maven hackery so that the SPI classes are compiled in the API project, but packaged and distributed in a separate SPI jar.

    This gets around the maven circular dependency problem while still creating two interdependent jars.

  3. Have the API call the static SPI method by reflection.

Azquelt commented 4 months ago

We've run out of time for this release and haven't found a good way of fixing this yet.