avaje / avaje-inject

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

hope to support `@Assisted...` Annotation #466

Closed taodongl closed 9 months ago

taodongl commented 10 months ago

I am using Guice. leveraging @AssistedInject and @Assisted, its factory module can create new instance with parameter. Avaje seems to lack this feature.

Refer to: https://github.com/google/guice/wiki/AssistedInject

SentryMan commented 10 months ago

I'm not sure if I get it, but that seems like something @Factory handles. You can inject beans into factories which themselves provide beans.

taodongl commented 10 months ago

@SentryMan Sorry make you confuse. I give you an scenario: when I traverse directory at runtime, I want to create multiple instances with each file path as parameter and inject some configuration at the same time.

SentryMan commented 10 months ago

I understand now, I'll see what I can do.

rbygrave commented 10 months ago

Be careful, I don't know if this is a good idea or not. I'm skeptical, I'd like to see an actual code example. Seems like it should to easy to separate the injected and non here.

On Sun, 3 Dec 2023, 5:01 am Josiah Noel, @.***> wrote:

I understand now, I'll see what I can do.

— Reply to this email directly, view it on GitHub https://github.com/avaje/avaje-inject/issues/466#issuecomment-1837189890, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABTATIKR6V3X5YNVMLBHI3YHNGGZAVCNFSM6AAAAABAEC57PKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMZXGE4DSOBZGA . You are receiving this because you are subscribed to this thread.Message ID: @.***>

rbygrave commented 10 months ago

I'll be more blunt, I think guice assisted injection is a bad idea. You can generally achieve the same result without it.

If we are to add it as a feature it needs to provide value, do something that can't be done already.

Ultimately this comes down to what is done with the instances that are created by the method call.

If you provide a code example and explain how the instances created per path are used ... then we can see the value or not.

Can you do that?

Thanks, Rob.

On Sun, 3 Dec 2023, 12:25 pm Rob Bygrave, @.***> wrote:

Be careful, I don't know if this is a good idea or not. I'm skeptical, I'd like to see an actual code example. Seems like it should to easy to separate the injected and non here.

On Sun, 3 Dec 2023, 5:01 am Josiah Noel, @.***> wrote:

I understand now, I'll see what I can do.

— Reply to this email directly, view it on GitHub https://github.com/avaje/avaje-inject/issues/466#issuecomment-1837189890, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABTATIKR6V3X5YNVMLBHI3YHNGGZAVCNFSM6AAAAABAEC57PKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMZXGE4DSOBZGA . You are receiving this because you are subscribed to this thread.Message ID: @.***>

SentryMan commented 10 months ago

It seems to me to be the halfway point between DI and no DI. you want some of the dependencies to be managed by the scope, while the rest should be provided manually.

SentryMan commented 10 months ago

dagger also seems to have it as well

SentryMan commented 10 months ago

I mean sure somebody could do this manually like so:

public class Car  {
    public Car(Paint paint, Engine engine) {
      ...
    }
...
}

public interface CarFactory {
    Car createPassengerVehicle(Paint paint);
}

@Factory
public class CarFactoryConfiguration {
    @Bean
    CarFactory carFactory(Engine engine) {
        return paint -> new Car(paint, engine);
    }
}

...

Car car = scope.get(CarFactory.class).createPassengerVehicle(blue);

but a simple

@Assisted
public class Car  {
    public Car(@Assist Paint paint, Engine engine) {
     ...
    } 
}

Car car = scope.getAssisted(blue);

is a lot less work.

rbygrave commented 10 months ago

Dagger was created by the same folks who created Guice - so yes that is not surprising.

I mean sure somebody could do this manually like so:

Well, I'm saying lets get a VERY CLEAR picture of exactly what is desired and lets not guess just in case we are missing something (because everything I've seen of assisted injection thus far strongly indicates that it isn't necessary).

is a lot less work.

It's "concept overhead" - it's adding a very different concept into DI. IF it is justified then let's see the justification but I am yet to see the clear justification for this. Until we know how the created instances are used we can't see the justification for this.

I'll wait to see the real world example of how this is being used / desired to work.

rbygrave commented 10 months ago

Car car = scope.get(CarFactory.class).createPassengerVehicle(blue);

This is NOT how it would actually be done in practice in that CarFactory would be injected into another component. The end result would not look like that. I'm not expecting scope to be used explicitly.

@taodongl You can provide the code example for this right ?

taodongl commented 10 months ago

Example about Guice Assisted Inject:

// application Module --- Load plugin module

final class BootstrapModule extends AbstractModule {
    @Override
    protected void configure() {
        ServiceLoader<BaseModule> loader = ServiceLoader.load(BaseModule.class);
        Multibinder<Workspace> workspaces = Multibinder.newSetBinder(binder(), Workspace.class);
        for (BaseModule module : loader) {
            install((Module) module);
            workspaces.addBinding().to(module.getWorkspace());
        }
        bind(Context.class).to(ContextImpl.class).asEagerSingleton();
    }
}

// plugin Module

public class CssModule extends AbstractModule implements BaseModule {
    @Override
    protected void configure() {
        install(new FactoryModuleBuilder()
                .implement(Scanner.class, Names.named("src"), CssScanner.class)
                .build(CssFactory.class));
    }
    @Override
    public Class<? extends Workspace> getWorkspace() {
        return CssWorkspace.class;
    }
}

// plugin Factory

interface CssFactory {
    @Named("src") Scanner getScanner(Path file);
}

// plugin workspace

class CssWorkspace implements Workspace {
    private final CssFactory factory;
    private final Context context;
    @Inject
    CssWorkspace(Context context, CssFactory factory) {
        this.context = context;
        this.factory = factory;
    }
    @Override
    public void categorize(Path file, String ext) {
        if (context.supportRTLIssue()) {
            var scanner = factory.getScanner(file); // here, Factory create new **CssScanner** with file argument and inject **Context** into scanner
            ... ...
            ... ...
        }
    }
}

// plugin scanner

class CssScanner implements Scanner  {
    private final Context context;
    private final Path file;
    @Inject
    public CssScanner(@Assisted Path file, Context context) {
        this.file = file;
        this.context = context;
    }
}
SentryMan commented 10 months ago

Your example is weakened by the fact that you also inject Context into the CssWorkspace class, but I get it. I can see situations where you wouldn't want to inject unrelated beans for the sake of the factory.

rob-bygrave commented 10 months ago

What calls the categorize(Path file, String ext) method?

[the question is heading towards understanding the "scope" of file, where does it come from etc, this is expected to be the same "scope" as the instance that returns from factory.getScanner(file)]

[Said differently, the scope of file (method argument) is the same scope as var scanner and neither of these things are in some DI scope. That is, we are not putting the var scanner anywhere special but just using it here - the scope of var scanner matches the file method argument ]

rob-bygrave commented 10 months ago

inject unrelated beans for the sake of the factory

BUT now it is less clear with respect to the scope and the boundary between DI and non-DI instances. At this stage it looks like we can do this very easily with 1 line of code without "assisted injection" and the benefit of doing it without "assisted" is that it keeps very clear boundaries and very clear scope. [which is why I an extremely hesitant on this].

SentryMan commented 10 months ago

BUT now it is less clear with respect to the scope and the boundary between DI and non-DI instances.

I don't follow, it looks clear to me. The CssFactory is managed by the DI scope but the objects it creates are not, which is to be expected.

SentryMan commented 10 months ago

I changed the PR to only generate factories, what do you think about that? So if we have:

@AssistFactory
class CssScanner implements Scanner  {
    @Inject
    public CssScanner(@Assist Path file, Context context) {
     ...
    }
}

Then we generate

/**
 * Generated source - Factory for CssScanner.
 */
@Generated("io.avaje.inject.generator")
@Component
public final class CssScanner$Factory {
  private final Context context;
  CssScanner$Factory(Context context) {
    this.context = context;
  }
  /**
   * Fabricates a new CssScanner.
   */
  public CssScanner create(Path file) {
    var bean = new CssScanner(file, context);
    return bean;
  }
}
rob-bygrave commented 10 months ago

I changed the PR to only generate factories, what do you think about that?

Step 1 is to completely understand the issue and the motivation. We should be only looking at the "How" after we have completed Step 1 (and we have not yet completed Step 1).

With Guice we have the following DSL to support CssFactory:

new FactoryModuleBuilder()
                .implement(Scanner.class, Names.named("src"), CssScanner.class)
                .build(CssFactory.class)

... and instead it could have been:

@Named("src")
@Component
public final class CssScannerFactory {
  private final Context context;
  CssScannerFactory(Context context) {
    this.context = context;
  }
  public CssScanner create(Path file) {
    return new CssScanner(file, context);
  }
}

and used in CssWorkspace like:

    @Inject
    CssWorkspace(CssScannerFactory scannerFactory, ... ) {

So why was it not done this way? What was the motivation to use @Assisted Path file instead? What are we gaining by using @Assisted?

rob-bygrave commented 10 months ago

To be explicit @taodongl I'm keen to here your answers to the following:

Edit: I want to understand this first before we outline what the explicit downsides of @Assisted are.

taodongl commented 10 months ago

To be explicit @taodongl I'm keen to here your answers to the following:

  • Q: Why didn't you just create a factory yourself?
  • Q: What was the motivation to use @assisted Path file instead?
  • Q: What are we gaining by using @assisted? (over just creating the factory ourselves explicitly in code)

Edit: I want to understand this first before we outline what the explicit downsides of @Assisted are.

I don't "create a factory yourself" because I want to guice help me to inject Context and Others to scanner. the above "CssScanner" is simplified, in fact:

            ResourceFileScanner \
                                          --> Scanner
CssScanner -> SourceFileScanner / 

As for ResourceFileScanner and SourceFileScanner, they use Member Inject separately

public abstract class SourceFileScanner implements Scanner {
    @Inject
    protected Context context;
    @Inject
    protected SpellChecker checker;
    protected Path file;
   SourceFileScanner(Path file) {
       this.file = file;
   }
    ... ...
}

CssScanner becomes

public class CssScanner extends SourceFileScanner {
    @Inject
    public CssScanner(@Assisted Path file) {
         super(file);
    }
}

Besides of CssScanner, there are HtmlScanner, JavascriptScanner, JsonScanner ... they belongs to different workspace and factory. In Files.walk(...) the related workspace is called based on file extension.

rob-bygrave commented 10 months ago

I want to guice help me to inject Context and Others to scanner

More specifically you want guice to create a factory for you that will create instances of CssScanner. You know that you could create the factory yourself fairly easily (like you would if you were using Spring or Micronaut of avaje-inject) but you want guice to create the factory for you. [and hence you want avaje-inject to also create a factory for you]

Right?

taodongl commented 10 months ago

without @Assisted, the parameter number of constructor method increases, and when constructor method of parent class is changed, all of inherited class are impacted.
DI framework could reduce code and manage dependencies. Guice depends on Guava, so size of their libraries is huge. And my console application is size-sensitive though proguard could remove some useless codes. If I follow what you said: create factory by myself without injection, the others injection becomes valueless --- after all, all of objects managed by DI framework can be replaced with global singleton object.

rob-bygrave commented 10 months ago

all of objects managed by DI framework can be replaced with global singleton object.

Sort of true as I see it. I myself for example manually write dependency injection code for some libraries that I have (and thus not requiring any DI library). What we get with using a DI library like avaje-inject is component testing plus a bunch of other things (other scopes like request scope and prototype scope, lifecycle support etc), but for a cli application with only a handful of components you could very easily write the wiring/DI code manually (just like I do for some cases).

create factory by myself without injection, the others injection becomes valueless

Noting that Spring DI and Micronaut both also do not have assisted injection and I also do not believe CDI has assisted injection either but I'm happy to corrected about CDI there. So those are pretty popular DI libraries that do not see the value.

Ultimately, that is because Guice @Assisted is pretty much syntax sugar for creating a component that acts as a factory and as such isn't anything more special than that - hence it doesn't exist as a feature of Spring, Micronaut or CDI.

What we are asking for here is then to add a feature that can be achieved by other means (as per Spring, Micronaut etc). We need to evaluate the pros and cons of that and understand that adding such a feature should have some reasonable net benefit.


Part of the problem as I see it is that @Assisted does not promote a good mental model for where DI starts and ends versus where "plain ordinary java code" takes over.

For example, an approximate mental model (when restricting to constructor injection) is that DI injects into the constructor only, and all other methods are "plain ordinary java code". We see that @Assisted really goes against that mental model ... and it does that by synthetically creating factories.

So looking at this example we see a mix of injected and not-injected

    @Inject
    protected Context context;
    @Inject
    protected SpellChecker checker;
    protected Path file;

... and see how only Path file is passed into the constructor, and context and checker are injected by DI but not into the constructor, and now they are not final fields, and perhaps this isn't great from a testing or understanding perspective.

public class CssScanner extends SourceFileScanner {
    @Inject
    public CssScanner(@Assisted Path file) {
         super(file);
    }
}

versus something like:

// Now I'm a plain ordinary class
public class CssScanner extends SourceFileScanner {

    public CssScanner(Path file, Context context, SpellChecker checker) {
         super(file, context, checker);
    }
}

/**
 * Creating CssScannerFactory by "hand" rather than `@Assisted` synthetically creating this factory code.
 */
@Named("src")
@Component
final class CssScannerFactory {

  private final Context context;
  private final SpellChecker checker;

  // DI wires my dependencies via constructor
  CssScannerFactory(Context context,  SpellChecker checker) {
    this.context = context;
    this.checker = checker;
  }

  // plain old java code ... 
  public CssScanner create(Path file) {
    return new CssScanner(file, context, checker);
  }
}

@Component
class CssWorkspace implements Workspace {

    private final CssFactory factory;
    private final Context context;

    @Inject
    CssWorkspace(Context context, CssFactory factory) {
        this.context = context;
        this.factory = factory;
    }
    @Override
    public void categorize(Path file, String ext) {
        if (context.supportRTLIssue()) {
            var scanner = factory.getScanner(file);
            ... ...
            ... ...
        }
    }
}
natros commented 10 months ago

Here is an example of dagger assisted injection:

import dagger.assisted.Assisted;
import dagger.assisted.AssistedFactory;
import dagger.assisted.AssistedInject;

public class DialogBox {
  private final Messages messages;

  @AssistedInject
  DialogBox(Messages messages, @Assisted String caption) {
    this.messages = messages;
    messages.log(String.format("creating DialogBox(%s)", caption));
  }

  public void show() {
    messages.log("show dialog");
  }

  @AssistedFactory
  public interface Factory {
    DialogBox create(String caption);
  }
}
public class App {
  private final DialogBox.Factory dialogBoxFactory;

  @Inject
  App(DialogBox.Factory dialogBoxFactory) {
    this.dialogBoxFactory = dialogBoxFactory;
  }

  public void run() {
    DialogBox dlg1 = dialogBoxFactory.create("dlg1");
    DialogBox dlg2 = dialogBoxFactory.create("dlg2");
  }
}

Micronaut supports assisted injection: https://github.com/micronaut-projects/micronaut-core/discussions/8461

rob-bygrave commented 10 months ago

Can you copy and paste up the Factory implementation that Dagger generates?

natros commented 10 months ago

I created a repo with the example: https://github.com/natros/dagger-assisted-demo

And here is the implemented factories:

DialogBox_Factory:


import dagger.internal.DaggerGenerated;
import dagger.internal.QualifierMetadata;
import dagger.internal.ScopeMetadata;
import javax.annotation.processing.Generated;
import javax.inject.Provider;

@ScopeMetadata
@QualifierMetadata
@DaggerGenerated
@Generated(
    value = "dagger.internal.codegen.ComponentProcessor",
    comments = "https://dagger.dev"
)
@SuppressWarnings({
    "unchecked",
    "rawtypes",
    "KotlinInternal",
    "KotlinInternalInJava"
})
public final class DialogBox_Factory {
  private final Provider<Messages> messagesProvider;

  public DialogBox_Factory(Provider<Messages> messagesProvider) {
    this.messagesProvider = messagesProvider;
  }

  public DialogBox get(String caption) {
    return newInstance(messagesProvider.get(), caption);
  }

  public static DialogBox_Factory create(Provider<Messages> messagesProvider) {
    return new DialogBox_Factory(messagesProvider);
  }

  public static DialogBox newInstance(Messages messages, String caption) {
    return new DialogBox(messages, caption);
  }
}

DialogBox_Factory_Impl:

import dagger.internal.DaggerGenerated;
import dagger.internal.InstanceFactory;
import javax.annotation.processing.Generated;
import javax.inject.Provider;

@DaggerGenerated
@Generated(
    value = "dagger.internal.codegen.ComponentProcessor",
    comments = "https://dagger.dev"
)
@SuppressWarnings({
    "unchecked",
    "rawtypes",
    "KotlinInternal",
    "KotlinInternalInJava"
})
public final class DialogBox_Factory_Impl implements DialogBox.Factory {
  private final DialogBox_Factory delegateFactory;

  DialogBox_Factory_Impl(DialogBox_Factory delegateFactory) {
    this.delegateFactory = delegateFactory;
  }

  @Override
  public DialogBox create(String caption) {
    return delegateFactory.get(caption);
  }

  public static Provider<DialogBox.Factory> create(DialogBox_Factory delegateFactory) {
    return InstanceFactory.create(new DialogBox_Factory_Impl(delegateFactory));
  }
}

App_Factory:

import dagger.internal.DaggerGenerated;
import dagger.internal.Factory;
import dagger.internal.QualifierMetadata;
import dagger.internal.ScopeMetadata;
import javax.annotation.processing.Generated;
import javax.inject.Provider;

@ScopeMetadata
@QualifierMetadata
@DaggerGenerated
@Generated(
    value = "dagger.internal.codegen.ComponentProcessor",
    comments = "https://dagger.dev"
)
@SuppressWarnings({
    "unchecked",
    "rawtypes",
    "KotlinInternal",
    "KotlinInternalInJava"
})
public final class App_Factory implements Factory<App> {
  private final Provider<DialogBox.Factory> dialogBoxFactoryProvider;

  public App_Factory(Provider<DialogBox.Factory> dialogBoxFactoryProvider) {
    this.dialogBoxFactoryProvider = dialogBoxFactoryProvider;
  }

  @Override
  public App get() {
    return newInstance(dialogBoxFactoryProvider.get());
  }

  public static App_Factory create(Provider<DialogBox.Factory> dialogBoxFactoryProvider) {
    return new App_Factory(dialogBoxFactoryProvider);
  }

  public static App newInstance(DialogBox.Factory dialogBoxFactory) {
    return new App(dialogBoxFactory);
  }
}
natros commented 10 months ago

Some real-world scenarios where assisted injection is useful

@Path("/pdf")
public class PdfResouce {
  private final Pdf.Factory pdfFactory;

  @Inject
  PdfResouce(Pdf.Factory pdfFactory) {
    this.pdfFactory = pdfFactory;
  }

  @GET
  @Path("{id}")
  public Response getPdfResouce(@PathParam("id") int id, @QueryParam("draft") boolean draft) {
    Pdf pdf = pdfFactory.create(id, draft);
    return Response.ok(pdf.generate(), "application/pdf").build();
  }
}
public class Pdf {
  @AssistedInject
  public Pdf(
      UserRepository userRepository,
      AgreementRepository agreementRepository,
      @Assisted("id") int id,
      @Assisted("dratf") boolean draft) {}

  public ByteArrayOutputStream generate() {...}

  @AssistedFactory
  public interface Factory {
    Pdf create(@Assisted("id") int id, @Assisted("dratf") boolean draft);
  }
}

Without partial injection I have to call the setters:

public class PdfResouce {
  private final Provider<Pdf> pdfProvider;

  @Inject
  PdfResouce(Provider<Pdf> pdfProvider) {
    this.pdfProvider = pdfProvider;
  }

  @GET
  @Path("{id}")
  public Response getPdfResouce(@PathParam("id") int id, @QueryParam("draft") boolean draft) {
    Pdf pdf = pdfProvider.get();
    pdf.setId(id);
    pdf.setDraft(draft);
    return Response.ok(pdfProvider.generate(), "application/pdf").build();
  }
}
rbygrave commented 10 months ago

Without partial injection I have to call the setters:

Well no you wouldn't (at least I expect not). Without partial injection we'd still have Pdf.Factory but now we'd write it ourselves by hand rather than have it generated for us.

The thing to me, is that for this case that the main "concept" here is the Pdf.Factory. It's API could in fact be:

interface Pdf.Factory {
  ByteArrayOutputStream generate(int id, boolean draft);
}

So I see Pdf.Factory as the main concept, that takes some input params and produces BAOS. With @Assisted that is more twisted such that Pdf is the main concept ... and that Pdf is this mix of "data" and "business logic/DI components".

Just to say what I see with these examples is:

Hmmm.

natros commented 10 months ago

For those projects that don't support parametrized beans, there's always Google AutoFactory that helps. I haven't tested it with avaje-inject. https://github.com/google/auto/tree/main/factory

SentryMan commented 10 months ago

we'd write it ourselves by hand rather than have it generated for us.

Pretty sure this is a main point of contention, having factories get auto-generated for us is quite convenient.

Just to say what I see with these examples is...

while immutability is indeed the best thing, it seems like this is a common pattern for guice/dagger users regardless.

rob-bygrave commented 10 months ago

support parametrized beans, there's always Google AutoFactory that helps

Yes. [and as I see it, to a large extent this is about adding a "auto factory" feature to avaje-inject]

having factories get auto-generated for us is quite convenient.

Yes. [and I'm slowly working through in my head if this is "a useful convenience" or "letting a bad idea propagate" ... and I'm working through the thinking behind why some people see a "bad idea propagating"]

immutability is indeed the best thing, it seems like this is a common pattern for guice/dagger users regardless

Right. I have an opinion on how it comes to be that guice/dagger folks see things differently from spring folks.

... but yes, I'm still working through the thinking around "a useful convenience" vs "letting a bad idea propagate" (and I don't hear any of you acknowledging that other perspective / the "bad ju ju" side of this so I get the impression you are not feeling that other side [so maybe it's all in my head right?].


So let me talk about:

... and get all my "this is bad ju ju" out and see where that goes.

SentryMan commented 10 months ago

you are not feeling that other side

This is the first time I've even heard of this concept so I've got no feelings one way or the other.

I might be unlikely to use it, but since inject is meant to be the "server-side dagger" it seems natural to have this dagger feature?

so this looks fairly "anti-record types".

what if you do something like this?

@AssistFactory
record Recorder(@Assist int num, @Assist String str) {
    @Inject
    void validate(Validator valid) {
       valid.validate(this);
    }
}

the idea being that you can perform some action using beans from the scope every time you create a new instance with the factory.

SentryMan commented 10 months ago

It's been 2 weeks, what are we thinking?

rbygrave commented 10 months ago

It's the super busy period before the extended summer break down under, so I've been busy ... might not get back to this until mid-Jan. In the meantime my gut is saying that there are 2 ways of doing these things - 1 via assisted where the factory is trivial and the logic is in the assisted bean, the other where the logic is moved into a component (stateless, same scope as all the other components etc). There are downsides to assisted in that it can no longer be composed/injected into other components and I don't see the upside to assisted. For example, move the logic into the PdfFactory/PdfGenerator and there is no need for Pdf or assisted injection at all (and then all components have the same scope, same nature in terms of being stateless etc).

So I'm happy to continue this conversation and maybe we won't conclude the conversation until Jan etc.

SentryMan commented 10 months ago

So I'm happy to continue this conversation and maybe we won't conclude the conversation until Jan etc.

sure, answer back whenever you find the time.

there are 2 ways of doing these things - 1 via assisted where the factory is trivial and the logic is in the assisted bean, the other where the logic is moved into a component (stateless, same scope as all the other components etc).

Assisted does seem a bit like a lateral move since they could do mostly the same thing without it.

There are downsides to assisted in that it can no longer be composed/injected into other components and I don't see the upside to assisted.

Combine with regular @Factory and you can inject them into other components just fine, but most aren't going to want to use assisted beans like that, so from that perspective there isn't any downside. I perceive the upside for them is that they can have the logic inside the assisted bean.

For me, the main angle against this is trying to avoid feature creep.

rob-bygrave commented 9 months ago

avoid feature creep

Well, I see this as a "feature" that doesn't provide a "net benefit" (pros out weigh the cons).

That is, adding support for @Assisted does not provide a mechanism/technique that can't already be done already using a normal component. It is a "style only" type of feature where the logic can instead be moved to a component to achieve the same thing.

I'm saying it is not a "net benefit" because it does not assist people in terms of their mental model of dependency injection and that the alternative "style" of moving the logic into a "stateless component" is a better longer term approach than @Assisted (which can't be composed and injected into other components and complicates the mental model people have).

That is, I see the benefit/pros of adding @Assisted as just allowing easier migration of Dagger/Guice applications but I don't see that benefit out weighing the cons.

Do other people see the pros out weighing the cons? Can you provide some comments on that?

SentryMan commented 9 months ago

That is, I see the benefit/pros of adding @Assisted as just allowing easier migration of Dagger/Guice applications.

See now this is a real selling point for me.

cons

Can you give a list of your cons? Just want to make sure we're on the same page.

which can't be composed and injected into other components

We can though? (unless I misunderstand your meaning) consider:

@Assisted
class Car  {
    public Car(@Assist Paint paint, Engine engine) {
     ...
    } 
}
@Factory
class FactoryBean {

  @Bean
   Car getCarFromFactory(Car$Factory carFactory){
      Paint paint =  //create paint somehow
      return carFactory.create(paint);
    }
}
rob-bygrave commented 9 months ago

Paint paint = //create paint somehow

Here is the problem - that paint instance must be passed in via application code.

We can though?

Nope.

Edit: Technically this is close to "Request scoped Controllers" where the Controller has the ServerRequest &/or ServerResponse injected into it. In this case avaje-inject / avaje-http generates the factory for the controller, the component that is injected in that case is the factory - the controller itself is not injectable. We do this because JAX-RS supports this - "request scoped controllers".

SentryMan commented 9 months ago

"stateless component" is a better longer term approach than @Assisted

Let's change the angle a little bit. Suppose I want to only create pure data classes and validate them with our avaje validator after construction:

@AssistFactory
record Recorder(@Assist int num, @Assist String str) {
  @Inject
  void validate(Validator valid) {
    valid.validate(this);
  }
}

@Singleton
class ServiceClass {

  @Inject Recorder$Factory factory;

  void doSomething(int num, String str) {
    Recorder recorder = factory.create(num, str);
    // ... do something with my validated recorder
  }
}

vs manually creating with non-assisted:

record Recorder(int num, String str) {}

@Singleton
class ManualRecorderFactory {
   Validator valid;
   ManualRecorderFactory(Validator valid) {
    this.valid = valid;
   }
  Recorder create(int num, String str) {
    var recorder = new Recorder(num, str);
    valid.validate(recorder);
    return recorder;
  }
}

@Singleton
class ServiceClass {

  @Inject ManualRecorderFactory factory;

  void doSomething(int num, String str) {
    Recorder recorder = factory.create(num, str);
    // ... do something with my validated recorder
  }
}

Is the latter really more correct?

rob-bygrave commented 9 months ago

Lets not forget it could just be: (given that the factory only exists to combine passed in state with DI components)

record Recorder(int num, String str) {}

@Singleton
class ServiceClass {

  @Inject Validator valid;

  void doSomething(int num, String str) {
    var recorder = new Recorder(num, str);
    valid.validate(recorder);
    // ... do something with my validated recorder
  }
}
SentryMan commented 9 months ago

Lets not forget it could just be:

Think bigger, if I have multiple service classes, duplicating the same code in each one isn't fun. Or suppose I'm creating a library and I want my data types to be created in a specific way via factory.

rbygrave commented 9 months ago

want my data types to be created in a specific way via factory.

This is the case for me in terms of understanding the value proposition. I looked back at the CssFactory CssScanner example, along the lines:

Inputs:

interface CssFactory {
    Scanner getScanner(Path file);
}

@Named("myName") // <!-- optional name qualifiers can be defined to copy to the generated component
@AssistedFactory(target=CssFactory) // <!-- specify the factory interface to generate
class CssScanner implements Scanner  {

    private final Context context;
    private final Path file;

    public CssScanner(@Assisted Path file, Context context) {
        this.file = file;
        this.context = context;
    }
}

// its possible that the `@Assisted` parameters can be inferred from the factory interface method.
// that is, we might not need the explicit `@Assisted` above for Path file.

Generating:

@Generated("io.avaje.inject.generator")
@Component
@Named("myName")   // <- Name qualifier if present is copied down 
final class CssScanner$AssistedFactory implements CssFactory {
  private final Context context;
  CssScanner$AssistedFactory(Context context) {
    this.context = context;
  }

  @Override
  public Scanner create(Path file) {
    var bean = new CssScanner(file, context);
    return bean;
  }
}

And then CssFactory can be injected and used like:

@Component
class CssWorkspace implements Workspace {

    private final CssFactory factory;
    private final Context context;

    CssWorkspace(Context context, @Named("myName") CssFactory factory) {
        this.context = context;
        this.factory = factory;
    }
    @Override
    public void categorize(Path file, String ext) {
        if (context.supportRTLIssue()) {
            var scanner = factory.getScanner(file);
            ... ...
        }
    }
}
SentryMan commented 9 months ago

ok, I can do this, though we can probably make the interface targeting an optional thing.

rbygrave commented 9 months ago

FYI Work-In-Progress update:

At least initially AssistFactory will need to specify a factory interface (as there is a limitation that we can't inject the generated factory initially probably because it does not exist in the first compilation round - so DI validation fails with it being a missing dependency).

rob-bygrave commented 9 months ago

Hi @taodongl @natros - AssisstedFactory / Assisted is in version 9.11-RC2 which was just released to maven central. Would you be able to try it out sometime and confirm it works as you expect?

Note that the CssFactory CssScanner tests are at: https://github.com/avaje/avaje-inject/blob/master/blackbox-test-inject/src/main/java/org/example/myapp/assist/css/CssScanner.java#L9 https://github.com/avaje/avaje-inject/blob/master/blackbox-test-inject/src/main/java/org/example/myapp/assist/CssFactory.java

CssFactory used at: https://github.com/avaje/avaje-inject/blob/master/blackbox-test-inject/src/main/java/org/example/myapp/assist/CssThing.java

CssFactoryTest: https://github.com/avaje/avaje-inject/blob/master/blackbox-test-inject/src/test/java/org/example/myapp/assist/css/CssFactoryTest.java#L15-L29

Thanks, Rob.