Repository navigation
fix(go): validate string enum exclusions #25089
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
Changes from all commits
047bc2b
026b1dd
786b3a5
dbafad3
9d6cec6
10b5284
c958155
c1b1b92
a890b52
c9b1d26
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| generatorName: go | ||
| outputDir: samples/client/others/go/oneof-not-enum | ||
| inputSpec: modules/openapi-generator/src/test/resources/3_1/go/oneof-not-enum.yaml | ||
| templateDir: modules/openapi-generator/src/test/resources/3_1/go | ||
| additionalProperties: | ||
| hideGenerationTimestamp: "true" | ||
| files: | ||
| oneof-not-enum_test.go: | ||
| destinationFilename: oneof-not-enum_test.go |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,6 +18,7 @@ | |
| package org.openapitools.codegen.languages; | ||
|
|
||
| import com.fasterxml.jackson.databind.node.ArrayNode; | ||
| import com.fasterxml.jackson.databind.node.TextNode; | ||
| import com.google.common.collect.Iterables; | ||
| import com.samskivert.mustache.Mustache; | ||
| import io.swagger.v3.oas.models.media.Schema; | ||
|
|
@@ -600,6 +601,147 @@ public ModelsMap postProcessModels(ModelsMap objs) { | |
| return objs; | ||
| } | ||
|
|
||
| @Override | ||
| public Map<String, ModelsMap> postProcessAllModels(Map<String, ModelsMap> objs) { | ||
| objs = super.postProcessAllModels(objs); | ||
| if (!generateUnmarshalJSON) { | ||
| return objs; | ||
| } | ||
|
|
||
| // Scope allowed-enum checks to oneOf variants whose sibling excludes values of the same property. | ||
| Map<CodegenModel, Set<String>> allowedOneOfProperties = new IdentityHashMap<>(); | ||
| for (ModelsMap models : objs.values()) { | ||
| for (ModelMap modelMap : models.getModels()) { | ||
| CodegenModel union = modelMap.getModel(); | ||
| for (String excludedMemberName : union.oneOf) { | ||
| CodegenModel excludedMember = ModelUtils.getModelByName(excludedMemberName, objs); | ||
| if (excludedMember == null) { | ||
| continue; | ||
| } | ||
| for (CodegenProperty excludedProperty : effectiveVars(excludedMember).values()) { | ||
| CodegenProperty not = excludedProperty.getComposedSchemas() == null | ||
| ? null : excludedProperty.getComposedSchemas().getNot(); | ||
| if (stringEnumComparison(not, true) == null) { | ||
| continue; | ||
| } | ||
| for (String allowedMemberName : union.oneOf) { | ||
| if (allowedMemberName.equals(excludedMemberName)) { | ||
| continue; | ||
| } | ||
| CodegenModel allowedMember = ModelUtils.getModelByName(allowedMemberName, objs); | ||
| if (allowedMember == null) { | ||
| continue; | ||
| } | ||
| CodegenProperty allowedProperty = effectiveVars(allowedMember).get(excludedProperty.baseName); | ||
| if (stringEnumComparison(allowedProperty, false) != null) { | ||
| allowedOneOfProperties.computeIfAbsent(allowedMember, ignored -> new HashSet<>()) | ||
| .add(excludedProperty.baseName); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| for (ModelsMap models : objs.values()) { | ||
| for (ModelMap modelMap : models.getModels()) { | ||
| CodegenModel model = modelMap.getModel(); | ||
| if (model.isEnum || hasOneOf(model) || hasAnyOf(model)) { | ||
| continue; | ||
| } | ||
| // allOf children may need to validate properties inherited from their parent. | ||
| Map<String, CodegenProperty> effectiveVars = effectiveVars(model); | ||
| Map<String, CodegenProperty> ownVars = new HashMap<>(); | ||
| for (CodegenProperty param : model.vars) { | ||
| ownVars.put(param.baseName, param); | ||
| } | ||
| List<CodegenProperty> validationVars = new ArrayList<>(); | ||
| boolean hasInheritedStringEnumValidation = false; | ||
| for (CodegenProperty param : effectiveVars.values()) { | ||
| String allowed = allowedOneOfProperties.getOrDefault(model, Collections.emptySet()).contains(param.baseName) | ||
| ? stringEnumComparison(param, false) : null; | ||
| CodegenProperty not = param.getComposedSchemas() == null ? null : param.getComposedSchemas().getNot(); | ||
| String excluded = stringEnumComparison(not, true); | ||
| if (allowed != null || excluded != null) { | ||
| hasInheritedStringEnumValidation |= !ownVars.containsKey(param.baseName); | ||
| if (ownVars.containsKey(param.baseName) && ownVars.get(param.baseName).vendorExtensions.containsKey("x-go-datatag") | ||
| && !param.vendorExtensions.containsKey("x-go-datatag")) { | ||
| param.vendorExtensions.put("x-go-datatag", ownVars.get(param.baseName).vendorExtensions.get("x-go-datatag")); | ||
| } | ||
| validationVars.add(param); | ||
| param.vendorExtensions.put("x-go-enum-property-name", TextNode.valueOf(param.baseName).toString()); | ||
| if (allowed != null) { | ||
| param.vendorExtensions.put("x-go-allowed-string-enum-comparison", allowed); | ||
| if (param.isNullable && (param.isEnumRef || ((List<?>) param.allowableValues.get("values")).contains(null))) { | ||
| param.vendorExtensions.put("x-go-allowed-string-enum-null", true); | ||
| } | ||
| } | ||
| if (excluded != null) { | ||
| param.vendorExtensions.put("x-go-excluded-string-enum-comparison", excluded); | ||
| } | ||
| } | ||
| } | ||
| if (!validationVars.isEmpty()) { | ||
| model.vendorExtensions.put("x-go-has-string-enum-validation", true); | ||
| model.vendorExtensions.put("x-go-string-enum-validation-vars", validationVars); | ||
| List<Map<String, String>> imports = models.getImports(); | ||
| if (imports.stream().noneMatch(i -> "fmt".equals(i.get("import")))) { | ||
| imports.add(createMapping("import", "fmt")); | ||
| } | ||
| if (model.hasRequired && validationVars.stream().anyMatch(param -> param.required)) { | ||
| // Required-key checks must match the case-insensitive field names accepted by encoding/json. | ||
| model.vendorExtensions.put("x-go-enum-required-case-fold", true); | ||
| if (imports.stream().noneMatch(i -> "strings".equals(i.get("import")))) { | ||
| imports.add(createMapping("import", "strings")); | ||
| } | ||
| } | ||
| imports.sort(Comparator.comparing(i -> i.get("import"))); | ||
| } | ||
| if (hasInheritedStringEnumValidation && !model.isAdditionalPropertiesTrue) { | ||
| for (CodegenProperty param : effectiveVars.values()) { | ||
| param.vendorExtensions.put("x-go-flattened-json-name", TextNode.valueOf(param.baseName + (param.required ? "" : ",omitempty")).toString()); | ||
| } | ||
| model.vendorExtensions.put("x-go-inherited-string-enum-validation", true); | ||
| model.vendorExtensions.put("x-go-flattened-vars", new ArrayList<>(effectiveVars.values())); | ||
| } | ||
| } | ||
| } | ||
| return objs; | ||
| } | ||
|
|
||
| /** Returns properties used for validation, including inherited allOf properties when present. */ | ||
| private static Map<String, CodegenProperty> effectiveVars(CodegenModel model) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. please add docstrings to newly-created functions
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added Javadocs for both |
||
| Map<String, CodegenProperty> vars = new LinkedHashMap<>(); | ||
| for (CodegenProperty param : model.parent == null ? model.vars : model.allVars) { | ||
| vars.put(param.baseName, param); | ||
| } | ||
| return vars; | ||
| } | ||
|
|
||
| /** Builds a Go comparison for supported string/null enum values, using equality in exclusion mode. */ | ||
| private static String stringEnumComparison(CodegenProperty property, boolean excluded) { | ||
| if (property == null || !(property.isString && property.isEnum || property.isEnumRef) | ||
| || property.allowableValues == null | ||
| || !(property.allowableValues.get("values") instanceof List)) { | ||
| return null; | ||
| } | ||
| StringJoiner comparisons = new StringJoiner(excluded ? " || " : " && "); | ||
| for (Object value : (List<?>) property.allowableValues.get("values")) { | ||
| if (value == null && excluded) { | ||
| comparisons.add("value == nil"); | ||
| continue; | ||
| } | ||
| if (value == null && property.isNullable) { | ||
| continue; | ||
| } | ||
| if (!(value instanceof String)) { | ||
|
cubic-dev-ai[bot] marked this conversation as resolved.
|
||
| return null; | ||
| } | ||
| comparisons.add("value " + (excluded ? "==" : "!=") + " " + TextNode.valueOf((String) value)); | ||
| } | ||
| return comparisons.length() == 0 ? (property.isNullable && !excluded ? "true" : null) : comparisons.toString(); | ||
| } | ||
|
|
||
| /** | ||
| * Prefixes the generated {@code unknown_default_open_api} enum case with the model name when enum class prefixing | ||
| * is disabled. | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion would be nice to add more comments in the new code block (line 606 - 708)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added concise comments around the non-obvious parts of the new logic: scoping allowed-enum checks to the affected oneOf variants, handling inherited allOf properties, and matching the case-insensitive JSON field behavior used by
encoding/json.