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

Discrepancy in addUsedSchema between v6 and v8 #1558

Open
misozask opened this issue Apr 21, 2021 · 5 comments · May be fixed by #2336
Open

Discrepancy in addUsedSchema between v6 and v8 #1558

misozask opened this issue Apr 21, 2021 · 5 comments · May be fixed by #2336
Labels
bug report

Comments

@misozask
Copy link

What version of Ajv are you using? Does the issue happen if you use the latest version?
Migrating from 6.11 to 8.1

Ajv options object
{allErrors: false, addUsedSchema: true/false}

Code

	const hex = {
		$id: 'hex',
		type: 'string',
		pattern: '^[A-Fa-f0-9]+$',
	}
	const combined = {
		$id: '2xhex',
		type: 'object',
		properties: {
			bar: hex,
			foo: hex,
		}
	}
	const ajv = new Ajv({allErrors: false, addUsedSchema: true})
	ajv.compile(combined)
	console.log(ajv.getSchema('2xhex') !== undefined && ajv.getSchema('hex') !== undefined) // true

	const ajv2 = new Ajv({allErrors: false, addUsedSchema: false})
	ajv2.compile(combined)
	console.log(ajv2.getSchema('2xhex') === undefined && ajv2.getSchema('hex') === undefined) // true

What results did you expect?
The above code works without issues in v6.
The combined schema will be compiled, and both schemas will be either cached or not( depending on addUsedSchema )

However in v8.1 the compilation fails with message Error: reference "hex" resolves to more than one schema
Thats might be acceptable for addUsedSchema: true, but for false I would expect to compile the combined schema.

Is this intended behavior? I would appreciate to not fail with addUsedSchema: false option, similar as in v6.
(BTW: I'm aware of $ref, I just find easier to work and define JSON schemas as plain JS objects and referencing them as objects )
@misozask misozask changed the title Dicrepancy in addUsedSchema between v6 and v8 Discrepancy in addUsedSchema between v6 and v8 Apr 22, 2021
@epoberezkin
Copy link
Member

epoberezkin commented Apr 24, 2021 •
edited

This is related to #1413

If you are combining the schemas as JS objects, the workaround is to not use $id's in inlined schemas: https://runkit.com/esp/6083d2fc1c64c9001adf7219

If $id is needed you need to use $refs currently.

@misozask
Copy link
Author

I see that now the schema is used as a key in a cache.
But looking at the code this seems to be something else.

In /lib/compile/resolve.ts in getSchemaRefs you have schemaRefs: Set<string> = new Set() which is caching all $ids.
Can we turn off adding schema reference when addUsedSchema: false?

    function addRef(this: Ajv, ref: string): string {
      ref = normalizeId(baseId ? URI.resolve(baseId, ref) : ref)
      if (this.opts.addUsedSchema === false) return ref  // skip adding schema reference
      
      if (schemaRefs.has(ref)) throw ambiguos(ref)
      schemaRefs.add(ref)
      ....

Would this harm something?

@epoberezkin
Copy link
Member

It is definitely worth trying - probably won't break anything.

If all the tests pass, I am ok to release it as the only change (which means not the next release) and see if anything breaks, in which case I can revert...

@Sachin-chaurasiya
Copy link

Hello @epoberezkin @misozask , I would like to take this up.

Sachin-chaurasiya added a commit to Sachin-chaurasiya/ajv that referenced this issue Oct 16, 2023
@Sachin-chaurasiya
fix( ajv-validator#1558): Discrepancy in addUsedSchema between v6 a…
7767534 
…nd v8
@Sachin-chaurasiya Sachin-chaurasiya linked a pull request Oct 16, 2023 that will close this issue
Open
@Sachin-chaurasiya
Copy link

Hello @epoberezkin @misozask , I would like to take this up.

I have raised the PR here #2336, would appreciate the review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug report
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix(#1558): Discrepancy in addUsedSchema between v6 and v8 Sachin-chaurasiya/ajv
3 participants
@epoberezkin @misozask @Sachin-chaurasiya