java: allow aliased types to have custom mappings #4182

fabiokung · 2019-10-19T00:41:50Z

~~Fixes #2858.~~ UPDATE: I was wrong and misundertood the code, this PR does not fix the issue, but #4182 (comment) discusses an alternative on how to accomplish the same. I'd still consider the original issue to be a bug, it should be possible to define type mappings for any type definition in the spec.

This will allow kubernetes-client-java to switch to openapi-generator (kubernetes-client/java#709).

The kubernetes API swagger spec has types aliased to primitive types, e.g.:

    "intstr.IntOrString": {
      "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"
    },

However, these types need to be mapped to custom types in the Java client, so we can have custom serialization for them. The generator is being invoked with the following options:

  <type-mappings>intstr.IntOrString=IntOrString,resource.Quantity=Quantity</type-mappings>
  <import-mappings>IntOrString=io.kubernetes.client.custom.IntOrString,Quantity=io.kubernetes.client.custom.Quantity</import-mappings>

Without changes in this PR, type-mappings gets ignored, since IntOrString is first resolved to its alias (string), before mappings are evaluated.

cc @wing328 @brendandburns @yue9944882

Fixes OpenAPITools#2858 This will allow `kubernetes-client-java` to switch to openapi-generator, since the kubernetes API swagger spec has types aliased to primitive types (e.g. intstr.IntOrString => string) that need to be mapped to custom types (and not be treated as a primitive type).

brendandburns · 2019-10-19T01:04:12Z

Thank you for figuring this out!!!

yue9944882 · 2019-10-21T07:46:56Z

awesome! politely bump @wing328 for a review!

wing328 · 2019-10-21T17:13:23Z

Looks good but I'll need to test it locally tomorrow (as it's 1am here ...) before merging it into master

fabiokung · 2019-10-21T23:22:36Z

Please hold on merging. I suspect there's more involved here. kubernetes-client-java seems to be hitting a different code path of the generator.

fabiokung · 2019-10-22T00:41:30Z

I believe this is now good to be merged.

I did some more digging, and found that typeMappings on aliased types will only be applied when they are of type string, and have a format field specified:

From DefaultCodegen#getPrimitiveType():

        } else if (ModelUtils.isStringSchema(schema)) {
            if (typeMapping.containsKey(schema.getFormat())) {
                // If the format matches a typeMapping (supplied with the --typeMappings flag)
                // then treat the format as a primitive type.
                // This allows the typeMapping flag to add a new custom type which can then
                // be used in the format field.
                return schema.getFormat();
            }
            return "string";
        }

With the changes in this PR I was able to tweak code generation for kubernetes-client-java and have it apply custom type mappings, by rewriting the spec to make sure types with custom mapping also have a custom format.

fabiokung · 2019-10-22T00:50:30Z

The tweaks to the configuration, and rewriting of the spec I had to do are here: yue9944882/gen#1

yue9944882 · 2019-10-22T05:19:16Z

great, i will rebase kubernetes-client/gen#117 after this pull is merged&released

fabiokung · 2019-10-22T23:20:28Z

@yue9944882 we will also need #4226

yue9944882

LGTM

wing328 · 2019-10-23T17:02:06Z

Thanks for the update. I'll review and run some tests locally.

wing328 · 2019-10-24T14:01:33Z

@fabiokung I tested it locally but no luck getting it working locally. Here is how I tested with fake petstore spec:

Pull the changes into a branch and build the project locally
Run ./bin/java-petstore-okhttp-gson.sh --type-mappings OuterString=CustomString
Some aliases to OuterString (example) should be mapped to CustomString instead of string
Grep CustomString in the output but didn't find anything

I also tested with https://raw.githubusercontent.com/kubernetes/kubernetes/master/api/openapi-spec/swagger.json but no luck with --type-mappings io.k8s.apimachinery.pkg.util.intstr.IntOrString=intstr.IntOrString

Am I testing the change correctly?

fabiokung · 2019-10-24T20:36:24Z

@wing328 I think I misunderstood how aliases are supposed to work, and this patch may not be necessary anymore (the term alias seems to mean different things in the codebase: both what type definitions are mapped to which primitive types, and what definitions have a $ref to another type - e.g.: ModelUtils.unaliasSchema() vs DefaultCodegen#getAllAliases()).

My conclusion is that this PR was not the root cause of why typeMappings was not working for kubernetes-client-java. As I mentioned in #4182 (comment), the current code only allows typeMappings on definitions mapped to a string type that has a custom format. You can follow the logic in DefaultCodegen#getSingleSchemaType() -> DefaultCodegen#getPrimitiveType().

In the end, I made the custom type mapping work (yue9944882/gen#1) by forcing all types that needed a mapping to have a format in the spec (rewriting the spec when necessary), and using the format name in the --type-mappings flag. With the petstore sample, it would look something like:

diff --git a/modules/openapi-generator/src/test/resources/2_0/petstore-with-fake-endpoints-models-for-testing.yaml b/modules/openapi-generator/src/test/resources/2_0/petstore-with-fake-endpoints-models-for-testing.yaml
index 73d734f75f..9028469cd2 100644
--- a/modules/openapi-generator/src/test/resources/2_0/petstore-with-fake-endpoints-models-for-testing.yaml
+++ b/modules/openapi-generator/src/test/resources/2_0/petstore-with-fake-endpoints-models-for-testing.yaml
@@ -1729,6 +1729,7 @@ definitions:
     type: number
   OuterString:
     type: string
+    format: outerStringFormat
   OuterBoolean:
     type: boolean
     x-codegen-body-parameter-name: boolean_post_body

Then:

$ ./bin/java-petstore-okhttp-gson.sh --type-mappings outerStringFormat=MyCustomOuterString

...

$ git diff | grep MyCustomOuterString | head -n5
+> MyCustomOuterString fakeOuterStringSerialize(body)
+    MyCustomOuterString body = new MyCustomOuterString(); // MyCustomOuterString | Input string as post body
+      MyCustomOuterString result = apiInstance.fakeOuterStringSerialize(body);
+ **body** | **MyCustomOuterString**| Input string as post body | [optional]
+[**MyCustomOuterString**](MyCustomOuterString.md)

I'd still say all definitions should be able to have a custom typeMapping, but DefaultCodegen#getSingleSchemaType() doesn't allow that today. I think I can get a simple patch to make it work for all definitions (not only the ones of type string with a format defined).

fabiokung · 2019-10-24T22:33:56Z

I take that back, the changes are not going to be trivial. For some reason swagger-parser-core does not seem to be filling the Schema#name property for each type definition. Checking if a type definition has a type mapping installed will be a bit tricky, since once the spec has been parsed, we lose access to the name of each Schema node.

I will leave that up to others that are more familiar with the codebase. @wing328 feel free to close and track improvements to type mappings in a different GH issue.

yue9944882 · 2019-10-25T04:30:03Z

    "io.k8s.apimachinery.pkg.util.intstr.IntOrString": {
      "description": "<trimmed>",
      "type": "string",
      "format": "int-or-string"
    },

@wing328 @fabiokung thank you for the deep insight. iiuc, tweaking a custom format is not supported in openapi specification, so the "format": "int-or-string" from kuberenetes' spec is applying invalid schema here. otoh currently we have another vendor extension x-kubernetes-int-or-string as a replacement of replacing $ref to intstr.IntOrString definition, it will be better if there's a way to do type-mappings from extension. anway i think kubernetes-client/java#721 can be considered solved, right?

wing328 · 2019-10-25T05:31:29Z

@fabiokung @yue9944882 I think I may have a better solution to come up with something you guys need. Will share later today when I've the details.

wing328 · 2019-10-25T08:10:39Z

The following seems to work for me to meet your requirement mapping IntOrString (schema) to something else and do a proper import accordingly:

$ mvn clean package -DskipTests
$ java -jar ./modules/openapi-generator-cli/target/openapi-generator-cli.jar generate -g java -i https://raw.githubusercontent.com/kubernetes/kubernetes/master/api/openapi-spec/swagger.json -o /tmp/k8sc --type-mappings int-or-string=IntOfString --import-mappings IntOfString=io.kubernetes.client.custom.IntOrString

Can you plesae give it a try?

fabiokung · 2019-10-25T17:41:16Z

@wing328 This seems to be exactly what I did already - #4182 (comment), unless I’m missing something?

fabiokung · 2019-10-25T21:25:02Z

(snip) anway i think kubernetes-client/java#721 can be considered solved, right?

@yue9944882 yes, I believe the changes I sent you through yue9944882/gen#1 fixes kubernetes-client/java#721, this PR is not needed because it relies on the custom format.

fabiokung · 2019-10-25T21:33:38Z

I edited the original PR description to make it clear this PR per-se is not fixing anything, and while I think it improves the code a bit, it is not really necessary.

wing328 · 2019-10-26T07:51:56Z

I edited the original PR description to make it clear this PR per-se is not fixing anything, and while I think it improves the code a bit, it is not really necessary.

Thanks for the clarification. I'll let others take a look and see if they've any comments on the change.

fabiokung · 2019-10-29T16:36:08Z

@yue9944882 I got a notification from your comment saying you couldn't make custom type mappings work with kubernetes-client/gen#117, but the comment seems to have gone away. Were you able to make it work?

fabiokung · 2020-03-06T18:57:45Z

Closing as this is not really needed, and ended up just being a red herring and suggestion for improvements to the code.

fabiokung mentioned this pull request Oct 19, 2019

Migrate to openapi-generator and okhttp3 kubernetes-client/java#709

Closed

wing328 added this to the 4.2.0 milestone Oct 21, 2019

wing328 added Client: Java Enhancement: General labels Oct 21, 2019

remove duplicated unaliasing of types

1d94be5

fabiokung mentioned this pull request Oct 22, 2019

update the openapi generator yue9944882/gen#1

Merged

yue9944882 approved these changes Oct 23, 2019

View reviewed changes

yue9944882 mentioned this pull request Oct 23, 2019

V1RollingUpdateDaemonSet maxUnavailable is 'String' but API specifies 'IntOrString' kubernetes-client/java#721

Closed

wing328 removed this from the 4.2.0 milestone Oct 30, 2019

fabiokung closed this Mar 6, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

java: allow aliased types to have custom mappings #4182

java: allow aliased types to have custom mappings #4182

fabiokung commented Oct 19, 2019 •

edited

Loading

brendandburns commented Oct 19, 2019

yue9944882 commented Oct 21, 2019

wing328 commented Oct 21, 2019

fabiokung commented Oct 21, 2019

fabiokung commented Oct 22, 2019

fabiokung commented Oct 22, 2019

yue9944882 commented Oct 22, 2019

fabiokung commented Oct 22, 2019

yue9944882 left a comment

wing328 commented Oct 23, 2019

wing328 commented Oct 24, 2019

fabiokung commented Oct 24, 2019 •

edited

Loading

fabiokung commented Oct 24, 2019 •

edited

Loading

yue9944882 commented Oct 25, 2019

wing328 commented Oct 25, 2019 •

edited

Loading

wing328 commented Oct 25, 2019

fabiokung commented Oct 25, 2019 via email •

edited

Loading

fabiokung commented Oct 25, 2019 •

edited

Loading

fabiokung commented Oct 25, 2019 •

edited

Loading

wing328 commented Oct 26, 2019

fabiokung commented Oct 29, 2019

fabiokung commented Mar 6, 2020

java: allow aliased types to have custom mappings #4182

java: allow aliased types to have custom mappings #4182

Conversation

fabiokung commented Oct 19, 2019 • edited Loading

brendandburns commented Oct 19, 2019

yue9944882 commented Oct 21, 2019

wing328 commented Oct 21, 2019

fabiokung commented Oct 21, 2019

fabiokung commented Oct 22, 2019

fabiokung commented Oct 22, 2019

yue9944882 commented Oct 22, 2019

fabiokung commented Oct 22, 2019

yue9944882 left a comment

Choose a reason for hiding this comment

wing328 commented Oct 23, 2019

wing328 commented Oct 24, 2019

fabiokung commented Oct 24, 2019 • edited Loading

fabiokung commented Oct 24, 2019 • edited Loading

yue9944882 commented Oct 25, 2019

wing328 commented Oct 25, 2019 • edited Loading

wing328 commented Oct 25, 2019

fabiokung commented Oct 25, 2019 via email • edited Loading

fabiokung commented Oct 25, 2019 • edited Loading

fabiokung commented Oct 25, 2019 • edited Loading

wing328 commented Oct 26, 2019

fabiokung commented Oct 29, 2019

fabiokung commented Mar 6, 2020

fabiokung commented Oct 19, 2019 •

edited

Loading

fabiokung commented Oct 24, 2019 •

edited

Loading

fabiokung commented Oct 24, 2019 •

edited

Loading

wing328 commented Oct 25, 2019 •

edited

Loading

fabiokung commented Oct 25, 2019 via email •

edited

Loading

fabiokung commented Oct 25, 2019 •

edited

Loading

fabiokung commented Oct 25, 2019 •

edited

Loading