Skip to content

Add a check for 'required' before deletion - #6920

Closed
haiyangdeperci wants to merge 2 commits into
encode:masterfrom
haiyangdeperci:master
Closed

Add a check for 'required' before deletion#6920
haiyangdeperci wants to merge 2 commits into
encode:masterfrom
haiyangdeperci:master

Conversation

@haiyangdeperci

Copy link
Copy Markdown

Description

This pull request attempts to resolve a KeyError that arises when no required fields are present. It checks if there is a 'required' key in the content of the request as described by #6918.

@xordoquy

xordoquy commented Sep 9, 2019

Copy link
Copy Markdown
Contributor

Could you elaborate about "a KeyError that arises when no required fields are present" ?
PATCH request bypasses the required fields so the current implementation looks legit to me.

@haiyangdeperci

haiyangdeperci commented Sep 9, 2019

Copy link
Copy Markdown
Author

@xordoquy The problem is that the content dictionary might not necessarily contain the key named 'required'. Thus, if that's the case, in the current implementation one gets a KeyError in del content['required'] because there is no such key.

Alternatively, you may change that portion of the code in the following way:

try:
delcontent['required']
exceptKeyError:
pass

Comment threadrest_framework/schemas/openapi.py Outdated
Co-Authored-By: Xavier Ordoquy <xordoquy@linovia.com>
# No required fields for PATCH
if method == 'PATCH':
if method == 'PATCH' and 'required' in content:
del content['required']

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ifmethod=='PATCH':
content.pop('required', None)

@carltongibsoncarltongibson left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Hi @guimunarolo — Could you add a test case (abstracting from the issue description) showing how the issue arises? Thanks!

@guimunarolo

guimunarolo commented Sep 20, 2019

Copy link
Copy Markdown
Contributor

Hi @guimunarolo — Could you add a test case (abstracting from the issue description) showing how the issue arises? Thanks!

Hey @carltongibson - I just open a PR fixing this #6944

@carlfarrington

Copy link
Copy Markdown
Contributor

Hi @guimunarolo — Could you add a test case (abstracting from the issue description) showing how the issue arises? Thanks!

Hey @carlfarrington - I just open a PR fixing this #6944

Hi, sorry, I think you meant @carltongibson :)

@guimunarolo

Copy link
Copy Markdown
Contributor

Hi @guimunarolo — Could you add a test case (abstracting from the issue description) showing how the issue arises? Thanks!

Hey @carlfarrington - I just open a PR fixing this #6944

Hi, sorry, I think you meant @carltongibson :)

mb 😅

@carltongibson

Copy link
Copy Markdown
Collaborator

Closing in favour of #6944. Thanks all.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

6 participants

@haiyangdeperci@xordoquy@guimunarolo@carlfarrington@carltongibson@rpkilby