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 file fields #69

Merged
merged 1 commit into from May 27, 2020
Merged

Fix file fields #69

merged 1 commit into from May 27, 2020

Conversation

jayvdb
Copy link
Contributor

@jayvdb jayvdb commented May 26, 2020

They are binary on upload, and usually type uri in responses.

They are binary on upload, and usually type uri in responses.
@codecov
Copy link

codecov bot commented May 26, 2020

Codecov Report

Merging #69 into master will decrease coverage by 0.14%.
The diff coverage is 50.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #69      +/-   ##
==========================================
- Coverage   96.81%   96.66%   -0.15%     
==========================================
  Files          38       38              
  Lines        2542     2549       +7     
==========================================
+ Hits         2461     2464       +3     
- Misses         81       85       +4     
Impacted Files Coverage Δ
drf_spectacular/openapi.py 91.91% <50.00%> (-0.62%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ddeb5bb...573cf09. Read the comment docs.

@jayvdb
Copy link
Contributor Author

jayvdb commented May 26, 2020

@tfranzel
Copy link
Owner

good point! it is the right idea, but it's not quite right yet.

this is cannot be represented with one component though, as one field would have to take 2 different types depending in req/resp. luckily, there is a mode for that in spectacular, where request and repsponse are split into 2 components (if applicable):

'COMPONENT_SPLIT_REQUEST': True # default: False

we can wrap it in there. That mode also has some other benefits regarding required and req/resp disparity issues. we use it heavily, but it is disabled by default.

@jayvdb
Copy link
Contributor Author

jayvdb commented May 26, 2020

Hmm, it sounds like you are saying the current changes in this PR are correct, but only cater for either a read-only or a write-only endpoint. wrt to File/Image, that is quite common, e.g. for sites where image uploading and page creation occurs in Django admin rather than via the API, and/or where uploads occur using separate 'direct upload to S3' apps and not through the standard model serializers.

So some other additional voodoo is needed elsewhere for a read+write file field, but that voodoo depends on a non-default mode COMPONENT_SPLIT_REQUEST?

COMPONENT_SPLIT_REQUEST looks global. If it is global and cant be enabled by default yet, this PR might need to fail gracefully somehow, if possible. If the field is required for both read+write, I guess a hard fail is needed until the user enables COMPONENT_SPLIT_REQUEST.

@tfranzel
Copy link
Owner

yes almost. let me clarify: this does not cater exclusively to readOnly writeOnly. What it does is simply building the component twice (request/response). while building the component again, any field can differ. its a rather simple idea actually.

this feature will likely stay off by default. component demultiplexing is mainly targeted to help client generation. I consider it a "hack" to take complexity out of the schema by making it more explicit. it is a global feature.

imho, graceful would be to ignore the request binary part if split is turned off. that is the best we can do. the only issue that it is a problem here to make "multipart" the first option.

@tfranzel tfranzel merged commit 573cf09 into tfranzel:master May 27, 2020
@tfranzel
Copy link
Owner

have a look at this. i added a question to the FAQ for file fields to explain how to use this.

on split: request: BINARY and response: URI/STR
fallback on non-split: request and response: URI/STR

@jayvdb
Copy link
Contributor Author

jayvdb commented Jun 1, 2020

I found that enabling COMPONENT_SPLIT_REQUEST resulted in a lot of unnecessarily duplicated components for the request/response where they appear to be identical. I didnt study it closely, so I could have missed some reason why they are being split.

@tfranzel
Copy link
Owner

tfranzel commented Jun 1, 2020

COMPONENT_SPLIT_REQUEST is basically tailored to APIs that are mainly asymmetric. When you have mostly read/write fields, the components do not differ much.

It would be possible to merge identical components together again, but that would be the same behavior you criticized in "disappearing enums", i.e. change one field and the "Request" component appears or disappears from the schema because they are are identical.

you can't have a minimal set of components and a stable set at the same time. i suppose you can't have you cake and eat it.

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.

None yet

2 participants