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

[C][Client] Support a custom data type (IntOrString) #11074

Merged
merged 1 commit into from
Dec 17, 2021

Conversation

ityuhui
Copy link
Contributor

@ityuhui ityuhui commented Dec 8, 2021

There is a custom data type from Kubernetes swagger specification:

   "description": "IntOrString is a type that can hold an int32 or a string.  When used in JSON or YAML marshalling and unmarshalling, it produces or consumes the inner type.  This allows you to have, for example, a JSON field that can accept a name or number.",
      "format": "int-or-string",
      "type": "string"
    },

And the model log (-DdebugModels) of openapi-generator-cli shows:

{
      "openApiType" : "string",
      "baseName" : "targetPort",
      "complexType" : "int_or_string",
      "getter" : "getTargetPort",
      "setter" : "setTargetPort",
      "description" : "IntOrString is a type that can hold an int32 or a string.  When used in JSON or YAML marshalling and unmarshalling, it produces or consumes the inner type.  This allows you to have, for example, a JSON field that can accept a name or number.",
      "dataType" : "int_or_string",
      "datatypeWithEnum" : "int_or_string",
      "dataFormat" : "int-or-string",
      "name" : "target_port",
      "defaultValueWithParam" : " = data.targetPort;",
      "baseType" : "int_or_string",
      "unescapedDescription" : "IntOrString is a type that can hold an int32 or a string.  When used in JSON or YAML marshalling and unmarshalling, it produces or consumes the inner type.  This allows you to have, for example, a JSON field that can accept a name or number.",
      "example" : "\"0\"",
      "jsonSchema" : "{\n  \"type\" : \"string\",\n  \"description\" : \"IntOrString is a type that can hold an int32 or a string.  When used in JSON or YAML marshalling and unmarshalling, it produces or consumes the inner type.  This allows you to have, for example, a JSON field that can accept a name or number.\",\n  \"format\" : \"int-or-string\"\n}",
      "exclusiveMinimum" : false,
      "exclusiveMaximum" : false,
      "required" : false,
      "deprecated" : false,
      "hasMoreNonReadOnly" : false,
      "isPrimitiveType" : false,
      "isModel" : false,
      "isContainer" : false,
      "isString" : true,
      "isNumeric" : false,
      "isInteger" : false,
      "isShort" : false,
      "isLong" : false,
      "isUnboundedInteger" : false,
      "isNumber" : false,
      "isFloat" : false,
      "isDouble" : false,
      "isDecimal" : false,
      "isByteArray" : false,
      "isBinary" : false,
      "isFile" : false,
      "isBoolean" : false,
      "isDate" : false,
      "isDateTime" : false,
      "isUuid" : false,
      "isUri" : false,
      "isEmail" : false,
      "isNull" : false,
      "isFreeFormObject" : false,
      "isAnyType" : false,
      "isArray" : false,
      "isMap" : false,
      "isEnum" : false,
      "isReadOnly" : false,
      "isWriteOnly" : false,
      "isNullable" : false,
      "isSelfReference" : false,
      "isCircularReference" : false,
      "isDiscriminator" : false,
      "vars" : [ ],
      "requiredVars" : [ ],
      "vendorExtensions" : { },
      "hasValidation" : false,
      "isInherited" : false,
      "nameInCamelCase" : "TargetPort",
      "nameInSnakeCase" : "TARGET_PORT",
      "uniqueItems" : false,
      "isXmlAttribute" : false,
      "isXmlWrapped" : false,
      "additionalPropertiesIsAnyType" : false,
      "hasVars" : false,
      "hasRequired" : false,
      "hasDiscriminatorWithNonEmptyMapping" : false,
      "hasMultipleTypes" : false,
      "datatype" : "int_or_string",
      "iexclusiveMaximum" : false
    } 

So this is a data type with below attributes:

  • isContainer: false
  • isPrimitiveType: false
  • isModel: false
  • isFreeFormObject: false

I add the support for this data type in C generator.

BTW: The header and implementation files of the custom data type cannot be provided by this PR because they need to be implemented by users themselves.

Hi @wing328 @zhemant @michelealbano

Could you please reivew this code change ? Thanks !

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.
  • File the PR against the correct branch: master (5.3.0), 6.0.x
  • If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.

@wing328
Copy link
Member

wing328 commented Dec 8, 2021

"description": "IntOrString is a type that can hold an int32 or a string. When used in JSON or YAML marshalling and unmarshalling, it produces or consumes the inner type. This allows you to have, for example, a JSON field that can accept a name or number.",
"format": "int-or-string",
"type": "string"
},

Looks like this should be described as oneOf in OpenAPI 3.0 spec.

oneOf:
  - type: integer
  - type: string

@zhemant
Copy link
Contributor

zhemant commented Dec 8, 2021

@ityuhui would it be possible for you to provide the whole API file?? As wing328 said, it will be oneOf parameter. I have local copy of Generator which I have modified to support oneOf in C. But as they are modifying lot of core I haven't pushed the PR yet. But if it works, then may be you can help me to complete the PR and add this as a supported feature to codegen.

@ityuhui
Copy link
Contributor Author

ityuhui commented Dec 9, 2021

Thank you @wing328 and @zhemant

The complete swagger file is https://raw.githubusercontent.com/kubernetes-client/java/master/kubernetes/swagger.json

I think the version of Kubernetes API swagger is v2.0

@zhemant
Copy link
Contributor

zhemant commented Dec 10, 2021

@ityuhui I created api's with the following YAML file:

openapi: 3.0.0
servers:
  - url: 'http://petstore.swagger.io/v2'
info:
  version: 1.0.0
  title: OpenAPI Petstore
paths:
  /pet:
    post:
      tags:
        - pet
      summary: Add a new pet to the store
      operationId: addPet
      responses:
        '200':
          description: successful operation
        '405':
          description: Invalid input
      requestBody:
        content:
          application/json:
            schema:
              $ref: '#/components/schemas/Pet'
components:
  schemas:
    Pet:
      title: Pet Order
      description: An order for a pets from the pet store
      type: object
      properties:
        id:
          type: integer
          format: int64
        petId:
          $ref: '#/components/schemas/Pet1'
    Pet1:
      oneOf:
        - type: integer
        - type: string

I was able to generate a pet1 model with following struct:

typedef struct openapi_petstore_pet1_t {
	int _int;    // number - _int
	char *_char; // string  - _char -
} openapi_petstore_pet1_t;

I am working on fixing json creation in parent object:
Here in openapi_petstore_pet1_convertToJSON function will be responsible for converting int or string in json. But we have to make sure it should not create an object. pet_id_local_JSON should be a pointer to JSON object created using cJSON_create_int or cjson_create_string.

	// openapi_petstore_pet->pet_id
	if (openapi_petstore_pet->pet_id) {
		cJSON *pet_id_local_JSON = openapi_petstore_pet1_convertToJSON( openapi_petstore_pet->pet_id);
		if (pet_id_local_JSON == NULL) {
			goto end; // model
		}
		cJSON_AddItemToObject(item, "petId", pet_id_local_JSON);
	}

As of now the function openapi_petstore_pet1_convertToJSON looks like this:

cJSON *
openapi_petstore_pet1_convertToJSON(openapi_petstore_pet1_t *openapi_petstore_pet1)
{
	cJSON *item = cJSON_CreateObject();

	if (!openapi_petstore_pet1) {
		goto end;
	}

	// openapi_petstore_pet1->_int
	if ((openapi_petstore_pet1->_int && openapi_petstore_pet1->_int)) {
		if (cJSON_AddNumberToObject(item, "_int", openapi_petstore_pet1->_int) == NULL) {
			goto end; // Numeric
		}
	}

	// openapi_petstore_pet1->_char
	if (openapi_petstore_pet1->_char) {
		if (cJSON_AddStringToObject(item, "_char", openapi_petstore_pet1->_char) == NULL) {
			goto end; // String
		}
	}

	return item;

I want to modify it to generate a function like this when its oneof:

cJSON *
openapi_petstore_pet1_convertToJSON(phoenix_mem_t *mem_pool, openapi_petstore_pet1_t *openapi_petstore_pet1)
{
	cJSON *item = NULL;

	if (!openapi_petstore_pet1) {
		goto end;
	}

	// openapi_petstore_pet1->_int
	if ((openapi_petstore_pet1->_int && openapi_petstore_pet1->_int)) {
		item = cJSON_AddNumberToObject(openapi_petstore_pet1->_int)
	}

	// openapi_petstore_pet1->_char
	if (openapi_petstore_pet1->_char) {
                item = cJSON_CreateString(openapi_petstore_pet1->_char)
	}
	return item;

The problem with C is that data is mandatory not like other language where we just have to declare variable. So to achieve this we will have to add flag to set isOneOf=true either in model or in vendorextensions depending on how to decide.

Let me know if you are looking for something similar then may be we can connect and try to figure out a way to fix this problem.

@ityuhui
Copy link
Contributor Author

ityuhui commented Dec 11, 2021

Hi @zhemant

I created the source files for "IntOrString" by manal

https://github.com/ityuhui/c/blob/yh-int-or-string-1129/kubernetes/model/int_or_string.h
https://github.com/ityuhui/c/blob/yh-int-or-string-1129/kubernetes/model/int_or_string.c

I think below struct is reasonable for oneOf

typedef enum {
    IOS_DATA_TYPE_INT = 1,
    IOS_DATA_TYPE_STRING = 2
} ios_data_type_t;

typedef struct int_or_string_t {
    ios_data_type_t type;
    union {
        int i;
        char *s;
    };
} int_or_string_t;

I admit "oneOf" should be handled by c generator automatically. But for my case(kubernetes open API spec), the OpenAPI version is 2.0, so I need the change of this PR which does not conflict with your implementation for Open API v3.0

@wing328
Copy link
Member

wing328 commented Dec 14, 2021

@ityuhui understood that kubernetes spec is still v2.0 and there's no concrete plan to upgrade to v3.x at the moment.

What about working around the issue in the C client generator to "redefine" int-or-string (dataType) as oneOf (type: integer, string) in the post process operations?

Usually we don't tailor-made a solution for a custom format in the project itself but we can give it a try in this case as Kubernetes C client is probably one of the most important use cases out there.

@ityuhui
Copy link
Contributor Author

ityuhui commented Dec 14, 2021

Hi @wing328

Almost all other Kubernetes clients (e.g. Java, CSharp, Javascript) use the same method of this PR to handle int-or-string. All of them implements "int_or_string" by manual themselves. I also implemented them for C client: int_or_string.h and int_or_string.c
I think it's enough to complete this work with OpenAPI v2 and suggest keeping the same solution with other Kubernetes clients.

For the oneOf support of OpenAPI v3, we'd better make another project to address it.
Redefining int-or-string (dataType) as oneOf (type: integer, string) in the post process operations will introduce complexity and burden of maintenance. And there is not great benefit now.

@wing328 wing328 added this to the 5.3.1 milestone Dec 17, 2021
@wing328
Copy link
Member

wing328 commented Dec 17, 2021

OK. Thanks for the explanation. Let's go with this change to start with. Later if @zhemant or anyone can come up with better implementation of oneOf/anyOf, we can always consider that as a replacement to this workaround.

@wing328 wing328 merged commit 490377c into OpenAPITools:master Dec 17, 2021
@ityuhui
Copy link
Contributor Author

ityuhui commented Dec 17, 2021

Thank you @wing328
I also think so.

@ityuhui ityuhui deleted the yh-c-int-or-string-1130 branch December 17, 2021 07:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants