search

Meteor-Community-Packages / meteor-collection2

A Meteor package that extends Mongo.Collection to provide support for specifying a schema and then validating against that schema when inserting and updating.
https://packosphere.com/aldeed/collection2
MIT License
1.02k stars 108 forks source link

Allow setting clean options #434

Closed harryadel closed 2 years ago

harryadel commented 2 years ago

https://github.com/Meteor-Community-Packages/meteor-collection2/issues/432

harryadel commented 2 years ago

I guess it's good to go @Prinzhorn. Any last comments?

harryadel commented 2 years ago

@Prinzhorn Kindly recheck.

Prinzhorn commented 2 years ago

Thanks!

StorytellerCZ commented 2 years ago

LGTM

StorytellerCZ commented 2 years ago

@harryadel would be nice if we could set this per collection or somehow disable it for a specific operation. Now looking at this I think I have a bug in one of my apps where this is trimming a place that should not be trimmed.

Prinzhorn commented 2 years ago

would be nice if we could set this per collection or somehow disable it for a specific operation

Can't you do exactly that already?

Per collection: unless you are re-using a schema, you can pass clean to the SimplSchema constructor https://github.com/longshotlabs/simpl-schema#set-default-options-for-one-schema

Per operation: https://github.com/Meteor-Community-Packages/meteor-collection2#inserting-or-updating-without-cleaning

harryadel commented 2 years ago

@Prinzhorn You can add the updated version to your app now.

@StorytellerCZ huh, that's weird. Will you please open up an issue so we can better track this?

Prinzhorn commented 2 years ago

I just upgraded and unfortunately it does not work as expected and I wonder if it ever did.

Prior to this PR there was a const with the default clean options

https://github.com/Meteor-Community-Packages/meteor-collection2/blob/b0e856db590d78423fec33ed01021ada7404c84a/package/collection2/collection2.js#L18-L24

and for every clean action it was merged together with the clean option defined on the schema level plus the ones passed to this particular call

https://github.com/Meteor-Community-Packages/meteor-collection2/blob/b0e856db590d78423fec33ed01021ada7404c84a/package/collection2/collection2.js#L371-L382

I was assuming schema._cleanOptions could be unset, otherwise (schema._cleanOptions || {}) would not make sense.

However, it is never undefined and simpl-schema already has a set of defaults:

https://github.com/longshotlabs/simpl-schema/blob/ea69d4eb7b4d8cc1151353989444a447bf0abc77/package/lib/SimpleSchema.js#L876-L888

https://github.com/longshotlabs/simpl-schema/blob/ea69d4eb7b4d8cc1151353989444a447bf0abc77/package/lib/SimpleSchema.js#L64-L67

What this means is that defaultCleanOptions (now Collection2.cleanOptions) never did anything. Maybe nobody noticed because the options are identical to the simpl-schema defaults anyway.

What that means is:

  1. It can be entirely removed
  2. What I was hoping for in #432 seems impossible given the way simpl-schema handles the clean options

Are my observation correct?

My use-case would be possible, however, via a different global setting that defaults to {} and is merged after schema._cleanOptions, but I think this entire thing is a mess and it should probably not be made more complex. In the end nobody will understand where the options actually come from.

harryadel commented 2 years ago

@Prinzhorn Great write up and analysis.

Indeed since ...(schema._cleanOptions || {}) is set after ...defaultCleanOptions it overrides any values you'd set in defaultCleanOptions rendering it meaningless.

What this means is that defaultCleanOptions (now Collection2.cleanOptions) never did anything. Maybe nobody noticed because the options are identical to the simpl-schema defaults anyway.

You're right, I guess it was set as precautionary measure in case simple schema values were not present for any reason.

it can be entirely removed What I was hoping for in Expose defaultCleanOptions #432 seems impossible given the way simpl-schema handles the clean options

I might release a fix/feature removing it altogether but I don't think we're in a hurry though.

My use-case would be possible, however, via a different global setting that defaults to {} and is merged after schema._cleanOptions, but I think this entire thing is a mess and it should probably not be made more complex. In the end nobody will understand where the options actually come from.

We end up looping back to the original point. This should be handled within simpl-schema and none of collection2 concern. But hey, we learnt something along the way so thank you for that! <3

harryadel commented 2 years ago

Indeed since ...(schema._cleanOptions || {}) is set after ...defaultCleanOptions it overrides any values you'd set in defaultCleanOptions rendering it meaningless.

This case should've accounted for in the tests and we should've listened for schema clean options instead of collection2 property.

https://github.com/Meteor-Community-Packages/meteor-collection2/blob/master/tests/defaultCleanOptions.tests.js#L15

harryadel commented 2 years ago

@Prinzhorn wouldn't this problem be solved if set defaultCleanOptions after ...(schema._cleanOptions || {}) but we then end up tampering with simpl-schema options. Honestly this whole entire debacle of how to simple schema clean options should work is out of collection2 goals.

Prinzhorn commented 2 years ago

Honestly this whole entire debacle of how to simple schema clean options should work is out of collection2 goals.

I agree, that's what I meant with

but I think this entire thing is a mess and it should probably not be made more complex. In the end nobody will understand where the options actually come from.

Originally I thought since collection2 already has defaultCleanOptions it would be a no-brainer to expose it. But now that I know more about the situation it's not worth the added complexity like you said.