Fix $ref to $id #107

mati-o · 2021-09-29T10:31:52Z

Hi,
I am attempting to fix #55 and microsoft/vscode#92042. I have implemented a simple workaround to detect whether a $ref points to an $id and then I attempt to find the definition behind that $id.

I have implemented it inside findSection of resolveSchemaContent as I though it would be the simplest low-level solution. I have also added some unit tests. Everything works just as before - all tests pass!

I'll be more than happy to see this PR accepted, let me know if anything else is required.

ghost · 2021-09-29T10:32:04Z

All CLA requirements met.

KapitanOczywisty · 2021-09-29T11:50:21Z

$id could be used in any place, not only in the definitions, even after referencing node. Allowing to use $id only in definitions is a step in the right direction, but still very limited upgrade.

I'd probably in resolveRefs function collect all $id and unresolved $refs (when $id occurs after $ref) to resolve them after walking all nodes.

{
  "type": "object",
  "properties": {
    "p1": {
      "$ref": "#hello"
    },
    "p2": {
      "$ref": "#world"
    },
    "p3": {
      "id": "#hello",
      "type": "string",
      "const": "hello"
    },
    "p4": {
      "type": "object",
      "properties": {
        "deep": {
          "$id": "#world",
          "type": "string",
          "const": "world"
        }
      },
      "additionalProperties": false
    }
  },
  "additionalProperties": false
}

Also there should be a check with node referencing their parent, to test recursion:

{
  "type": "object",
  "definitions": {
    "foo": {
      "id": "#foo",
      "type": "object",
      "properties": {
        "bar": {
          "type": "string",
          "const": "hello"
        },
        "foo": {
          "$ref": "#foo"
        }
      },
      "additionalProperties": false
    }
  },
  "properties": {
    "foo": {
      "$ref": "#foo"
    }
  },
  "additionalProperties": false
}

{
  "foo": {
    "foo": {
      "foo": {
        "bar": "hello"
      }
    }
  }
}

And lastly I'd add some test when $ref is referencing not existing $id to test correct error handling.

mati-o · 2021-09-29T12:25:52Z

I'll take a look later, it won't hurt to add that test. As far as supporting $id everywhere, while true, I have supported definitions since afaik it's a common practice. I'll try to see how it is possible to implement with minimal performance penalty, probably like you said - collect them during resolution. Hope it won't be complicated 😅

mati-o · 2021-10-01T16:31:20Z

src/services/jsonSchemaService.ts

+		const merge = (target: JSONSchema, section: any): void => {
+			for (const key in section) {
+				if (section.hasOwnProperty(key) && !target.hasOwnProperty(key)) {
+					(<any>target)[key] = section[key];
+				}
+			}
+		};
+
+		const mergeByJsonPointer = (target: JSONSchema, sourceRoot: JSONSchema, sourceURI: string, refSegment: string | undefined): void => {
 			const path = refSegment ? decodeURIComponent(refSegment) : undefined;
 			const section = findSection(sourceRoot, path);
 			if (section) {
-				for (const key in section) {
-					if (section.hasOwnProperty(key) && !target.hasOwnProperty(key)) {
-						(<any>target)[key] = section[key];
-					}
-				}
+				merge(target, section);
 			} else {
 				resolveErrors.push(localize('json.schema.invalidref', '$ref \'{0}\' in \'{1}\' can not be resolved.', path, sourceURI));
 			}
 		};


I have refactored merge to mergeByJsonPointer which calls findSection and the actual merge which just copies the properties over. I did it since when I resolve a subschema, I don't need to look for it since I have the object in hand, but merge is still required.

mati-o · 2021-10-01T16:34:13Z

src/services/jsonSchemaService.ts

+		const pendingSubSchemas: Record<string, JSONSchema[]> = {};
+		const resolvedSubSchemas: Record<string, JSONSchema> = {};
+
+		const tryMergeSubSchema = (uri: string, target: JSONSchema) : boolean => {
+			if (resolvedSubSchemas[uri]) { // subschema is resolved, merge it to the target
+				merge(target, resolvedSubSchemas[uri]);
+				return true; // return success
+			} else { // This subschema has not been resolved yet
+				if (!pendingSubSchemas[uri]) {
+					pendingSubSchemas[uri] = [];
+				}
+
+				// Remember the target to merge later once resolved
+				pendingSubSchemas[uri].push(target);
+				return false; // return failure - merge didn't occur
+			}
+		};
+


This is the heart of the solution. It basically keeps two maps for subschema references. One for those subschemas that we may or may not encounter later and the latter for those subschemas we have resolved.

The tryMergeSubSchema merges already resolved subschemas into the targets that $ref them, or keeps a record of the target object against the full URI of the subschema for later use.

mati-o · 2021-10-01T16:35:41Z

src/services/jsonSchemaService.ts

+			const handleId = (next: JSONSchema) => {
+				// TODO figure out should loops be preventse
+				const id = next.$id || next.id;
+				if (typeof id === 'string' && id.charAt(0) === '#') {
+					delete next.$id;
+					delete next.id;
+					// Use a blank separator, as the $id already has the '#'
+					const fullId = reconstructRefURI(parentSchemaURL, id, '');
+
+					if (!resolvedSubSchemas[fullId]) {
+						// This is the place we fill the blanks in
+						resolvedSubSchemas[fullId] = next;
+					} else if (resolvedSubSchemas[fullId] !== next) { 
+						// Duplicate may occur in recursive $refs, but as long as they are the same object
+						// it's ok, otherwise report and error
+						resolveErrors.push(localize('json.schema.duplicateid', 'Duplicate id declaration: \'{0}\'', id));
+					}
+
+					// Resolve all pending requests and cleanup the queue list
+					if (pendingSubSchemas[fullId]) {
+						while (pendingSubSchemas[fullId].length) {
+							const target = pendingSubSchemas[fullId].shift();
+							merge(target!, next);
+						}
+
+						delete pendingSubSchemas[fullId];
+					}
+				}
+			};
+


This function resolves the actual subschema, whenever it encounters an $id or id in an object. It keeps a record of the full URI and the object and resolves any pending requests for that subschema.

mati-o · 2021-10-01T16:38:43Z

src/services/jsonSchemaService.ts

+
+				// A placeholder promise that might execute later a ref resolution for the newly resolved schema
+				let externalLinkPromise: Thenable<any> = Promise.resolve(true);
+				if(!isSubSchemaRef(refSegment))  {
+					// This is not a sub schema, merge the regular way
+					mergeByJsonPointer(node, unresolvedSchema.schema, uri, refSegment);
+				} else {
+					// This is a reference to a subschema
+					const fullId = reconstructRefURI(uri, refSegment);
+
+					if (!tryMergeSubSchema(fullId, node)) {
+						// We weren't able to merge currently so we'll try to resolve this schema first to obtain subschemas
+						// that could be missed
+						externalLinkPromise = resolveRefs(unresolvedSchema.schema, unresolvedSchema.schema, uri, referencedHandle.dependencies);
+					}
+				}
+				return externalLinkPromise.then(() => resolveRefs(node, unresolvedSchema.schema, uri, referencedHandle.dependencies));


In case we have referenced a subschema in an external schema i.e schema1 -> $ref -> schema2#foo, we'll need to find schema2's #foo so we trigger schema2 resolution eagerly using that externalLinkPromise.

mati-o · 2021-10-01T16:39:42Z

I think I have figured this out! See the comments in the code too, hope it is clear enough. Let me know if more tests are needed

src/test/schema.test.ts

aeschli · 2021-11-12T17:17:44Z

@mati-o Thanks a lot for the PR, it looks good!
I'm quite new to the feature but read https://json-schema.org/understanding-json-schema/structuring.html

is $anchor the same as $id? Do we need to support it for refs?
a guess is more work needed for bundling?

mati-o · 2021-11-13T11:35:39Z

is $anchor the same as $id? Do we need to support it for refs?

Apparently it is, never seen one using this though in my practice. Guess it could be addressed at some point later.

a guess is more work needed for bundling?

I've never seen this form of schema but it indeed requires some effort. In our regular $ref case, we could split everything using the # character easily. However in this case it's trickier to know which / is part of the original URI and which isn't, without some relative URL code. I think it would be better to wait for someone to complain about it :)

aeschli · 2021-11-15T15:06:35Z

I pushed two tests that show a problem when resolving $id across schemas.

The problem is when resolving schemas the $id nodes are removed. They can't be found anymore when the same schema is used a second time.
A solution would be to build up a map of id to node that we remember along with the resolved (or unresolved schema).

mati-o · 2021-11-15T15:20:55Z

I pushed two tests that show a problem when resolving $id across schemas.

The problem is when resolving schemas the $id nodes are removed. They can't be found anymore when the same schema is used a second time.

A solution would be to build up a map of id to node that we remember along with the resolved (or unresolved schema).

I'll give it a look towards the end of the ~~week~~ day :), sounds like an easy fix

mati-o · 2021-11-15T20:08:53Z

src/services/jsonSchemaService.ts

 			while (toWalk.length) {
 				const next = toWalk.pop()!;
 				if (seen.indexOf(next) >= 0) {
 					continue;
 				}
 				seen.push(next);
+				handleId(next);


@aeschli I was thinking about the two tests you have added and since I already had a map from the full URI to the full schema, it got me thinking. The root cause of the problem was that the handleRef function was called before handleId, thus all the schemas with $ref and $id together were missing that $ref. By simply changing the order between them I have managed to solve these two cases without compromising the rest (luckily).

We still need to remember the full URI to schema mapping also with the resolved schema. Resolving happens on the unresolved schema nodes, so we don't have the information afterwards anymore when the schema is used again.
I added a test case for that: Resolving external $ref to already resolved schema

aeschli · 2021-11-16T07:56:50Z

src/services/jsonSchemaService.ts

 			while (toWalk.length) {
 				const next = toWalk.pop()!;
 				if (seen.indexOf(next) >= 0) {
 					continue;
 				}
 				seen.push(next);
+				handleId(next);


We still need to remember the full URI to schema mapping also with the resolved schema. Resolving happens on the unresolved schema nodes, so we don't have the information afterwards anymore when the schema is used again.
I added a test case for that: Resolving external $ref to already resolved schema

aeschli · 2021-11-29T14:52:09Z

I pushed a change to keep the map of resolved ids in the schema handle.

Sorry I had to 'force push' as merge always ended up with too many changes.

aeschli · 2021-11-29T15:06:13Z

I think the current state is ready for testing and useful. so I'm pushing the PR.
We can test the new feature in the upcoming test week.

@mati-o Thanks for all your help!

Of course, there's still some gaps to fill. What comes to my mind:

More tests, in particular from the JSON schema test suite: See Consider using the official test suite? #102
Support for anchor
Support for bundling
No need to resolve the full schema to find an anchor:

vscode-json-languageservice/src/services/jsonSchemaService.ts

Line 519 in 9d65fa7

// to improve: it would be enough to find the nodes, no need to resolve the full schema

if you are interested in continuing to help, that would be fantastic! Let's continue in a new PR then.

mati-o · 2021-12-01T16:51:39Z

Thanks for merging! Glad I could help. I've been busy in the past two weeks so sorry for not being able to assist. I hope to find time in the near future to assist more

michalchar · 2021-12-19T10:59:04Z

src/services/jsonSchemaService.ts

-								tryMergeSubSchema(fullId, next);
+							const id = segments[1];
+							if (id !== undefined && isSubSchemaRef(id)) { // A $ref to a sub-schema with an $id (i.e #hello)
+								tryMergeSubSchema(next, id, handle);


@aeschli Pay attention you've replaced parentSchemaURL with handle.
Therefore local refs in external schemas wont work.

Thanks, created #121

mati-o added 3 commits September 29, 2021 13:05

Fix $ref to local $id

4272f18

Fix tabs

d3f6309

Fix spaces to tabs

eee61a8

mati-o changed the title ~~Fix ref to $id~~ Fix $ref to $id Sep 29, 2021

mati-o added 2 commits October 1, 2021 11:35

wip

53c444a

Implement subschema $id resolution

2c9db44

mati-o commented Oct 1, 2021

View reviewed changes

aeschli mentioned this pull request Oct 25, 2021

Question about a minor test case with $refs #108

Closed

aeschli reviewed Nov 12, 2021

View reviewed changes

src/test/schema.test.ts Outdated Show resolved Hide resolved

Fix Resolving $refs 3 - match id and schema key

871713c

tests for $ref resolution across schemas

70d64f7

aeschli force-pushed the fix-ref-to-id branch from 766e64e to 70d64f7 Compare November 15, 2021 15:17

Fix multi-level $refs recursion

7d970dd

mati-o commented Nov 15, 2021

View reviewed changes

add test for resolving external $ref to already resolved schema

f7386b4

aeschli requested changes Nov 16, 2021

View reviewed changes

aeschli added 3 commits November 16, 2021 09:28

Use set instead of array

6962401

Merge branch 'main' into fix-ref-to-id

29ceb88

store resolved ids in parentHandle.anchors

b4c3512

rename SchemaHandle.url to uri

a4cc925

aeschli force-pushed the fix-ref-to-id branch 2 times, most recently from 278c86f to a4cc925 Compare November 29, 2021 14:48

Merge branch 'main' into fix-ref-to-id

9d65fa7

aeschli added this to the November 2021 milestone Nov 29, 2021

aeschli self-assigned this Nov 29, 2021

update change log

e244bcf

aeschli approved these changes Nov 29, 2021

View reviewed changes

aeschli merged commit 98cf9e0 into microsoft:main Nov 29, 2021

aeschli mentioned this pull request Nov 29, 2021

[json] support "$id" references. microsoft/vscode#92042

Closed

michalchar reviewed Dec 19, 2021

View reviewed changes

aeschli mentioned this pull request Dec 29, 2021

Fix resolving refs in external refs #121

Closed

Fix $ref to $id #107

Fix $ref to $id #107

Uh oh!

Conversation

mati-o commented Sep 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Sep 29, 2021 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

KapitanOczywisty commented Sep 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mati-o commented Sep 29, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mati-o commented Oct 1, 2021

Uh oh!

Uh oh!

aeschli commented Nov 12, 2021

Uh oh!

mati-o commented Nov 13, 2021

Uh oh!

aeschli commented Nov 15, 2021

Uh oh!

mati-o commented Nov 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aeschli commented Nov 29, 2021

Uh oh!

aeschli commented Nov 29, 2021

Uh oh!

mati-o commented Dec 1, 2021

Uh oh!

michalchar Dec 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mati-o commented Sep 29, 2021 •

edited

Loading

ghost commented Sep 29, 2021 •

edited by ghost

Loading

KapitanOczywisty commented Sep 29, 2021 •

edited

Loading

mati-o commented Nov 15, 2021 •

edited

Loading

michalchar Dec 19, 2021 •

edited

Loading