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] DELETE methods should always return response status code 204 #367

Merged
merged 3 commits into from
Apr 5, 2023

Conversation

irvinesunday
Copy link
Contributor

Fixes #366

This PR:

  • Ensures that delete methods always return status code 204. This is regardless of whether UseSuccessStatusCodeRange is true.
  • Adds test to validate the above.

@sonarqubecloud
Copy link

sonarqubecloud bot commented Apr 5, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

100.0% 100.0% Coverage
45.5% 45.5% Duplication

@github-advanced-security
Copy link

You have successfully added a new SonarCloud configuration ``. As part of the setup process, we have scanned this repository and found no existing alerts. In the future, you will see all code scanning alerts on the repository Security tab.

@irvinesunday irvinesunday merged commit ef33618 into master Apr 5, 2023
@irvinesunday irvinesunday deleted the fix/is/delete-methods-2xx branch April 5, 2023 10:28
@@ -81,7 +81,11 @@ protected override void SetParameters(OpenApiOperation operation)
/// <inheritdoc/>
protected override void SetResponses(OpenApiOperation operation)
{
operation.AddErrorResponses(Context.Settings, true);
// Response for Delete methods should be 204 No Content
OpenApiConvertSettings settings = Context.Settings.Clone();
Copy link
Member

Choose a reason for hiding this comment

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

I'm a bit late to the party here. But if we could avoid allocs for that kind of thing, it'd be better. Could we not check the operation method/verb in the AddErrorResponses method instead? @irvinesunday

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought about it, but I was avoiding growing this function by adding an extra optional parameter (which will only be utilized by delete methods) and conditional checks here. I also wanted to be explicit that we are not using success status code range for delete methods.

Copy link
Member

Choose a reason for hiding this comment

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

thanks for the additional context here. Would it have been another parameter since it's an extension method on operation and thus it has access to the operation properties?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

ah right, I forgot about the dictionary pattern, which is a bit painful to carry the context around. Fine, leave it as is for now, thanks for taking the time to discuss this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No worries.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DELETE methods with no schema have 2XX response.
3 participants