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

Add test for process KSP Json subtypes. See #768 #875

Merged
merged 6 commits into from
Jan 8, 2025

Conversation

altro3
Copy link
Contributor

@altro3 altro3 commented Jun 21, 2024

See #768

@altro3
Copy link
Contributor Author

altro3 commented Oct 30, 2024

@dstepanov Are you planned to fix it? I still have some tests disabled in micronaut openapi for KSP because of this bug :-(

@dstepanov dstepanov changed the base branch from 2.11.x to 2.13.x November 29, 2024 07:57
@dstepanov
Copy link
Contributor

@altro3 It works correctly, you need to have visible = false because the bird class doesn't have the class property

@altro3
Copy link
Contributor Author

altro3 commented Nov 29, 2024

@dstepanov Then I have a question: why does this code work correctly with KAPT and java, but not only with KSP?

@altro3
Copy link
Contributor Author

altro3 commented Nov 29, 2024

@dstepanov You are wrong. The Bird class inherits from the Animal class. The Animal class has a property class
{2A9FBF20-8F02-418D-8B2D-7758EA83907F}

@altro3
Copy link
Contributor Author

altro3 commented Nov 29, 2024

@dstepanov Tets should pass exactly with the configuration as I wrote. I think that KSP does not see the property class, because it is in the parent class

@altro3
Copy link
Contributor Author

altro3 commented Nov 29, 2024

I just want to say: if a property is not in the constructor, it does not mean that the parent does not have the property. Yes, you will not be able to set values ​​for these properties in the constructor, but you will be able to specify them additionally. In our case, like this:

{B3F1CD85-F665-451A-9261-602FE6906323}

@dstepanov
Copy link
Contributor

Needs @field:JsonProperty(JSON_PROPERTY_PROPERTY_CLASS)

@altro3
Copy link
Contributor Author

altro3 commented Nov 29, 2024

Well, that's a bug. Why do all other annotations work fine without the "field:" prefix, but only in this case does it not work without the prefix?

@altro3
Copy link
Contributor Author

altro3 commented Nov 29, 2024

@dstepanov As you see, in case withou prefix @filed:, @JsonProperty annotation was processed and DeserBean class found right information about discriminator property:
{57C97479-AB80-49C2-A101-F22EF01FDD7D}

But why does the processing continue incorrectly?

@altro3
Copy link
Contributor Author

altro3 commented Nov 29, 2024

I think I understand what the problem is and it is clearly in the lkotlin introspection, that is, in the core module.

In this case, the annotations specified in the superclass construct - Animal are ignored, but these annotations are also valid for fields. This was, among other things, the point of the KSP, to get rid of these stupid "field:" prefixes.

@altro3
Copy link
Contributor Author

altro3 commented Nov 29, 2024

In short, it is necessary that the annotations specified in the superconstructor are added to the annotations of the fields. Otherwise, you get a hodgepodge - in one case you can omit the "field" prefix, and in others - not.

@dstepanov
Copy link
Contributor

Good question, I think the annotation in the default variant is only added to the constructor and we only take field, getter and setter annotations to the final annotation metadata. You can debug and see. Kapt probably is putting in everywere in the stub

@altro3
Copy link
Contributor Author

altro3 commented Nov 29, 2024

Yes, I understand what the problem is. Question for you: can this be fixed, or should we just give in and shut up? :-)

@altro3
Copy link
Contributor Author

altro3 commented Nov 30, 2024

@dstepanov I decided to test another case: if we do not redefine class attributes, but simply pass arguments of parent constructors in all child constructors. This way we are guaranteed to always use a constructor, and not setters for individual attributes.

Look at the test structure:

@Serdeable
@JsonIgnoreProperties(
    value = ["myType"], // ignore manually set type, it will be automatically generated by Jackson during serialization
    allowSetters = true, // allows the type to be set during deserialization
)
@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, property = "myType", visible = true)
@JsonSubTypes(
    JsonSubTypes.Type(value = BasicBookInfo::class, name = "BASIC"),
    JsonSubTypes.Type(value = DetailedBookInfo::class, name = "DETAILED"),
)
open class BookInfo(

    var name: String,

    @JsonProperty("myType")
    var type: BookInfoType? = null,
)

@Serdeable
open class BasicBookInfo(

    @NotNull
    @Size(min = 3)
    var author: String,

    name: String,
    type: BookInfoType? = null,

) : BookInfo(name, type)

@Serdeable
class DetailedBookInfo(

    @Pattern(regexp = "[0-9]{13}")
    var isbn: String,

    author: String,
    name: String,
    type: BookInfoType? = null,

) : BasicBookInfo(author, name, type)

As you can see, the type attribute has the @JsonProperty annotation, but it is only specified in the constructor argument of the main parent.

So, why is this annotation ignored when introspection occurs?

Yes, of course, I can specify the field prefix and everything will work, but the idea is that constructor annotations are taken into account throughout the class hierarchy.

I believe this is a bug

@dstepanov
Copy link
Contributor

Well, JsonProperty is not inherited, so it should not appear on the inherited parameter.

Looking at AbstractKotlinPropertyElement and PropertyElementAnnotationMetadata I think we need to merge the annotations added to the property. The concept that doesn't exist in Java so that's why it's happening.

@altro3
Copy link
Contributor Author

altro3 commented Dec 2, 2024

Well, look what I got:

  1. I improved the generator, taking into account your comment about the field prefix

  2. Also, the generator now duplicates the annotations of the constructor arguments in each child class - this solves the second problem.

After these changes, the tests in this PR began to pass correctly

Now let's get back to the test in micronaut-openapi. And the test still doesn't pass! And here's the problem: in the "class" field it writes the class name (!!!!!), although there should be a discriminator value!!!

But it looks like the problem is not in the micronaut-serde library, but in the http client!

Can you help me figure out this problem? It only occurs when using KSP and the problem is definitely not in the annotations, since they are now guaranteed to be set correctly

This is a screenshot showing the problem. Look at the value of the propertyClass field - the class name is Bird, but it should be ave !

326349724-501fe05f-4260-4dcd-af12-a5ba24fd7cd6

@dstepanov
Copy link
Contributor

What do the bean's definitions look like?

@altro3
Copy link
Contributor Author

altro3 commented Dec 2, 2024

Look, the problem is NOT in the HTTP client, but in the controller!!!

It looks like there is a serialization and deserialization error somewhere, and the real class name is automatically substituted into the discriminator value field!

I really don't understand where the "substitution" of the discriminator value for the class name occurs... In the test in the current PR, I specifically made a similar model, as in my test - but in this PR everything works correctly... It turns out that the problem is in micronaut http

Get sources fro mthis branch: https://github.com/altro3/micronaut-openapi/tree/reactive-tests

Run test in module test-suite-kotlin-ksp-server-generator run test RequestBodyControllerTest.testSendModelWithDiscriminatorChild1

  1. look to the request model:
    изображение

propertyClass field is null, and we have Bird object

  1. In controller we see this:
{27C800E6-56A4-49A2-88A7-F5B17AFE67C7}

As you see in propertyClass field we have value Bird, but must be ave

@altro3
Copy link
Contributor Author

altro3 commented Dec 2, 2024

Here you will see all generated controller interfaces and data models

{4B807F2E-11C8-49B5-9CD1-9AAC24F87294}

@altro3
Copy link
Contributor Author

altro3 commented Dec 2, 2024

With java and KAPT this test works fine

@altro3
Copy link
Contributor Author

altro3 commented Dec 3, 2024

@dstepanov I started debugging the process of sending a request in the test. This is what I see

  1. The problem really occurs when serializing the request in the client.

  2. The screenshot shows that the injected value is incorrect and corresponds to the class name, and not the value from the annotations

It is also clear that in the object itself this field is empty, that is, it is injected by micronaut-serde .

I suspect that everything is connected with the name of the field - class and if this field is called something else, for example, type, then everything will work. But this is only an assumption. which I have not yet checked

So, look at the screenshot with KSP:
{3CD63A05-9000-4282-B9CB-0257E49DCEFE}

The same checkpoint with KAPT:
{16292EE4-DBB8-489C-95B5-67AE97C6B973}

Look, the injected value is correct - ave . So now I'm 100% sure that the bug is in micronaut-serialization introspection

@altro3
Copy link
Contributor Author

altro3 commented Dec 3, 2024

@dstepanov I think, found it. Looks to $Bird$Introspection.class (left - KAPT, right - KSP):

{A6C0C6C0-ACAD-4309-A323-4B94174F2EF2}

altro3 added a commit to altro3/micronaut-serialization that referenced this pull request Dec 4, 2024
@altro3
Copy link
Contributor Author

altro3 commented Dec 4, 2024

@dstepanov Added correct test to reproduce this problem: example.RequestBodyControllerTest

Please, help to fix it!

@dstepanov
Copy link
Contributor

The main problem is that KSP doesn't allow fetching the annotation model from a string and reading the annotations. We need to get the repeatable container.
There is a trick to get the class if it's on the classpath, but it looks like KSP has some classloading mishmash.

micronaut-projects/micronaut-core#11408

I can make the build pass if I add:

        Thread.currentThread().setContextClassLoader(visitorContext.getClass().getClassLoader());

in JsonSubTypesMapper

@altro3
Copy link
Contributor Author

altro3 commented Dec 25, 2024

@dstepanov This fix doesn't seem to help :-(

@dstepanov dstepanov merged commit 9019c28 into micronaut-projects:2.13.x Jan 8, 2025
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants