fix: BinaryField's schema type should be string #505 #506
Conversation
Codecov Report
@@ Coverage Diff @@
## master #506 +/- ##
==========================================
- Coverage 98.58% 98.56% -0.02%
==========================================
Files 57 57
Lines 5710 5723 +13
==========================================
+ Hits 5629 5641 +12
- Misses 81 82 +1
Continue to review full report at Codecov.
|
drf_spectacular/openapi.py
Outdated
| @@ -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): | |||
There was a problem hiding this comment.
BinaryField has existed since Django 1.6, so we don't need to check whether it exists.
| elif hasattr(models, 'BinaryField') and isinstance(model_field, models.BinaryField): | |
| elif isinstance(model_field, models.BinaryField): |
There was a problem hiding this comment.
This code is safe. Not happened nothing. We do not want get error.
There was a problem hiding this comment.
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.
drf_spectacular/openapi.py
Outdated
| @@ -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): | |||
| return build_basic_type(OpenApiTypes.STR) | |||
There was a problem hiding this comment.
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
If this were Base64 encoded we should be using OpenApiTypes.BYTE:
drf-spectacular/drf_spectacular/types.py
Line 67 in 508f4e5
See the data types section for OpenAPI v3.0.3.
There was a problem hiding this comment.
You are right! I fixed for OpenApiTypes.BYTE
There was a problem hiding this comment.
You've misunderstood me - this should be using OpenApiTypes.BINARY as BinaryField doesn't store Base64 encoded data, it stores raw binary data.
There was a problem hiding this comment.
I am confused. JSON format should be base64 type. I fixed for OpenApiTypes.BINARY.
There was a problem hiding this comment.
@ngnpope thanks for chiming in. your statement about the version check was correct. however, the statement about the type was incorrect.
i had a look and checked what is actually happening. DRF has no dedicated serializer field for this. what happens is that DRF tranlates models.BinaryField to a generic serializers.ModelField, which is then serialized with the models value_to_string/to_python. These methods translate the binary data (in the db field) to and from base64. I experimentally verified this just now.
tl;dr: this is correct:
elif isinstance(model_field, models.BinaryField):
return build_basic_type(OpenApiTypes.BYTE)@jtamm-red: that was an excellent catch! thank you. please update the PR and squash & force push to one commit for a clean history.
Ah, unexpected, but then I've always avoided |
No description provided.