Skip to content

Conversation

@wtrocki
Copy link
Member

@wtrocki wtrocki commented Mar 4, 2025

Overview

This PR implements a new set of Spectral rules for validating DELETE operations in OpenAPI specifications according to the IPA-108 guidelines. The rules ensure consistent implementation of DELETE endpoints across our APIs.

Features Implemented

Rule Definitions

Created IPA-108.yaml ruleset defining four validation rules for DELETE operations:
- Ensuring DELETE responses are empty (204 No Content)
- Validating that DELETE methods return a 204 status code
- Preventing DELETE methods from having request bodies
- Requiring DELETE methods to include 404 responses for not found resources

Rule Functions

Implemented four function modules that perform the actual validation:

  • deleteMethodResponseShouldBeVoid.js - Validates that 204 responses have no content
  • deleteMethodNoRequestBody.js - Ensures DELETE operations don't include request bodies
  • deleteMethod404Response.js - Checks for required 404 status codes
  • deleteMethod204Response.js - Validates that DELETE operations return 204 status

Test Coverage

Added comprehensive test cases for each rule:

  • Valid configurations to ensure rules don't trigger false positives
  • Invalid configurations to verify rules catch violations
  • Exception handling cases to demonstrate how to bypass rules when needed

Technical Details

  • All rules support exceptions via the x-xgen-IPA-exception extension
  • Rules implement adoption tracking to monitor compliance
  • Error messages include links to documentation (http://go/ipa/108)
  • Path targeting is properly configured to ensure accurate error locations

Related Documentation
IPA-108 guidelines for DELETE operations: http://go/ipa/108

@wtrocki wtrocki requested a review from a team as a code owner March 4, 2025 16:30
severity: error
given: $.paths[*].delete.responses['404']
then:
field: content
Copy link
Member Author

Choose a reason for hiding this comment

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

IMHO not worth to have function here

Copy link
Collaborator

Choose a reason for hiding this comment

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

To be able to collect adoption, we need custom functions for every rule

Copy link
Collaborator

Choose a reason for hiding this comment

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

In addition, error schema checks belongs to IPA-114: Errors. Checking if DELETE method must include 404 response will be fine for the IPA-108:Delete

Copy link
Member Author

Choose a reason for hiding this comment

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

Agree. Possible fix for IPA to move this claim to errors etc.

description: 'DELETE method must return 204 No Content'
message: '{{error}} http://go/ipa/108'
severity: error
given: '$.paths.*'
Copy link
Collaborator

Choose a reason for hiding this comment

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

This can be $.paths[*].delete similar to xgen-IPA-104-get-method-response-code-is-200 rule, the custom function would be much cleaner

description: 'DELETE method must not have request body'
message: '{{error}} http://go/ipa/108'
severity: error
given: '$.paths.*'
Copy link
Collaborator

Choose a reason for hiding this comment

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

This can be $.paths[*].delete, similar to the comment above

description: 'DELETE method must include 404 response'
message: '{{error}} http://go/ipa/108'
severity: error
given: '$.paths.*'
Copy link
Collaborator

Choose a reason for hiding this comment

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

This can be $.paths[*].delete, similar to the comment above

function: deleteMethod404Response

xgen-IPA-108-delete-204-content-type:
description: '204 response must not include content-type header'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you lead me to the IPA guideline mentioning this?

import { collectAdoption, collectAndReturnViolation, collectException } from './utils/collectionUtils.js';

const RULE_NAME = 'xgen-IPA-108-delete-response-should-be-void';
const ERROR_MESSAGES = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This function should only capture NON_EMPTY_RESPONSE case

xgen-IPA-108-delete-response-should-be-void:
description: 'Delete method response must be void http://go/ipa/108'
message: '{{error}} http://go/ipa/108'
severity: error
Copy link
Collaborator

Choose a reason for hiding this comment

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

All the severity levels should be warning

@mongodb mongodb deleted a comment from yelizhenden-mdb Mar 4, 2025
@wtrocki wtrocki changed the title IPA 108 CLOUDP-271997: IPA-108 - Deletion coverage Mar 4, 2025
@andreaangiolillo andreaangiolillo requested a review from Copilot March 5, 2025 09:59
@wtrocki wtrocki force-pushed the IPA-108 branch 5 times, most recently from 40b2bce to a559d88 Compare March 5, 2025 21:57
@wtrocki
Copy link
Member Author

wtrocki commented Mar 5, 2025

@yelizhenden-mdb previous PR were not ready for review. This should work now.
For purpose of simpler review I can split this PR into multiple PRs if needed.

@wtrocki wtrocki closed this Mar 6, 2025
@wtrocki
Copy link
Member Author

wtrocki commented Mar 6, 2025

Closing to experiment with Auto PR creation feature

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.

2 participants