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

Fix Prototype Factory Providers #540

Closed
wants to merge 4 commits into from
Closed

Conversation

SentryMan
Copy link
Collaborator

@SentryMan SentryMan commented Apr 13, 2024

Before, prototype scoped Providers were generating incorrectly.

  • Fixes ProtoType Providers
  • Simplifies lazy inject generation (it turns out registering as a normal provider is already lazy)

Now can do:

@Factory
public class FactoryProvider {

  @Bean
  @Prototype
  Provider<Integer> random() {
    return new Random()::nextInt;
  }
}

And this will generate:

@Generated("io.avaje.inject.generator")
public final class FactoryProvider$DI  {
  /**
   * Create and register Provider via factory bean method FactoryProvider#random().
   */
  public static void build_random(Builder builder) {
    if (builder.isAddBeanFor(Integer.class)) {
      var factory = builder.get(FactoryProvider.class);
      var bean = factory.random();
      builder.asPrototype().registerProvider(bean);
    }
  }
}

turns out we don't need a special method for lazyness
@SentryMan SentryMan added the bug Something isn't working label Apr 13, 2024
@SentryMan SentryMan added this to the 9.13 milestone Apr 13, 2024
@SentryMan SentryMan requested a review from rbygrave April 13, 2024 19:50
@SentryMan SentryMan self-assigned this Apr 13, 2024
@SentryMan SentryMan enabled auto-merge April 13, 2024 19:50
@rbygrave
Copy link
Contributor

Fixes ProtoType Providers

In that a @Prototype Provider<T> registers a Provider<Provider<T>> - with double nested Provider and really it should only be Provider<T> in this case.

rbygrave added a commit that referenced this pull request Apr 14, 2024
Extracted this fix from #540

@prototype with Provider<T> currently produces code that registers Provider<Provider<T>> instead of Provider<T> and this fixes that.
rbygrave added a commit that referenced this pull request Apr 14, 2024
Extracted this refactoring from #540

The existing .registerProvider() without a leading .asPrototype() is already a "lazy only once" style provider. This change uses that and then removes the LazyProvider and registerLazy() as they are no longer needed or used.

This also changes the synchronized block to use a ReentrantLock in OnceProvider just in case the provider invocation does something interesting and using Java 21+ Virtual Threads.
@rbygrave
Copy link
Contributor

Thanks. I've ended up pulling this out into 2 separate PRs because I kind of needed to do that to understand the changes and see the impact and also because we should really have bug fixes in their own PR and not combined with other things like a refactor.

So yeah, created #541 and #542 and yes these are pretty much exactly what is in this PR.

Cheers, Rob.

rbygrave added a commit that referenced this pull request Apr 14, 2024
Extracted this fix from #540

@prototype with Provider<T> currently produces code that registers Provider<Provider<T>> instead of Provider<T> and this fixes that.
rbygrave added a commit that referenced this pull request Apr 14, 2024
Extracted this refactoring from #540

The existing .registerProvider() without a leading .asPrototype() is already a "lazy only once" style provider. This change uses that and then removes the LazyProvider and registerLazy() as they are no longer needed or used.

This also changes the synchronized block to use a ReentrantLock in OnceProvider just in case the provider invocation does something interesting and using Java 21+ Virtual Threads.
@rbygrave rbygrave closed this Apr 14, 2024
auto-merge was automatically disabled April 14, 2024 10:05

Pull request was closed

@SentryMan SentryMan deleted the providers branch April 14, 2024 13:25
@rbygrave rbygrave modified the milestones: 9.13, 10.0 Apr 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants