KSP wrong order of running TypeElementVisitors

[altro3]

Expected Behavior

Look to the micronaut-openapi project:

Simple structure for kotlin test:

package test

import com.fasterxml.jackson.annotation.*
import com.fasterxml.jackson.annotation.JsonIgnoreProperties
import com.fasterxml.jackson.annotation.JsonProperty
import com.fasterxml.jackson.annotation.JsonSubTypes
import com.fasterxml.jackson.annotation.JsonTypeInfo
import io.micronaut.core.annotation.Nullable
import io.micronaut.http.annotation.Body
import io.micronaut.http.annotation.Controller
import io.micronaut.http.annotation.Put
import io.micronaut.serde.annotation.Serdeable
import io.swagger.v3.oas.annotations.media.Schema
import jakarta.validation.Valid
import jakarta.validation.constraints.*
import jakarta.validation.constraints.NotNull
import reactor.core.publisher.Mono
import java.math.BigDecimal

@Controller
class HelloController {

    @Put("/sendModelWithDiscriminator")
    fun sendModelWithDiscriminator(
        @Body @NotNull @Valid animal: Animal
    ): Mono<Animal> = Mono.empty()
}

@Serdeable
@JsonIgnoreProperties(
        value = ["class"], // ignore manually set class, it will be automatically generated by Jackson during serialization
        allowSetters = true // allows the class to be set during deserialization
)
@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, property = "class", visible = true)
@JsonSubTypes(
        JsonSubTypes.Type(value = Bird::class, name = "ave")
)
open class Animal (
    @Nullable
    open var color: String? = null,
    @Nullable
    open var propertyClass: String? = null,
)

@Serdeable
data class Bird (
    @Nullable
    var numWings: Int? = null,
    @Nullable
    var beakLength: BigDecimal? = null,
    @Nullable
    var featherDescription: String? = null,
): Animal()

The same structure for java test:

package test;

import com.fasterxml.jackson.annotation.*;
import com.fasterxml.jackson.annotation.JsonIgnoreProperties;
import com.fasterxml.jackson.annotation.JsonProperty;
import com.fasterxml.jackson.annotation.JsonSubTypes;
import com.fasterxml.jackson.annotation.JsonTypeInfo;
import io.micronaut.core.annotation.Nullable;
import io.micronaut.http.annotation.Body;
import io.micronaut.http.annotation.Controller;
import io.micronaut.http.annotation.Put;
import io.micronaut.serde.annotation.Serdeable;
import io.swagger.v3.oas.annotations.media.Schema;
import jakarta.validation.Valid;
import jakarta.validation.constraints.*;
import jakarta.validation.constraints.NotNull;
import reactor.core.publisher.Mono;
import java.math.BigDecimal;
import java.util.Objects;
import java.util.Optional;

@Controller
class HelloController {

    @Put("/sendModelWithDiscriminator")
    Mono<Animal> sendModelWithDiscriminator(
        @Body @NotNull @Valid Animal animal
    ) {
        return Mono.empty();
    }
}

@Serdeable
@JsonIgnoreProperties(
        value = "class", // ignore manually set class, it will be automatically generated by Jackson during serialization
        allowSetters = true // allows the class to be set during deserialization
)
@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, property = "class", visible = true)
@JsonSubTypes({
    @JsonSubTypes.Type(value = Bird.class, name = "ave"),
})
class Animal {

    protected String propertyClass;
    private String color;

    /**
     * @return the propertyClass property value
     */
    public String getPropertyClass() {
        return propertyClass;
    }

    /**
     * Set the propertyClass property value
     */
    public void setPropertyClass(String propertyClass) {
        this.propertyClass = propertyClass;
    }

    /**
     * @return the color property value
     */
    public String getColor() {
        return color;
    }

    /**
     * Set the color property value
     */
    public void setColor(String color) {
        this.color = color;
    }
}

@Serdeable
class Bird extends Animal {

    private Integer numWings;
    private BigDecimal beakLength;
    private String featherDescription;

    /**
     * @return the numWings property value
     */
    public Integer getNumWings() {
        return numWings;
    }

    /**
     * Set the numWings property value
     */
    public void setNumWings(Integer numWings) {
        this.numWings = numWings;
    }

    /**
     * @return the beakLength property value
     */
    public BigDecimal getBeakLength() {
        return beakLength;
    }

    /**
     * Set the beakLength property value
     */
    public void setBeakLength(BigDecimal beakLength) {
        this.beakLength = beakLength;
    }

    /**
     * @return the featherDescription property value
     */
    public String getFeatherDescription() {
        return featherDescription;
    }

    /**
     * Set the featherDescription property value
     */
    public void setFeatherDescription(String featherDescription) {
        this.featherDescription = featherDescription;
    }
}

When I run micronaut-openapi process with KSP, I see this in schemes:

components:
  schemas:
    Animal:
      type: object
      properties:
        color:
          type: string
          nullable: true
        propertyClass:
          type: string
          nullable: true

With java I see this:

components:
  schemas:
    Animal:
      type: object
      properties:
        propertyClass:
          type: string
        color:
          type: string
      discriminator:
        propertyName: class
        mapping:
          ave: '#/components/schemas/Bird'
      oneOf:
      - $ref: '#/components/schemas/Bird'
    Bird:
      type: object
      allOf:
      - $ref: '#/components/schemas/Animal'
      - properties:
          numWings:
            type: integer
            format: int32
          beakLength:
            type: number
          featherDescription:
            type: string

The problem with wrong order of running TypeElementVisitors:

KSP run OpenApiJacksonVisitor with order -10 AFTER OpenApiControllerVisitor with order 50 (this is wrong)

Actual Behaviour

No response

Steps To Reproduce

No response

Environment Information

No response

Example Application

No response

Version

4.3.3

[altro3]

@dstepanov Another one bug with KSP. But I think,, now it's last

[dstepanov]

I checked the code, and it looks fine; maybe KSP processes some of the elements in the second round, and that's why it's different. Can you create please a test case in core?

[altro3]

Well, I checked source code and now I don't think that the problem with order. The problem lies in the different logic of selecting visitors for execution. I found the following differences:

1. In Java I see filtering by type of visitor and some visitors are not included in the final list. First step, we are loading ISOLATING visitors. Totally we have 15 visitors, 11 ofthem are ISOLATING:
2. After that, sorting is performed - OrderUtil.reverseSort(loadedVisitors);
3. In second step we load AGGREGATING visitors and forun 4 of 15 visitors:

That's witth Java all works correct: first process ISOLATING visitors, and second AGGREGATING.

With KSP you don't filter visitor by kind:

And because of this, all 15 typeElementVisitors are present in loadedVisitors.

As I understand it, all you need to do is add filtering by type of visitors and run the execution first ISOLATING visitors, then AGGREGATING, in general, bring the logic to the same form as in Java

[dstepanov]

I see; I'm not very familiar with all this. Do you see that TypeElementVisitorProcessor is initialized twice for Java, once for isolated, and then for aggregated configuration?

In this case, you can also change the type of the Jackson visitor as it belongs to the aggregated processing. Not sure if it makes sense to split the visitors for KSP, but it can be done.

[altro3]

I'm not sure if you're right, that it's possible to change this logic so easily. I'll explain what will go wrong: isolating visitors map annotations to other annotations. Aggregators then process the final code.

If you do as you say, this is what happens: I have an isolated visitor class processed, but only this class has time to process, and immediately after that an aggregating visitor is launched, which will receive only 1 class processed by an isolating visitor. Thus, if he checks the descendants inside himself (as micronaut-openapi does), he will receive false information about the annotations, because the isolating visitors have not processed them yet.

That's why it's worth starting processing strictly in this order: first, all the isolating processors, in a given order, and then the aggregating ones.

Of course, I may be wrong, but now I see obvious problems with the fact that java and ksp work differently

[dstepanov]

I think the schema info that the visitor is adding is only read/needed in the aggregated visitors.

[altro3]

I'm sorry, but I don't understand what you're suggesting in the end. At the moment, it is clear that visitors work differently in java and ksp. And it is clear that now java openapi is processed correctly, but not with ksp.

So what should I do? Should I write that ksp is not fully supported?

You definitely don't need to fix the code, because everything works correctly with java, groovy and kapt

[graemerocher]

what @dstepanov is saying is it sounds like OpenApiJacksonVisitor is incorrect. It is defined an isolating visitor and is exploiting internal behaviour in that it is designed to run before the OpenAPI visitors in your code but the issue is this is not really an isolating visitor if it is contributing to the aggregated result. It is incorrectly defined and should also be an aggregating visitor with a getOrder() implementation that allows it to run before the others.

[altro3]

I still don't understand why the processing for java and kotlin is different. Shouldn't the logic be identical?

Once again, now the processing for java goes like this: first, all isolating visitors are executed in a given order, after that, all aggregating visitors are executed in a given order

In kotlin, the handler now runs isolating and aggregating visitors in one pile and they can go in any order

Besides, I still can't figure out how the isolating visitor differs from the aggregating one.

By the way, I didn't write this visitor OpenApiJacksonVisitor, it was written long before I started making changes.

[dstepanov]

Ideally, it should be the same, but it's different because we have entirely different implementations for the annotation processing. Ideally, we would have a separate annotations cache for aggregated/isolated processing.

It's related to incremental compilation https://docs.gradle.org/current/userguide/java_plugin.html#example_registering_incremental_annotation_processors

We can correct the visitor to be aggregated and adjust the order in this case.

[graemerocher]

So the way incremental works in Java vs KSP is fundamentally different so I don't think we should be relying on order. It kind of doesn't matter that you didn't write it. It is a visitor that contributes to an aggregated result and therefore is in the wrong phase and incorrect.

To explain the concepts:

1. Isolating - A processor that generates a single file from a single originating source (Foo generates FooBuilder) therefore is isolated to Foo and if Foo changes we only to rebuild Foo to recreate FooBuilder.
2. Aggregating - a processor that visits multiple sources to generate a single file (OpenAPI visits FooController, BarController, Whatever and generated single YAML file). In the aggregating case for incremental compile to rebuild the YAML file we have to recompile all contributing sources for the rebuilt file to be valid.

OpenApiJacksonVisitor contributes to the aggregating result therefore cannot be an isolating visitor and the fact it happened to work before because of ordering doesn't mean it is correct.

[graemerocher]

Also btw I have expressed my concerns before about this modules behaviour with regards to incremental compilation because currently it is not correct. That has only gotten worse over time with the processor taking different kinds of inputs (properties files, configuration etc.) to build the aggregated result.

Whether that is fixable now is debatable.

[altro3]

@graemerocher @dstepanov In general, I conclude that we do not have enough visitors methods right now. So, what is working in java right now and what is not in the KSP processing:

I need the entire project to be processed by specific visitors first, i.e. first I need to do annotation mapping and preliminary calculations and only after that start processing with the main visitor.

This can be done using the specified ISOLATING type in the viewer. Because in java, all ISOLATING visitors are ALWAYS processed first, and then all AGGREGATING visitors. This is how OpenApiJacksonVisitor worked. Yes, it looks like a crutch, but this method works.

Now it turns out that I do not have the opportunity to first launch a certain set of visitors for preprocessing, and then do the processing itself.

Here is an example of how micronaut-openapi works now:

1. ISOLATING visitors are started first and they perform preliminary annotation mapping (OpenApiJacksonVisitor)

2. Then the controllers are processed, from which we get the classes of circuits that we need with the correct set of annotations.

In KSP, it turns out that the preprocessing does NOT have time to start, because we begin to process the class schema from the controller's visitor. After processing it, the visitor starts with the mapping OpenApiJacksonVisitor - but it's too late, because the scheme has already been processed.

So, it turns out that a tool is required that would allow you to group visitors so that first one group of visitors runs on all classes, and then another.

As for incrementality: in principle, this processing cannot be incremental. Here is an explicit example that cannot be handled correctly with incremental processing:

We have 2 controller classes, the first one describes 5 endpoints, and the second one does the same. We have generated an openapi. Everything is OK here, there will be 10 endpoints.

Next, we remove 2 endpoints from the first class, and add 3 other endpoints in the same first class.

With incremental processing, we will get not 11 endpoints, as we should, but 13 endpoints. Because it will be impossible to understand that some of the endpoints have been deleted, because during incremental processing we only see the changed classes, but we do not know from the swagger file in which of the controllers the endpoints were described

[altro3]

Under these conditions, the only solution I see is to get rid of OpenApiJacksonVisitor and add a few dozen more lines to explicitly process annotations that should have been written. I would really hate to do this.

[dstepanov]

Have you tried to convert OpenApiJacksonVisitor to be aggregating and have added the order lower than all other visitors?

[altro3]

Well, I explained what the problem is: OpenApiJacksonVisitor handles the Animal class - that is, the schema class

but! The first class I come across is the controller - HelloController.

Thus, the handler for controllers is started first and generates a schema BEFORE the OpenApiJacksonVisitor is launched.

And the order won't help here

[altro3]

I need the OpenApiJacksonVisitor to fully work first, and after that the main visitor will start. I say that this was achieved by putting down the ISOLATING type, because ISOLATING visitors were always launched BEFORE AGGREGATING. That's how it worked.

[dstepanov]

I think you are overthinking it. Just change it to aggregating and add:

@Override
    public int getOrder() {
        return Ordered.HIGHEST_PRECEDENCE;
    }

It will run as first.

[altro3]

I think you completely misunderstood what the problem is. Please read carefully what I wrote again.

The problem is NOT that the visitors are launched in the wrong order, but that the controller class is processed first - it is processed by only one visitor - OpenApiControllerVisitor. He also looks at which classes are used in endpoints and processes them to generate schemas. And OpenApiJacksonVisitor smokes on the sidelines, because this is not his class, he processes only schema classes with com.annotations.fasterxml.jackson.annotation.JsonSubTypes or com.fasterxml.jackson.annotation.JsonTypeInfo annotations.

So, OpenApiControllerVisitor processed the controller, generated the schemas and says - "I'm done! Next one!"

So, the controller class has been processed and we are moving on to processing the schema class with the com.annotation.fasterxml.jackson.annotation.JsonSubTypes - great! OpenApiJacksonVisitor is launched and it's cool! It's my job to convert jackson annotations to swagger annotations. And he does his job with great joy

Everything is cool, except that the schema class has ALREADY been processed by OpenApiControllerVisitor and all changes to OpenApiJacksonVisitor are simply ignored.

[dstepanov]

Please explain why putting OpenApiJacksonVisitor to run first doesn't work. It should process the Jackson annotations first, and the controller visitor should process the controllers.

[altro3]

That's right, but on condition that your class is handled by both visitors. Then they will run in the specified order But the controller class is NOT handled by OpenApiJacksonVisitor and should not, because it is a controller class. It is handled only by OpenApiControllerVisitor.

It just so happens that the HelloController class is processed earlier than the Animal class, so OpenApiJacksonVisitor does not have time to perform mapping.

[dstepanov]

By processed earlier, do you mean there are multiple rounds?

[altro3]

That is, the OpenApiControllerVisitor processes the HelloController controller class. During processing, it sees that there is an Animal class, and analyzes it to generate a schema. After processing the HelloController class, we move on to processing the Animal class, and here our OpenApiJacksonVisitor starts. But it doesn't matter anymore, because the Animal class has already been analyzed.

In short, I cannot tell the visitor that he must first process the Animal class, and only after that the HelloController class.

[dstepanov]

I cannot tell the visitor that he must first process the Animal class, and only after that the HelloController class.

If both elements Animal and HelloController are present in the round (which I should) visitors with the highest presence should process both elements first

[altro3]

No, it means that there is a different principle of code processing here. If a visitor is called for one class, it does not mean that he does not need other classes. That is, the processing of the controller class (once again, it is the controller, not the circuit, but the controller) leads to the analysis of the circuit classes and the generation of openapi.

OpenApiJacksonVisitor does NOT handle controller classes (once again, it handles only schema classes, NOT controller classes).

OpenApiControllerVisitor works earlier, because it is the controller - HelloController - that comes to processing first.

OpenApiJacksonVisitor - works later, because the Animal schema class is processed later than the HelloController controller class.

The order of processing classes by visitors is set inside the kernel, but I understand that it is fixed for a certain set of classes. That is, no matter how many times you run the processing, the order will always be fixed.

Once again: in java, YOU CAN launch a single group of visitors that will process absolutely all classes. And then the following visitors will be launched, which will also process all classes. but! they are guaranteed to start after the first group.

That is, if you put down the ISOLATING type of visitor, then it is GUARANTEED to start before the AGGREGATING one and it is GUARANTEED to work on all classes BEFORE launching any of the AGGREGATING visitors.

Therefore, everything works well and correctly in java and does not work in KSP

[altro3]

Explain to me why to a visitor who has no idea what a controller is, but only knows that he has to convert the com annotation.fasterxml.jackson.annotation.JsonSubTypes in the Schema annotation, handle the controller class?

or do you suggest doing a deep analysis of the code in each visitor, understanding ALL possible variations of the schemes from the controller structure and then mapping them.

In general, if the problem is not clear from what I have written, I wash my hands of it. I'm sorry, I just don't know any other words to describe the problem anymore. The hell with it

[dstepanov]

I will investigate

[altro3]

It's just that I got the impression that you categorically refuse to understand the problem and just repeat the same thing to me about the order of visitors. And I've already explained the problem 4 times in different words. But if that's not enough, then let everything stay as it is.

[graemerocher]

Whilst I agree we need some way for OpenApiControllerVisitor to run after OpenApiJacksonVisitor this doesn't change the fact that OpenApiJacksonVisitor is incorrectly defined as isolating when it contributes to the aggregating result. This is fundamentally broken there we have to find out another way to do this that doesn't rely on fragile visitor ordering IMO.

The fact that it works for Java is a side effect of the way multiple rounds and aggregating/isolating works and cannot necessarily be replicated in the KSP implementation which works very differently.

So we need to start crying "it's broken" and think of a solution to the actual problem that works for both KSP and Java since the behaviour the OpenAPI processor is exploiting cannot necessary be fixed for KSP. For me it seems like all OpenApiJacksonVisitor is doing is modifying some annotations on a class element. It seems like we need a new API like ElementMapper or something similar to AnnotationMapper but for elements that always runs whenever an element is created. @dstepanov WDYT?

[graemerocher]

It's just that I got the impression that you categorically refuse to understand the problem and just repeat the same thing to me about the order of visitors. And I've already explained the problem 4 times in different words. But if that's not enough, then let everything stay as it is.

@altro3 We are all trying to arrive to the right solution here it is not about categorically refusing. It has been noted that OpenApiJacksonVisitor is not defined correctly and now we have to think about the correct solution not the quickest one that allows the OpenAPI processor to continue to exploit behaviour that is clearly wrong.

[dstepanov]

I think I understand the problem, and my solution is this https://github.com/micronaut-projects/micronaut-openapi/pull/1430

You can check how visitors are processing here https://github.com/micronaut-projects/micronaut-core/blob/4.3.x/inject-java/src/main/java/io/micronaut/annotation/processing/TypeElementVisitorProcessor.java#L248

[altro3]

That's what I'm trying to explain to you. I lack a tool to group the instances and say. What I want is for ALL classes to be processed by one group of visitors first, and only after that the processing of another group of visitors starts.

As I wrote above, the way for java through ISOLATING kind looks like a hack (crutch), but it works. And it doesn't matter. it violates theoretical principles or something, it's a way to make alive what should be dead. I follow the same approach in my solutions, especially when it is so difficult to solve the main problem. I haven't written so much text at a time yet

[altro3]

@dstepanov Before making such changes, it is better to write a test and make sure that you are wrong. Well, you can't approach the problem that way. This is another denial of the problem. Your change will cause this visitor to stop working for java as well as for KSP. In the end, you'll just break the crutch, but you won't solve the problem.

[dstepanov]

What I want is for ALL classes to be processed by one group of visitors first, and only after that the processing of another group of visitors starts.

It works like that already. The first visitor will process all the elements (if not restricted by annotations), then the next will process all the elements, etc.

I personally don't see any problems here.

There are already tests for this use case, changing the order will make some of the tests fail.

[dstepanov]

@altro3 Do you have a branch with failing KSP use cases?

[altro3]

This is simple test will show you that your solution doesn't work. Just add it to OpenApiPojoControllerKotlinSpec file and run.

    void "test ksp jackson visitor"() {

        when:
        buildBeanDefinition('test.MyBean', '''
package test

import com.fasterxml.jackson.annotation.*
import com.fasterxml.jackson.annotation.JsonIgnoreProperties
import com.fasterxml.jackson.annotation.JsonProperty
import com.fasterxml.jackson.annotation.JsonSubTypes
import com.fasterxml.jackson.annotation.JsonTypeInfo
import io.micronaut.core.annotation.Nullable
import io.micronaut.http.annotation.Body
import io.micronaut.http.annotation.Controller
import io.micronaut.http.annotation.Put
import io.micronaut.serde.annotation.Serdeable
import io.swagger.v3.oas.annotations.media.Schema
import jakarta.validation.Valid
import jakarta.validation.constraints.*
import jakarta.validation.constraints.NotNull
import reactor.core.publisher.Mono
import java.math.BigDecimal

@Controller
class HelloController {

    @Put("/sendModelWithDiscriminator")
    fun sendModelWithDiscriminator(
        @Body @NotNull @Valid animal: Animal
    ): Mono<Animal> = Mono.empty()
}

@Serdeable
@JsonIgnoreProperties(
        value = ["class"], // ignore manually set class, it will be automatically generated by Jackson during serialization
        allowSetters = true // allows the class to be set during deserialization
)
@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, property = "class", visible = true)
@JsonSubTypes(
        JsonSubTypes.Type(value = Bird::class, name = "ave")
)
open class Animal (
    @Nullable
    open var color: String? = null,
    @Nullable
    open var propertyClass: String? = null,
)

@Serdeable
data class Bird (
    @Nullable
    var numWings: Int? = null,
    @Nullable
    var beakLength: BigDecimal? = null,
    @Nullable
    var featherDescription: String? = null,
): Animal()

@jakarta.inject.Singleton
class MyBean {}
''')
        then: "the state is correct"
        Utils.testReference != null

        when: "The OpenAPI is retrieved"
        OpenAPI openAPI = Utils.testReference
        def schemas = openAPI.components.schemas

        then: "the components are valid"
        schemas.Animal
        schemas.Bird
    }

[altro3]

@dstepanov Okay, I agree that your solution works with java. But it doesn't change anything.

That is, it works with java now and after your changes it works with java too

It didn't work with KSP before your changes and it doesn't work with KSP after your changes

[dstepanov]

KSP type visitor processor needs to be corrected 🤦‍♂️

[altro3]

We are all trying to arrive to the right solution here it is not about categorically refusing. It has been noted that OpenApiJacksonVisitor is not defined correctly and now we have to think about the correct solution not the quickest one that allows the OpenAPI processor to continue to exploit behaviour that is clearly wrong.

I strongly disagree with you! At the moment, this is the best solution for java in the whole world. And the question of "correctness" is a subjective concept