handle deepObject and explode: true #971
Conversation
|
question:
I can continue with this. but first, need to get input from the maintainers ... |
|
@hjacobs @prawn-cake @jmcs this is the PR we talked in chat about ... |
|
It's been a while since I worked on this, but IIRC there was some part of this that needed to happen early in the request flow (maybe before schema validation?). Have you seen this file: https://github.com/zalando/connexion/blob/master/connexion/decorators/uri_parsing.py#L149 ? I think object-level and array-level decoding was happening in that file, and then basic type casting happens later on. |
|
@dtkav it was absolutely helping. i was wondering already why i had to implement param parsing twice. but this file was the key, now i just once disect the object 👍 converted this PR to a real one, to triger the tests |
|
I consider this now ready4review ... |
connexion/decorators/uri_parsing.py
Outdated
| possible_key = groups.group(1) | ||
| param_defn = self.param_defns.get(possible_key) | ||
| if param_defn \ | ||
| and param_defn.get('style', None) == 'deepObject' and param_defn.get('explode', False): |
There was a problem hiding this comment.
check out _resolve_parameter_duplicates(...) and _split(...)
these should get rid of the array unwrapping that you are doing in other places.
There was a problem hiding this comment.
not sure what you mean with that. but i moved the array unnesting now to uri_parsing. would that be sufficient enough?
There was a problem hiding this comment.
Nice work! This will be a great feature to have.
I think it's getting really close! Let me know if you have questions on my feedback.
Also, it's worth noting that I can provide review, but I don't have merge permissions - at the end of the day you need to pass review from a zalando employee.
|
@dtkav thx a lot for the thorough review. i pushed some changes regarding your comments. please check again @hjacobs @jmcs @dtkav @prawn-cake |
| description: object response | ||
| content: | ||
| application/json: | ||
| schema: |
There was a problem hiding this comment.
do you need to set allowReserved: true?
AFAICT your examples send non-percent-encoded nevermind this only applies to parameter values. Not sure if that's also in scope, but it would be a good feature to implement soon as well.[ ] characters
There was a problem hiding this comment.
shall we create a new issue to keep track of this? for us, this won't be needed for now.
drummerwolli
force-pushed
the
fix_exploded_obejcts
branch
from
2f8ad09 to
a0e9038
Compare
|
@ancelotpinto @hjacobs @dtkav @jmcs @prawn-cake can we get this reviewed and merged somehow? we've been running this already from the fork since weeks now, no problems so far. |
|
👍 |
|
thx @hjacobs |
|
The default setting code in the PR isn't scoped properly, so (for example) it will take query defaults and set the request.form to the query defaults. I'll try to put up a minimal working example that shows this behavior and a fix. |
|
#1092 shows the bug also |
Fixes #970 .
Changes proposed in this pull request: