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

[csharp][java] Fix enum discriminator default value #19614

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

david-marconis
Copy link
Contributor

@david-marconis david-marconis commented Sep 18, 2024

When you define a schema with an enum discriminator and you use allOf to include the parent schema in the child schema the current generated code fails to compile due to the generator thinking the discriminator is of type string instead of enum.

This is mentioned in some issues I could find which this PR should fix:
#12412 #16073 #16672 #19194 #19261

These PRs attempt to solve it, but they have some flaws.
#10959 #16394

This was tested with java and csharp generators which both have issues. Other generators might also have issues with this if they implement custom logic of handling default values of enums like the csharp generator currently does.

The issue in the code lied in the fact that when checking whether or not the discriminator is an enum it only checks the properties of the child schema, however the property might be defined in the referenced parent schema.

        // The schema here might not have the discriminator type property.
        if (schema.getProperties() != null &&
                schema.getProperties().get(discriminatorPropertyName) instanceof StringSchema) {
            StringSchema s = (StringSchema) schema.getProperties().get(discriminatorPropertyName);
            if (s.getEnum() != null && !s.getEnum().isEmpty()) { // it's an enum string
                discriminator.setIsEnum(true);
            }
        }

This can be verified by running the added tests against the current master and observe them fail.

The code was modified to return the referenced schema in addition to the discriminator when searching for it recursively and when checking for the property check both in the child schema and the schema that defines the discriminator.

I am unsure if it is possible to have a situation where the property is neither in the child schema or the schema with the discriminator, but in an intermediary in the ref chain. This would be uncommon and I am unsure if it is possible with the current code, but I don't understand it well enough to tell whether this is possible or if it is a valid spec. Perhaps it would be better to traverse up the allOf reference chain, but I an not familiar enough with how objects can be defined with combinations of allOf, anyOf and oneOf. At least this implementation avoids another recursive traversal of the schema tree. Let me know if this should be changed or if the new implementation is good enough.

I changed the code to check all parents and all descendants for the discriminator property. This should cover most if not all possible use cases. Here is the schema I used with a mix of anyOf, allOf and oneOf mappings:

openapi: 3.0.3
info:
title: Test schema with enum discriminator
version: v1
paths:
"/animal":
get:
responses:
'200':
description: OK
content:
application/json:
schema:
"$ref": "#/components/schemas/Animal"
components:
schemas:
Animal:
type: object
properties:
# Inline enum type
petType:
type: string
enum:
- Dog
- Catty
- Gecko
- Camo
discriminator:
propertyName: petType
# Mapping with implicit (Dog, Gecko), explicit ref (Catty -> Cat), and explicit schema name (Camo -> Chameleon)
mapping:
Catty: '#/components/schemas/Cat'
Camo: 'Chameleon'
required:
- petType
Cat:
type: object
allOf:
- $ref: '#/components/schemas/Animal'
properties:
meow:
type: string
Dog:
type: object
allOf:
- $ref: '#/components/schemas/Animal'
properties:
bark:
type: string
Lizard:
oneOf:
- $ref: '#/components/schemas/Gecko'
- $ref: '#/components/schemas/Chameleon'
Gecko:
type: object
allOf:
- $ref: '#/components/schemas/Animal'
properties:
lovesRocks:
type: string
Chameleon:
type: object
allOf:
- $ref: '#/components/schemas/Animal'
properties:
currentColor:
type: string
# Car inheritance tree: Car -> Truck -> SUV
# Car -> Van -> MiniVan
# Car -> Van -> CargoVan
# Car -> Sedan
Car:
type: object
required:
- carType
# Discriminator carType not defined in Car properties, but in child properties
discriminator:
propertyName: carType
CarType:
type: string
enum:
- Truck
- SUV
- Sedan
- MiniVan
- CargoVan
Truck:
type: object
allOf:
- $ref: '#/components/schemas/Car'
properties:
carType:
$ref: '#/components/schemas/CarType'
required:
- carType
SUV:
type: object
# SUV gets its discriminator property from Truck
allOf:
- $ref: '#/components/schemas/Truck'
Sedan:
type: object
allOf:
- $ref: '#/components/schemas/Car'
required:
- carType
properties:
carType:
$ref: '#/components/schemas/CarType'
Van:
oneOf:
- $ref: '#/components/schemas/MiniVan'
- $ref: '#/components/schemas/CargoVan'
MiniVan:
type: object
allOf:
- $ref: '#/components/schemas/Car'
properties:
carType:
$ref: '#/components/schemas/CarType'
required:
- carType
CargoVan:
type: object
allOf:
- $ref: '#/components/schemas/Car'
properties:
carType:

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/configs/*.yaml
    ./bin/utils/export_docs_generators.sh
    
    (For Windows users, please run the script in Git BASH)
    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*.
    IMPORTANT: Do NOT purge/delete any folders/files (e.g. tests) when regenerating the samples as manually written tests may be removed.
  • File the PR against the correct branch: master (upcoming 7.6.0 minor release - breaking changes with fallbacks), 8.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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant