WIP: AutoSchema: Inherit DRF AutoSchema #46
Conversation
drf_spectacular/openapi.py
Outdated
| if result.get('properties'): | ||
| # Move 'type' to top | ||
| new = {'type': 'object'} | ||
| new.update(result) |
There was a problem hiding this comment.
this is only needed to minimise changes to the .yml file.
| if '$ref' in result and len(result) > 1: | ||
| return {'allOf': [{'$ref': schema.pop('$ref')}], **schema} | ||
|
|
||
| new = {} |
There was a problem hiding this comment.
this is only needed to minimise changes to the .yml file.
| @@ -161,8 +161,10 @@ components: | |||
| minimum: -1000 | |||
| field_file: | |||
| type: string | |||
| format: binary | |||
There was a problem hiding this comment.
This is a byproduct of using DRF's map_field. I can avoid it if it is problematic.
There was a problem hiding this comment.
Diagnosed and raised as #69
jayvdb
changed the title
|
Currently fails on drf3.10 and fails test_recursion |
Replace custom _map_basic_serializer with _map_field, and invoke super _map_serializer as a fallback. DRF AutoSchema PR 7257 makes all of these fields public, so Spectacular AutoSchema should implement them and use them when appropriate for interoperability. Related to tfranzel#31 Related to tfranzel#45
|
hi! could you comment on the motivation for this change? was something broken, because now it is. we can talk about slight schema changes, but that change raises exceptions and has recursion loops. that inheritance choice was actually on purpose. interoperability is broken on several places as those two implementation diverged significantly. also i fixed the travis integration and reran all PR. apparently sometimes those oauth2 credentials break without anything changing. |
|
https://github.com/openwisp/django-rest-framework-gis/pull/223/files#r418517749 gives a bit of the motivation.
It is a WIP. As mentioned above "Currently fails on drf3.10 and fails test_recursion". I'll attempt to the recursion with proper inheritance, but it might require a dirty hack. drf3.10 is just time and effort that isnt a priority for me ATM while there are other larger problems to solve. IMO yes, it is "broken" to use DRF's |
|
ahh now i understand! ... and that is exactly the reason why DRF's recommended usage of AutoSchema just does not scale. I read in several DRF PR that people should just subclass the AutoSchema to their needs. Problem is that if everybody does that, you get 4 incompatible AutoSchemas for 4 libraries you are using. sounds very brittle to me. i'm afraid your approach also might not scale that well. if i understand correctly, you want to fuse those 2 implementations together via "mixin" inheritance and make spectacular aware of that. the problem i see there is that implementations might break unexpected things in their subclass. that looks very tricky. all i can think of is inventing a kind of AutoSchema adapter for 3rd party lib AutoSchema implementations, so basically composition over inheritance. That would keep multiple implementations separate from each other and other 3rd party libs. what do you think? and btw how is that django-oscar-api coming? |
jayvdb
force-pushed
the
inherit-autoschema
branch
2 times, most recently
from
ba2893a to
6aa61d2
Compare
Codecov Report
@@ Coverage Diff @@
## master #46 +/- ##
==========================================
- Coverage 95.85% 95.32% -0.54%
==========================================
Files 36 36
Lines 2075 2117 +42
==========================================
+ Hits 1989 2018 +29
- Misses 86 99 +13
Continue to review full report at Codecov.
|
| serializer = force_instance(serializer) | ||
| serializer_extension = OpenApiSerializerExtension.get_match(serializer) | ||
|
|
||
| if serializer_extension: | ||
| if serializer_extension and direction: |
There was a problem hiding this comment.
This and direction is optional, and probably needs to be removed.
|
All green. The horrible hack for recursion error isnt permanent - it only isolates that problem until I get to solving it properly.
Sure it can be brittle if(when) they are incompatible, but that is assuming the worst case, and can be solved by Django apps working together, which is done all the time, and is increasing as the Django ecosystem becomes more stable with larger and more inclusive maintainer teams replacing single-developer-maintainer who cant scale/collaborate/endure/etc. Incompatibilities are constantly 'fixed', re-broken, 'fixed' again, etc. And hopefully integration tests added on one or both sides when someone is motivated enough to prevent future breakages.
Right. Multiple inheritance. The PR as-is doesnt quite achieve it wrt drf-gis, but getting close. |
|
I've got it working with jayvdb/django-rest-framework-gis@85983bc ; but did have a few more drf-spectacular changes needed which I'll add to this PR tmr. Worth noting that this approach for GIS currently results in an ugly schema because the DRF-GIS schema doesnt use I still think this PR is useful and important, but it isnt the best path for GIS at the moment. |
|
stale and not the direction we want to go in atm |
Replace custom _map_basic_serializer with _map_field,
and invoke super _map_serializer as a fallback.
DRF AutoSchema PR 7257 makes all of these fields public,
so Spectacular AutoSchema should implement them and use
them when appropriate for interoperability.
Related to #31
Related to #45