Skip to content

Remove reflected values from validation message #695

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

Merged
merged 4 commits into from
Feb 27, 2023
Merged

Remove reflected values from validation message #695

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
cf053fa
Remove reflected values from validation message
srchase Feb 16, 2023
ad9eb16
Add length back to message
srchase Feb 16, 2023
8f2d13d
Add test for unique items validation message
srchase Feb 16, 2023
bc2c592
Format TS
srchase Feb 18, 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
28 changes: 19 additions & 9 deletions smithy-typescript-ssdk-libs/server-common/src/validation/index.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import {
PatternValidationFailure,
RangeValidationFailure,
RequiredValidationFailure,
UniqueItemsValidationFailure,
} from "./index";

describe("message formatting", () => {
Expand All @@ -32,8 +33,7 @@ describe("message formatting", () => {
path: "/test",
};
expect(generateValidationMessage(failure)).toEqual(
"Value zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz... " +
"(truncated) at '/test' failed to satisfy constraint: Member must satisfy regular expression pattern: ^[a-c]$"
"Value at '/test' failed to satisfy constraint: Member must satisfy regular expression pattern: ^[a-c]$"
);
});
it("omits null values", () => {
Expand All @@ -50,7 +50,7 @@ describe("message formatting", () => {
it("formats required failures", () => {
const failure = new RequiredValidationFailure("/test");
expect(generateValidationMessage(failure)).toEqual(
"Value null at '/test' failed to satisfy constraint: Member must not be null"
"Value at '/test' failed to satisfy constraint: Member must not be null"
);
});
it("formats enum failures", () => {
Expand All @@ -61,7 +61,7 @@ describe("message formatting", () => {
path: "/test",
};
expect(generateValidationMessage(failure)).toEqual(
"Value pear at '/test' failed to satisfy constraint: Member must satisfy enum value set: [apple, banana]"
"Value at '/test' failed to satisfy constraint: Member must satisfy enum value set: [apple, banana]"
);
});
it("formats integer enum failures", () => {
Expand All @@ -72,7 +72,7 @@ describe("message formatting", () => {
path: "/test",
};
expect(generateValidationMessage(failure)).toEqual(
"Value 3 at '/test' failed to satisfy constraint: Member must satisfy enum value set: [1, 2]"
"Value at '/test' failed to satisfy constraint: Member must satisfy enum value set: [1, 2]"
);
});
describe("formats length failures", () => {
Expand Down Expand Up @@ -118,7 +118,7 @@ describe("message formatting", () => {
path: "/test",
};
expect(generateValidationMessage(failure)).toEqual(
"Value xyz at '/test' failed to satisfy constraint: Member must satisfy regular expression pattern: ^[a-c]$"
"Value at '/test' failed to satisfy constraint: Member must satisfy regular expression pattern: ^[a-c]$"
);
});
describe("formats range failures", () => {
Expand All @@ -130,7 +130,7 @@ describe("message formatting", () => {
path: "/test",
};
expect(generateValidationMessage(failure)).toEqual(
"Value 3 at '/test' failed to satisfy constraint: Member must be greater than or equal to 7"
"Value at '/test' failed to satisfy constraint: Member must be greater than or equal to 7"
);
});
it("with only max values", () => {
Expand All @@ -141,7 +141,7 @@ describe("message formatting", () => {
path: "/test",
};
expect(generateValidationMessage(failure)).toEqual(
"Value 3 at '/test' failed to satisfy constraint: Member must be less than or equal to 2"
"Value at '/test' failed to satisfy constraint: Member must be less than or equal to 2"
);
});
it("with min and max values", () => {
Expand All @@ -152,7 +152,17 @@ describe("message formatting", () => {
path: "/test",
};
expect(generateValidationMessage(failure)).toEqual(
"Value 2 at '/test' failed to satisfy constraint: Member must be between 3 and 7, inclusive"
"Value at '/test' failed to satisfy constraint: Member must be between 3 and 7, inclusive"
);
});
it("with unique items", () => {
const failure: UniqueItemsValidationFailure = {
constraintType: "uniqueItems",
failureValue: [5, 9],
path: "/test",
};
expect(generateValidationMessage(failure)).toEqual(
"Value with repeated values at '/test' failed to satisfy constraint: Member must have unique values"
Copy link
Contributor

Choose a reason for hiding this comment

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

This reads odd. Was ok when it was Value [80, 80] with repeated values at '/test'.... But now, probably better to say Value at 'test'... similar to the other messages. Would have to update the protocol tests too.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok to merge this now and update when the protocol test is updated.

);
});
});
Expand Down
18 changes: 2 additions & 16 deletions smithy-typescript-ssdk-libs/server-common/src/validation/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,20 +103,6 @@ export const generateValidationSummary = (failures: readonly ValidationFailure[]
};

export const generateValidationMessage = (failure: ValidationFailure): string => {
let failureValue;
if (failure.constraintType === "required") {
failureValue = "null ";
} else if (failure.failureValue === null) {
failureValue = "";
} else {
const rawFailureValue = failure.failureValue.toString();
if (rawFailureValue.length > 64) {
failureValue = rawFailureValue.substr(0, 49) + "... (truncated) ";
} else {
failureValue = rawFailureValue + " ";
}
}

let prefix = "Value";
let suffix: string;
switch (failure.constraintType) {
Expand All @@ -136,7 +122,7 @@ export const generateValidationMessage = (failure: ValidationFailure): string =>
}
case "length": {
if (failure.failureValue !== null) {
prefix = prefix + " with length";
prefix = prefix + " with length " + failure.failureValue;
}
const min = failure.constraintValues[0];
const max = failure.constraintValues[1];
Expand Down Expand Up @@ -170,5 +156,5 @@ export const generateValidationMessage = (failure: ValidationFailure): string =>
suffix = "must have unique values";
Copy link
Contributor

Choose a reason for hiding this comment

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

looks like we are missing tests for this case. can you add some?

Copy link
Contributor Author

@srchase srchase Feb 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Added here, and to the protocol tests in smithy-lang/smithy#1622.

}
}
return `${prefix} ${failureValue}at '${failure.path}' failed to satisfy constraint: Member ${suffix}`;
return `${prefix} at '${failure.path}' failed to satisfy constraint: Member ${suffix}`;
};