smallrye / smallrye-graphql

Implementation for MicroProfile GraphQL
Apache License 2.0
160 stars 92 forks source link

Some parameter are not getting mapped in the Input class #2143

Open miltonlm opened 3 months ago

miltonlm commented 3 months ago

We have defined a graphql method that generates this schema:

mutation { collectionPaymentsCreate( collectionPayments: { cashRegisterId: "65a0c514-0a02-4eca-97e8-de71495ba3f9" customerId: "bddf6de2-db3f-4e74-9afd-33e4a4d45626" branchId: "6f304ded-9078-49ea-b932-c00854d75247" sellerId: "7ca42902-5c77-45a2-a1f4-1d4e56c23c82" paymentFormId: "66a30334-eb84-4f46-b5e9-73e52af98229" currencyId: "8e799f20-17dd-41a6-9089-95acbf418f3b" paymentStatusId: "1944823c-c577-482f-9d71-3c3c2b4cd433" documentType: "a6fca760-f738-4ee5-b685-601532dd2b8f" collectionAccountId: 802716263 customerAccountId: 574833393 retentionTypeId: "58bc866d-5c2b-4305-b12a-05dd0011d9c3" collectionId: "ac75c1f8-ee22-409a-8cd2-a15006e4987e" xId: "95090f21-6e21-4048-8901-2312e58dcc5a" zId: "13c9b2e0-0650-4d39-a3a3-ff9cac7128e5" documentNumber: 8663838971428344963 series: "16646" issuedDate: "2024-07-03T18:33:56.719792700" returnedDate: "2024-07-03T18:33:56.719792700" canceledDate: "2024-07-03T18:33:56.719792700" documentReference: 231956995 documentSeries: "377976" amount: 8.460966165622535E8 comment: "325163028" printed: true exchangeRate: 6.370691098868833E8 multipleCollection: true transactionNumber: 453223353 retentionAmount: 1.4482197106224442E8 lineNumber: 7634103834451622320 accounted: true accountedReturned: true seatId: 800237185 type: 7984437249198708856 } ) { id cashRegisterId customerId branchId sellerId createdBy paymentFormId currencyId paymentStatusId documentType collectionAccountId customerAccountId retentionTypeId collectionId xId zId documentNumber series issuedDate returnedDate canceledDate creationDate documentReference documentSeries amount comment printed exchangeRate multipleCollection transactionNumber retentionAmount lineNumber accounted accountedReturned seatId type } }

Even though we send the parameters xId and zId with values, they are not getting mapped in the Input object. I believe it has something to be with variable names, because these two are the only two values that don't get mapped. This is not the only method that defines these two variables, there are many graphql methods that define xId and zId and in none of them are getting mapped.

With the payload specified above, this is what happens:

image

These three fields are defined as follows:

var xId: UUID? = null
var zId: UUID? = null
var registerId: UUID? = null

However the registerId gets mapped correctly, but neither xId nor zId get mapped. Again this is not the only method in which we define xId and zId, however in none of them get mapped, it fails in each and every method. It has to be something with the variable names.

Thanks in advance.

phillip-kruger commented 3 months ago

Can you share a reproducer ? Or if you can not, the code for CollectionPaymentsInput

miltonlm commented 3 months ago

Please find attached a reproducer:

business.zip

call it using postman and you will see that even though you specify all values in the request, in the code some values are null.

phillip-kruger commented 2 months ago

Do you know what the class will look like for the Kotlin Pojo ? I suspect the issue will be somewhere there. Or let me put it another way, using java and a record for the pojo should work. (That would confirm it's a Kotlin problem)

miltonlm commented 2 months ago

Hello @phillip-kruger , thanks for your answers.

there are online tools to convert Kotlin to Java online, look at what one of those generated:

image

Kotlin code:

class Input {
    var xId: UUID? = null
    var zId: UUID? = null
    var registerId: UUID? = null
}

Java code:

import java.util.UUID;

class Input {
    private UUID xId;
    private UUID zId;
    private UUID registerId;

    public UUID getXId() {
        return xId;
    }

    public void setXId(UUID xId) {
        this.xId = xId;
    }

    public UUID getZId() {
        return zId;
    }

    public void setZId(UUID zId) {
        this.zId = zId;
    }

    public UUID getRegisterId() {
        return registerId;
    }

    public void setRegisterId(UUID registerId) {
        this.registerId = registerId;
    }
}
miltonlm commented 2 months ago

Hello Phillip,

After testing with Java POJOS, this is the POJO I used to perform the test:

package com.mundoware.business.finance.collectionPayments;

import java.util.*;

public class CollectionPaymentsInput {
    private String name;
    private UUID xid;
    private UUID zId;
    private UUID yId;

    public String getName() {
        return name;
    }

    public void setName(String name) {
        this.name = name;
    }

    public UUID getXid() {
        return xid;
    }

    public void setXid(UUID xid) {
        this.xid = xid;
    }

    public UUID getZId() {
        return zId;
    }

    public void setZId(UUID zId) {
        this.zId = zId;
    }

    public UUID getYId() {
        return yId;
    }

    public void setYId(UUID yId) {
        this.yId = yId;
    }
}

but again, it is not mapping the values correctly, the output to this request:

query {
    collectionPayments(collectionPayments: {
        name: "asda",
        xid: "96e979a1-7ba0-41da-a13d-6f36c138f60a"
        zId: "96e979a1-7ba0-41da-a13d-6f36c138f60a"
        yId: "96e979a1-7ba0-41da-a13d-6f36c138f60a"
    })
}

is:

asda 96e979a1-7ba0-41da-a13d-6f36c138f60a null null

phillip-kruger commented 2 months ago

@jmartisk - this seems like a bug somewhere when we check bean methods (getters and setters). Can you look at this ?

miltonlm commented 2 months ago

Please find attached the related project with the bug:

business.zip

jmartisk commented 2 months ago

I'll see if I get enough time this week, otherwise I'll leave it to @mskacelik

miltonlm commented 2 months ago

@jmartisk or @mskacelik maybe I can help you if you tell me which package or java file I should check? Thanks a lot.

phillip-kruger commented 2 months ago

I would start here: https://github.com/smallrye/smallrye-graphql/blob/fa02098cabe9db9be480d16425e0336e7532680c/common/schema-builder/src/main/java/io/smallrye/graphql/schema/helper/MethodHelper.java#L110

phillip-kruger commented 2 months ago

I am not sure if this is where the issue is, but here we look at the method name, and specifically a setter. In this module we build a common model of the user's classes. This allows us to do this work at build time, and then we create graphQL from the model at runtime (rather that having to scan annotations) I would first check that the model is correct, and if so, then check the graphql.

miltonlm commented 2 months ago

Hi @phillip-kruger after a detailed debugging I have found exactly where the variables become null and it is when converting a json string to the input class type (CollectionPaymentsInput).

Please find attached a video of the debugging process:

Screencast from 2024-07-09 15-21-14.webm

And some context:

Screencast from 2024-07-09 15-29-00.webm

phillip-kruger commented 2 months ago

Thanks @miltonlm ! This helps a lot and should get us going. It would be interesting to create a basic main method Java class that use JsonB to convert the above in a standalone way, to see if the issue is with JsonB and maybe something we need to configure ?

miltonlm commented 2 months ago

Hello all,

I have created a basic main method as suggested by @phillip-kruger and these are the results:

Jsonb json = JsonBCreator.getJsonB("com.mundoware.business.finance.collectionPayments.CollectionPaymentsInput");
Jsonb json2 = JsonbBuilder.create();
String jsonString = """
        {
          "name": "ASD ASDAS",
          "xid": "1b768067-0212-462f-88da-b5ecf0537e41",
          "yId": "1b768067-0212-462f-88da-b5ecf0537e41",
          "zId": "1b768067-0212-462f-88da-b5ecf0537e41"
        }
""";
CollectionPaymentsInput collectionPaymentsInput = json.fromJson(jsonString, CollectionPaymentsInput.class);

System.out.println(collectionPaymentsInput.getName());
System.out.println(collectionPaymentsInput.getXid());
System.out.println(collectionPaymentsInput.getYId());
System.out.println(collectionPaymentsInput.getZId());

System.out.println("==============JSONBUILDER DEFAULT CONFIG=============");

collectionPaymentsInput = json2.fromJson(jsonString, CollectionPaymentsInput.class);

System.out.println(collectionPaymentsInput.getName());
System.out.println(collectionPaymentsInput.getXid());
System.out.println(collectionPaymentsInput.getYId());
System.out.println(collectionPaymentsInput.getZId());

And this is the output:

ASD ASDAS 1b768067-0212-462f-88da-b5ecf0537e41 null null ==============JSONBUILDER DEFAULT CONFIG============= ASD ASDAS 1b768067-0212-462f-88da-b5ecf0537e41 null null

phillip-kruger commented 2 months ago

Interesting, so this seems to be a JsonB issue. Did you check if there is a way to configure the behavior ?

miltonlm commented 2 months ago

I have tested a few configurations, and the default configuration with no luck. I tried converting with Google Gson Library and it works correctly.

phillip-kruger commented 2 months ago

I don't think we want to add Gson as a dependency (We already have JsonB and Jackson). What we spoke about in the past is to remove JsonB, and only use Jackson, but still support JsonB constructs (so if users use JsonB annotations, it will still work). Did you check if Jackson works correctly ?

miltonlm commented 2 months ago

I just tested using Jackson Library and it throws an exception:

Exception in thread "main" com.fasterxml.jackson.databind.exc.UnrecognizedPropertyException: Unrecognized field "yId" (class com.mundoware.business.finance.collectionPayments.CollectionPaymentsInput), not marked as ignorable (4 known properties: "name", "yid", "zid", "xid"]) at [Source: (byte[])" { "name": "ASD ASDAS", "xid": "1b768067-0212-462f-88da-b5ecf0537e41", "yId": "1b768067-0212-462f-88da-b5ecf0537e41", "zId": "1b768067-0212-462f-88da-b5ecf0537e41" } "; line: 4, column: 19] (through reference chain: com.mundoware.business.finance.collectionPayments.CollectionPaymentsInput["yId"]) at com.fasterxml.jackson.databind.exc.UnrecognizedPropertyException.from(UnrecognizedPropertyException.java:61) at com.fasterxml.jackson.databind.DeserializationContext.handleUnknownProperty(DeserializationContext.java:1138) at com.fasterxml.jackson.databind.deser.std.StdDeserializer.handleUnknownProperty(StdDeserializer.java:2224) at com.fasterxml.jackson.databind.deser.BeanDeserializerBase.handleUnknownProperty(BeanDeserializerBase.java:1709) at com.fasterxml.jackson.databind.deser.BeanDeserializerBase.handleUnknownVanilla(BeanDeserializerBase.java:1687) at com.fasterxml.jackson.databind.deser.BeanDeserializer.vanillaDeserialize(BeanDeserializer.java:320) at com.fasterxml.jackson.databind.deser.BeanDeserializer.deserialize(BeanDeserializer.java:177) at com.fasterxml.jackson.databind.deser.DefaultDeserializationContext.readRootValue(DefaultDeserializationContext.java:323) at com.fasterxml.jackson.databind.ObjectMapper._readMapAndClose(ObjectMapper.java:4825) at com.fasterxml.jackson.databind.ObjectMapper.readValue(ObjectMapper.java:3833) at com.mundoware.Main.main(Main.java:51)

This is the POJO class we are trying to map:

package com.mundoware.business.finance.collectionPayments;

import java.util.*;

public class CollectionPaymentsInput {
    private String name;
    private UUID xid;
    private UUID zId;
    private UUID yId;

    public String getName() {
        return name;
    }

    public void setName(String name) {
        this.name = name;
    }

    public UUID getXid() {
        return xid;
    }

    public void setXid(UUID xid) {
        this.xid = xid;
    }

    public UUID getZId() {
        return zId;
    }

    public void setZId(UUID zId) {
        this.zId = zId;
    }

    public UUID getYId() {
        return yId;
    }

    public void setYId(UUID yId) {
        this.yId = yId;
    }
}

If I put all keys in lowercase in the json string it works ok with Jackson., but not with JsonB.

phillip-kruger commented 2 months ago

Jackson might have more configuration options that JsonB. Maybe this can be enabled with config ?

miltonlm commented 2 months ago

If we add annotations to the fields in the POJO class, Jackson maps ok:

public class CollectionPaymentsInput {
    private String name;
    private UUID xid;
    @JsonProperty("zId")
    private UUID zId;
    @JsonProperty("yId")
    private UUID yId;
}
t1 commented 2 months ago

Maybe it's not relevant, but in the Java code, the xid is with a small I, while the other two have a capital I. And I remember that in the Turkish alphabet there is a capital I with a dot as well as a lower case i without... they are simply two distinct characters: with and without the dot. So it could theoretically be an encoding issue that nobody sees because everybody copies stuff with full UTF encoding. But that's just a wild guess.

miltonlm commented 2 months ago

Hey @t1 Thanks for your message. No, it is not an encoding problem, it is the English alphabet I for some libraries it is not camelCase, but mixed case which is the problem here.

If you can look at Jackson exception message, it requires four lowercase variables:

not marked as ignorable (4 known properties: "name", "yid", "zid", "xid"])

For some reason it is not camelCasing variables correctly. I don't know what's the matter with yId and zId, some libraries don't handle them correctly, I believe the issue is with [a-z]Id pattern.

phillip-kruger commented 2 months ago

It's been a while, but is there not a @JsonProperty like annotation for JsonB ?

miltonlm commented 2 months ago

Yes there's a @JsonbProperty but after annotating fields, it still does not work:

image

ASD ASDAS
1b768067-0212-462f-88da-b5ecf0537e41
null
null
==============JSONBUILDER DEFAULT CONFIG=============
ASD ASDAS
1b768067-0212-462f-88da-b5ecf0537e41
null
null
mskacelik commented 2 months ago

I tried to play around with the isolated JsonB reproducer, and it seems that the overall pattern ^[a-z][A-Z].*$ does not work in general (not only Id).

mskacelik commented 2 months ago

PropertyNamingStrategy.CASE_INSENSITIVE seems to fix the problem.

JsonbConfig config = new JsonbConfig()
                .withPropertyNamingStrategy(PropertyNamingStrategy.CASE_INSENSITIVE);
Jsonb json = JsonBCreator.getJsonB("com.mundoware.business.finance.collectionPayments.CollectionPaymentsInput");
Jsonb json2 = JsonbBuilder.create(config);
String jsonString = """
        {
          "name": "ASD ASDAS",
          "xid": "1b768067-0212-462f-88da-b5ecf0537e41",
          "yId": "1b768067-0212-462f-88da-b5ecf0537e41",
          "zId": "1b768067-0212-462f-88da-b5ecf0537e41"
        }
""";
CollectionPaymentsInput collectionPaymentsInput = json.fromJson(jsonString, CollectionPaymentsInput.class);

System.out.println(collectionPaymentsInput.getName());
System.out.println(collectionPaymentsInput.getXid());
System.out.println(collectionPaymentsInput.getYId());
System.out.println(collectionPaymentsInput.getZId());

System.out.println("==============JSONBUILDER DEFAULT CONFIG=============");

collectionPaymentsInput = json2.fromJson(jsonString, CollectionPaymentsInput.class);

System.out.println(collectionPaymentsInput.getName());
System.out.println(collectionPaymentsInput.getXid());
System.out.println(collectionPaymentsInput.getYId());
System.out.println(collectionPaymentsInput.getZId());
ASD ASDAS
1b768067-0212-462f-88da-b5ecf0537e41
null
null
==============JSONBUILDER DEFAULT CONFIG=============
ASD ASDAS
1b768067-0212-462f-88da-b5ecf0537e41
1b768067-0212-462f-88da-b5ecf0537e41
1b768067-0212-462f-88da-b5ecf0537e41

Yes there's a @JsonbProperty but after annotating fields, it still does not work:

image

ASD ASDAS
1b768067-0212-462f-88da-b5ecf0537e41
null
null
==============JSONBUILDER DEFAULT CONFIG=============
ASD ASDAS
1b768067-0212-462f-88da-b5ecf0537e41
null
null

@miltonlm Have you tried adding @JsonbProperty("...") onto the setters? That seems to work on my end. (not sure how it will be handled in kotlin)

mskacelik commented 2 months ago

Another way might be to add a new custom name strategy. By setting the CASE_INSENSITIVE property, it fixes the issue by acknowledging yId field, but it would not differentiate between yid and yId see: https://javaee.github.io/jsonb-spec/docs/user-guide.html#naming-strategies