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

Schema absolute references failure #2446

Closed
jsumners opened this issue Jul 29, 2020 · 12 comments
Closed

Schema absolute references failure #2446

jsumners opened this issue Jul 29, 2020 · 12 comments
Assignees
Labels
discussion Issues or PRs with this label will never stale feature request New feature to be added

Comments

@jsumners
Copy link
Member

🐛 Bug Report

When registering routes with schemas that include properties with absolute references to other schemas the following error is generated:

FastifyError [Error]: Failed building the serialization schema for GET: /, due to error Bad arguments: First argument should be a string, second should be an array of strings
    at Boot.<anonymous> (/private/tmp/29/schema-ref-test/node_modules/fastify/lib/route.js:271:19)

To Reproduce

Steps to reproduce the behavior:

"use strict";

const AJV = require("ajv");
const ajvInstance = new AJV();

const baseSchema = {
  $id: "http://example.com/schemas/base",
  definitions: {
    hello: { type: "string" },
  },
  type: "object",
  properties: {
    hello: { $ref: "#/definitions/hello" },
  },
};

const refSchema = {
  $id: "http://example.com/schemas/ref",
  type: "object",
  properties: {
    hello: { $ref: "http://example.com/schemas/base#/definitions/hello" },
  },
};

ajvInstance.addSchema(baseSchema);
ajvInstance.addSchema(refSchema);

/***********/

const fastify = require("fastify")({ logger: { level: "debug" } });
fastify.setValidatorCompiler(function ({ schema }) {
  return ajvInstance.compile(schema);
});

fastify.route({
  path: "/",
  method: "GET",
  schema: {
    response: {
      "2xx": ajvInstance.getSchema("http://example.com/schemas/ref").schema,
    },
  },
  handler(req, res) {
    res.send({ hello: "world" });
  },
});

fastify.inject({ url: "/" }, (err, response) => {
  if (err) throw err;

  console.log(response);
});

Expected behavior

The route schema should resolve any references without crashing.

Your Environment

  • node version: 12
  • fastify version: >=3.1.1
  • os: Mac
@mcollina
Copy link
Member

Given that you are not using addSchema from fastify itself fast-json-stringify does not know about your added schemas. You might want to override https://www.fastify.io/docs/latest/Server/#serializercompiler as well to provide fjs your references.

@jsumners
Copy link
Member Author

Given that you are not using addSchema from fastify itself fast-json-stringify does not know about your added schemas. You might want to override https://www.fastify.io/docs/latest/Server/#serializercompiler as well to provide fjs your references.

Actually, that doesn't mesh with our docs:

  1. Indicates that setting a serializer compiler is for overriding which serializer is used, i.e. if you want to use the standard JSON.stringify over fast-json-stringify -- https://www.fastify.io/docs/v3.1.x/Validation-and-Serialization/#serializer-compiler

  2. We make no mention of needing a "serializer compiler" in the documentation for bringing your own schema container. Indeed, we specifically call out AJV in the footnote as a possible source -- https://www.fastify.io/docs/v3.1.x/Validation-and-Serialization/#validator-compiler

  3. Our migration guide, and @Eomm's article linked from it, do not mention needing this combination. It specifically calls for reducing the v2 configuration from a compiler and resolver down to just a compiler -- https://github.com/fastify/fastify/blob/61e836108944e2d86b174205e8f59a89a06cb47b/docs/Migration-Guide-V3.md#changed-schema-validation-options-2023

@Eomm do you have any insight to offer?

@Eomm
Copy link
Member

Eomm commented Jul 30, 2020

Unfortunately, the footnote is right for the "serializer compiler" too.

I think it would simple to implement, here:

this.setSerializerCompiler(buildDefaultSerializer(sBucket.getSchemas()))

we should write sBucket.getSchemas() || this[kValidatorCompiler].getSchemas():

But it is not doable (right now) because:

  • the *compilers are agnostic functions, so the user should attach a getSchemas key to the input compiler/function
  • ajv expose a singe getSchema(<id>) and not a globally getSchemas()

@jsumners
Copy link
Member Author

jsumners commented Jul 30, 2020

It sounds to me like we have a broken feature.

As I suggested in #2023 (comment), I think we should define an API contract for user supplied schema containers. Something like:

fastify.setSchemaContainer(containerInstance)

Where containerInstance must look something like:

{
  addSchema({name, schema}) {}, // `name` is optional if `schema` has a `$id` property
  getSchemaValidator (id) {}, // returns function to validate data against schema
  getSchema (id) {} // returns raw JSON Schema
}

@stale
Copy link

stale bot commented Aug 15, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale Issue or pr with more than 15 days of inactivity. label Aug 15, 2020
@Eomm Eomm added discussion Issues or PRs with this label will never stale feature request New feature to be added labels Aug 16, 2020
@stale stale bot removed the stale Issue or pr with more than 15 days of inactivity. label Aug 16, 2020
@jsumners jsumners self-assigned this Aug 16, 2020
@jsumners
Copy link
Member Author

Leaving myself a note to consider #2568 as part of this.

@Eomm
Copy link
Member

Eomm commented Dec 16, 2020

Regarding the interface of the setSchemaContainer I would point out these issues I had in past:

  • when you define a new fastify's context the schemas of the instance are cloned to the new one to grant the encapsulation

instance[kSchemas] = buildSchemas(old[kSchemas])

  • all the schemas of a fastify instance are stored in the [kSchemas] symbol and this object track if new schemas are added (as optimization)

schema.newSchemasAdded = false

  • on the server start (by default) the ajv instance has not been instantiated yet, so the first route registered will build the ajv instance for the whole context:

this.setValidatorCompiler(buildPerformanceValidator(schemaBucket.getSchemas(), this[kOptions].ajv))

  • fastify's context children will inherit the ajv instance (and its schemas as well) unless the child context has the flag that tracks if a new schema has been added to the [kSchemas] store, in this case, a new ajv instance will be created and assigned to the child context and its children (in this situation the cloned [kSchemas] object at the first step it is necessary)

The process for the serialization is quite the same:

  • fast-json-stringify is not instantiated at the startup but when the first route with response schemas is loaded
  • it will use the same [kSchemas] as the validator and it will be rebuilt only if necessary

return fastJsonStringify(schema, { schema: externalSchemas })

NB: in this case, you are experiencing the issue that origins this ticket: the fastify [kSchemas] is just empty (cause the custom validator) and for this reason fast-json-stringify triggers the error.


So I think we should consider that the schemaContainer should:

  • support the encapsulation out of the box
  • provide a better interface to configure validator and serializer (the schemas used should be the same, but the compiled function is different)

I hope this will help to understand the flow and I'm more than happy to help.
I'm thinking a proposal, but the main challenge is to be semver-minor here

@jsumners
Copy link
Member Author

😕

If we have to go semver-major, so be it. I'm not opposed to skipping v3 (I cannot use v3 until this is solved). But if we go with a major change, then we should take some time to work out what the container interface should be. My work on this specific issue would probably be a very minimal hack just to make things work. I'm really not familiar with all of the pieces of schema code within Fastify.

@Eomm
Copy link
Member

Eomm commented Dec 16, 2020

As you said in the AJV7 issue, I totally agree that moving AJV out of the dependencies with an abstraction layer would improve and simplify a lot the usage of schemas and the customization of validation/serialization!

I don't want to be harsh here or slow down your idea on how to solve this specific issue

--

PS: when I worked on it for v3 my target was to avoid the tons of $ref issues caused by the "shared schema feature", for v4 we could reach the final goal and shape of this key feature!

@jsumners
Copy link
Member Author

I'm not offended. The only thing I'm disappointed about is not yet being able to prioritize fixing this "on the clock" and having to do it on my own time.

If you get an itch to work on this before I push up a branch, feel free. But I am not expecting anyone to work on this other than myself.

@jsumners
Copy link
Member Author

So I think we should consider that the schemaContainer should:

  • support the encapsulation out of the box

I don't think this is accurate. I think the schema container needs to provide schemas, full stop. It is Fastify's responsibility to acquire schemas from that container and use them in whatever context it deems necessary. For input validation, this is rather simply a case of getSchema('id') to get a compiled validation function. The tricky bit is for output serialization. In that case, we need the raw JSON Schema definition. But those schemas could contain references, which themselves could contain references. So someone will need to resolve those references. Probably the easiest approach is to pass the container along to the serializer so that it can request the schema whenever it encounters a reference.

@Aex12
Copy link

Aex12 commented Feb 5, 2021

I'm experiencing the same issue while trying to use my own AJV instance. I need to define a custom keyword in AJV, and seems like its impossible to do so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion Issues or PRs with this label will never stale feature request New feature to be added
Projects
None yet
Development

No branches or pull requests

4 participants