-
Notifications
You must be signed in to change notification settings - Fork 24
Add parameters for top-level request body properties #29
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
Conversation
7ce5669
to
2288915
Compare
# If there's a request body build a list of | ||
# parameters from the top-level of keys in the | ||
# request body in case that body is a dictionary. | ||
body_params = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This construction gets a lot more complex with request body parameters but hopefully the comments make things easier to follow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe you can write an example case?
for param in self.required_params: | ||
if param.in_ == "body": | ||
body_params.append(param) | ||
if len(body_params) != 1 or not body_params[0].named_body: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there's more than one body parameter we are called body
@@ -1,24 +0,0 @@ | |||
class {{ component.class_name }}({% if component.is_response %}JSONResponse{% else %}dict{% endif %}): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not doing anything with structured responses yet so deleting this for now. However structured responses can be directly added on to what we have without it being a breaking change (since structured responses will still be accessible like a dict/list)
def expand_refs(x, depth=0): | ||
# Hack to prevent recursive expansion, we only | ||
# really need top-most layers expanded. | ||
if depth > 10: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's some reference loops within requestBody
schemas that needs resolving, for now this is an easy way to ensure we resolve all $ref
instances as far as we need without writing a "better" ref resolver.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you want to prevent circular reference in $ref
? I found this comment about circular ref in OpenAPI.
2288915
to
7631bc8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only point that I raise is about the possible naming conflict between params, body and query. This is because you are passing everything in the constructor, right? Maybe you can define an alternative way, for instance leaving the body as is and providing a builder class to create the body using parameters.
Inspired by @ezimuel I'd like to add named body parameters where possible while preserving the ability to call
body=...
in the free-form body cases.x-codegen-request-body-name
body
if there is any optional parameters or if the body is completely freeform (likeput_schema()
)body
for nowAlso while working on this found that
WorkplaceSearch.indexDocuments
spec likely should havex-codegen-request-body-name
equal todocuments
to matchAppSearch.indexDocuments
parameter name.