avaje / avaje-config

Application configuration / properties loading for JVM applications
https://avaje.io/config
Apache License 2.0
47 stars 7 forks source link

Document or fix hard dependency on avaje-inject when using module-path #71

Closed NoonRightsWarriorBehindHovering closed 1 year ago

NoonRightsWarriorBehindHovering commented 1 year ago

Hi,

when trying to evaluate avaje i could not help but notice, that there is a hard dependency on avaje-inject, albeit the require static statement in the modules-info.

A MRE can be found here: https://github.com/NoonRightsWarriorBehindHovering/mre-avaje-config-1

The issue is expected as far as i can see, given that the official documentation says this: A module M declares that it 'uses p.S' or 'provides p.S with ...' but package p is neither in module M nor exported to M by any module that M "reads". Source: https://docs.oracle.com/javase/9/docs/api/java/lang/module/package-summary.html

As such without avaje-inject being read by any module explicitely (requires static does not "read", because it it optional), the above fails.

A simple workaround would be to require it in the consuming application. This might not be feasible, when inject is not being used. Alternatively the provides statement could (naively) be moved somewhere else (e. g. a new jar or avaje-inject).

In any case the intended outcome should be documented.

Sorry for opening two bugs already in quick succession. :)

SentryMan commented 1 year ago

Sorry for opening two bugs already in quick succession. :)

You and I have a different definition of quick it seems. In any case, that MRE repo link givers me a 404.

NoonRightsWarriorBehindHovering commented 1 year ago

Sorry about that! I forgot to publish the repository.

SentryMan commented 1 year ago

happens to the best of us

rbygrave commented 1 year ago

The failure being:

$ ./shell.sh
Error occurred during initialization of boot layer
java.lang.module.ResolutionException: Module io.avaje.config does not read a module that exports io.avaje.inject.spi
SentryMan commented 1 year ago

so to be clear, are you getting a module not found exception?

EDIT: ninja'd, it seems I have not built your MRE correctly

SentryMan commented 1 year ago

when I use the mvn compiler plugin, it works without needing inject.

    <build>
        <plugins>
          <plugin>
            <groupId>org.apache.maven.plugins</groupId>
            <artifactId>maven-compiler-plugin</artifactId>
            <version>3.11.0</version>
          </plugin>
        </plugins>
    </build>

EDIT: ahh, no it doesn't

rbygrave commented 1 year ago

It compiles ... but it gives an error at runtime. We see:

$ ./shell.sh
Error occurred during initialization of boot layer
java.lang.module.ResolutionException: Module io.avaje.config does not read a module that exports io.avaje.inject.spi
SentryMan commented 1 year ago

yeah, I'm still having trouble getting the script to run on my local (with both Windows and MacOS)

rbygrave commented 1 year ago

FYI: I emailed jigsaw-dev and Alan pointed me to - https://bugs.openjdk.org/browse/JDK-8299504

SentryMan commented 1 year ago

@NoonRightsWarriorBehindHovering Yeah, this one might take a while to resolve, thanks for finding this.

rob-bygrave commented 1 year ago

I've posted some thoughts/comments to jigsaw-dev and waiting to see what the response is.

If the angle is "this works in classpath but not in module-path" ... then its a JDK bug in module resolution and specifically module resolution should not fail in the case for a provides p.S with .. when p.S is not in the module-path (just like this does not fail in the classpath case).


A potential alternative solution is to invert the relationship by NOT implementing these Plugin API's in avaje-config (and avaje-jsonb) but instead changing avaje-inject to detect the existence of avaje-config (and avaje-jsonb) in classpath/module-path via say Class.forName() and creating the implementations of these Plugin APIs in avaje-inject itself.

Using Class.forName() seems a bit "dirty" and less desirable but I think this should work ok for the avaje-config PropertyRequiresPlugin case [modify how avaje-inject determines the default implementation of PropertyRequiresPlugin].

However, my guts says this approach will not be great for the Plugin & avaje-jsonb case (as we want to determine the types all the plugins provide etc and that looks like it might get 'hacky').

SentryMan commented 1 year ago

However, my guts says this approach will not be great for the Plugin & avaje-jsonb case (as we want to determine the types all the plugins provide etc and that looks like it might get 'hacky').

Yeah, this leaves a bad taste in my mouth.

It'll also make it harder to maintain, suppose we come up with more ideas for more plugins. (like the avaje/hibernate validator and http resolver). Are we just gonna keep modifying inject with Class.forName to add the defaults?

rob-bygrave commented 1 year ago

Are we just gonna keep modifying inject with

I recon we are going to implement our own ServiceLoader.

So avaje-inject will have it's own code that does:

Edit:

SentryMan commented 1 year ago

use reflection

I'd really prefer not to.

rob-bygrave commented 1 year ago

use reflection

This is what ServiceLoader does under the hood.