Skip to content

Conversation

@yelizhenden-mdb
Copy link
Collaborator

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

Proposed changes

Jira ticket: CLOUDP-304053

This PR implements deep object comparison without third party dependencies, and replace the existing third party solutions

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

"apache-arrow": "^19.0.1",
"dotenv": "^16.4.7",
"eslint-plugin-jest": "^28.10.0",
"lodash": "^4.17.21",
Copy link
Member

Choose a reason for hiding this comment

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

❤️

function: 'createMethodShouldNotHaveQueryParameters'
xgen-IPA-106-create-method-request-body-is-get-method-response:
description: 'The Create method request should be a Get method response. http://go/ipa/106'
description: |
Copy link
Member

Choose a reason for hiding this comment

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

❤️

!isEqual(
omitDeep(postMethodRequestContentPerMediaType.schema, 'readOnly', 'writeOnly'),
omitDeep(getMethodResponseContentPerMediaType.schema, 'readOnly', 'writeOnly')
omitDeep(postMethodRequestContentPerMediaType.schema, ...ignoredValues),
Copy link
Member

Choose a reason for hiding this comment

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

❤️

@wtrocki
Copy link
Member

wtrocki commented Mar 11, 2025

LGTM! Will review fully once put out of draft mode

@yelizhenden-mdb yelizhenden-mdb marked this pull request as ready for review March 11, 2025 15:46
@yelizhenden-mdb yelizhenden-mdb requested a review from a team as a code owner March 11, 2025 15:46
* @param {*} value2 Second value to compare
* @returns {boolean} Whether the values are deeply equal
*/
export function isDeepEqual(value1, value2) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps it could be useful to add some unit tests for this function specifically? So it's covered by a unit test as well as spectral test in action

Copy link
Collaborator

Choose a reason for hiding this comment

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

E.g. /__tests__/utils

Copy link
Member

Choose a reason for hiding this comment

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

Great idea @lovisaberggren Can we create separate jira to have utils (our framework for exceptions and metics) covered by tests. Problem is not specific to this PR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure, let me create one @wtrocki
Included some test cases for the functions defined in compareUtils.js

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks, yeah at least for the more complicated helper functions we should have tests

Copy link
Collaborator

@lovisaberggren lovisaberggren left a comment

Choose a reason for hiding this comment

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

🚀

@yelizhenden-mdb yelizhenden-mdb merged commit b163e0f into main Mar 11, 2025
13 checks passed
@yelizhenden-mdb yelizhenden-mdb deleted the CLOUDP-304053/fix branch March 11, 2025 16:32
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.

4 participants