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

Detect circular dependency and fail compile with nice error - Was BeanContextFactory.createContext calls builders in the wrong order #51

Closed
aletundo opened this issue Sep 7, 2020 · 11 comments
Assignees
Labels
bug Something isn't working
Milestone

Comments

@aletundo
Copy link

aletundo commented Sep 7, 2020

Hi @rbygrave,

It's not the first time I'm facing this issue. Even if @DependecyMeta annotation contains the correct classes within the dependsOn list, the createContext method calls the builders in the wrong order causing:

Injecting null for com.example.MyClass when creating class com.example.MyClass2 - potential beans to inject: []

The generated createContext methods is like:

  @Override
  public BeanContext createContext(Builder parent) {
    builder.setParent(parent);
    // other builders
    build_MyClass2();
    // other builders
    build_MyClass();
    return builder.build();
  }

Any workarounds I can implement to force the generator to call the builders respecting the dependencies?

Thanks a lot 🙏

@rbygrave
Copy link
Contributor

rbygrave commented Sep 7, 2020

Are you able to provide a test case?

The build order should honour/respect the dependencies so we need to see why that isn't the case.

@aletundo
Copy link
Author

aletundo commented Sep 8, 2020

Hi,

I shared here the source code of the affected project.

Since one of the dependencies is not yet avaialable on a Maven public repository, I'm attaching the generated _di$Factory.java source. Otherwise, varys-api-jclient project (in the same repo) needs to be installed with Maven to success in compiling the affected project.

// package ...
// imports ...
@Generated("io.avaje.inject.generator")
@ContextModule(name="io.varys.monitoringclaimcontroller")
public class _di$Factory implements BeanContextFactory {

  private final Builder builder;

  public _di$Factory() {
    this.builder = BuilderFactory.newBuilder("io.varys.monitoringclaimcontroller", null, null);
  }

  @Override
  public String getName() {
    return builder.getName();
  }

  @Override
  public String[] getProvides() {
    return builder.getProvides();
  }

  @Override
  public String[] getDependsOn() {
    return builder.getDependsOn();
  }

  @Override
  public BeanContext createContext(Builder parent) {
    builder.setParent(parent);
    build_ObjectMapperFactory();
    build_ApiFactory();
    build_RedisConfig();
    build_ChmmServiceImpl();
    build_ObjectMapper();
    build_ApiClient();
    build_MonitoringClaimApi();
    build_MonitoringUnitApi();
    build_ProbeApi();
    build_RedisClientFactory();
    build_RedissonClient();
    build_RedisClient();
    build_MonitoringUnitServiceImpl();
    build_LockServiceImpl();
    build_ProbeServiceImpl();
    build_Controller();
    build_ReservedSidecarSingleProbePatternService();
    build_MonitoringClaimServiceImpl();
    return builder.build();
  }

  @DependencyMeta(type="io.varys.monitoringclaimcontroller.factory.ObjectMapperFactory")
  protected void build_ObjectMapperFactory() {
    ObjectMapperFactory$di.build(builder);
  }

  @DependencyMeta(type="io.varys.monitoringclaimcontroller.factory.ApiFactory")
  protected void build_ApiFactory() {
    ApiFactory$di.build(builder);
  }

  @DependencyMeta(type="io.varys.monitoringclaimcontroller.config.RedisConfig")
  protected void build_RedisConfig() {
    RedisConfig$di.build(builder);
  }

  @DependencyMeta(type="io.varys.monitoringclaimcontroller.service.impl.ChmmServiceImpl",provides={"io.varys.monitoringclaimcontroller.service.ChmmService"})
  protected void build_ChmmServiceImpl() {
    ChmmServiceImpl$di.build(builder);
  }

  @DependencyMeta(type="com.fasterxml.jackson.databind.ObjectMapper", method="io.varys.monitoringclaimcontroller.factory.ObjectMapperFactory$di.build_buildObjectMapper",dependsOn={"io.varys.monitoringclaimcontroller.factory.ObjectMapperFactory"})
  protected void build_ObjectMapper() {
    ObjectMapperFactory$di.build_buildObjectMapper(builder);
  }

  @DependencyMeta(type="io.varys.api.jclient.ApiClient", method="io.varys.monitoringclaimcontroller.factory.ApiFactory$di.build_buildApiClient",dependsOn={"io.varys.monitoringclaimcontroller.factory.ApiFactory"})
  protected void build_ApiClient() {
    ApiFactory$di.build_buildApiClient(builder);
  }

  @DependencyMeta(type="io.varys.api.jclient.endpoint.MonitoringClaimApi", method="io.varys.monitoringclaimcontroller.factory.ApiFactory$di.build_buildMonitoringClaimApi",dependsOn={"io.varys.monitoringclaimcontroller.factory.ApiFactory","io.varys.api.jclient.ApiClient"})
  protected void build_MonitoringClaimApi() {
    ApiFactory$di.build_buildMonitoringClaimApi(builder);
  }

  @DependencyMeta(type="io.varys.api.jclient.endpoint.MonitoringUnitApi", method="io.varys.monitoringclaimcontroller.factory.ApiFactory$di.build_buildMonitoringUnitApi",dependsOn={"io.varys.monitoringclaimcontroller.factory.ApiFactory","io.varys.api.jclient.ApiClient"})
  protected void build_MonitoringUnitApi() {
    ApiFactory$di.build_buildMonitoringUnitApi(builder);
  }

  @DependencyMeta(type="io.varys.api.jclient.endpoint.ProbeApi", method="io.varys.monitoringclaimcontroller.factory.ApiFactory$di.build_buildProbeApi",dependsOn={"io.varys.monitoringclaimcontroller.factory.ApiFactory","io.varys.api.jclient.ApiClient"})
  protected void build_ProbeApi() {
    ApiFactory$di.build_buildProbeApi(builder);
  }

  @DependencyMeta(type="io.varys.monitoringclaimcontroller.factory.RedisClientFactory",dependsOn={"io.varys.monitoringclaimcontroller.config.RedisConfig"})
  protected void build_RedisClientFactory() {
    RedisClientFactory$di.build(builder);
  }

  @DependencyMeta(type="org.redisson.api.RedissonClient", method="io.varys.monitoringclaimcontroller.factory.RedisClientFactory$di.build_buildRedissonClient",dependsOn={"io.varys.monitoringclaimcontroller.factory.RedisClientFactory"})
  protected void build_RedissonClient() {
    RedisClientFactory$di.build_buildRedissonClient(builder);
  }

  @DependencyMeta(type="io.lettuce.core.RedisClient", method="io.varys.monitoringclaimcontroller.factory.RedisClientFactory$di.build_buildRedisClient",dependsOn={"io.varys.monitoringclaimcontroller.factory.RedisClientFactory"})
  protected void build_RedisClient() {
    RedisClientFactory$di.build_buildRedisClient(builder);
  }

  @DependencyMeta(type="io.varys.monitoringclaimcontroller.service.impl.MonitoringUnitServiceImpl",provides={"io.varys.monitoringclaimcontroller.service.MonitoringUnitService"},dependsOn={"com.fasterxml.jackson.databind.ObjectMapper","io.varys.api.jclient.endpoint.MonitoringUnitApi"})
  protected void build_MonitoringUnitServiceImpl() {
    MonitoringUnitServiceImpl$di.build(builder);
  }

  @DependencyMeta(type="io.varys.monitoringclaimcontroller.service.impl.LockServiceImpl",provides={"io.varys.monitoringclaimcontroller.service.LockService"},dependsOn={"org.redisson.api.RedissonClient"})
  protected void build_LockServiceImpl() {
    LockServiceImpl$di.build(builder);
  }

  @DependencyMeta(type="io.varys.monitoringclaimcontroller.service.impl.ProbeServiceImpl",provides={"io.varys.monitoringclaimcontroller.service.ProbeService"},dependsOn={"com.fasterxml.jackson.databind.ObjectMapper","io.varys.api.jclient.endpoint.ProbeApi","io.varys.monitoringclaimcontroller.service.ChmmService"})
  protected void build_ProbeServiceImpl() {
    ProbeServiceImpl$di.build(builder);
  }

  @DependencyMeta(type="io.varys.monitoringclaimcontroller.Controller",dependsOn={"io.varys.monitoringclaimcontroller.config.RedisConfig","io.lettuce.core.RedisClient","io.varys.monitoringclaimcontroller.service.MonitoringClaimService"})
  protected void build_Controller() {
    Controller$di.build(builder);
  }

  @DependencyMeta(type="io.varys.monitoringclaimcontroller.service.impl.ReservedSidecarSingleProbePatternService",provides={"io.varys.monitoringclaimcontroller.service.PatternService"},dependsOn={"io.varys.monitoringclaimcontroller.service.ProbeService","io.varys.monitoringclaimcontroller.service.MonitoringUnitService","io.varys.monitoringclaimcontroller.service.MonitoringClaimService"})
  protected void build_ReservedSidecarSingleProbePatternService() {
    ReservedSidecarSingleProbePatternService$di.build(builder);
  }

  @DependencyMeta(type="io.varys.monitoringclaimcontroller.service.impl.MonitoringClaimServiceImpl",provides={"io.varys.monitoringclaimcontroller.service.MonitoringClaimService"},dependsOn={"io.varys.api.jclient.endpoint.MonitoringClaimApi","io.varys.monitoringclaimcontroller.service.PatternService","io.varys.monitoringclaimcontroller.service.LockService"})
  protected void build_MonitoringClaimServiceImpl() {
    MonitoringClaimServiceImpl$di.build(builder);
  }

}

@rbygrave
Copy link
Contributor

rbygrave commented Sep 8, 2020 via email

@aletundo
Copy link
Author

aletundo commented Sep 8, 2020

I'm so sorry, I forgot to specify it!

build_Controller() is called before build_MonitoringClaimServiceImpl().

However, I just noticed that I probably created a circular dependecy between MonitoringClaimServiceImpl and ReservedSidecarSingleProbePatternService, which builder is in fact called among build_Controller() and build_MonitoringClaimServiceImpl().

You can see they both depend on each other...

Could be it the mistake that caused the issue?

@rbygrave
Copy link
Contributor

rbygrave commented Sep 8, 2020 via email

@rbygrave
Copy link
Contributor

rbygrave commented Sep 8, 2020

For example:

@Singleton
class AppleService {

  @Inject
  BananaService bananaService;

  @Inject
  PeachService peachService;

AppleService uses field injection to inject BananaService and PeachService. That means it does not have then in dependsOn as dependencies.

  @DependencyMeta(type="org.example.coffee.fruit.AppleService")
  protected void build_AppleService() {
    AppleService$di.build(builder);
  }
public class AppleService$di  {

  public static void build(Builder builder) {
    if (builder.isAddBeanFor(AppleService.class)) {
      AppleService bean = new AppleService();
      AppleService $bean = builder.register(bean, null);
      builder.addInjector(b -> {
        $bean.bananaService = b.get(org.example.coffee.fruit.BananaService.class);
        $bean.peachService = b.get(org.example.coffee.fruit.PeachService.class);
      });
    }
  }

}

and build_AppleService(); can be before build_PeachService(); and build_BananaService();

The field injection happens later as a separate step.

1 - build the beans in order to satisfy constructor injection
2 - perform all field injection
3 - execute all @PostConstruct lifecycle methods

@rbygrave
Copy link
Contributor

rbygrave commented Sep 8, 2020

I think it would be nice to detect the circular dependency case and generate a nice error. With the algorithm we have all the circular dependencies end up "push down the build order" (and in this example pushed below build_Controller() so ideally there is a change to detect that.

@aletundo
Copy link
Author

aletundo commented Sep 8, 2020

Ok, thanks for the details and also for the fix. It worked!

Maybe, you could also specify in the documentation that circular dependencies can be overcome using field injection.
I read again the Injection doc page but it is not reported.

@aletundo aletundo closed this as completed Sep 8, 2020
@rbygrave
Copy link
Contributor

rbygrave commented Sep 8, 2020

Great. I'll re-open this ticket with the view that we:

  • Should add detection in for circular constructor dependency (and provide a nicer error message for that case)
  • Make some documentation improvements about this

Cheers, Rob.

@rbygrave rbygrave reopened this Sep 8, 2020
@rbygrave
Copy link
Contributor

rbygrave commented Sep 9, 2020

Test case:

@Singleton
public class CircA {

  private final CircB circB;

  public CircA(CircB circB) {
    this.circB = circB;
  }
}

@Singleton
public class CircB {

  private final CircC circC;

  public CircB(CircC circC) {
    this.circC = circC;
  }
}

@Singleton
public class CircC {

  // to handle circular dependency we need to make one of
  // the dependencies use field injection rather than
  // constructor injection
//  @Inject
//  CircA circA;

  private final CircA circA;

  public CircC(CircA circA) {
    this.circA = circA;
  }
}

The inject generator will actually detect the circular constructor dependencies and error with:

Error:java: Circular dependencies detected with beans [org.example.coffee.circular.CircC, org.example.coffee.circular.CircB, org.example.coffee.circular.CircA] To handle circular dependencies consider using field injection rather than constructor injection on one of the dependencies.
Error:java: Circular dependency - org.example.coffee.circular.CircC dependsOn org.example.coffee.circular.CircA for org.example.coffee.circular.CircA
Error:java: Circular dependency - org.example.coffee.circular.CircB dependsOn org.example.coffee.circular.CircC for org.example.coffee.circular.CircC
Error:java: Circular dependency - org.example.coffee.circular.CircA dependsOn org.example.coffee.circular.CircB for org.example.coffee.circular.CircB

@rbygrave rbygrave changed the title BeanContextFactory.createContext calls builders in the wrong order Detect circular dependency and fail compile with nice error - Was BeanContextFactory.createContext calls builders in the wrong order Sep 9, 2020
@rbygrave rbygrave self-assigned this Sep 9, 2020
@rbygrave rbygrave added this to the 1.1 milestone Sep 9, 2020
@rbygrave rbygrave added the bug Something isn't working label Sep 9, 2020
rbygrave added a commit that referenced this issue Sep 9, 2020

Verified

This commit was signed with the committer’s verified signature. The key has expired.
mmarchini mary marchini
rbygrave added a commit that referenced this issue Sep 9, 2020

Verified

This commit was signed with the committer’s verified signature. The key has expired.
mmarchini mary marchini
Update test
@rbygrave
Copy link
Contributor

rbygrave commented Sep 9, 2020

Updated documentation: https://avaje.io/inject/#circular

rbygrave added a commit that referenced this issue Sep 9, 2020

Unverified

This user has not yet uploaded their public signing key.
@rbygrave rbygrave closed this as completed Sep 9, 2020
rbygrave added a commit that referenced this issue Oct 29, 2020

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
rbygrave added a commit that referenced this issue Oct 30, 2020
* #51 Add jpms module-info using Multi-Release jar

* #53 - Improve error message for JPMS use and no modules.

* #53 - Make javax.inject and org.slf4j transitive. Fix associated readme example.

* #53 - Add explicit requires javax.inject to avaje-inject-generator

* #53 - Tidy avaje-inject-generator module-info, remove javax.inject as brought in transitively
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

No branches or pull requests

2 participants