[jooby-apt] Repeatable @Parameter annotation issue

[kliushnichenko]

Faced some quite weird bug recently.
Since 3.2.x, using repeatable annotations simply hangs the compilation. No exceptions, no errors, it exits JoobyProcessor and hangs for about 4 minutes (could be more or less depending on the environment)

import io.jooby.annotation.GET;
import io.jooby.annotation.Path;
import io.jooby.annotation.QueryParam;
import io.swagger.v3.oas.annotations.Parameter;

@Path("/mvc")
public class Controller {

    @GET
    @Parameter(name = "paramA", description = "paramA")
    @Parameter(name = "paramB", description = "paramB")
    public void repeatable(@QueryParam String paramA, @QueryParam String paramB) {

    }
}

[kliushnichenko]

a bit more investigation
it is not about any repeatable annotation, it is specifically about @Parameter

One annotation gives a small delay to compilation, increasing the number of it increases compile time accordingly.
Moving annotation from the method level to the param level solves the issues but it is just a W/A

Maybe it somehow interferes with @org.apache.maven.plugins.annotations.Parameter ...

[jknack]

Almost sure it is javac bug... The issue (don't ask me why) it is the array() attribute of Parameter.

Parameters.java:

package app;

import java.lang.annotation.Inherited;
import java.lang.annotation.Retention;
import java.lang.annotation.RetentionPolicy;
import java.lang.annotation.Target;

import static java.lang.annotation.ElementType.ANNOTATION_TYPE;
import static java.lang.annotation.ElementType.METHOD;

@Target({METHOD, ANNOTATION_TYPE})
@Retention(RetentionPolicy.RUNTIME)
@Inherited
public @interface Parameters {
    /**
     * An array of Parameters Objects for the operation
     *
     * @return the parameters
     */
    Parameter[] value() default {};
}

Parameter.java:

package app;


import io.swagger.v3.oas.annotations.enums.Explode;
import io.swagger.v3.oas.annotations.enums.ParameterIn;
import io.swagger.v3.oas.annotations.enums.ParameterStyle;
import io.swagger.v3.oas.annotations.extensions.Extension;
import io.swagger.v3.oas.annotations.media.ArraySchema;
import io.swagger.v3.oas.annotations.media.Content;
import io.swagger.v3.oas.annotations.media.ExampleObject;
import io.swagger.v3.oas.annotations.media.Schema;

import java.lang.annotation.*;

import static java.lang.annotation.ElementType.*;
import static java.lang.annotation.ElementType.ANNOTATION_TYPE;

@Target({PARAMETER, METHOD, FIELD, ANNOTATION_TYPE})
@Retention(RetentionPolicy.RUNTIME)
@Repeatable(Parameters.class)
@Inherited
public @interface Parameter {
    /**
     * The name of the parameter.
     *
     * @return the parameter's name
     **/
    String name() default "";

    /**
     * The location of the parameter.  Possible values are "query", "header", "path" or "cookie".  Ignored when empty string.
     *
     * @return the parameter's location
     **/
    ParameterIn in() default ParameterIn.DEFAULT;

    /**
     * Additional description data to provide on the purpose of the parameter
     *
     * @return the parameter's description
     **/
    String description() default "";

    /**
     * Determines whether this parameter is mandatory. If the parameter location is "path", this property is required and its value must be true. Otherwise, the property may be included and its default value is false.
     *
     * @return whether or not the parameter is required
     **/
    boolean required() default false;

    /**
     * Specifies that a parameter is deprecated and should be transitioned out of usage.
     *
     * @return whether or not the parameter is deprecated
     **/
    boolean deprecated() default false;

    /**
     * When true, allows sending an empty value.  If false, the parameter will be considered \"null\" if no value is present.  This may create validation errors when the parameter is required.
     *
     * @return whether or not the parameter allows empty values
     **/
    boolean allowEmptyValue() default false;

    /**
     * Describes how the parameter value will be serialized depending on the type of the parameter value. Default values (based on value of in): for query - form; for path - simple; for header - simple; for cookie - form.  Ignored if the properties content or array are specified.
     *
     * @return the style of the parameter
     **/
    ParameterStyle style() default ParameterStyle.DEFAULT;

    /**
     * When this is true, parameter values of type array or object generate separate parameters for each value of the array or key-value pair of the map. For other types of parameters this property has no effect. When style is form, the default value is true. For all other styles, the default value is false.  Ignored if the properties content or array are specified.
     *
     * @return whether or not to expand individual array members
     **/
    Explode explode() default Explode.DEFAULT;

    /**
     * Determines whether the parameter value should allow reserved characters, as defined by RFC3986. This property only applies to parameters with an in value of query. The default value is false.  Ignored if the properties content or array are specified.
     *
     * @return whether or not the parameter allows reserved characters
     **/
    boolean allowReserved() default false;

    /**
     * The schema defining the type used for the parameter.  Ignored if the properties content or array are specified.
     *
     * @return the schema of the parameter
     **/
    Schema schema() default @Schema();

    /**
     * The schema of the array that defines this parameter.  Ignored if the property content is specified.
     *
     * @return the schema of the array
     */
    // ArraySchema array() default @ArraySchema();

    /**
     * The representation of this parameter, for different media types.
     *
     * @return the content of the parameter
     **/
    Content[] content() default {};

    /**
     * Allows this parameter to be marked as hidden
     *
     * @return whether or not this parameter is hidden
     */
    boolean hidden() default false;

    /**
     * An array of examples  of the schema used to show the use of the associated schema.
     *
     * @return array of examples of the parameter
     **/
    ExampleObject[] examples() default {};

    /**
     * Provides an example of the schema.  When associated with a specific media type, the example string shall be parsed by the consumer to be treated as an object or an array.  Ignored if the properties examples, content or array are specified.
     *
     * @return an example of the parameter
     **/
    String example() default "";

    /**
     * The list of optional extensions
     *
     * @return an optional array of extensions
     */
    Extension[] extensions() default {};

    /**
     * A reference to a parameter defined in components parameter.
     *
     * @since 2.0.3
     * @return the reference
     **/
    String ref() default "";
}

If you commented out the array attribute, it works

[kliushnichenko]

Counter question:

Could you pls explain why we generate all these route attributes with openapi info(below)
if OpenAPIGenerator doesn't rely on it for MVC routes

 app.get("/pets", this::getPets)
        .setAttributes(java.util.Map.ofEntries(
            java.util.Map.entry("ApiResponse.description", "Pets list"),
            java.util.Map.entry("ApiResponse.responseCode", "200"),
            java.util.Map.entry("ApiResponse.useReturnTypeSchema", false),
            java.util.Map.entry("Operation.deprecated", false),
            java.util.Map.entry("Operation.hidden", false),
            java.util.Map.entry("Operation.ignoreJsonView", false),
            java.util.Map.entry("Operation.requestBody", 
              java.util.Map.of(
                 "RequestBody.required", false,
                 "RequestBody.useParameterTypeSchema", false)),
            java.util.Map.entry("Operation.summary", "Get Pets"),
            java.util.Map.entry("Parameter.allowEmptyValue", false),
            java.util.Map.entry("Parameter.allowReserved", false),
            java.util.Map.entry("Parameter.array", 
              java.util.Map.of(
           ...
           and many more below

Why I'm asking is because...
when I add this compiler arg

-Ajooby.skipAttributeAnnotations=io.swagger.v3.oas.annotations.Parameter

the following occures:

1. javac issue has gone, no compiler hanging anymore
2. we reduce tonnes of route attributes that I doubt someone needs at runtime
3. OpenAPI specification is still correctly generated, and nothing lost
4. everybody happy

and we can basically skip everything from io.swagger.v3.*

what I'm missing?
Or you are going to change OpenAPIGenerator logic to rely on attributes in the future?

[jknack]

Completely forgot about this... so it is a jooby-apt bug not javac.

We should removed all these openapi metadata generation (by default or hardcoded). It's useless.

[agentgt]

@kliushnichenko Do you have a reproducible project on the compiler hanging? Ideally we do a thread dump and see what is going on.

@jknack There is a limit on varargs I think of 255 so perhaps the generated code should not be using Map.ofEntries.

[kliushnichenko]

It is already there
https://github.com/jooby-project/jooby/blob/3.x/modules/jooby-apt/src/test/java/tests/i3525/Issue3525.java
If you run this test on the revision before the swagger exclusion - test execution takes 4-5 mins.

Where I stopped digging is io.swagger.v3.oas.annotations.media.Schema. Excluding it from processing removes the delay completely. It contains 33 fields, a lot but less than 255.

When I started excluding some of these 33 fields, the delay started varying between 15 sec to a couple of minutes, depending on what to exclude, but still didn't find any interdependence between execution time and excluded fields, still pretty random.

Nevertheless, setMvcMethod and setReturnType have never been an issue here. I guess @jknack wants to disable them by default just bc nobody using it on average

[agentgt]

Nevertheless, setMvcMethod and setReturnType have never been an issue here. I guess @jknack wants to disable them by default just bc nobody using it on average

I know now why he wants to disable it. I did not know that setMvcMethod takes the actual java.lang.reflect.Method which would absolutely impact startup performance. I think I have a solution for that:

#3529 (comment)

Then if there is anybody else that actually uses the original getMvcMethod for the java.lang.reflect.Method they have enough data to get it but it would be dynamically retrieved.

@jknack I can put in a PR for the custom MvcMethod type which would avoid reflective lookup while still providing the method meta data I need. If I'm the only one using this stuff this seems like a good opportunity to fix it and make it better.

As for this:

Where I stopped digging is io.swagger.v3.oas.annotations.media.Schema. Excluding it from processing removes the delay completely. It contains 33 fields, a lot but less than 255.

My guess is the annotation tree OpenAPI has is very deep and the apt module is loading up that entire tree every time on each class compiled. That is the normal javac has the symbol tree not full loaded but the attribute processing fully loads it everytime. There is probably some cacheing that can be done of the TypeMirror of the annotations.

[jknack]

I guess @jknack wants to disable them by default just bc nobody using it on average

@kliushnichenko That is correct, yea. Return type played a role in jooby 2.x but not anymore in 3.x. Mvc method is just metadata based on reflection and the goal of 3.x was to remove reflection as much as possible.

My guess is the annotation tree OpenAPI has is very deep and the apt module is loading up that entire tree every time on each class compiled. That is the normal javac has the symbol tree not full loaded but the attribute processing fully loads it everytime. There is probably some cacheing that can be done of the TypeMirror of the annotations.

@agentgt The tests run successfully after 1 or 2 minutes running locally on my machine. Now if I run in standalone project, maven never comes back for me.

Anyway, I'm ok with the solution we took: 1) remove defaults values from annotations, 2) excludes open-api anootation bc it is useless, 3) turn off returnType and mvcMethod generation by default (but still these two are not related to this issue, just wanted to clean them up).