CLOUDP-304938: Adds rule xgen-IPA-105-list-method-response-is-get-method-response #535

lovisaberggren · 2025-03-13T12:09:35Z

Proposed changes

Adds rule xgen-IPA-105-list-method-response-is-get-method-response which checks that the paginated List method results reference the same schema as the resource Get method. Also handles version matching, which affected the existing rule xgen-IPA-106-create-method-request-body-is-get-method-response.

New rule xgen-IPA-105-list-method-response-is-get-method-response has 2 violations
Existing rule xgen-IPA-106-create-method-request-body-is-get-method-response has one new violation
I'll do follow-up to add exceptions to both above

Added tests for the rule and unit tests for the functions used to get the responses based on a version.

Jira ticket: CLOUDP-304938

…hod-response

lovisaberggren · 2025-03-14T11:13:34Z

tools/spectral/ipa/rulesets/functions/utils/resourceEvaluation.js

  }
  return path;
 }
-


Moved to methodUtils

lovisaberggren · 2025-03-14T11:14:31Z

tools/spectral/ipa/rulesets/functions/utils/methodUtils.js

+    return null;
+  }
+
+  const versions = Object.keys(response.content);


Added version matching here used in both getResponseOfListMethodByMediaType and getResponseOfGetMethodByMediaType

yelizhenden-mdb · 2025-03-14T11:53:08Z

tools/spectral/ipa/rulesets/IPA-105.yaml

+  xgen-IPA-105-list-method-response-is-get-method-response:
+    description: >-
+      The response body of the List method should consist of the same resource object returned by the Get method. Validation checks that the List method results items reference the same schema as the Get method response.<br/>
+      - Validation applies to List methods for resource collections only<br/>


As far as I understand:

Validation makes schemaName level comparison only

Validation also does not make inline schema comparison

Are these assumptions correct? If so,
List-Get relationship is strict, so I am okay with expecting exactly the same schema.
Do we assume Get response always has schema reference, and should we consider inline schemas?

Validation makes schemaName level comparison only

Yes, it checks that it refers to the exact same schema

Validation also does not make inline schema comparison

The paginated schema can be inline, but the resource schema itself is expected to be a schema ref, if the get doesn't have a ref, there will be a violation. I can add a test to cover this to make sure

yelizhenden-mdb · 2025-03-14T11:57:36Z

tools/spectral/ipa/rulesets/functions/listMethodResponseIsGetMethodResponse.js

+  }
+
+  // Ignore if the List method does not have a response schema
+  const listMethodResponse = getResponseOfListMethodByMediaType(mediaType, resourcePath, oas);


Why do we call this method? Isn't the rule iterating list method response content already?

Good catch, we can just get the response, I'll update 👍

wtrocki · 2025-03-14T12:21:30Z

tools/spectral/ipa/rulesets/IPA-105.yaml

      function: 'eachResourceHasListMethod'
+  xgen-IPA-105-list-method-response-is-get-method-response:
+    description: >-
+      The response body of the List method should consist of the same resource object returned by the Get method. Validation checks that the List method results items reference the same schema as the Get method response.<br/>


That is based on assumption that "list operations" return pagginated objects - with results, total etc.
I seen that one before. Would we have validation and helper to check for that assumption each rule can base on it?

Suggested change

The response body of the List method should consist of the same resource object returned by the Get method. Validation checks that the List method results items reference the same schema as the Get method response.<br/>

The response body of the List method should consist of the same resource object returned by the Get method. Validation checks that the List method response contains items property with reference the same schema as the Get method response.<br/>

I really like that I can see how you interpreted the rule inside the PR. Saves us back and forth with code based discussion.

Yes this one only applies to paginated lists (we ignore if it's not paginated). We will have another IPA to cover pagination in general

@lovisaberggren Yes. I'm aware. My intention for this comment was to add note to the yaml/docs.

tools/spectral/ipa/rulesets/IPA-105.yaml

wtrocki · 2025-03-14T12:30:55Z

tools/spectral/ipa/rulesets/functions/listMethodResponseIsGetMethodResponse.js

+  }
+
+  // Ignore if the List method response is not found or not paginated
+  if (!resolvedListSchema || !schemaIsPaginated(resolvedListSchema)) {


Unrelated to the PR but helper implementation might need additional checks as it now based on weak assumption that property named results means paggination. This logic should be covered in the rule description etc.

e.g "Pagginated property is determined by object returning results property with type array or something like this.

IMHO we should at least check if results type is array
return fields.includes('properties') && Object.keys(schema['properties']).includes('results');

Yeah, I'll add a note in the description, and also add a check that the results is an array

Awesome. Second comment can be follow up jira as you haven't introduced that helper in this PR

yelizhenden-mdb · 2025-03-14T14:09:57Z

tools/spectral/ipa/rulesets/functions/listMethodResponseIsGetMethodResponse.js

+  }
+
+  // Ignore if the List method does not have a response schema
+  const listMethodResponse = oas.paths[resourcePath].get.responses['200'].content[mediaType];


Suggested change

const listMethodResponse = oas.paths[resourcePath].get.responses['200'].content[mediaType];

const listMethodResponse = resolveObject(documentInventory.resolved, path);

yelizhenden-mdb · 2025-03-14T14:13:38Z

tools/spectral/ipa/rulesets/functions/listMethodResponseIsGetMethodResponse.js

+      return [
+        {
+          path,
+          message: `Could not validate that the List method returns the same resource object as the Get method. The Get method does not have a schema.`,


Q: Should we return the same error for the createMethodRequest comparison?

Sure, that makes sense 👍

wtrocki · 2025-03-14T14:40:34Z

tools/spectral/ipa/rulesets/IPA-105.yaml

      function: 'eachResourceHasListMethod'
+  xgen-IPA-105-list-method-response-is-get-method-response:
+    description: >-
+      The response body of the List method should consist of the same resource object returned by the Get method. http://go/ipa/105


Link to IPA is no longer needed as it is already part of the docs and in failure we do reference to the docs in open source repo

Suggested change

The response body of the List method should consist of the same resource object returned by the Get method. http://go/ipa/105

The response body of the List method should consist of the same resource object returned by the Get method.

Meta comment/ Not this PR/Project:

In retrospect we could use public shortener to link to this repo to be kind for non MongoDB use cases.
Technically you could make blog post covering new mongodb validations and then go links will be the only blocker. All the info will be availabile upstream.
This can be addressed once we get our plans around IPA publication done.

wtrocki

LGTM

All comments are nits.

yelizhenden-mdb

LGTM! Thanks!

lovisaberggren added 5 commits March 13, 2025 12:09

CLOUDP-304938: Adds rule xgen-IPA-105-list-method-response-is-get-met…

49653ef

…hod-response

CLOUDP-304938: Fixes

33da4b1

CLOUDP-304938: Merge conflict

e38cdfa

CLOUDP-304938: Prettier

e3c2cc9

CLOUDP-304938: Fix

be555c1

lovisaberggren commented Mar 14, 2025

View reviewed changes

tools/spectral/ipa/rulesets/functions/utils/resourceEvaluation.js

}

return path;

}

Copy link

Collaborator Author

lovisaberggren Mar 14, 2025

Choose a reason for hiding this comment

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

Moved to methodUtils

lovisaberggren commented Mar 14, 2025

View reviewed changes

lovisaberggren marked this pull request as ready for review March 14, 2025 11:17

lovisaberggren requested a review from a team as a code owner March 14, 2025 11:17

yelizhenden-mdb reviewed Mar 14, 2025

View reviewed changes

CLOUDP-304938: Add error for inline get schema

0a65b64

wtrocki reviewed Mar 14, 2025

View reviewed changes

tools/spectral/ipa/rulesets/IPA-105.yaml Outdated Show resolved Hide resolved

wtrocki reviewed Mar 14, 2025

View reviewed changes

tools/spectral/ipa/rulesets/IPA-105.yaml Outdated Show resolved Hide resolved

lovisaberggren added 3 commits March 14, 2025 12:27

CLOUDP-304938: Fix docs

827b98f

Merge branch 'main' into CLOUDP-304938

ade7f67

CLOUDP-304938: Fix docs

226c7a1

wtrocki reviewed Mar 14, 2025

View reviewed changes

lovisaberggren added 4 commits March 14, 2025 12:31

CLOUDP-304938: Fix docs

7f2edee

CLOUDP-304938: Fix

6a3912f

CLOUDP-304938: Clarify pagination

54519bc

CLOUDP-304938: Fix

77cbbf7

yelizhenden-mdb reviewed Mar 14, 2025

View reviewed changes

CLOUDP-304938: Error for create is get rule missing get schema

18f9ffd

wtrocki reviewed Mar 14, 2025

View reviewed changes

wtrocki approved these changes Mar 14, 2025

View reviewed changes

yelizhenden-mdb approved these changes Mar 14, 2025

View reviewed changes

lovisaberggren merged commit 7449c54 into main Mar 14, 2025
13 checks passed

lovisaberggren deleted the CLOUDP-304938 branch March 14, 2025 15:08

	The response body of the List method should consist of the same resource object returned by the Get method. Validation checks that the List method results items reference the same schema as the Get method response.<br/>
	The response body of the List method should consist of the same resource object returned by the Get method. Validation checks that the List method response contains items property with reference the same schema as the Get method response.<br/>

	const listMethodResponse = oas.paths[resourcePath].get.responses['200'].content[mediaType];
	const listMethodResponse = resolveObject(documentInventory.resolved, path);

Uh oh!

CLOUDP-304938: Adds rule xgen-IPA-105-list-method-response-is-get-method-response #535

CLOUDP-304938: Adds rule xgen-IPA-105-list-method-response-is-get-method-response #535

Uh oh!

Conversation

lovisaberggren commented Mar 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yelizhenden-mdb Mar 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

wtrocki Mar 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wtrocki left a comment

Choose a reason for hiding this comment

Uh oh!

yelizhenden-mdb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

lovisaberggren commented Mar 13, 2025 •

edited

Loading

yelizhenden-mdb Mar 14, 2025 •

edited

Loading

wtrocki Mar 14, 2025 •

edited

Loading