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.

Sign up for GitHub

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

Jump to bottom

consider null as a value #317

Merged
merged 5 commits into from
Mar 1, 2023
Merged

consider null as a value #317

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
fc7eafe
consider null as a value
Freezystem Feb 18, 2023
b8b5e2b
Add new validator option considerNullAsAValue
Freezystem Feb 20, 2023
927e0c4
add documentation about new flag in README.md
Freezystem Feb 20, 2023
fe92395
should not replace null when field is not explicitly nullable
Freezystem Feb 20, 2023
9e81f1a
add specific test for default value
Freezystem Feb 20, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions lib/validator.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ class Validator {
let handleNoValue;

let skipUndefinedValue = rule.schema.optional === true || rule.schema.type === "forbidden";
let skipNullValue = rule.schema.optional === true || rule.schema.nullable === true || rule.schema.type === "forbidden";
let skipNullValue = rule.schema.nullable !== false || rule.schema.type === "forbidden";

if (rule.schema.default != null) {
// We should set default-value when value is undefined or null, not skip! (Except when null is allowed)
Expand Down Expand Up @@ -504,11 +504,11 @@ class Validator {
schema.optional = true;

// Check 'nullable' flag
const isNullable = schema.rules
const isNotNullable = schema.rules
.map(s => this.getRuleFromSchema(s))
.every(rule => rule.schema.nullable === true);
if (isNullable)
schema.nullable = true;
.every(rule => rule.schema.nullable === false);
if (isNotNullable)
schema.nullable = false;
}

if (schema.$$type) {
Expand Down
2 changes: 1 addition & 1 deletion test/rules/any.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ describe("Test rule: any", () => {
it("should give back true anyway", () => {
const check = v.compile({ $$root: true, type: "any" });

expect(check(null)).toEqual([{ type: "required", actual: null, message: "The '' field is required." }]);
expect(check(null)).toEqual(true);
Copy link
Owner

Choose a reason for hiding this comment

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

It's not good that this case is changed. As you wrote in the PR name null is a value, so the required rule should accept it. It should be true if nullable: false

Copy link
Contributor Author

@Freezystem Freezystem Feb 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Actually I'm not sure I'm following you on this point.
In my mind, the case seems good:
Here we consider that nullable is true by default as it's not explicitly set to false.
Given that any accept any type and with $$root modifier any accept any value, it can be anything but undefined which is not a value.
So when any type is used, null should be a valid value.
At the contrary, if nullable=false is set, any could be anything expect null or undefined.

Unless we consider the default value of nullable to false but I'm scared that it will defeat the purpose and implies bigger changes to the test suite.

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah, you are right, sorry I was wrong, I changed the lines in my mind. By the way, I would like to avoid the breaking change....breaking FV generates breaking changes in Moleculer, it means, we can't use it only just in the next version 0.15

To cover similar issue, we add a global option which switch the old/new behaviour. Can we do something similar here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No worries, I also had trouble getting my mind around it at first 😄
I'll add to this PR a global setting to be able to switch between old and new behaviour.

Copy link
Contributor Author

@Freezystem Freezystem Feb 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I've also added some documentation and tests that seems relevant for specific edge cases.

4 cases are tested when considerNullAsAValue option is set to true:

  • field is required and nullable, default value will NOT apply. (default)
  • field is required but NOT nullable, default value will apply if field is null or undefined.
  • field is optional and nullable, default will NOT apply.
  • field is optional but NOT nullable, default value will apply if field is null or undefined.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @icebob, feel free to tell me if you need me to do some other changes.

Copy link
Owner

Choose a reason for hiding this comment

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

Thanks, sorry for late, I'm checking...

expect(check(undefined)).toEqual([{ type: "required", actual: undefined, message: "The '' field is required." }]);
expect(check(0)).toEqual(true);
expect(check(1)).toEqual(true);
Expand Down
2 changes: 1 addition & 1 deletion test/rules/array.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,7 +223,7 @@ describe("Test rule: array", () => {
it ("should not convert into array if null or undefined", () => {
// Null check
const value = { data: null };
expect(check(value)).toEqual([{ type: "required", field: "data", actual: null, message: "The 'data' field is required." }]);
expect(check(value)).toEqual(true);
expect(value.data).toEqual(null);
// Undefined check
const value2 = { data: undefined };
Expand Down
20 changes: 20 additions & 0 deletions test/typescript/integration.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1198,6 +1198,26 @@ describe('TypeScript Definitions', () => {
expect(check(o2)).toBe(true);
expect(o2.foo).toBe(5);
});

it("should not accept null value even if optional", () => {
const schema = { foo: { type: "number", nullable: false, optional: true } };
const check = v.compile(schema);

expect(check({ foo: 3 })).toBe(true);
expect(check({ foo: undefined })).toBe(true);
expect(check({})).toBe(true);
expect(check({ foo: null })).toEqual([{"actual": null, "field": "foo", "message": "The 'foo' field is required.", "type": "required"}]);
});

it("should accept null as value", () => {
const schema = {foo: {type: "number", nullable: true, optional: false}};
const check = v.compile(schema);

expect(check({ foo: 3 })).toBe(true);
expect(check({ foo: undefined })).toEqual([{"actual": undefined, "field": "foo", "message": "The 'foo' field is required.", "type": "required"}]);
expect(check({})).toEqual([{"actual": undefined, "field": "foo", "message": "The 'foo' field is required.", "type": "required"}]);
expect(check({ foo: null })).toBe(true);
});
});
})

Expand Down
2 changes: 1 addition & 1 deletion test/typescript/rules/any.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ describe('TypeScript Definitions', () => {
it('should give back true anyway', () => {
const check = v.compile({ $$root: true, type: 'any' });

expect(check(null)).toEqual([{ type: 'required', actual: null, message: 'The \'\' field is required.' }]);
expect(check(null)).toEqual(true);
expect(check(undefined)).toEqual([{ type: 'required', actual: undefined, message: 'The \'\' field is required.' }]);
expect(check(0)).toEqual(true);
expect(check(1)).toEqual(true);
Expand Down
2 changes: 1 addition & 1 deletion test/typescript/rules/array.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,7 @@ describe('TypeScript Definitions', () => {
it ("should not convert into array if null or undefined", () => {
// Null check
const value = { data: null };
expect(check(value)).toEqual([{ type: "required", field: "data", actual: null, message: "The 'data' field is required." }]);
expect(check(value)).toEqual(true);
expect(value.data).toEqual(null);
// Undefined check
const value2 = { data: undefined };
Expand Down