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.

Sign up for GitHub

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

Jump to bottom

fix: BinaryField's schema type should be string #505 #506

Merged
merged 6 commits into from
Sep 8, 2021
Merged

fix: BinaryField's schema type should be string #505 #506

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
54af383
fix: BinaryField's schema type should be string #505
jtamm-red Sep 7, 2021
6a41f43
fix: BinaryField's schema type should be byte #505
jtamm-red Sep 8, 2021
0e4e76f
fix: BinaryField's schema type should be binary #505
jtamm-red Sep 8, 2021
a7bfcb3
revert: OpenApiTypes.BYTE #505
jtamm-red Sep 8, 2021
cb40727
fix: BinaryField's schema type should be byte #505
jtamm-red Sep 7, 2021
c4daa0e
Merge remote-tracking branch 'origin/fix-binary-field-schema-type' in…
jtamm-red Sep 8, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions drf_spectacular/openapi.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -435,6 +435,8 @@ def _map_model_field(self, model_field, direction):
elif hasattr(models, 'JSONField') and isinstance(model_field, models.JSONField):
# fix for DRF==3.11 with django>=3.1 as it is not yet represented in the field_mapping
return build_basic_type(OpenApiTypes.OBJECT)
elif hasattr(models, 'BinaryField') and isinstance(model_field, models.BinaryField):
Copy link
Contributor

Choose a reason for hiding this comment

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

BinaryField has existed since Django 1.6, so we don't need to check whether it exists.

Suggested change
elif hasattr(models, 'BinaryField') and isinstance(model_field, models.BinaryField):
elif isinstance(model_field, models.BinaryField):

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This code is safe. Not happened nothing. We do not want get error.

Copy link
Contributor

Choose a reason for hiding this comment

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

It is unnecessary. Django 1.5 has been end-of-life for years - long before this package existed.

The equivalent check for JSONField is required as that was added more recently in Django 3.1 and Django 2.2 is still supported.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well. I will fix.

return build_basic_type(OpenApiTypes.STR)
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems wrong to me. BinaryField is documented as storing raw binary data. That implies that we should be using OpenApiTypes.BINARY:

drf-spectacular/drf_spectacular/types.py

Line 68 in 508f4e5

OpenApiTypes.BINARY: {'type': 'string', 'format': 'binary'},

If this were Base64 encoded we should be using OpenApiTypes.BYTE:

drf-spectacular/drf_spectacular/types.py

Line 67 in 508f4e5

OpenApiTypes.BYTE: {'type': 'string', 'format': 'byte'},

See the data types section for OpenAPI v3.0.3.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right! I fixed for OpenApiTypes.BYTE

Copy link
Contributor

Choose a reason for hiding this comment

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

You've misunderstood me - this should be using OpenApiTypes.BINARY as BinaryField doesn't store Base64 encoded data, it stores raw binary data.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am confused. JSON format should be base64 type. I fixed for OpenApiTypes.BINARY.

elif hasattr(models, model_field.get_internal_type()):
# be graceful when the model field is not explicitly mapped to a serializer
internal_type = getattr(models, model_field.get_internal_type())
Expand Down