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

Fix ChoiceField schema type with empty choices=[] #1240

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

Conversation

intgr
Copy link
Contributor

@intgr intgr commented May 16, 2024

Currently ChoiceField(choices=[]) will get {"type": "boolean"} in schema. With this change, type wil be omitted from schema.

Currently ChoiceField(chocies=[], allow_blank=True) will generate schema {"type": "boolean", "enum": [""]}, that's a bit nonsensical, as the only enum listed value "" is string, not boolean. With this change, {"type": "string", "enum": [""]} will be generated.

This PR fixes these edge cases.

Comment on lines +401 to +405
schema = build_choice_field(serializers.ChoiceField(
choices=[1, 2], allow_blank=True
))
assert schema['enum'] == [1, 2, '']
assert 'type' not in schema
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that this is a change in behavior.

Previously this generated {"type": "integer"} because all choices= arguments are int and allow_blank wasn't considered.

I think this is more correct, but I'm open to restoring the old behavior for this.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to https://swagger.io/docs/specification/data-models/data-types/#mixed-type, type is expected to be a single value so this looks more correct to me too.
Would be nice to support oneOf someday that would help in this case to have:

oneOf:
  - int
  - string

But that's out-of-scope for this PR.

@romainletendart
Copy link

Hey 👋
Thank you very much for opening this PR.
As far as I can see the issue is still there.
@tfranzel Any chance you can review it?

Copy link

@romainletendart romainletendart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Comment on lines +401 to +405
schema = build_choice_field(serializers.ChoiceField(
choices=[1, 2], allow_blank=True
))
assert schema['enum'] == [1, 2, '']
assert 'type' not in schema

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to https://swagger.io/docs/specification/data-models/data-types/#mixed-type, type is expected to be a single value so this looks more correct to me too.
Would be nice to support oneOf someday that would help in this case to have:

oneOf:
  - int
  - string

But that's out-of-scope for this PR.

@tfranzel
Copy link
Owner

will review this asap. quite pressed for time right now

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.

3 participants