Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(schema): Deprecated setting schema paths to primitive values #12832

Merged
merged 4 commits into from Feb 4, 2023

Conversation

lpizzinidev
Copy link
Contributor

@lpizzinidev lpizzinidev commented Dec 24, 2022

fix #7558

Summary
Prevent setting a Schema path to a boolean value by throwing an error.
The issue refers to primitive types in general, let me know if the check needs to be extended to other types or refactored in some way.

@hasezoey
Copy link
Collaborator

hasezoey commented Jan 7, 2023

i am not too familiar with the code in question, but couldnt we just check !(typeof type === "function" || typeof type === "object") (or just remove the function check if evaluated before the error) or just simply try to check against Schema.Types.* first, then all primitive constructors (like String, Buffer, etc) in a if-else or via a tracking variable and if none of them matches give a warning (/ throw a error in a new major)?

also you may want to change it to deprecation warning instead of error for the 6.x branch and a error for 7.x (next major) or directly target the 7.x branch

@lpizzinidev lpizzinidev changed the base branch from master to 7.0 January 8, 2023 08:37
Copy link
Collaborator

@AbdelrahmanHafez AbdelrahmanHafez left a comment

Choose a reason for hiding this comment

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

LGTM, thanks. 👍

lib/schema.js Outdated
@@ -615,6 +615,12 @@ Schema.prototype.add = function add(obj, prefix) {
if (key === '_id' && val === false) {
continue;
}
// Deprecate setting schema paths to primitive types (gh-7558)
if (key !== '_id' && typeof val === 'boolean') {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Change this to key !== '_id' && ((typeof val !== 'object' && typeof val !== 'function') || val == null). Not sufficient to check for boolean, we want to disallow all primitives.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 I've added an extra case to handle primitives specified as strings, otherwise, tests like this were failing.

Copy link
Collaborator

@vkarpov15 vkarpov15 left a comment

Choose a reason for hiding this comment

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

Thanks 👍

@vkarpov15 vkarpov15 merged commit 7624196 into Automattic:7.0 Feb 4, 2023
@vkarpov15 vkarpov15 added this to the 7.0 milestone Feb 23, 2023
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.

Deprecate setting schema paths to primitives (like 'foo: false')
4 participants