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

4.x: Pico generated module-info.java has redundant uses #6631

Closed
tomas-langer opened this issue Apr 14, 2023 · 3 comments
Closed

4.x: Pico generated module-info.java has redundant uses #6631

tomas-langer opened this issue Apr 14, 2023 · 3 comments
Assignees
Labels
4.x Version 4.x declarative Helidon Declarative must-have P3
Milestone

Comments

@tomas-langer
Copy link
Member

tomas-langer commented Apr 14, 2023

Pico - generates uses and provides, even though it does not. uses should only be explicitly defined if the service loader is used from that module, provides is only to declare that the module implements a service provider interface loaded using service loader.

Example from a test (pico/tests/resources-pico/src/test/resources/expected/module-info.java._pico_):

    uses jakarta.inject.Provider;
    uses io.helidon.pico.tests.plain.interceptor.IA;
    uses io.helidon.pico.tests.plain.interceptor.IB;
    // Pico application - Generated(value = "io.helidon.pico.tools.ApplicationCreatorDefault", comments = "version=1")
    provides io.helidon.pico.api.Application with io.helidon.pico.tests.pico.Pico$$Application;
@tomas-langer tomas-langer added 4.x Version 4.x declarative Helidon Declarative labels Apr 14, 2023
@m0mus m0mus added the P3 label Apr 17, 2023
@m0mus m0mus added this to the 4.0.0-M1 milestone Apr 17, 2023
@trentjeff
Copy link
Member

trentjeff commented May 18, 2023

From my reading (see below) it seems to me it is slightly more than this.

Practically speaking, however, it is really impossible for us to know what is Service Loaded and what is not. The heuristic that is in use right now will look at the externalContracts that each service declares. External contracts are interfaces declared in module A, and advertised from module B. These ultimately generate a 'uses clause'.

For example:

public class FallbackInterceptor$$Pico$$Activator
            extends io.helidon.pico.runtime.AbstractServiceProvider<FallbackInterceptor> {
    private static final DefaultServiceInfo serviceInfo =
        DefaultServiceInfo.builder()
            .serviceTypeName(io.helidon.nima.faulttolerance.FallbackInterceptor.class.getName())
            .addExternalContractsImplemented(io.helidon.pico.api.Interceptor.class.getName())
            .activatorTypeName(FallbackInterceptor$$Pico$$Activator.class.getName())
            .addScopeTypeName(jakarta.inject.Singleton.class.getName())
            .addQualifier(io.helidon.pico.api.DefaultQualifierAndValue.create(jakarta.inject.Named.class, "io.helidon.nima.faulttolerance.FtFallback"))
            .declaredWeight(60.0)
            .build();

The Pico code-generated interceptor (in module B) implements the Interceptor contract (from module A).

Fwiw, Interceptor is declared to be a @Contract since the Pico service implementation will need to lookup interceptors. However, all of Pico relies on ModuleComponents being service loadable but not the contracts themselves. So we can definitely remove the @Contract declaration (and the uses) will/should fall off. But doing so will also make these Interceptors not able to be looked up from the Services registry.

The bottom line here is I think Pico is erroring on the side of safety since it doesn't really know what is Service Loaded. If you have a suggestion for a better heuristic then LMK.

@tomas-langer
Copy link
Member Author

uses in module info is strictly for usages of Java ServiceLoader.
Contract or ExternalContract does not imply it is a ServiceLoader provider interface.

@trentjeff
Copy link
Member

Per discussion offline w/ @tomas-langer , he suggested to remove the uses. The net is that while the uses is not harmful, it is not proper to have regardless. Since Pico has no way to know if a contract is service loaded, then this must be the responsibility of the developer to manually add and it can therefore not be implicitly generated.

@trentjeff trentjeff modified the milestones: 4.0.0-M1, 4.0.0-M2 Jun 22, 2023
trentjeff added a commit to trentjeff/helidon that referenced this issue Jul 6, 2023
trentjeff added a commit that referenced this issue Jul 10, 2023
* fix for issue #7081
* fix for issue #6631
@m0mus m0mus added this to Backlog Aug 12, 2024
@m0mus m0mus moved this to Closed in Backlog Aug 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4.x Version 4.x declarative Helidon Declarative must-have P3
Projects
Archived in project
Development

No branches or pull requests

3 participants