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

Performances problems fixes on big files with a lot of references on same big object #195

Merged
merged 2 commits into from
Dec 9, 2020
Merged

Performances problems fixes on big files with a lot of references on same big object #195

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e1c3417
add processedObject list to avoid recursion in already processed tree…
Oct 20, 2020
991e1a0
missing jsdoc
paztis Nov 3, 2020
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
108 changes: 72 additions & 36 deletions lib/dereference.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ module.exports = dereference;
*/
function dereference (parser, options) {
// console.log('Dereferencing $ref pointers in %s', parser.$refs._root$Ref.path);
let dereferenced = crawl(parser.schema, parser.$refs._root$Ref.path, "#", [], parser.$refs, options);
let dereferenced = crawl(parser.schema, parser.$refs._root$Ref.path, "#", [], [], {}, parser.$refs, options);
parser.$refs.circular = dereferenced.circular;
parser.schema = dereferenced.value;
}
Expand All @@ -28,60 +28,65 @@ function dereference (parser, options) {
* @param {string} path - The full path of `obj`, possibly with a JSON Pointer in the hash
* @param {string} pathFromRoot - The path of `obj` from the schema root
* @param {object[]} parents - An array of the parent objects that have already been dereferenced
* @param {object[]} processedObjects - An array of all the objects that have already been processed
* @param {object} dereferencedCache - An map of all the dereferenced objects
* @param {$Refs} $refs
* @param {$RefParserOptions} options
* @returns {{value: object, circular: boolean}}
*/
function crawl (obj, path, pathFromRoot, parents, $refs, options) {
function crawl (obj, path, pathFromRoot, parents, processedObjects, dereferencedCache, $refs, options) {
let dereferenced;
let result = {
value: obj,
circular: false
};

if (obj && typeof obj === "object" && !ArrayBuffer.isView(obj)) {
parents.push(obj);
if (options.dereference.circular === "ignore" || processedObjects.indexOf(obj) === -1) {
P0lip marked this conversation as resolved.
Show resolved Hide resolved
if (obj && typeof obj === "object" && !ArrayBuffer.isView(obj)) {
parents.push(obj);
processedObjects.push(obj);

if ($Ref.isAllowed$Ref(obj, options)) {
dereferenced = dereference$Ref(obj, path, pathFromRoot, parents, $refs, options);
result.circular = dereferenced.circular;
result.value = dereferenced.value;
}
else {
for (let key of Object.keys(obj)) {
let keyPath = Pointer.join(path, key);
let keyPathFromRoot = Pointer.join(pathFromRoot, key);
let value = obj[key];
let circular = false;

if ($Ref.isAllowed$Ref(value, options)) {
dereferenced = dereference$Ref(value, keyPath, keyPathFromRoot, parents, $refs, options);
circular = dereferenced.circular;
// Avoid pointless mutations; breaks frozen objects to no profit
if (obj[key] !== dereferenced.value) {
obj[key] = dereferenced.value;
}
}
else {
if (parents.indexOf(value) === -1) {
dereferenced = crawl(value, keyPath, keyPathFromRoot, parents, $refs, options);
if ($Ref.isAllowed$Ref(obj, options)) {
dereferenced = dereference$Ref(obj, path, pathFromRoot, parents, processedObjects, dereferencedCache, $refs, options);
result.circular = dereferenced.circular;
result.value = dereferenced.value;
}
else {
for (let key of Object.keys(obj)) {
let keyPath = Pointer.join(path, key);
let keyPathFromRoot = Pointer.join(pathFromRoot, key);
let value = obj[key];
let circular = false;

if ($Ref.isAllowed$Ref(value, options)) {
dereferenced = dereference$Ref(value, keyPath, keyPathFromRoot, parents, processedObjects, dereferencedCache, $refs, options);
circular = dereferenced.circular;
// Avoid pointless mutations; breaks frozen objects to no profit
if (obj[key] !== dereferenced.value) {
obj[key] = dereferenced.value;
}
}
else {
circular = foundCircularReference(keyPath, $refs, options);
if (parents.indexOf(value) === -1) {
dereferenced = crawl(value, keyPath, keyPathFromRoot, parents, processedObjects, dereferencedCache, $refs, options);
circular = dereferenced.circular;
// Avoid pointless mutations; breaks frozen objects to no profit
if (obj[key] !== dereferenced.value) {
obj[key] = dereferenced.value;
}
}
else {
circular = foundCircularReference(keyPath, $refs, options);
}
}
}

// Set the "isCircular" flag if this or any other property is circular
result.circular = result.circular || circular;
// Set the "isCircular" flag if this or any other property is circular
result.circular = result.circular || circular;
}
}
}

parents.pop();
parents.pop();
}
}

return result;
Expand All @@ -94,14 +99,37 @@ function crawl (obj, path, pathFromRoot, parents, $refs, options) {
* @param {string} path - The full path of `$ref`, possibly with a JSON Pointer in the hash
* @param {string} pathFromRoot - The path of `$ref` from the schema root
* @param {object[]} parents - An array of the parent objects that have already been dereferenced
* @param {object[]} processedObjects - An array of all the objects that have already been dereferenced
paztis marked this conversation as resolved.
Show resolved Hide resolved
* @param {$Refs} $refs
* @param {$RefParserOptions} options
* @returns {{value: object, circular: boolean}}
*/
function dereference$Ref ($ref, path, pathFromRoot, parents, $refs, options) {
function dereference$Ref ($ref, path, pathFromRoot, parents, processedObjects, dereferencedCache, $refs, options) {
// console.log('Dereferencing $ref pointer "%s" at %s', $ref.$ref, path);

let $refPath = url.resolve(path, $ref.$ref);

if (dereferencedCache[$refPath]) {
const cache = dereferencedCache[$refPath];

const refKeys = Object.keys($ref);
if (refKeys.length > 1) {
const extraKeys = {};
for (let key of refKeys) {
if (key !== "$ref" && !(key in cache.value)) {
extraKeys[key] = $ref[key];
}
}
return {
circular: cache.circular,
value: Object.assign({}, cache.value, extraKeys),
Copy link
Member

Choose a reason for hiding this comment

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

This might screw the order of properties and override some of them.
I'd suggest we bail out to regular scenario, same as we do in case of storing the cache at line 176.
Alternatively, we could maintain 'before$RefKeys' and after$RefKeys and merge them in that order.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes it will set extraKeys after basic ones
Is this order important ?

I do it to support some unit tests where an object has a description, then a reference of it overrides the description.
The cache key is the same for both (reference is unique). But after dewreference, ths description in both versions is not the same.
You've got specific unit tests for this.

Copy link
Member

Choose a reason for hiding this comment

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

Is this order important ?

Yeah it might matter at times if you have the property with the same key - it'll get overriden, won't it?
IMHO the current implementation may have a minor bug here.
I went through tests and we don't seem to have anything that'd validate the scenario I mentioned.

The below scenario fails

  it("should preserve the order of properties", async () => {
    let parser = new $RefParser();
    const schema = await parser.dereference(
      // This file doesn't actually need to exist. But its path will be used to resolve external $refs
      path.abs("path/that/does/not/exist.yaml"),
      // This schema object does not contain any external $refs
      {
        foo: {
          minLength: 1,
        },
        bar: {
          $ref: "#/foo",
        },
        baz: {
          $ref: "#/foo",
          minLength: 4,
        },
      },
      // An options object MUST be passed (even if it's empty)
      {});

    expect(schema).to.deep.equal({
      foo: {
        minLength: 1
      },
      bar: {
        minLength: 1
      },
      baz: {
        minLength: 4
      },
    });
  });

An improved implementation could look as follows

  if (refKeys.length > 1) {
      const value = {};
      for (let key of refKeys) {
        if (key === "$ref") {
          Object.assign(value, cache.value);
        }
        else {
          value[key] = $ref[key];
        }
      }

      return {
        circular: cache.circular,
        value,
      };
    }

This is questionable, however, since, if I am not mistaken, until draft 2019-09 you couldn't place $refs to any other properties, but as of 2019-09 you can and IIRC they are merged similarly to YAML merge keys.
I took at look at the spec, but couldn't really find any information regarding the order of merging.
Perhaps @philsturgeon has a clue on this one?

};
}

return cache;
}


let pointer = $refs._resolve($refPath, path, options);

if (pointer === null) {
Expand All @@ -122,7 +150,7 @@ function dereference$Ref ($ref, path, pathFromRoot, parents, $refs, options) {
// Crawl the dereferenced value (unless it's circular)
if (!circular) {
// Determine if the dereferenced value is circular
let dereferenced = crawl(dereferencedValue, pointer.path, pathFromRoot, parents, $refs, options);
let dereferenced = crawl(dereferencedValue, pointer.path, pathFromRoot, parents, processedObjects, dereferencedCache, $refs, options);
circular = dereferenced.circular;
dereferencedValue = dereferenced.value;
}
Expand All @@ -138,10 +166,18 @@ function dereference$Ref ($ref, path, pathFromRoot, parents, $refs, options) {
dereferencedValue.$ref = pathFromRoot;
}

return {

const dereferencedObject = {
circular,
value: dereferencedValue
};

// only cache if no extra properties than $ref
if (Object.keys($ref).length === 1) {
dereferencedCache[$refPath] = dereferencedObject;
}

return dereferencedObject;
}

/**
Expand Down