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

Main schemas isolation #81

Closed
2 tasks done
ivan-tymoshenko opened this issue Sep 10, 2022 · 14 comments
Closed
2 tasks done

Main schemas isolation #81

ivan-tymoshenko opened this issue Sep 10, 2022 · 14 comments

Comments

@ivan-tymoshenko
Copy link
Member

ivan-tymoshenko commented Sep 10, 2022

Prerequisites

  • I have written a descriptive issue title
  • I have searched existing issues to ensure the issue has not already been raised

Issue

I'm working on some optimization for FJS and trying to resolve the endpoints' schemas isolation. I was wondering how it was resolved here, but it turns out all endpoints' schemas are in the same context.

The problem is that you can define a nested schema in a global namespace.

t.test('global refs in main schemas', (t) => {
  t.plan(2)
  const factory = AjvCompiler()
  const compiler = factory({}, fastifyAjvOptionsDefault)
  const validatorFunc1 = compiler({
    schema: {
      definitions: {
        globalIdSchema: {
          $id: 'globalId',
          type: 'integer',
        }
      },
      $ref: 'globalId'
    }
  })
  const validatorFunc2 = compiler({
    schema: {
      definitions: {
        globalIdSchema: {
          $id: 'globalId',
          type: 'string',
        }
      },
      $ref: 'globalId'
    }
  })
  t.equal(validatorFunc1(3), true)
  t.equal(validatorFunc2(3), false)
})
@jsumners
Copy link
Member

jsumners commented Sep 10, 2022

I think those two schemas are just wrong. You've written two schemas that self-referencially create schemas with conflicting identifiers. Every schema in the container should have a unique identifier. If schemas have conflicting identifiers then the schema author(s) have done it wrong.

See https://json-schema.org/draft/2020-12/json-schema-core.html#name-the-id-keyword

@ivan-tymoshenko
Copy link
Member Author

ivan-tymoshenko commented Sep 10, 2022

So users can't create two endpoints with the same nested schemas? So users can't create two endpoints with the same nested schemas? I thought that all context schemas that you add with fastify.addSchema should be unique (and have unique nested schemas), but I thought that endpoint schemas should be isolated from each other.

@jsumners So you think that users should not create routes like that?

fastify.post('/a', {
  handler () { },
  schema: {
    type: 'object',
    properties: {
      a: { $id: 'globalId', type: 'string' },
      b: { $ref: 'globalId' },
    }
  }
})

fastify.post('/b', {
  handler () { },
  schema: {
    type: 'object',
    properties: {
      a: { $id: 'globalId', type: 'integer' },
      b: { $ref: 'globalId' },
    }
  }
})

@jsumners
Copy link
Member

You are specifying anonymous schemas in that example. They do not have identifiers. Your $ref in the b definitions are referring a property within the same schema, but they are also declared incorrectly. They should really be { $ref: '#/properties/a' }. See https://json-schema.org/understanding-json-schema/structuring.html#defs

@ivan-tymoshenko
Copy link
Member Author

ivan-tymoshenko commented Sep 10, 2022

You can reference the property either with json pointer or schema $id. I wrote valid JSON schema draft 7 syntaxes.
Here is an example:
https://json-schema.org/understanding-json-schema/structuring.html#bundling

+ Ajv works in this way.

@jsumners
Copy link
Member

I wrote valid JSON schema draft 7 syntaxes.

I didn't say the schemas are invalid, only that the values used for the $ref keywords are invalid. Your link says the same thing I wrote previously.

@ivan-tymoshenko
Copy link
Member Author

ivan-tymoshenko commented Sep 10, 2022

Ok, I wrote valid values in $ref. Why do you think that it's invalid?

That's a quote from the link you sent me.
"Although we can identify any subschema using JSON Pointers or NAMED ANCHORS, the $defs keyword gives us a standardized place to keep subschemas intended for reuse in the current schema document."

@jsumners
Copy link
Member

Look at the example you linked. All $ref values should be fragment identifiers or relative to the base URI of the schema.

@ivan-tymoshenko
Copy link
Member Author

  1. In Draft 4-7, an $id in a subschema did not indicate an embedded schema. Instead it was simply a base URI change in a single schema document.
  2. I can give you an example with relative to the base URL.
fastify.post('/a', {
  handler () { },
  schema: {
    $id: 'hello',
    type: 'object',
    properties: {
      a: { $id: 'hello/globalId', type: 'string' },
      b: { $ref: 'hello/globalId' },
    }
  }
})

fastify.post('/b', {
  handler () { },
  schema: {
    $id: 'hello',
    type: 'object',
    properties: {
      a: { $id: 'hello/globalId', type: 'integer' },
      b: { $ref: 'hello/globalId' },
    }
  }
})

@ivan-tymoshenko
Copy link
Member Author

@jsumners
Copy link
Member

I'm sorry, I really don't understand what you are trying to prove. As I originally stated, schema identifiers are URIs by spec (https://json-schema.org/draft/2020-12/json-schema-core.html#name-the-id-keyword). URIs, by spec (https://www.rfc-editor.org/rfc/rfc3986#section-1.1), are unique globally:

URIs have a global scope and are interpreted consistently regardless
of context, though the result of that interpretation may be in
relation to the end-user's context.

Therefore, schemas in a schema container, e.g. an AJV instance, can either be anonymous or identified by identifiers specified by their $id property and are globally unique in the container.

@ivan-tymoshenko
Copy link
Member Author

I'm talking about isolation. I'm asking maybe two route schemas shouldn't be in the same container.
If we have set up like that.

fastify.addSchema(/* A */)
fastify.addSchema(/* B */)

fastify.post('/foo', {
  handler () { },
  schema: { /* C */ }
})

fastify.post('/bar', {
  handler () { },
  schema: { /* D */ }
})

Maybe we should have 2 containers: { 'A', 'B', 'C' } and { 'A', 'B', 'D' }

@ivan-tymoshenko
Copy link
Member Author

If you and someone else will confirm that it's a user's responsibility to manage conflicts between routes' schemas, that they are in the same namespace, and that they can be referenced to each other, I would be ok with it.

@Eomm
Copy link
Member

Eomm commented Sep 10, 2022

I think your test is a reasonable use case but creating an ajv container for each route is overkilling.

We could set the addUsedSchema: false as default and it solves this use case and it will be not necessary to create more containers.

Unluckily, because of the coerceTypes: 'array' setting, your example will always fail because the number is coerced. See the example at the bottom.

const Ajv = require('ajv')
const ajv = new Ajv({
  addUsedSchema: false,
  coerceTypes: 'array', // uncomment
  useDefaults: true,
  removeAdditional: true,
  addUsedSchema: false,
  allErrors: false
})

let validate

validate = ajv.compile({

  definitions: {
    globalIdSchema: {
      $id: 'globalId',
      type: 'integer'
    }
  },
  $ref: 'globalId'

})
test(3)

validate = ajv.compile({
  definitions: {
    globalIdSchema: {
      $id: 'globalId',
      type: 'string'
    }
  },
  $ref: 'globalId'
})
test(3)

function test (data) {
  const valid = validate(data)
  if (valid) console.log('Valid!')
  else console.log('Invalid: ' + ajv.errorsText(validate.errors))
}

@ivan-tymoshenko
Copy link
Member Author

@Eomm It looks like addUsedSchema already resolves that. And the error was caused by type coercion.

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

No branches or pull requests

3 participants