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

Add Avaje Inject Module #3416

Merged
merged 12 commits into from
May 12, 2024
Merged

Add Avaje Inject Module #3416

merged 12 commits into from
May 12, 2024

Conversation

SentryMan
Copy link
Contributor

@SentryMan SentryMan commented Apr 24, 2024

First pass at an avaje inject module

helps alleviate #3415

Copy link
Contributor

@edgarespinawt edgarespinawt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good addition, thank you. Just one comment

@SentryMan SentryMan marked this pull request as ready for review April 24, 2024 22:47
Copy link
Contributor

@ludmiloff ludmiloff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I grabbed the source and tried locally. It results in multiple Hikari re-initialisation and database connections and application crash. This is due to beanScope.provideDefault(key.getName(), key.getType(), e::getValue); which adds the bean as secondary.
I suggest changing it to beanScope.bean(key.getType(), e.getValue());.

With this change my testing code works, still needs @InjectModule(requires) everywhere.

Copy link
Contributor

@ludmiloff ludmiloff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, change service registration as follows:

if (key.getName() == null) {
  beanScope.provideDefault(key.getType(), e::getValue);
} else {
  beanScope.bean(key.getName(), key.getType(), e.getValue());
}

This:

  1. prevents adding multiple secondary beans for same type, Avaje complains about that
  2. allows named injection, e.g. @Named("db1") Jdbi

ps: adding package-info.java with specific InjectModule(name = 'jooby-modules', requires = {...}) is required for pass avaje-generator compillation

@SentryMan
Copy link
Contributor Author

@ludmiloff how's this?

@ludmiloff
Copy link
Contributor

ludmiloff commented Apr 25, 2024

@ludmiloff how's this?

@SentryMan, this does not work, at least for me.
This is the error:

[2024-04-25 17:01:53,520]-[main] ERROR io.jooby.Jooby - Application initialization resulted in exceptionjava.lang.IllegalStateException: Expecting only 1 bean match but have multiple secondary beans org.jdbi.v3.core.Jdbi@7754195b and org.jdbi.v3.core.Jdbi@7754195b
	at jars//io.avaje.inject.spi.DContextEntry$EntryMatcher.checkSecondary(DContextEntry.java:195)

[ERROR] execution of com.Main resulted in exception
io.jooby.exception.StartupException: Application initialization resulted in exception
    at io.jooby.Jooby.createApp (Jooby.java:1222)
    at io.jooby.Jooby.runApp (Jooby.java:1183)
    at io.jooby.Jooby.runApp (Jooby.java:1143)
    at com.Main.main (Main.java:10)
Caused by: java.lang.IllegalStateException: Expecting only 1 bean match but have multiple secondary beans org.jdbi.v3.core.Jdbi@7754195b and org.jdbi.v3.core.Jdbi@7754195b
    at io.avaje.inject.spi.DContextEntry$EntryMatcher.checkSecondary (DContextEntry.java:195)
    at io.avaje.inject.spi.DContextEntry$EntryMatcher.candidate (DContextEntry.java:189)
    at io.avaje.inject.spi.DContextEntry$EntryMatcher.findMatch (DContextEntry.java:134)
    at io.avaje.inject.spi.DContextEntry$EntryMatcher.match (DContextEntry.java:118)
    at io.avaje.inject.spi.DContextEntry.get (DContextEntry.java:53)
    at io.avaje.inject.spi.DBeanMap.get (DBeanMap.java:115)
    at io.avaje.inject.spi.DBuilder.getMaybe (DBuilder.java:141)
    at io.avaje.inject.spi.DBuilder.getBean (DBuilder.java:384)
    at io.avaje.inject.spi.DBuilder.get (DBuilder.java:333)
    at com.DataCache$DI.build (DataCache$DI.java:20)

where my DataCache class is declared as singleton with constructor like this

    @Inject
    public DataCache(Jdbi jdbi, Config config) {
       ......
    }

My proposal above works fine, though.

Should I prepare some example code for experiments?

Copy link
Contributor

@ludmiloff ludmiloff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks ok, and is working for me.

Copy link
Contributor

@ludmiloff ludmiloff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As @jknack mentioned before, there must be an existence check in JoobyPropertyPlugin, line 30
@SentryMan, may I suggest the change like this:

return config.hasPath(property) && Objects.equals(config.getString(property), value);

Copy link
Contributor

@ludmiloff ludmiloff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SentryMan, your latest commit about named property injection works in the following scenario:

@Inject @Named("myProp") String myProp

and the actual value is available during/after PostConstruct phase.

Constructor injection of named config item (Jooby 1 style, still good :) ) does not work, however.

Existence check in JoobyPropertyPlugin is fine.

Copy link
Contributor

@ludmiloff ludmiloff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A bit of explanation about my latest comment:
constructor injection if named config item does not work because it needs to be declared as external dependency, which might be very cumbersome.

@SentryMan
Copy link
Contributor Author

SentryMan commented Apr 25, 2024

you mean with the @InjectModule(requires={String.class}) thing? For that, you only need to say String.class once and it'll take care of things. I'll need to make a note of that in the docs.

@ludmiloff
Copy link
Contributor

you mean with the @InjectModule(requires={String.class}) thing? For that, you only need to say String.class once and it'll take care of things. I'll need to make a note of that in the docs.

Yes, this way it works, thanks.

Copy link
Contributor

@ludmiloff ludmiloff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this module is already done, at least for a sort of EAP.
My notes about:

  1. Needs documentation
  2. Injecting property (incl. com.typesafe.config.Config items by name) works
  3. Constructor injection works
  4. All JoobyModule(s) currently in use are injected as Avaje Beans, named injection is also supported
  5. All Avaje Beans are accessible via Jooby require
  6. Installing AvajeInjectModule does not need a specific order in code along other Jooby modules
  7. Packaging my test project into a single jar produces a working deployment without any problem.

Some limitation and caveats:

  1. Using this module needs a package-info.java with proper InjectModule(require) clause. This is very Avaje specific.
  2. Config item injection is currently limited to String types or string representation (toString). Might be improved with Integer and Boolean classes, though.
  3. Using this module needs a special maven/gradle plugin and setup for Avaje code generation
  4. Using this module adds a small overhead in startup time, on my testing machine 8 cores M1, java 17, small size project with 3 other jooby modules and 200 lines of config items, cold start in dev mode - around 20ms for wiring beans at runtime (as reported by Avaje), hope this is acceptable and much better than Guice

Finally, this is ready for approval and merging, IMO.
@jknack, @SentryMan, thank you for your support.

edit:
mvc also works OTB, something like mvc(MyController.class); when MyController is declared as a singleton bean.

@SentryMan
Copy link
Contributor Author

  1. Using this module needs a special maven/gradle plugin and setup for Avaje code generation

This should only be required if you are running on the module-path and also happen to have custom avaje autoconfig libraries. But yeah it is what it is

@jknack jknack added the feature label May 6, 2024
@jknack jknack added this to the 3.1.0 milestone May 6, 2024
@jknack jknack merged commit 8c9dfd2 into jooby-project:3.x May 12, 2024
1 check passed
@SentryMan SentryMan deleted the avaje-inject branch May 13, 2024 11:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants