Skip to content

Conversation

@yelizhenden-mdb
Copy link
Collaborator

@yelizhenden-mdb yelizhenden-mdb commented Mar 16, 2025

Proposed changes

Jira ticket: CLOUDP-304053

Refinements:

  • If the Get response and Create Request both references a schema and if their names are equal, it means that they are equal and no need to proceed with deep equality check.
  • If one of them or two of them does not reference to a schema, deep equality check will be made
  • Before deep equality check, title, description and required fields will be removed
  • Deep equality check:
    -- Properties will be compared based on their names and types
    -- Resolved oneOf, allOf, anyOf, and discriminator field values are expected to be completely same

Checklist

  • I have signed the MongoDB CLA
  • I have added tests that prove my fix is effective or that my feature works

Changes to Spectral

  • I have read the README file for Spectral Updates

Further comments

@yelizhenden-mdb yelizhenden-mdb marked this pull request as ready for review March 16, 2025 23:35
@yelizhenden-mdb yelizhenden-mdb requested a review from a team as a code owner March 16, 2025 23:35
@wtrocki
Copy link
Member

wtrocki commented Mar 17, 2025

Resolved oneOf, allOf, anyOf, and discriminator field values are expected to be completely same

Super important assumption! We need to have it across all rules!

##### Implementation details
Validation checks the POST method for resource collection paths.
- Validation ignores resources without a Get method.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- Validation ignores resources without a Get method.
- Validation ignores resources without a GET method.

Copy link
Member

Choose a reason for hiding this comment

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

Seems like wider issue. Can be done across all the docs.

Comment on lines 87 to 93
if (postRequestSchemaRef && getResponseSchemaRef) {
const postRequestSchemaName = getSchemaNameFromRef(postRequestSchemaRef);
const getResponseSchemaName = getSchemaNameFromRef(getResponseSchemaRef);
if (postRequestSchemaName === getResponseSchemaName) {
return [];
}
}
Copy link
Member

@wtrocki wtrocki Mar 17, 2025

Choose a reason for hiding this comment

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

Why not compare all the refs instead.

Suggested change
if (postRequestSchemaRef && getResponseSchemaRef) {
const postRequestSchemaName = getSchemaNameFromRef(postRequestSchemaRef);
const getResponseSchemaName = getSchemaNameFromRef(getResponseSchemaRef);
if (postRequestSchemaName === getResponseSchemaName) {
return [];
}
}
if (postRequestSchemaRef && getResponseSchemaRef) {
if (postRequestSchemaRef === getResponseSchemaRef) {
return [];
}
}

Copy link
Member

Choose a reason for hiding this comment

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

Applied on your behalf as this is minor correction. We can restore it if that is an issue.

Copy link
Member

Choose a reason for hiding this comment

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

There are multiple ways to define components in openapi thus relying on last part of the reference is not reliable:

https://swagger.io/docs/specification/v3_0/components

return removePropertyByFlag(schema, 'writeOnly');
export function removeRequestProperties(schema) {
let result = removePropertiesByFlag(schema, 'writeOnly');
return removePropertyKeys(result, 'title', 'description', 'required');
Copy link
Member

@wtrocki wtrocki Mar 17, 2025

Choose a reason for hiding this comment

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

Not blocking/General comment:

hardcoding filtered properties in helper method can be seen as technical debt. Helper should get those values injected as array from config or caller. Otherwise we might be seeing ourselves creating urgent PR's to add more properties etc. over the time.

Copy link
Member

@wtrocki wtrocki left a comment

Choose a reason for hiding this comment

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

My commit only fixed formatting.

@wtrocki wtrocki merged commit 8567812 into main Mar 17, 2025
13 checks passed
@wtrocki wtrocki deleted the CLOUDP-304053 branch March 17, 2025 11:06
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