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

[node-core-library] Include support for parsing JSON files with object keys that are reserved words and fix an issue with JSON schema validation. #4736

Merged
merged 3 commits into from
May 25, 2024

Conversation

iclanton
Copy link
Member

Summary

With the upgrade to use ajv for JSON schema validation, we accidentally introduced a regression where using the "uniqueItems": true option in a schema for an array with items that are objects causes an exception to be thrown when calling JsonFile.loadAndValidate. This is because the jju parses JSON files by default does not populate the functions from Object.prototype on parsed JSON objects and ajv uses a.valueOf() === b.valueOf() as part of its deep comparison of objects (via the fast-deep-equal package). The valueOf function isn't populated on objects, so this function call fails.

This PR adds the reserved_keys: 'replace' jju parse option to populate these functions. This also allows parsing of objects with these properties. This introduces an edge case where a JSON object with a "valueOf" key will fail with a similar error in a similar scenario, but this is a much less common case than the currently-broken functionality.

How it was tested

Introduced tests for these scenarios.

@iclanton iclanton force-pushed the fix-issue-with-json-schema branch from 809185c to f19abdc Compare May 23, 2024 22:36
@iclanton iclanton merged commit 05be637 into microsoft:main May 25, 2024
5 checks passed
@iclanton iclanton deleted the fix-issue-with-json-schema branch May 25, 2024 02:04
@@ -571,7 +571,10 @@ export class JsonFile {
}

private static _buildJjuParseOptions(options: IJsonFileParseOptions = {}): jju.ParseOptions {
const parseOptions: jju.ParseOptions = {};
const parseOptions: jju.ParseOptions = {
reserved_keys: 'replace'
Copy link
Collaborator

Choose a reason for hiding this comment

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

JJU's behavior is absolutely bizarre. I assumed they were trying to protect against this kind of mistake:

image

The compiler catches the above case, but neither the compiler nor our lint rules catch [key] access like this:

image

Approach #1: A sensible protection would be to delete the prototype so that JSON parsed objects don't have this problem. That's what I thought JJU was doing. @iclanton noted that it breaks ajv's object analysis however, which doesn't expect deleted prototypes.

Approach #2: What JJU is actually doing: It deletes the prototype but also discards actual keys using reserved names. This results in inexplicable data loss:

image

Approach #3: The replace behavior enabled by this PR is to correctly parse keys like "constructor", allowing the them to shadow the prototype property of the same name. Nothing is actually "replaced", it's just shadowed according to normal JavaScript prototype inheritance.

Empirical experimentation shows that Approach #3 is used by JSON.parse() as well as these 3 highly popular parsing libraries:

  • json5
  • yaml
  • js-yaml

Therefore despite the pitfalls of JavaScript, I agree that PR #4736 is the best and most conventional way to go.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

4 participants