fix: validate enum values in nested models #163

ctreatma · 2023-10-16T21:24:23Z

The Java generator for openapi-generator completely skips over enum validation in the validateJsonElement function that is generated for each model class. As a result, enum values are validated inconsistently; if I have a model X that has a property that is an enum, the enum values will be validated if I am deserializing a JSON blob in which the top-level object is an instance of X, but the enum values will not be validated if I am deserializing a JSON blob that has an instance of X nested in it.

// Let's say I have a schema component `X` that has a `this_is_an` property that is a string and a `status` property that is an enum of `"active"` or `"failed"`.
// Given that component, the below is not a valid `X` and would cause an error in metal-java <= v0.10.0
{
   "this_is_an": "X",
   "status": "not-a-status"
}

// The below is an object that has a property, `x_list` that is a list of `X` instances.  The `X` in this case is still not valid, but this would not cause an error in metal-java <= v0.10.0
{
  "x_list": [{
     "this_is_an": "X",
     "status": "not-a-status"
  }]
}

This PR introduces custom templates so that we can add enum validation to validateJsonElement and ensure that enum values are validated even for nested models.

We needed to modify two templates:

libraries/okhttp-gson/pojo.mustache had to be updated to generate code that validates model properties that are enums
modelEnum.mustache and modelInnerEnum.mustache had to be updated to generate a validateJsonElement function for all enums

Template changes happen in this commit: ddc37db
Code is updated by the bot in this commit: b0f102a

This PR also includes an example that I used to replicate the validation issue and confirm that my tests fixed it; I intended to remove it before merge, since it's really not an example of how the SDK should be used, but I'm open to keeping it. In the meantime, at least, this PR can be poked at by checking it out locally and running these commands in the project root:

# if you check out the first commit on this PR, you will need to regenerate the code so the example uses the old code
make generate
# recompile the code so that the example is using the code that was generated above
make build_client
# tell Java where to find the compiled code
export ExampleClassPath="equinix-openapi-metal/target/equinix-openapi-metal-0.10.0.jar:equinix-openapi-metal/target/lib/*"
# run the example (doesn't hit an API, so doesn't need a token)
java -classpath $ExampleClassPath examples/VirtualCircuit.java

displague · 2023-10-18T01:43:12Z

templates/libraries/okhttp-gson/pojo.mustache

@@ -553,6 +553,20 @@ public class {{classname}} {{#parent}}extends {{{.}}} {{/parent}}{{#vendorExtens
      }
      {{/required}}
      {{/isModel}}
+      {{#isEnum}}
+      {{#required}}


Should we have concerns about required readOnly and writeOnly schema properties?

(as called out for a similar change in metal-go)
equinix-labs/metal-go#132 (comment)

This PR is making a small update to the validateJsonElement section of the template, and the added code aligns with the validation that already exists for other properties.

For reference/comparison, both this block and the subsequent isEnumRef block are essentially copy-pastes of the preceding isModel block: ddc37db#diff-f6e410ad0dea301581f6b3054b9f8fb2bcdb8f05807630eefbdddca891cd1371R544-R555

ctreatma · 2023-10-18T17:34:11Z

templates/libraries/okhttp-gson/pojo.mustache

+      {{{datatypeWithEnum}}}.validateJsonElement(jsonObj.get("{{{baseName}}}"));
+      {{/required}}
+      {{^required}}
+      // validate the optional field `{{{baseName}}}`
+      if (jsonObj.get("{{{baseName}}}") != null && !jsonObj.get("{{{baseName}}}").isJsonNull()) {
+        {{{datatypeWithEnum}}}.validateJsonElement(jsonObj.get("{{{baseName}}}"));


This is, I think, the only difference between the isModel block and the new enum blocks; through inspection of the template object I found that inline enums store the enum class name in datatypeWithEnum, not in dataType.

ctreatma · 2023-10-18T17:40:38Z

templates/modelEnum.mustache

@@ -76,6 +77,11 @@ import com.google.gson.stream.JsonWriter;
      return {{{datatypeWithEnum}}}{{^datatypeWithEnum}}{{{classname}}}{{/datatypeWithEnum}}.fromValue({{#isNumber}}new BigDecimal({{/isNumber}}value{{#isNumber}}){{/isNumber}});
    }
  }
+
+  public static void validateJsonElement(JsonElement jsonElement) throws IOException {


This is largely a copy-paste of the read function above, modified to work with a JsonElement rather than a JsonReader: ddc37db#diff-e633e9a83ea415a1143582253f12897bcac9cda46fe610c6145443ab6e14ed2dR75-R78

ctreatma · 2023-10-18T17:41:08Z

templates/modelInnerEnum.mustache

@@ -71,6 +71,11 @@
        return {{{datatypeWithEnum}}}{{^datatypeWithEnum}}{{classname}}{{/datatypeWithEnum}}.fromValue({{#isNumber}}new BigDecimal({{/isNumber}}value{{#isNumber}}){{/isNumber}});
      }
    }
+
+    public static void validateJsonElement(JsonElement jsonElement) throws IOException {


This is largely a copy-paste of the read function above, modified to work with a JsonElement rather than a JsonReader: ddc37db#diff-450b10b94dc0a9d1e3ca4b6aa10dcf879ce42fddf07d6704d95c203a0398ff4dR69-R73

displague · 2023-10-18T19:35:18Z

This PR also includes an example that I used to replicate the validation issue and confirm that my tests fixed it; I intended to remove it before merge, since it's really not an example of how the SDK should be used, but I'm open to keeping it. In the meantime, at least, this PR can be poked at by checking it out locally and running these commands in the project root

Can we preserve this as a test?

If https://github.com/equinix-labs/metal-java/tree/main/equinix-functional-test/src/test/java/com/equinix/test tests "functional" changes, perhaps a "behavioral" set of tests could be kept in the project to ensure that our patches and templates changes (work-arounds) behave as we expect.

Approving as-is. We can open a new issue to address this.

displague · 2023-10-18T19:36:17Z

Is this a change that should be pushed upstream to openapitools-generator?

ctreatma · 2023-10-18T19:50:02Z

Is this a change that should be pushed upstream to openapitools-generator?

Yes, this will be proposed upstream

github-actions · 2023-10-18T19:58:33Z

This PR is included in version 0.10.1 🎉

ctreatma force-pushed the deeper-oneof-validation branch 2 times, most recently from 150fbb0 to 84d8b36 Compare October 17, 2023 19:19

ctreatma marked this pull request as ready for review October 17, 2023 20:00

displague reviewed Oct 18, 2023

View reviewed changes

ctreatma changed the title ~~fix: validate enum values via oneOf models~~ fix: validate enum values in nested models Oct 18, 2023

ctreatma added 3 commits October 18, 2023 12:12

TEMP example exercising validation code

50fb267

add support for custom templates

d939a27

update custom templates to validate ref'd enums

ddc37db

ctreatma force-pushed the deeper-oneof-validation branch from 056d38f to ddc37db Compare October 18, 2023 17:26

sync: generate client with patched spec

b0f102a

ctreatma commented Oct 18, 2023

View reviewed changes

displague approved these changes Oct 18, 2023

View reviewed changes

ctreatma merged commit 974a694 into main Oct 18, 2023
2 checks passed

ctreatma deleted the deeper-oneof-validation branch October 18, 2023 19:49

displague mentioned this pull request Oct 18, 2023

Add initial behavioral tests to verify patches and templates work as intended #164

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: validate enum values in nested models #163

fix: validate enum values in nested models #163

ctreatma commented Oct 16, 2023 •

edited

Loading

displague Oct 18, 2023 •

edited

Loading

ctreatma Oct 18, 2023

ctreatma Oct 18, 2023

ctreatma Oct 18, 2023

ctreatma Oct 18, 2023

ctreatma Oct 18, 2023

displague commented Oct 18, 2023

displague commented Oct 18, 2023

ctreatma commented Oct 18, 2023

github-actions bot commented Oct 18, 2023

fix: validate enum values in nested models #163

fix: validate enum values in nested models #163

Conversation

ctreatma commented Oct 16, 2023 • edited Loading

displague Oct 18, 2023 • edited Loading

Choose a reason for hiding this comment

ctreatma Oct 18, 2023

Choose a reason for hiding this comment

ctreatma Oct 18, 2023

Choose a reason for hiding this comment

ctreatma Oct 18, 2023

Choose a reason for hiding this comment

ctreatma Oct 18, 2023

Choose a reason for hiding this comment

ctreatma Oct 18, 2023

Choose a reason for hiding this comment

displague commented Oct 18, 2023

displague commented Oct 18, 2023

ctreatma commented Oct 18, 2023

github-actions bot commented Oct 18, 2023

ctreatma commented Oct 16, 2023 •

edited

Loading

displague Oct 18, 2023 •

edited

Loading