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

fix: merged schemas cache key #729

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

DamienVicet
Copy link

Hello, this PR fixes #724.

When diving in the bug, I found that the context.mergedSchemasIds Map, added by @ivan-tymoshenko, is using objects as keys. This caused the schema to infinitely try to build and results into Maximum call stack size exceeded error.
So I replaced the key by a hash of the object, then I got another problem : anyOf inside allOf test failed because, to my understanding, we try to cache different merged schemas using the same key in context.mergedSchemasIds (like merged schemas from buildOneOf and from buildAllOf functions).

I am not sure of the solution. Thank you for your review !

Copy link
Member

@gurgunday gurgunday left a comment

Choose a reason for hiding this comment

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

This should work, i don't know about the performance of that package, but this could be a lot faster way of setting unique identifiers:

const idMap = new WeakMap();
const getId = (obj) => {
  if (!idMap.has(obj)) {
    idMap.set(obj, Symbol('id'));
  }
  return idMap.get(obj);
};

const id = getId(object);

Copy link
Member

@gurgunday gurgunday left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

I'm a bit scared of this as it would likely increase our build/startup time.

Copy link
Member

@ivan-tymoshenko ivan-tymoshenko left a comment

Choose a reason for hiding this comment

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

thank you for your PR. I'm rejecting it for now, because I remember I was using objects pointers as keys on purpose to compare objects when going through the refs cycles. I need a bit more time to look at this PR. I will try to review asap.

@ivan-tymoshenko
Copy link
Member

I found the problem. I'm working on the solution. Unfortunatelly merging schemas with recurcive references is the most complecated part of this library and take tons of time every time when we need to fix the bug.

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

Successfully merging this pull request may close these issues.

Multiple fields with anyOf and self ref causes Maximum call stack size exceeded
4 participants