Default condition to avoid field Injection and haveConstructor Injection

[smfaizalkhan]

I'm fascinated by the API and have started using it our project.Lately i discovered that few
people in our project were using fieldInjection and not constructor injections

So we thought of using ArchRule and was expecting a default rule as below .

     @ArchTest
public static final ArchRule NO_FIELD_WITH_AUTOWIRED_ANNOTATION_AND_HAVE_CDI = 
               classes().that().resideInAPackage("xxx.xxx..xxx.xxx")
        .should(NO_AUTOWIRING).andShould(HAVE_CONSTRUCTOR_DEPENDANCY_INJECTION);

But by default it was not available ,so we wrote a conditions to it,

Avoiding Field Injection

      @Override
public void check(JavaClass javaClass, ConditionEvents conditionEvents) {
     javaClass.getAllFields().stream().filter(javaField -> 
       javaField.isAnnotatedWith(Autowired.class))
          .forEach(conditionEvent -> 
   conditionEvents.add(SimpleConditionEvent.violated(conditionEvent,
                  String.format("class %s contains a Field %s with AutoWired",javaClass.getName() , 
       conditionEvent))));
       }

Ensuring CDI

     @Override
public void check(JavaClass javaClass, ConditionEvents conditionEvents) {

    List<Dependency> classDependencies = 
       javaClass.getDirectDependenciesFromSelf().stream().collect(Collectors.toList());

    List<String> constructorParams = getConstructorParams(classDependencies);

    List<JavaField> classFields = javaClass.getAllFields().stream().collect(Collectors.toList());
    classFields.stream().filter(classField ->  
       !constructorParams.contains(classField.getRawType().getName()))
            .forEach(conditionEvent -> 
     conditionEvents.add(SimpleConditionEvent.violated(conditionEvent,
                    String.format("class %s  has dependency on %s which is not properly injected in 
     the constructor", javaClass.getName(), conditionEvent))));
   }

 private List<String> getConstructorParams(List<Dependency> classDependencies) {
    return classDependencies.stream()
            .filter(classDependency -> 
       !classDependency.getTargetClass().isEquivalentTo(Object.class))
            .filter(classDependency -> 
      classDependency.getDescription().startsWith("Constructor"))
            .map(filteredClassDependency -> 
       filteredClassDependency.getTargetClass().getName()).collect(Collectors.toList());
 }

Would like to know if it can be added as default behaviour in the API.

Regards,
Faizal

[rweisleder]

In my own library of ArchUnit rules I added a similar rule to prevent field injection:

public static final ArchRule DO_NOT_USE_FIELD_INJECTION = noFields()
		.should(beAnnotatedWith("org.springframework.beans.factory.annotation.Autowired"))
		.orShould(beAnnotatedWith("org.springframework.beans.factory.annotation.Value"))
		.because("field injection is evil, see http://olivergierke.de/2013/11/why-field-injection-is-evil/");

Maybe we could add this to GeneralCodingRules? If so, we could also add javax.inject.Inject and javax.annotation.Resource to this list. (Are there more common annotations for field injection?)

[smfaizalkhan]

@rweisleder

Thanks for including the fieldInjectioncheck
BTW ,Can we have the code for checking CDI as well as part of the GeneralCodingRule

Ensuring CDI

 @Override
 public void check(JavaClass javaClass, ConditionEvents conditionEvents) {

  List<Dependency> classDependencies = 
   javaClass.getDirectDependenciesFromSelf().stream().collect(Collectors.toList());

 List<String> constructorParams = getConstructorParams(classDependencies);

 List<JavaField> classFields = javaClass.getAllFields().stream().collect(Collectors.toList());
   classFields.stream().filter(classField ->  
     !constructorParams.contains(classField.getRawType().getName()))
         .forEach(conditionEvent -> 
   conditionEvents.add(SimpleConditionEvent.violated(conditionEvent,
                String.format("class %s  has dependency on %s which is not properly injected in 
 the constructor", javaClass.getName(), conditionEvent))));
 }

private List<String> getConstructorParams(List<Dependency> classDependencies) {
  return classDependencies.stream()
        .filter(classDependency -> 
    !classDependency.getTargetClass().isEquivalentTo(Object.class))
        .filter(classDependency -> 
     classDependency.getDescription().startsWith("Constructor"))
        .map(filteredClassDependency -> 
        filteredClassDependency.getTargetClass().getName()).collect(Collectors.toList());
 }

[smfaizalkhan]

@rweisleder

I was just trying add another condition like say if we have a field with value initialised ,then we will have filter it out as it dosent has to be a part of CDI

List classFields = javaClass.getAllFields().stream().collect(Collectors.toList());
Here if we have a filter to check if filed is assigned a value then we can filter it out like below
javaClass.getAllFields().stream().filter(javaField -> !javaField.isValueAssigned())

Regards,
Faizal

[rweisleder]

@smfaizalkhan, if you want to ensure that your developers use constructor injection when using CDI, I suggest making all fields final. This has two main advantages:

1. The Java compiler fails if a field is not initialized in the constructors or in the field declaration.
2. The ArchUnit rule gets much shorter: fields().that().areDeclaredInClassesThat().xyz().should().beFinal().check(javaClasses)

Or is there a reason I'm missing why classes that are created by the CDI container should not contain final fields?

[smfaizalkhan]

@rweisleder

Thanks ,we have used it this way
ArchCondition

@Override
 public void check(JavaField javaField, ConditionEvents conditionEvents) {
    List<JavaField> fieldsType = Collections.singletonList(javaField);
    List<Dependency> classDependencies = 
     javaField.getOwner().getDirectDependenciesFromSelf().stream().collect(Collectors.toList());
    List<String> constructorParams = getConstructorParams(classDependencies);
    

    fieldsType.stream().filter(fieldType -> 
      !constructorParams.contains(fieldType.getRawType().getName()))
            .forEach(conditionEvent -> 
      conditionEvents.add(SimpleConditionEvent.violated(conditionEvent,
                    String.format("class %s  has dependency on %s which is not properly injected in the 
       constructor", javaField.getOwner(), conditionEvent))));
     }

   private List<String> getConstructorParams(List<Dependency> classDependencies) {
    return classDependencies.stream()
            .filter(classDependency -> 
     !classDependency.getTargetClass().isEquivalentTo(Object.class))
            .filter(classDependency -> classDependency.getDescription().startsWith("Constructor"))  
            .map(filteredClassDependency -> 
      filteredClassDependency.getTargetClass().getName()).collect(Collectors.toList());
       }

Interface

@PublicAPI(usage = ACCESS)
    public interface CDIRule {

 ArchCondition HAVE_CONSTRUCTOR_DEPENDENCY_INJECTION = new 
       HaveConstructorDependencyInjectionField();

 @PublicAPI(usage = ACCESS)

 ArchRule CLASS_SHOULD_HAVE_CDI = ArchRuleDefinition.fields().that().areNotFinal()
   .and().areNotStatic().should(HAVE_CONSTRUCTOR_DEPENDENCY_INJECTION);
   }

[smfaizalkhan]

@rweisleder
I have the below packages in pom

    <dependency>
        <groupId>com.tngtech.archunit</groupId>
        <artifactId>archunit-junit5-api</artifactId>
        <version>0.12.0</version>
        <scope>compile</scope>
    </dependency>


    <dependency>
        <groupId>com.tngtech.archunit</groupId>
        <artifactId>archunit-junit5-engine</artifactId>
        <version>0.12.0</version>
        <scope>compile</scope>
    </dependency>

    <dependency>
        <groupId>com.tngtech.archunit</groupId>
        <artifactId>archunit</artifactId>
        <version>0.12.0</version>
        <scope>compile</scope>
    </dependency>

but i don't get the below highlighted class

 import static com.tngtech.archunit.core.domain.**TestUtils**.importClasses;

Is it part of different package if so ,what is it?

Regards,
Faizal

[hankem]

TestUtils is a helper class for ArchUnit's own tests; it is not bundled in any of ArchUnit's published artifacts. But as you can see in the source code, TestUtils.importClasses is just a shortcut for new ClassFileImporter().importClasses.

However, when using ArchUnit with JUnit5^†, you don't have to use this low-level import on your own, but can use ArchUnit's JUnit support.

---

^† By the way: 1. you don't need to declare the com.tngtech.archunit:archunit dependency explicitly; it automatically comes as a transitive dependency of com.tngtech.archunit:archunit-junit5-api, and 2. one would typically use those ArchUnit dependencies with <scope>test</scope>, cf. ArchUnit User Guide.

[smfaizalkhan]

@hankem

Thanks for the response,BTW scope tag was a mistake ,we are scoping it as test .Dont how it popped here :)

[codecholeric]

My take on GenericCodingRules, perhaps we should include a couple more, and perhaps we should also use synergies with the ArchUnit Maven Plugin, compare e.g. https://github.com/societe-generale/arch-unit-build-plugin-core/tree/master/src/main/java/com/societegenerale/commons/plugin/rules

They have meanwhile created a bunch of generally available rules, maybe it would make sense to add some to ArchUnit itself and reuse them from ArchUnit Maven and Gradle Plugin... Should discuss this at some point, how they would feel about it.

With the field injection rule I just have some stomach pain because it's super dependent on your framework. Do you use Spring, CDI, Guice, xyz, every time there are different framework specific annotations. Maybe it would work to write a generic rule that simply flags all fields as illegal that don't have a single "set access" 😉 (because those are neither initialized in the constructor nor have any setter, thus it must be some reflection magic at work or they are useless)