fix c-libcurl generator for int and boolean values

[hirishh]

Hello, while working with the kubernetes-c library that is generated with openai-generator, I had one case where I got a null CJSON object. More info here: kubernetes-client/c#193

Basically for required int and boolean fields, there is a check that is made that has no logic. This is an example:

    // v1_daemon_set_status->number_misscheduled
    if (!v1_daemon_set_status->number_misscheduled) {
        goto fail;
    }
    if(cJSON_AddNumberToObject(item, "numberMisscheduled", v1_daemon_set_status->number_misscheduled) == NULL) {
    goto fail; //Numeric
    }

in this case number_misscheduled is a required int and if this value is 0, this conversion will fail.

The PR fix this issue for int and boolean values.

PR checklist

• Read the contribution guidelines.
• Pull Request title clearly describes the work in the pull request and Pull Request description provides details about how to validate the work. Missing information here may result in delayed response from the community.
• Run the following to build the project and update samples:

  ./mvnw clean package 
  ./bin/generate-samples.sh
  ./bin/utils/export_docs_generators.sh
  

  Commit all changed files.
  This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master.
  These must match the expectations made by your contribution.
  You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example ./bin/generate-samples.sh bin/configs/java*.
  For Windows users, please run the script in Git BASH.
• In case you are adding a new generator, run the following additional script :

  ./bin/utils/ensure-up-to-date
  

  Commit all changed files.
• File the PR against the correct branch: master (6.3.0) (minor release - breaking changes with fallbacks), 7.0.x (breaking changes without fallbacks)
• If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.

[hirishh]

FYI C coommittee @zhemant (2018/11) @ityuhui (2019/12) @michelealbano (2020/03)

[zhemant]

Thank you for the PR @hirishh

If I understand correctly, this PR is removing the required field completely for numeric and boolean. So if we have a numeric field as required, the API won't be able to enforce it.

I think it would be better if we add limits in case of numeric and fixed checks for boolean(or we can skip for boolean as we are using bool and there is no unknown value for bool, ideally we should be using a custom struct which has true/false/null).

E.g. something like this would handle lower limits, however, the upper limits have issues as we are using numeric for handling numbers instead of int/long/float so they go over the int limit if the API is defining long. But for the issue described here, I think this would work fine as we will take into consideration the minimum value, if we have an API with limits for numeric which says 1 - 65565 we will be able to handle this in the checks.

({{#minimum}}{{minimum}} {{#exclusiveMinimum}}<{{/exclusiveMinimum}}{{^exclusiveMinimum}}<={{/exclusiveMinimum}}{{/minimum}} {{{classname}}}->{{{name}}})

For numeric I have seen API definitions like

    Uint16:
      type: integer
      minimum: 0
      maximum: 65535
    Uint64:
      type: integer
      minimum: 0
      maximum: 18446744073709551615 #(2^64)-1

[hirishh]

Hello @zhemant,

Thank you for the replay. I will let @ityuhui jump in the discussion since he's also maintainer of the kubernetes-c lib.
I'm just an outsider trying to help at his first experience with openapi-generator :)

[ityuhui]

Thanks @hirishh

[wing328]

not entirely related to this PR but shall we add a github workflow to ensure samples/client/petstore/c/ compiles?

currently we do it manually and locally to ensure the pestore sample compiles

https://github.com/actions/starter-workflows/blob/main/ci/c-cpp.yml

[ityuhui]

not entirely related to this PR but shall we add a github workflow to ensure samples/client/petstore/c/ compiles?

currently we do it manually and locally to ensure the pestore sample compiles

I agree. It's better to have it. I have found an existing example for QT client https://github.com/OpenAPITools/openapi-generator/blob/master/.github/workflows/samples-cpp-qt-client.yaml.

[wing328]

@ityuhui yes, we can use that one as a starting point.

[ityuhui]

Hi @zhemant

This PR is not to remove the required field for numeric and boolean.
On the contrary, this PR makes numberic and boolean mandatory regardless of #isRequired

Because our c-libcurl template has a problem:
We use int data type to store the numberic and bool, then the vaue 0 for numberic and the value false for boolean cannot be handled correctly since c-libcurl template cannot tell if field has value 0 or field does not exist (NULL)

The best solution is we use int* to hold the numberic and bool, but I believe it will take a lot of effort to change the current code.

So @hirishh and I create this PR to fix the issue that "0" for numberic is not handled correctly now.

[wing328]

@hirishh can you please pull the latest master into your branch? that should trigger the github workflow to build the C petstore client.

[zhemant]

@ityuhui

e.g. https://github.com/OpenAPITools/openapi-generator/pull/15782/files#diff-3886166eda9ca0f2f9581c129d495a64e82c89169017161906dc47502a874b6fL45 or in the changelogs, I see all the if statements are removed from the pet store code, which means now if we send 0, it will be accepted as a valid value and will be added in the final JSON payload.

I would avoid int *, and use custom definitions e.g. for boolean, have 3 values, true|false|null( in the API's I use, the bool value could be optional and sending false/true is not the correct, we can add null and with checks we can skip the addition of bool when value is null) and for int use the limits from spec otherwise the default ones are populated in the libcurl java file.

These changes mean we need a section for if as per datatype instead of generalizing the if block for required or non required.

e.g.

if(
                {{^isContainer}}
                    {{^isPrimitiveType}}
                        {{#isModel}}
                            {{#isEnum}}
                                {{{classname}}}->{{{name}}}
                            {{/isEnum}}
                            {{^isEnum}}
                                {{{classname}}}->{{{name}}}
                            {{/isEnum}}
                        {{/isModel}}
                        {{#isUuid}}
                            {{{classname}}}->{{{name}}}
                        {{/isUuid}}
                        {{#isEmail}}
                            {{{classname}}}->{{{name}}}
                        {{/isEmail}}
                        {{#isFreeFormObject}}
                            {{{classname}}}->{{{name}}}
                        {{/isFreeFormObject}}
                    {{/isPrimitiveType}}
                    {{#isPrimitiveType}}
                        {{#isNumeric}}
                                ({{#minimum}}{{minimum}} {{#exclusiveMinimum}}<{{/exclusiveMinimum}}{{^exclusiveMinimum}}<={{/exclusiveMinimum}}{{/minimum}} {{{classname}}}->{{{name}}})
                        {{/isNumeric}}
                        {{#isEnum}}
                            {{#isString}}
                                {{{classname}}}->{{{name}}}
                            {{/isString}}
                        {{/isEnum}}
                        {{^isEnum}}
                            {{#isString}}
                                {{{classname}}}->{{{name}}}
                            {{/isString}}
                        {{/isEnum}}
                        {{#isByteArray}}
                            {{{classname}}}->{{{name}}}
                        {{/isByteArray}}
                        {{#isBinary}}
                            {{{classname}}}->{{{name}}}
                        {{/isBinary}}
                        {{#isBoolean}}
                                ({{{classname}}}->{{{name}}} == true || {{{classname}}}->{{{name}}} == false)
                        {{/isBoolean}}
                        {{#isDate}}
                            {{{classname}}}->{{{name}}}
                        {{/isDate}}
                        {{#isDateTime}}
                            {{{classname}}}->{{{name}}}
                        {{/isDateTime}}
                    {{/isPrimitiveType}}
                {{/isContainer}}
                {{#isContainer}}
                    {{#isListContainer}}
                        {{{classname}}}->{{{name}}}
                    {{/isListContainer}}
                    {{#isMapContainer}}
                        {{{classname}}}->{{{name}}}
                    {{/isMapContainer}}
                {{/isContainer}} )

The reason for not adding max values is we are not handling numeric type correctly. We use int for int,long,double etc. for which max values are different and it causes warnings during compiling.

[ityuhui]

@zhemant

I completely agree that booleans should be implemented by a custom struct like yours.
But for Numberic, your suggested solution still doesn't recognize NULL and 0 like this PR.
e.g.
When 0 is the minimum value, exclusiveMinimum is false, user sends NULL for a parameter, it will be accepted as a valid value 0 and will be added in the final JSON payload.

BTW:
Java client uses Integer to resolve this problem.
C++ QT client adds a property bool m_xxx_isSet to resolve this problem https://github.com/OpenAPITools/openapi-generator/blob/master/samples/client/petstore/cpp-qt/client/PFXUser.h.

[zhemant]

@ityuhui

E.g.

    // api_response->code
    if(cJSON_AddNumberToObject(item, "code", api_response->code) == NULL) {
    goto fail; //Numeric
    }

This is a snippet from the updated pet store of this PR. This will add "code" every time without considering if it is required or not.

When 0 is the minimum value, exclusiveMinimum is false, the user sends NULL for a parameter, it will be accepted as a valid value 0 and will be added in the final JSON payload.

I agree with you, but this is a drawback of C for not having NULL. The user will specifically have to send out-of-range values like -1 if 0 is a valid value. If 0 is a valid value and the user is sending NULL(which is not possible as the user will send 0 during initialization) then the code has to consider 0 as a valid value if it is specified as a lower limit valid value. With the suggestions in the previous comment, we will take into consideration the limits if they are specified in the API specification.

{{#exclusiveMinimum}}<{{/exclusiveMinimum}}{{^exclusiveMinimum}}<={{/exclusiveMinimum}} If the exclusive minimum is false, then the API is allowing 0 as a valid value else the API should set these flags or limits correctly.

But the issue of NULL is going to be there no matter whether we keep the if check or remove the if check. Then it is not the correct use of C.

C++ QT client adds a property bool m_xxx_isSet to resolve this problem https://github.com/OpenAPITools/openapi-generator/blob/master/samples/client/petstore/cpp-qt/client/PFXUser.h.

This is adding setters and getters for the object. I believe this will change the architecture too far from C, instead of using function parameters to initialize, we will have to add for every parameter a setter and getter.

Update: We can use pointers to support NULL then we dont use basic type for numeric but pointers. This would be okay I think, but this would mean we have to change the api and model definitions to handle int as a pointer everywhere, then we can support NULL and all values.

[ityuhui]

@zhemant
I got your point:

The user will specifically have to send out-of-range values like -1 if 0 is a valid value

It's a good solution !
Unfortunately, some API specifications do not specify a minimum value for integers, such as Kubernetes. So we cannot use this solution to our problem.

Currently, I also agree that int * is the most viable solution. I'm going to do a further investigation.
Anyway, let's close this PR because it's not accepted yet.

Thanks @hirishh for your work. I'll make another PR and let you know.

[zhemant]

@ityuhui I think I have a solution for this which could be quick change

In https://github.com/OpenAPITools/openapi-generator/blob/master/modules/openapi-generator/src/main/java/org/openapitools/codegen/languages/CLibcurlClientCodegen.java#L423 we have the possibility to play with schema and here specifically integer/numeric. I think we can add code here, to check if the API is not setting the limits, then we set the limits and make 0 as a minimum as default, this will take care of the issues when API is not specifying limits and then 0 is also a valid value. If this helps, then I can make a PR which will do this and it should

But then there is again a case when API is not specifying but accepts 0 as valid. I don't think we can cover all possibilities anyway unless the API is written clearly enough. If any API is expecting 1 and above it must make it clear in API definitions.

[ityuhui]

@zhemant From my point of view, I would not set a default minimum value of integers to 0 because actually 0 is just the mininum of unsigned integer. Like your examples:

Uint16:
      type: integer
      minimum: 0
      maximum: 65535
Uint64:
      type: integer
      minimum: 0
      maximum: 18446744073709551615 #(2^64)-1

I think the integer parameters would accept negative integers in this c client library.

[zhemant]

@ityuhui that's right, unfortunately, it is difficult to make everyone happy. The same will be for int *, as the user will have to do a bit more stuff to make C happy.

[wing328]

CI tests passed via https://github.com/OpenAPITools/openapi-generator/actions/runs/5792016752/job/15697678399?pr=16278

[wing328]

FYI. About unsigned integer, there's a rule in openapi-normalizer related to that: https://github.com/OpenAPITools/openapi-generator/blob/master/docs/customization.md#openapi-normalizer

ADD_UNSIGNED_TO_INTEGER_WITH_INVALID_MAX_VALUE: when set to true, auto fix integer with maximum value 4294967295 (2^32-1) or long with 18446744073709551615 (2^64-1) by adding x-unsigned to the schema
Example:

java -jar modules/openapi-generator-cli/target/openapi-generator-cli.jar generate -g java -i modules/openapi-generator/src/test/resources/3_0/addUnsignedToIntegerWithInvalidMaxValue_test.yaml -o /tmp/java-okhttp/ --openapi-normalizer ADD_UNSIGNED_TO_INTEGER_WITH_INVALID_MAX_VALUE=true

currently, the csharp client generator supports x-unsigned

[ityuhui]

@wing328

This issue has little to do with "unsigned integer".

I think there are two better options so far:

1. Replace int with *int for the number and boolean function parameters, NULL means no parameter was specified.
2. Create parameter structs such as list_pod_parameter for the function parameter, just like other C++ templates.
   For example:
   C++ QT client adds a property bool m_xxx_isSet

https://github.com/OpenAPITools/openapi-generator/blob/36cb3ce6b9052c16e71bb2893b111e9bd2eca251/samples/client/petstore/cpp-qt/client/PFXUser.h#L96C9-L96C9

openapi-generator/samples/client/petstore/cpp-qt/client/PFXUserApi.cpp

Line 448 in 36cb3ce

void PFXUserApi::getUserByName(const QString &username) {

[wing328]

@ityuhui thanks for the explanation. I won't merge it until this PR is finalized and ready.