Skip to content

Add failing test case for schema generation #7080

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

Closed
wants to merge 2 commits into from
Closed

Add failing test case for schema generation #7080

wants to merge 2 commits into from

Conversation

dhaval-mehta
Copy link
Contributor

Notes:
As per OpenAPI specs, type is required. While generating schema, type is not provided for choice field. This leads to wrong schema in swagger and javascript error in redoc.

Someone has already reported this issue. See #7023

@dhaval-mehta
Copy link
Contributor Author

dhaval-mehta commented Dec 8, 2019

@tomchristie I would like to fix this issue.

My idea:

There is no constraint on the datatypes of choices.

While debugging schema generation logic, I found a comment with 2 questions at rest_framework/schemas/openapi.py:247

 # ChoiceFields (single and multiple).
        # Q:
        # - Is 'type' required?
        # - can we determine the TYPE of a choicefield?

I would like to answer these questions.

Question: Is 'type' required?
Answer: Yes
Updated Answer: No but It can not be null.

Question: can we determine the TYPE of a choicefield?
Answer: Any datatype can be passed to the choice-field but in most of the use cases, the datatype of choices will be one of [number, string, integer, boolean].
OpenAPI spec supports Object, Integer, Number, Boolean, Array as datatypes.
If the type of choices is not in [number, string, integer, boolean], we can assign object as of now.

I know it is not a 100% proper solution but "something is better than nothing"

@dhaval-mehta dhaval-mehta changed the title add failing test case for schema generation Add failing test case for schema generation Dec 8, 2019
@dhaval-mehta
Copy link
Contributor Author

Any Update on this?

@carltongibson
Copy link
Collaborator

HI @dhaval-mehta. Sorry I missed this one. I've seen it now. I'll take a look. Will just need a cycle. Sorry for the delay.

@sigbjornlo
Copy link

@dhaval-mehta Where does the OpenAPI spec say that it requires type?

https://spec.openapis.org/oas/v3.0.2#properties states that enum (which is what ChoiceField gets mapped to) is "taken directly from the JSON Schema definition and follow the same specifications".

https://tools.ietf.org/html/draft-wright-json-schema-validation-00#section-5.20 states that " Elements in the array MAY be of any type, including null."

https://json-schema.org/understanding-json-schema/reference/generic.html#enumerated-values also supports the usage of 'any' type in enums.

I do agree that we should try to add the type property to the OpenAPI output where it can be inferred, or add some way to explicitly state which type the enum should be mapped to, but I couldn't find anywhere in the spec that says it has to be.

In my current codebase I am outputting typescript interfaces from DRF, and for the time being I have something akin to this code to do the mapping:

if isinstance(field, serializers.ChoiceField):
    choices = list(field.choices)
    if all(isinstance(item, Number) for choice in choices):
        return {
    		'type': 'number', 
			'enum': choices
		}
    if all(isinstance(item, str) for choice in choices):
		return {
    		'type': 'string', 
			'enum': choices
		}
		
	return {
		'enum': choices
	}

@dhaval-mehta
Copy link
Contributor Author

dhaval-mehta commented Jan 20, 2020

@dhaval-mehta Where does the OpenAPI spec say that it requires type?

https://spec.openapis.org/oas/v3.0.2#properties states that enum (which is what ChoiceField gets mapped to) is "taken directly from the JSON Schema definition and follow the same specifications".

https://tools.ietf.org/html/draft-wright-json-schema-validation-00#section-5.20 states that " Elements in the array MAY be of any type, including null."

https://json-schema.org/understanding-json-schema/reference/generic.html#enumerated-values also supports the usage of 'any' type in enums.

I do agree that we should try to add the type property to the OpenAPI output where it can be inferred, or add some way to explicitly state which type the enum should be mapped to, but I couldn't find anywhere in the spec that says it has to be.

In my current codebase I am outputting typescript interfaces from DRF, and for the time being I have something akin to this code to do the mapping:

if isinstance(field, serializers.ChoiceField):
    choices = list(field.choices)
    if all(isinstance(item, Number) for choice in choices):
        return {
    		'type': 'number', 
			'enum': choices
		}
    if all(isinstance(item, str) for choice in choices):
		return {
    		'type': 'string', 
			'enum': choices
		}
		
	return {
		'enum': choices
	}

Thanks for pointing out.

As per https://tools.ietf.org/html/draft-wright-json-schema-validation-00#section-5.20,
The value of this keyword MUST be either a string or an array. If it is an array, elements of the array MUST be strings and MUST be unique.

String values MUST be one of the seven primitive types defined by the core specification.

Here, the DRF schema generator is set type as null which is wrong.

Also, I have already suggested that we can generate a type for the choice-field in specific cases. for the rest of the cases, we should not set type at all.

Anyway, PR to fix this issue is already open. Let's wait for the release. :)

@kevin-brown
Copy link
Member

This was automatically closed because the PR fixing the root issue (a null type being generated) was merged.

Also, I have already suggested that we can generate a type for the choice-field in specific cases. for the rest of the cases, we should not set type at all.

I'm open to reviewing a PR which adds this functionality in a clean way.

@carltongibson
Copy link
Collaborator

Let's reopen. (Good catch @kevin-brown!)

@carltongibson carltongibson reopened this Jan 22, 2020
@dhaval-mehta
Copy link
Contributor Author

Thanks for the great support.

I am actively working to provide type for choice/enum field.

@dhaval-mehta
Copy link
Contributor Author

As #7161 is merged into master, I am closing this issue.

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.

5 participants