Skip to content

Conversation

gribnoysup
Copy link
Collaborator

Seems like this was an unintentional breaking change in data-service that resulted in compass-schema-validation not being able to update schema. The schema-validation plugin saveValidation method calls updateCollection like this, just providing a database name in namespace and passing collMod separately:

dataService.updateCollection(
namespace.database,
{
collMod: namespace.collection,
validator: savedValidation.validator,
validationAction: savedValidation.validationAction,
validationLevel: savedValidation.validationLevel
},

Before latest changes in data-service, the merge was happening in a way where collectionName derived from provided namespace was basically treated as a default value, not an override for whatever was provided in the options. This PR restores this behavior

Copy link
Contributor

@rose-m rose-m left a comment

Choose a reason for hiding this comment

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

Oh nooooo - I'm sorry! And I thought I had eliminated all the places where I messed that order up :(

Copy link
Collaborator

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

Maybe leave a comment to prevent somebody from reverting this again? Because the "broken" code would definitely have been the one that would have looked more correct to me

…s full namespace to the updateCommand; Add comments explaining why the order of args is important
@gribnoysup
Copy link
Collaborator Author

@addaleax yeah, fair point. Seems like this updateCommand call in schema-validation was the only one that wasn't passing the full namespace to the method, so I changed that and removed collMod from the passed flags. Saying that, I learned something new today! 😄 The order of keys in command passed to the server is important and because of that collMod needs to be the first property there. So I'm keeping the change in native-client, but I made flag types stricter and added a comment explaining what's going on. How does it look now?

@gribnoysup gribnoysup changed the title fix(mongodb-data-service): Change merge order to restore pre-typescript method behavior fix(@mongodb-js/compass-schema-validation): Pass full namespace to updateCollection instead of passing collMod option Aug 27, 2021
@gribnoysup gribnoysup merged commit 1676135 into main Aug 27, 2021
@gribnoysup gribnoysup deleted the change-merge-order branch August 27, 2021 16:35
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.

3 participants