longshotlabs / simpl-schema

A JavaScript schema validation package that supports direct validation of MongoDB update modifier objects
https://www.npmjs.com/package/simpl-schema
MIT License
560 stars 115 forks source link

Optional flag not working for sub-schemas #475

Closed vpalos closed 9 months ago

vpalos commented 1 year ago

In version 3.4.0 of the package, setting optional: true on an object property which has its own required sub-fields, will enforce the object's sub-fields as required, even if the entire object is missing (which should be allowed, since the object itself is marked as optional).

See example below:

const addressSchema = new Schema({
    city: String,
});

const schema = new Schema({
    name: String,
    homeAddress: addressSchema,
    billingAddress: {
        type: addressSchema,
        optional: true,
    },
});

const context = schema.newContext();
context.validate({
  // EMPTY OBJECT
});

console.error(context.validationErrors());

The console output will show these errors:

0:{name: 'name', type: 'required', value: undefined} // OK
1:{name: 'homeAddress', type: 'required', value: undefined} // OK
2:{name: 'homeAddress.city', type: 'required', value: undefined} // OK
3:{name: 'billingAddress.city', type: 'required', value: undefined} // THIS IS WRONG!!!

Expected behavior: The last error should not happen, of course because the entire billingAddress object is set as optional: true, therefore it's internal properties should only be enforced if (and only if) the object exists in the first place.

This expected behavior is also described in the documentation: https://github.com/longshotlabs/simpl-schema#optional

github-actions[bot] commented 1 year ago

Thank you for submitting an issue!

If this is a bug report, please be sure to include, at minimum, example code showing a small schema and any necessary calls with all their arguments, which will reproduce the issue. Even better, you can link to a saved online code editor example, where anyone can immediately run the code and see the issue.

If you are requesting a feature, include a code example of how you imagine it working if it were implemented.

If you need to edit your issue description, click the [...] and choose Edit.

Be patient. This is a free and freely licensed package that I maintain in my spare time. You may get a response in a day, but it could also take a month. If you benefit from this package and would like to see more of my time devoted to it, you can help by sponsoring.

pozylon commented 1 year ago

Same here, had to downgrade to 3.2.0, it's a regression

loliarz commented 1 year ago

I'm using nested objects validation a lot and also got this issue, are there any plans to fix that?

jmdsg commented 1 year ago

Yeah, I am also facing the same issue. Are there any plans?

herchu commented 1 year ago

I came across this issue as well. And after some debugging I found it's a change (a regression in my opinion) between 3.2.0 and 3.3.0.

Here is a sandbox to play with code, where you can switch between simpl-schema versions. I shortened the case to this schema:

const addressSchema = new SimpleSchema({
  city: String,
  street: String
});
const personSchema = new SimpleSchema({
  name: String,
  address: { type: addressSchema, optional: true }
});

Here, a person's address should be optional, but if it appears, it must contain a city and address. An object such as { name: 'Joe' } should pass validation, and it does in 3.2.0, but it does not in 3.3 and 3.4.

We found the issue while doing a round of package updates; we will stick for 3.2.0 for now, but I would appreciate if anyone knows why this was changed -- or otherwise, if there is any way to mark as optional nested objects with mandatory fields (I think that's a good description of what the issue is).

red-meadow commented 1 year ago

We are facing the same issue. Can the change be reversed? @aldeed

robbterr commented 1 year ago

it´s somewhere in the commit e8f91dcd5016b3998cbec425fa77f337b398fc96 for "feat: improved oneOf support" by @znewsham and @aldeed but i can´t find the bug

moonrockfamily commented 1 year ago

it´s somewhere in the commit e8f91dc for "feat: improved oneOf support" by @znewsham and @aldeed but i can´t find the bug

It seems the def.optional reference: https://github.com/longshotlabs/simpl-schema/blob/3e258c550cb056f7e19b08330833591de1d551e7/src/validation/validateField.ts#L318 will never be evaluated as the def reference is only assigned within the for loop: https://github.com/longshotlabs/simpl-schema/blob/3e258c550cb056f7e19b08330833591de1d551e7/src/validation/validateField.ts#L152 resulting in the temporary assignment of an {} empty object even when the definition has specified the object is optional!

moonrockfamily commented 1 year ago

Thought I'd share another gotcha with version 3.2.0 and its dependency on mongo-object 3.0.1 with a 'length' field bug! The below image shows the 'collection' variable value, with a length value and causing the 'while' loop to work away...

image

Kind of stuck! This latest version with the nested 'optional' misbehavior and the older version with its looping 'length' bug... lots of fun.

moonrockfamily commented 1 year ago

I'll open an issue on the mongo-object project for the each functions unexpected looping behavior when processing an object with length property but not really an array. This package's clean function is using mongo-object's removeArrayItems function which uses the mentioned each utility function.

Something like this isArrayLike function may solve it, but I'll need to write a bunch of tests first to contemplate the scenarios thoroughly to ensure the addition of the object has numeric keys condition is desirable.

/**
 * Checks if the given object is array-like, meaning it is not null or undefined, has a finite length, and has numeric keys.
 * @param {Object} obj - The object to check.
 * @returns {boolean} - True if the object is array-like, false otherwise.
 */
function isArrayLike(obj) {
  return obj != null && typeof obj === 'object' && isFinite(obj.length) && obj.length >= 0 && obj.length === Math.floor(obj.length) && obj.length < Number.MAX_SAFE_INTEGER && (obj[0] !== undefined || obj[obj.length-1] !== undefined || Object.keys(obj).length == 0);
}
drone1 commented 10 months ago

Looks like it's been over a year since this was reported -- any plans to fix this? To not fix this? Any update or suggestions for a temporary fix would be greatly appreciated. Thanks.

github-actions[bot] commented 9 months ago

:tada: This issue has been resolved in version 3.4.6 :tada:

The release is available on:

If this makes you happy, please consider becoming a sponsor.

Your semantic-release bot :package::rocket:

aldeed commented 9 months ago

Thanks for your help debugging @moonrockfamily and others. I believe this should be fixed in 3.4.6