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

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

Open
MikeEdgar opened this issue Jan 9, 2024 · 9 comments · Fixed by #581
Open

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

MikeEdgar opened this issue Jan 9, 2024 · 9 comments · Fixed by #581

Comments

@MikeEdgar
Copy link
Member

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

  • remove spi
  • provide a Maven replication from spi to api
@Azquelt
Copy link
Member

Azquelt commented Jan 19, 2024

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
Copy link
Member Author

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

@MikeEdgar MikeEdgar reopened this Jan 19, 2024
@MikeEdgar MikeEdgar changed the title Consider deprecating/removing the spi module Consider deprecating/removing the spi package in the api module Jan 19, 2024
@MikeEdgar
Copy link
Member Author

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
Copy link
Member

Azquelt commented Jan 19, 2024

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
Copy link
Member Author

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
Copy link
Member

Azquelt commented Jan 23, 2024

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

@MikeEdgar
Copy link
Member Author

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

@Azquelt
Copy link
Member

Azquelt commented Jan 23, 2024

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
Copy link
Member

Azquelt commented Jun 4, 2024

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

@Azquelt Azquelt modified the milestones: MP OpenAPI 4.0, MP OpenAPI 4.1 Jun 4, 2024
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 a pull request may close this issue.

2 participants