Adding multiple items to the security array of the JSON Schema breaks all the types

DemianD commented 2 years ago

Prerequisites

[X] I have written a descriptive issue title
[X] I have searched existing issues to ensure the bug has not already been reported

Fastify version

4.5.3

Plugin version

2.3.0

Node.js version

18.7.0

Operating system

macOS

Operating system version (i.e. 20.04, 11.3, 10)

11.5.2

Description

We have a problem that our types are not automatically inferred when we add multiple items to the security property of the JSON schema that you can define on the route.

You can see this in the example below:

Steps to Reproduce

import { TypeBoxTypeProvider } from '@fastify/type-provider-typebox';
import { Type } from '@sinclair/typebox';
import Fastify from 'fastify';

export const fastify = Fastify().withTypeProvider<TypeBoxTypeProvider>();

fastify.get('/', {
  schema: {
    body: Type.Object({
      x: Type.String(),
      y: Type.Number(),
      z: Type.Boolean(),
    }),
    // Adding multiple items here breaks the automatically type inferring. 
    security: [{ OAuth2: ['x:readOnly'] }, { OpenIDConnect: [] }],
  },
}, (req) => {
  // The `x`, `y`, `z` types are not inferred anymore because there are multiple items in the array
  const { x, y, z } = req.body;
});

Expected Behavior

We'd like that our types are inferred when adding multiple items to the security array.

mcollina commented 2 years ago

cc @fastify/typescript

Amphaal commented 1 year ago

Had the same issue, but when providing an additional onRequest handler instead of security.

Using the alternative syntax using route() for route declaration instead of get() / post() / whateverVerb() seems to make the issue disappear.

mcollina commented 1 year ago

@sinclairzx81 could you take a look?

sinclairzx81 commented 1 year ago

Hi @mcollina ! Just had a quick look at the original issue code, this seems to be working ok...

TypeScript Link Here

General Recommendation:

Ensure latest version of Fastify (noting this issue dates back to Sep last year so there may have been some type updates since)
Ensure latest version of the TS compiler (currently typescript@4.9.5)
Ensure compilerOptions: { strict: true } is configured in tsconfig.json

Happy to look a bit deeper if there's a way to repro the inference issue, but this appears to be working alright :)

Hope this helps! S

mcollina commented 1 year ago

Thanks!

Amphaal commented 1 year ago

Hi @mcollina ! Just had a quick look at the original issue code, this seems to be working ok...

TypeScript Link Here

General Recommendation:

Ensure latest version of Fastify (noting this issue dates back to Sep last year so there may have been some type updates since)

Ensure latest version of the TS compiler (currently typescript@4.9.5)

Ensure compilerOptions: { strict: true } is configured in tsconfig.json

Happy to look a bit deeper if there's a way to repro the inference issue, but this appears to be working alright :)

Hope this helps! S

Well, seems like my first assumption was not correct, my issue popped up in all (fastify.post | fastify.get) && fastify.route scenarii after all.

The issue I had, which I do not think is related to OP's issue, was that the function which I wanted to override onRequest with had explicitly defined parameters as generic FastifyRequest / FastifyReply, instead of my fastify's instance super-seeded generics (FastifyRequest<..., TypeBoxTypeProvider> / FastifyReply<..., TypeBoxTypeProvider>)

...by enforcing these basic types, functionality breaks and my request components all resolves as unknown, which is kinda understandable.

Thank you guys @mcollina @sinclairzx81 for your time :)

andreaspalsson commented 1 year ago

I ran into the same issues with multiple items in the security array making body unknown however it's using FastifyPluginAsyncTypebox could that be a another source for this issue. I have tested with the latest version of everything but still get the same issues. I created a small repo where the issue is reproduced.

https://github.com/andreaspalsson/typebox-schema-security

Also ran it through GitHub Actions to make sure it wasn't something local and the types fails there as well

https://github.com/andreaspalsson/typebox-schema-security/actions/runs/4232737696/jobs/7352833222

Versions:

"@fastify/swagger": "^8.3.1",
"@fastify/type-provider-typebox": "^2.4.0",
"@sinclair/typebox": "^0.25.23",
"fastify": "^4.13.0",
"typescript": "^4.9.5"

sinclairzx81 commented 1 year ago

@andreaspalsson Hey, thanks for the repro! Had another look at this and can reproduce the issue on the TSP. The issue seems to be related to the definitions inside @fastify/swagger. See TypeScript link below and uncomment the following line to reproduce.

TypeScript Link Here

// import '@fastify/swagger' // uncomment to introduce error

Issue

The swagger package performs a declaration merge on the FastifySchema to include a set of additional properties specific to swagger. One of which being the security property. This is defined as follows (code can be found here)

  interface FastifySchema {
    hide?: boolean;
    deprecated?: boolean;
    tags?: string[];
    description?: string;
    summary?: string;
    consumes?: string[];
    produces?: string[];
    externalDocs?: OpenAPIV2.ExternalDocumentationObject | OpenAPIV3.ExternalDocumentationObject;
    security?: Array<{ [securityLabel: string]: string[] }>; // <-- issue here !
    /**
     * OpenAPI operation unique identifier
     */
    operationId?: string;
  }

TypeScript can have some difficulties dealing with arbitrary length array / property key definitions when mapping types, but its curious why it's causing issues here as this property is not mapped. My best guess is due to the declaration merge, this is causing the security property (and type) to be propagated to the route (via req.routeSchema.security) and as the property type is non-mappable, it's causing TS to inference to break down (even though there's no mapping on it, which is strange)

In situations like this though, it's common to turn to readonly and as const assertions, this to provide TS enough assurances as to the shape of the security array.

Possible Fix

The following updates the security property to be readonly. This will need to be updated in the @fastify/swagger declarations here).

// ------------------------------------------------------------------------------
// Fix: @fastify/swagger
//
// Here we mandate that the security array is readonly, and the caller must
// pass with a 'as const' assertion. This enables TS to propagate the type as 
// fixed readonly data structure. Similar techniques can be seen in libraries
// like json-schema-to-ts which mandate 'as const' to infer object property
// keys encoded in JSON Schema.
// ------------------------------------------------------------------------------

declare module "fastify" {
  // added: readonly is required for propagation of dictionary types.
  export type SecurityArray = readonly {
    [securityLabel: string]: readonly string[]
  }[]
  interface FastifySchema {
    hide?: boolean;
    deprecated?: boolean;
    tags?: string[];
    description?: string;
    summary?: string;
    consumes?: string[];
    produces?: string[];
    externalDocs?: OpenAPIV2.ExternalDocumentationObject | OpenAPIV3.ExternalDocumentationObject;
    security?: SecurityArray; // Fix
    /**
     * OpenAPI operation unique identifier
     */
    operationId?: string;
  }
}

Example

Here's an example of things working in the TSP with the as const applied to the security array per route.

TypeScript Link Here

  server.post(
    "/double-security",
    {
      schema: {
        body: Type.Object({
          foo: Type.Number(),
          bar: Type.String(),
        }),
        security: [{ apiKey: ['foo'] }, { authHeader: ['bar'] }] as const // mandate const readonly assertion
      },
    },
    (request, reply) => {
      const { foo, bar } = request.body; // ok
      return { foo, bar };
    }
  );
};

Future

While in TS 4.0 the as const assertion is required, In TS 5.0, it should be possible to automatically apply the as const with the upcoming TS 5.0 const generics language feature. Setting up readonly now should get things up for a later TS 5.0 enhancement once it lands.

Hope this helps S

CC @mcollina @Amphaal

sinclairzx81 commented 1 year ago

@andreaspalsson @Amphaal Oh, one other thing. In the interim, you should be able to get around the inference issues with an as any assertion on the security array (which isn't ideal, but works)

security: [{ apiKey: [] }, { authHeader: [] }] as any // replace with "as const" on fix

Cheers

andreaspalsson commented 1 year ago

@sinclairzx81 Thanks for a great explanation of the problem and a workaround. We only have a few endpoints that have multiple security items so this interim solution will work fine for us. Thanks for the quick help.

mcollina commented 1 year ago

Is this a problem i @fastify/swagger then? Could we fix it there?

sinclairzx81 commented 1 year ago

@mcollina Yeah, it might be worth opening an issue on @fastify/swagger and linking back to this one. The suggested fix would require an update in the swagger declarations (so would need a review from the Fastify TS team), also there's actually a few ways to approach a solution, so would be good to discuss those within the context of the swagger project.

mcollina commented 1 year ago

Should we move this issue?

fastify / fastify-type-provider-typebox