for #5178

Nice! Good, now all the magic happens around here:

Here is where we set the Validator Compiler, which is the function you are testing in your test case.
We need to ensure it does respect the fact that a custom validator is already in place for the current SchemaController instance.

@metcoder95 correct. I was thinking it should just check this.isCustomSerializerCompiler this.isCustomValidatorCompiler and not use the ValidatorBuilder mechanism to create a validatorCompiler in that case. I just didn't feel confident to know if that matched your intent or perhaps matters elsewhere. Seems like it is a straight up bug though.

my analysis: The test cases unfortunately don't actually use any schema extensions and just succeed because the default validator works totally fine as long as you don't extend JSON schema itself. So that's why it never got detected that it does nothing in these cases.

@Eomm yes it would make the problem go away, but I believe it also disables the functionality of recreating the validatorCompiler if schemas are added. Note the second part of the isReady expression. It clearly is trying to do something like 'recreate validator compiler if dirty.'

I could have fixed it by just checking the extra boolean I mentioned but I wasn't sure why standard ValidatorBuilder (essentially Factory<ValidatorCompiler>) and custom callback (also essentially Factory<ValidatorCompiler>) are handled separately to begin with. Maybe it is just merged code of two different origins and really they should have just been the same. I don't know to what degree you prefer cleaning up stuff like that versus only making minimal changes?

[edit: what I said was wrong. the boolean isCustomValidatorCompiler is apparently shared between using a custom builder or just using a custom compiler directly, so that isn't safe.]

Ok I will take a stab at developing a minimal fix, since now I am invested. Easier to discuss when we are looking at code...

the boolean isCustomValidatorCompiler

checking that boolean seems more correct 👍🏼

   if (isReady) {
      return
    }

this.validatorCompiler = this.isCustomValidatorCompiler ? this.validatorCompiler : this.getValidatorBuilder()(this.schemaBucket.getSchemas(), serverOptions.ajv)

I don't know to what degree you prefer cleaning up stuff like that versus only making minimal changes?

I would keep it minimal.
But in general I agree with your opinion: we have 2 different interfaces for the same thing and I will open a discussion to drop the old one. The schemaController should be preferred because it lets you do more than the setSchemaValidator only, but the latter is "easier" to use.

It clearly is trying to do something like 'recreate validator compiler if dirty.'

Correct, but when the user set a custom validator using setSchemaValidator it is up to the dep to deal with the external schemas. It is documented + the test adds the schema to the ajv instance because of it. The dirty flag is used only and only if the dev is using the schemaController.
Plus, I trust our test suite, so if it is green, we will not break anything 😄

I can remove the very verbose comment from the code after we discuss :)

Basically the ugly part is that one mechanism (schema controller) does have separate references to the factory (builder) and the compiler, and the other mechanism (setSchemaValidator) doesn't. So we can't actually recreate the compiler in that case, which is reflected in the now quite ugly conditional. But it is the truth...

But in general I agree with your opinion: we have 2 different interfaces for the same thing and I will open a discussion to drop the old one. The schemaController should be preferred because it lets you do more than the setSchemaValidator only, but the latter is "easier" to use.

For your discussion: as a new user of the framework, I would have much preferred if setSchemaValidator didn't exist or was marked deprecated and bad, because it is unsafe and trying to use it wasted a bunch of time. If I had found the other mechanism was there (schema controller opts) then I would have just used that. Unfortunately sample code and google hits made me find the old ("easy" but also dangerous) mechanism and I didn't know it.

Found a mistake in the change I made. It would also no longer recreate compilers if you use a custom builder. I will try again.

Here's a test from schema-serialization.test.js that succeeds in main but doesn't with a fix I was going to ~propose.

Could you please explain why the get request for /default is expected to return default? It seems the main fastify instance had the custom serializer installed and so it should absolutely return that?

[edit: never mind, figured it out; I did not understand register creates a new instance for the plugin, which has its own child schema controller]

test('Custom serializer per route', async t => {
  const fastify = Fastify()

  const outSchema = {
    $id: 'test',
    type: 'object',
    properties: {
      mean: { type: 'string' }
    }
  }

  fastify.get('/default', {
    handler (req, reply) { reply.send({ mean: 'default' }) },
    schema: { response: { 200: outSchema } }
  })

  let hit = 0
  fastify.register((instance, opts, done) => {
    instance.setSerializerCompiler(({ schema, method, url, httpStatus }) => {
...```
If anyone is available on the discord to take this offline, I am there as 'derammo'

ok new version is up, it is all green. I did not squash and rebase since there were discussions on the previous commits. Would you prefer I squash and rebase to have a single commit for discussion?

ok new version is up, it is all green. I did not squash and rebase since there were discussions on the previous commits. Would you prefer I squash and rebase to have a single commit for discussion?

Sure, plain merge or rebase works

unrelated: are the unnamed (arrow) functions I introduced to make the compiler function into a builder causing the coverage check failures? do I need to tag them somehow as "these don't count?"

[update: yes that was the problem. I can resolve it by making them into something more disgusting like a bound named function, if you're ok with that. (see 7aa414a)]

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.