spring-projects / spring-boot

Spring Boot
https://spring.io/projects/spring-boot
Apache License 2.0
74.96k stars 40.65k forks source link

Configuration property descriptions may be lost when building incrementally #28075

Open wilkinsona opened 3 years ago

wilkinsona commented 3 years ago

We have two @ConfigurationProperties annotated classes:

package com.example;

import org.springframework.boot.context.properties.ConfigurationProperties;

@ConfigurationProperties(prefix = "more")
public class MoreProperties {

    /**
     * Another example property.
     */
    private String someProperty;

    public void setSomeProperty(String someProperty) {
        this.someProperty = someProperty;
    }

    public String getSomeProperty() {
        return this.someProperty;
    }

}
package com.example;

import org.springframework.boot.context.properties.ConfigurationProperties;

@ConfigurationProperties(prefix = "some")
public class SomeProperties {

    /**
     * An example property.
     */
    private String someProperty;

    public void setSomeProperty(String someProperty) {
        this.someProperty = someProperty;
    }

    public String getSomeProperty() {
        return this.someProperty;
    }

}

compileJava produces the following metadata:

{
  "groups": [
    {
      "name": "more",
      "type": "com.example.MoreProperties",
      "sourceType": "com.example.MoreProperties"
    },
    {
      "name": "some",
      "type": "com.example.SomeProperties",
      "sourceType": "com.example.SomeProperties"
    }
  ],
  "properties": [
    {
      "name": "more.some-property",
      "type": "java.lang.String",
      "description": "Another example property.",
      "sourceType": "com.example.MoreProperties"
    },
    {
      "name": "some.some-property",
      "type": "java.lang.String",
      "description": "An example property.",
      "sourceType": "com.example.SomeProperties"
    }
  ],
  "hints": []
}

We then change the description of more.some-property to Another example property with a new description.. compileJava now produces the following metadata:

{
  "groups": [
    {
      "name": "more",
      "type": "com.example.MoreProperties",
      "sourceType": "com.example.MoreProperties"
    },
    {
      "name": "some",
      "type": "com.example.SomeProperties",
      "sourceType": "com.example.SomeProperties"
    }
  ],
  "properties": [
    {
      "name": "more.some-property",
      "type": "java.lang.String",
      "description": "Another example property with a new description.",
      "sourceType": "com.example.MoreProperties"
    },
    {
      "name": "some.some-property",
      "type": "java.lang.String",
      "sourceType": "com.example.SomeProperties"
    }
  ],
  "hints": []
}

The description of the property that was not changed, some.some-property, has been lost. Upon running clean compileJava it reappears.

wilkinsona commented 3 years ago

During the second compilation, where the property's description disappears, this.env.getElementUtils().getDocComment(element) returns null. element is the field for the property. getElementUtils returns a com.sun.tools.javac.model.JavacElements.

The doc comment can't be found as com.sun.tools.javac.comp.Enter doesn't have an entry for the unaltered class which ultimately causes getTreeAndTopLevel(Element) to return null and getDocComment(Element) to also return null.

Looking at the output produced by Gradle when running with --debug, it's only the .java file that's been modified that is passed into the compiler. It's not yet clear to me why the other, unmodified type is then passed into the annotation processor.

wilkinsona commented 3 years ago

Digging some more, Gradle's calling javax.tools.JavaCompiler.getTask(…) which states the following in its javadoc:

Note that annotation processing can process both the compilation units of source code to be compiled, passed with the compilationUnits parameter, as well as class files, whose names are passed with the classes parameter.

This is exactly what's happening in this case. The .java file that has been modified is passed in via the compilationUnits parameter while the .class file for the unmodified file is passed in via the classes parameter. As a result of the unmodified file being passed in via the classes parameter, its source model is unavailable making its javadoc comments null.

Based on the above, this is a bug in our annotation processor.

wilkinsona commented 3 years ago

As far as I can tell, there's no way to tell if a TypeElement is backed by a .class file or a .java file without using compiler-specific API.

If we cast javax.lang.model.util.Elements to com.sun.tools.javac.model.JavacElements, we could call getTree(element). It'll return null for elements backed by a .class file. This is an internal of the JDK's compiler so it's quite risky to depend on it.

com.sun.source.util.Trees is an alternative. It's specific to the JDK's compiler, but it does appear to offer better stability guarantees. com.sun.source.util.Trees.instance(ProcessingEnvironment) will give us an instance and then I think we can call com.sun.source.util.Trees.getTree(Element). It'll return null for a TypeElement backed by a .class file.

wilkinsona commented 2 years ago

Using com.sun.source.util.Trees to identify if the javadoc was unavailable seems to work. It makes it possible to differentiate between a property that has no description because one hasn't been provided and a property that has no description because the javadoc wasn't available during incremental compilation. The hope was that this would allow us to reuse the description from the previous metadata when the javadoc was unavailable. Unfortunately, this doesn't work as Gradle always deletes the spring-configuration-metadata.json file before incremental compilation begins.

I think we have two, perhaps three, options:

  1. find a way to preserve the previous metadata and make it available to the annotation processor
  2. stop declaring the annotation processor as incremental
  3. use something other than javadoc, for example an annotation with appropriate retention, to provide property descriptions

The second option would increase compile times. The third would require code changes to move away from javadoc to the new mechanism. I am going to explore the first option as, in theory, it offers a solution to the problem without affecting users.