avaje / avaje-inject

Dependency injection via APT (source code generation) ala "Server-Side Dagger DI"
https://avaje.io/inject
Apache License 2.0
243 stars 24 forks source link

Add all detected modules/plugins to generated `META-INF/services/io.avaje.inject.spi.InjectExtension` entries #731

Open SentryMan opened 4 days ago

SentryMan commented 4 days ago

So I was thinking about how I did https://github.com/avaje/avaje-inject/pull/704, and then it struck me. What if we do the same for all detected modules/plugins?

In this way, users would no longer need to manually configure gradle or the maven shade/assembly plugin to merge service files when using multi-module projects.

rbygrave commented 3 days ago

when using multi-module projects.

Do I misunderstand this? This ONLY applies when using something like the maven shade plugin to merge all the modules into a single jar right? ... but in that case there are other services etc not just avaje-inject ones?

What problem is this actually solving?

SentryMan commented 3 days ago

This ONLY applies when using something like the maven shade plugin to merge all the modules into a single jar right?

suppose I have a module A in a dependency:

@InjectModule()
public final class AModule implements AvajeModule {

  @Override
  public Type[] autoProvides() {
    return new Type[] {
      A.class
    };
  }
  //.. the rest of it
}

Then I try to wire it in my project:

@Singleton
public class ServiceClass {
  private final A a;
  public ServiceClass(A a) {
    this.a = a;
  }
  1. Does it compile without missing dependency errors? ✅
  2. Does it run on my IDE? ✅
  3. Does it run when I make a fat jar? ❌

The reason is that by default the shade/assembly plugin doesn't properly combine the two META-INF/services/io.avaje.inject.spi.InjectExtension entries (My project's generated one, and the one from the dependency)

It's so common a mistake I had to create a section on the site addressing it.

There are other services etc not just avaje-inject ones?

I mean sure if you want to get technical, but I'm focused on inject here since as a DI framework it's a lot more likely that a given project is going to need to merge InjectExtension service entries to get things to wire.

What problem is this actually solving?

It makes the developer experience better

rbygrave commented 3 days ago

So what is happening when it's a multi-module project that isn't packaged as an Uber jar? Is it not the case that we now have duplicates in META-INF/services ?

SentryMan commented 3 days ago

Is it not the case that we now have duplicates in META-INF/services ?

Yes, but service loader doesn't care as the docs say If a service provider class name is listed more than once in a provider-configuration file then the duplicate is ignored. If a service provider class is named in more than one configuration file then the duplicate is ignored.

SentryMan commented 3 days ago

Scenarios:

rbygrave commented 3 days ago

duplicates in META-INF/services

We are saying that there are:

"No known negative side effects of having duplicates in META-INF/services/io.avaje.inject.spi.InjectExtension"

... so its a question of whether there are unknown side effects that we haven't thought of yet or hit yet. Hmmm.

SentryMan commented 3 days ago

unknown side effects

In terms of the ServiceLoader, the Javadocs define the behavior so it's not unknown there.

If some other thing is using it, well that's a different story. But idk what that would be or if that's even our concern

rbygrave commented 3 days ago

For avaje-inject my mindset is that we have to view it in a 20 year horizon. That is, think of avaje-inject lasting 20 years - being the DI library that we and others use for 20 years. That might sound like a long time but for example Ebean is now at 18 years already.

So this is the mindset we use when we assess features that do slightly unexpected or interesting things (like duplicate entries in META-INF/services).

unknown side effects ... idk what that would be

I don't either. At the moment I'm getting reasonably comfortable that we can merge this in. It's more that we explicitly think about it because this is doing something slightly unexpected (dup entries) and we need to have some confidence that this is reasonably unlikely to mess up other people's applications.

or if that's even our concern

It's more that we need to have some confidence that this is reasonably unlikely to mess up other people's applications OR restrict avaje-inject in the future in some way (be a restriction on some future feature) - to me this seems highly unlikely.


Just explicitly noting that, we are only here with this issue because Java doesn't have a standard "Uber Jar packaging" hence the shade plugin [but module path is impacted so my gut is thinking that this is going to change sometime]. We are also looking at Uber Jar because say we deploy into AWS Lambda's but that also could change to instead use a more layering distribution more like docker etc.

It's likely that this Uber Jar packaging is going to change over the next 20 years (but probably not soon either).

SentryMan commented 3 days ago

It's more that we need to have some confidence that this is reasonably unlikely to mess up other people's applications

The only applications this would mess up are those that depend on META-INF/services/io.avaje.inject.spi.InjectExtension specifically not having any duplicate entries in external dependencies' META-INF/services/io.avaje.inject.spi.InjectExtension. (because if you only look at the current module there would appear to be no duplicates)

Seems like very oddly specific edge case considering the spec for META-INF/services allows for duplicates