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(api): Actually order body params by required vs optional #57244

Closed
wants to merge 2 commits into from

Conversation

schew2381
Copy link
Member

@schew2381 schew2381 commented Sep 29, 2023

Closed in favor of manual dereferencing in: #57639
There were concerns about the jsonrefs package and it's actually quite simple to manually dereference

Previously, this ordering silently failed because the request body schema we were fetching only had a reference to it's actual key-value pairs. At the very start of the hook, we now replace all references with a lazy lookup object, which will replace the reference with the real key-value pairs when called upon.

See jsonref documentation for more info

@schew2381 schew2381 requested a review from a team as a code owner September 29, 2023 21:21
@schew2381 schew2381 self-assigned this Sep 29, 2023
@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Sep 29, 2023
Copy link
Member

@sentaur-athena sentaur-athena left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neat!

@JoshFerge
Copy link
Member

I would also be interesting explroring dropping dereferencing. i'm not sure if we actually have to do that.

@schew2381
Copy link
Member Author

I would also be interesting explroring dropping dereferencing. i'm not sure if we actually have to do that.

Is it drf-spec that's automatically dereferencing or something else internally?

@JoshFerge
Copy link
Member

here:

return JsonRefs.resolveRefs(root, options).then(

You could also consider modifying that code, it may be easier to do that than to add a new python library. and yes, can see if it would make sense not to derference the JSON.

@schew2381 schew2381 marked this pull request as draft October 3, 2023 07:18
@schew2381 schew2381 closed this Oct 5, 2023
@schew2381 schew2381 added the Do Not Merge Don't merge label Oct 5, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Oct 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Do Not Merge Don't merge Scope: Backend Automatically applied to PRs that change backend components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants