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

Support Generic Auto Provides/Requires #498

Merged
merged 31 commits into from
Apr 17, 2024

Conversation

SentryMan
Copy link
Collaborator

@SentryMan SentryMan commented Feb 11, 2024

  • Changes the Module interface to use Type[] instead of Class<?>[]
  • Updates generator to allow generic types to be added to auto provides/requires via GenericType
  • updates the maven/gradle plugin to use Type[] instead of Class<?>[]

Now we can have a generic (factory) bean in Module A:

@Component
public class OtherComponentGeneric implements Supplier<String> {

  @Override
  public String get() {
    return "hello";
  }
}

The generated Avaje Module class will have:

public final class ModuleClass implements Module {
// ...rest of the module class
  @Override
  public Type[] autoProvides() {
    return autoProvides;
  }
  private static final Type[] autoProvides = {
    new GenericType<java.util.function.Supplier<java.lang.String>>(){},
    OtherComponentGeneric.class,
  };
// ...rest of the module class

and in Module B we can reference via:

@Component
public class HasExternalDependencies {

  public final Supplier<String> stringSupplier;

  public HasExternalDependencies(
      Supplier<String> stringSupplier) {
    this.stringSupplier = stringSupplier;
  }
}

@SentryMan SentryMan added the enhancement New feature or request label Feb 11, 2024
@SentryMan SentryMan added this to the 9.11 milestone Feb 11, 2024
@SentryMan SentryMan self-assigned this Feb 11, 2024
@SentryMan SentryMan requested a review from rbygrave February 11, 2024 22:27
@rbygrave
Copy link
Contributor

Did somebody hit a limitation here / was there a requirement for this?

The autoProvides is for wiring across modules. I tend to believe that cross-module dependencies should be interfaces and I tend to think they shouldn't use generic types. Has someone hit an issue here?

@SentryMan
Copy link
Collaborator Author

Is it so inconceivable that a module would provide generic types? I've got a mongo common library that handles authentication and provides some common MongoCollection beans. Currently, I have to wrap the generic types in a non-generic wrapper class to wire across modules.

@SentryMan
Copy link
Collaborator Author

Factory beans that return Maps are also not uncommon. Overall, wrapping generic types to get them to wire across modules is quite a hassle.

@rob-bygrave
Copy link
Contributor

Currently, I have to wrap the generic types in a non-generic wrapper class to wire across modules.

That sounds like a concrete class being used across a module boundary - so yeah I'm really surprised as that sounds like relatively tight coupling between modules. Can you share an example? [so that I can get a sense of the coupling, I'm also wondering if this is changing with the java module system in terms of tight coupling]

Noting that this is a breaking change - it would need a bump to the major version.

@SentryMan
Copy link
Collaborator Author

SentryMan commented Feb 13, 2024

Noting that this is a breaking change - it would need a bump to the major version

Surprisingly, it doesn't break anything except the plugins. Class<?> extends Type so modules generated before this change will load and work as expected.

@SentryMan
Copy link
Collaborator Author

Can you share an example?

an example of wrapping?
In module A:

@Singleton
public record WrapperClass(Map<String, String> map){}

Then you can inject WrapperClass into a bean in some module B.

@rob-bygrave
Copy link
Contributor

an example of wrapping?

I meant a real world example - as in, we are using tight coupling between 2 modules with a concrete class (and not an interface). What is real world example where this is a good idea (versus a bad idea because that is tight coupling between modules that uses an implementation - a non-abstract implementation class to coupling 2 modules together ... and I'm going "Really??").

@SentryMan
Copy link
Collaborator Author

SentryMan commented Feb 14, 2024

modules with a concrete class (and not an interface)

I don't follow, the point of this PR is that we can avoid concrete wrapper classes and use the generic interface/super classes directly

@SentryMan
Copy link
Collaborator Author

SentryMan commented Feb 14, 2024

modules with a concrete class (and not an interface)

I did think of an example though. Some time ago I was helping out a guy on discord with avaje and he was quite confused about the current behavior, he had a common library that would configure a Jackson ObjectMapper for his other applications.

The problem was that ObjectMapper is auto provided as Versioned, which nobody recognizes.

public final class ModuleClass implements Module {
// ...rest of the module class
  @Override
  public Class<?>[] autoProvides() {
    return autoProvides;
  }
  private static final Class<?>[] autoProvides = {
    Versioned.class,
  };

so naturally his compilation would fail. I ended up telling him he had to manually use @InjectModule(provides=ObjectMapper.class) to get it to work.

Honestly, any sort of avaje-based configuration library that configures third-party classes would appreciate #501

@SentryMan SentryMan removed this from the 9.11 milestone Feb 20, 2024
@SentryMan SentryMan enabled auto-merge April 7, 2024 04:13
@SentryMan SentryMan requested a review from rob-bygrave April 15, 2024 00:25
@rbygrave rbygrave added this to the 10.0 milestone Apr 17, 2024
@rbygrave rbygrave disabled auto-merge April 17, 2024 08:40
@rbygrave rbygrave merged commit cd4f647 into avaje:master Apr 17, 2024
4 checks passed
@SentryMan SentryMan deleted the generic-requires branch April 17, 2024 12:15
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.

3 participants