fastify / fastify-request-context Goto Github PK

Prerequisites

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

Fastify version

3.25.0

Plugin version

2.2.0

Node.js version

16.x

Operating system

macOS

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

12.0.1 Monterey

Description

With a project using fastify and fastify-request-context, the user could get confused because

declare module 'fastify-request-context' {
    interface RequestContextData {
       a: number;
    }
}

const value = request.requestContext.get('string');

and value is set to number | undefined. In the documentation it states that the value should be set to a type of number, but this is not always the case.

Steps to Reproduce

Create a new folder/nodejs project with these files.

package.json

{
  "name": "fastify-request-context-typescript",
  "version": "1.0.0",
  "description": "",
  "main": "index.js",
  "scripts": {
    "test": "echo \"Error: no test specified\" && exit 1"
  },
  "keywords": [],
  "author": "",
  "license": "ISC",
  "dependencies": {
    "fastify": "^3.25.0",
    "fastify-request-context": "^2.2.0"
  },
  "devDependencies": {
    "@types/node": "^17.0.0"
  }
}

tsconfig.json

{
    "compilerOptions": {
        "strict": true // the cause of the issue
    }
}

app.ts

import fastify, { FastifyLoggerInstance } from 'fastify';
import { fastifyRequestContextPlugin } from 'fastify-request-context';

import { simpleController } from './a';

declare module 'fastify-request-context' {
    interface RequestContextData {
        logger: FastifyLoggerInstance;
    }
}

const server = fastify();

server.register((app) => {
    app.get('/', {
        handler: simpleController
    });
}, {
    prefix: '/helloworld',
})

server.register(fastifyRequestContextPlugin, {
    defaultStoreValues: {
        logger: server.log,
    },
});

a.ts

import { RouteHandler } from 'fastify';

export const simpleController: RouteHandler = async (req, reply) => {
    const logger = req.requestContext.get('logger');
    
    // logger is possibly undefined, so this is a typescript error
    logger.info('do a thing');

    reply.code(204);
};

The issue is due to this typing I think:

https://github.com/fastify/fastify-request-context/blob/master/index.d.ts#L8

Since the type is RequestContextData[K] | undefined, in strict typing mode I think that this causes logger above in a.ts to be FastifyLoggerInstance | undefined. This is not the case if you add "strictNullChecks": false to the tsconfig.json.

Expected Behavior

Could the .get method just be typed as get<K extends keyof RequestContextData>(key: K): RequestContextData[K]? This seems to work with "strictNullChecks" set to either true (implicitly by "strict": true), or with "strict": true and "strictNullChecks": false.

Prerequisites

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

🚀 Feature Proposal

I would propose swapping this:

  export interface RequestContextData {
    [key: string]: any
  }

For plain:

  export interface RequestContextData {
  }

Motivation

This way someone like me who wants to get help ensuring I don't mistype any context names can use this plugin really strictly typed, and if someone wants to be lenient on the key names but strict with the types, then they could easily do:

declare module '@fastify/request-context' {
  interface RequestContextData {
    [key: string]: unknown
  }
}

And those who wants to have it like today can simply do:

declare module '@fastify/request-context' {
  interface RequestContextData {
    [key: string]: any
  }
}

By leaving out the default [key: string]: any it becomes possible to customize the type in ways otherwise not possible.

Example

No response

Prerequisites

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

🚀 Feature Proposal

In #136 / #137 defaultStoreValues was allowed to be a factory, but no parameters was actually sent to it.

I would like it if it was sent the request instance as an argument.

Motivation

Sending request as an argument to the defaultStoreValues factory allows one to easily bootstrap the request context with values derived from the request, which is something I have been doing for eg a logging function.

I will do a PR.

Example

fastify.register(fastifyRequestContextPlugin, { 
  hook: 'preValidation',
  defaultStoreValues: () => ({
    log: request.log.child({ foo: 'bar }),
  }),
});

Prerequisites

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

🚀 Feature Proposal

The defaultStoreValue could accept a factory function.

Motivation

This is a similar approach used in different store libs (ie. vuex, pinia).

Currently when passing default values by the store, when the value is an object, it is shared between requests what could be not what we want. The side effects of this can be hard to find when someone is not aware of this and instead of setting the whole data by key mutates the members of object what user got from context (like in #125).

Example

fastify.register(fastifyRequestContextPlugin, { 
  hook: 'preValidation',
  defaultStoreValues: () => ({
    user: { id: 'system' } 
  })
});

Can somebody take a look?

Prerequisites

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

Fastify version

3.28.0

Plugin version

2.2.0

Node.js version

16.3.2

Operating system

Linux

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

20.04.4 LTS

Description

When running E2E tests, even calling fastify.close(), Jest is reporting an open handle from fastify-request-context:

`
Jest has detected the following 1 open handle potentially keeping Jest from exiting:

● fastify-request-context

  35 |   it('test', (done) => {
  36 |     console.log('test');
> 37 |     fastify.inject({
     |             ^
  38 |         method: 'GET',
  39 |         url: '/'
  40 |       }, (err, response) => {

  at Object.ready (node_modules/fastify/fastify.js:481:13)
  at Object.inject (node_modules/fastify/fastify.js:459:12)
  at Object.<anonymous> (test.js:37:13)
  at TestScheduler.scheduleTests (node_modules/@jest/core/build/TestScheduler.js:333:13)
  at runJest (node_modules/@jest/core/build/runJest.js:404:19)
  at _run10000 (node_modules/@jest/core/build/cli/index.js:320:7)
  at runCLI (node_modules/@jest/core/build/cli/index.js:173:3)

`

Steps to Reproduce

package.json:

{
    "name": "test",
    "version": "1.0.0",
    "scripts": {
        "test": "jest  --detectOpenHandles"
    },
    "dependencies": {
        "fastify": "3.28.0",
        "fastify-request-context": "2.2.0"
    },
    "devDependencies": {
        "jest": "27.5.1"
    }
}

test.js:

const Fastify = require('fastify');
const { requestContext, fastifyRequestContextPlugin } = require('fastify-request-context');

async function buildFastify () {
  const fastify = Fastify();

  fastify.get('/', function (request, reply) {
    reply.send({ hello: 'world' });
  });

  // If we comment the following lines, no open handles
  await fastify.register(fastifyRequestContextPlugin, {
    hook: 'onRequest',
    defaultStoreValues: {},
  });
  fastify.addHook('preValidation', (request, _reply, done) => {
    requestContext.set('body', request.body);
    done();
  });

  return fastify;
};

describe('GET `/` route', () => {
  let fastify;

  beforeAll(async () => {
    fastify = await buildFastify();
  });

  afterAll(async () => {
    await fastify.close();
  });

  it('test', (done) => {
    fastify.inject({
        method: 'GET',
        url: '/'
      }, (err, response) => {
        expect(response.statusCode).toEqual(200);
        done();
      });
  });  
})

Steps to reproduce:

1. Create the files above
2. npm install
3. npm run test
4. Most of the times Jest will say there is an open handle related to fastify-request-context

Expected Behavior

There shouldn't be any open handles in an E2E test after calling fastify.close()

Prerequisites

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

Fastify version

3.22.0

Plugin version

websocket 4.0.0

Node.js version

16.4.2

Operating system

Windows

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

10

Description

I have this defined to augment FastifyRequest:

 declare module "fastify" {
  export interface FastifyRequest {
    config: {
      sessionParms: any;
    };
  }
}

I am getting this message:

[FSTDEP006] FastifyDeprecation: You are decorating Request/Reply with a reference type. This reference is shared amongst all requests. Use onRequest hook instead.

I have this defined in the onRequest hook :

server.addHook("onRequest", (request, reply, done) => {
  request.config.sessionParms = setSystem(apiParms, request, reply); // make sessionParms accessible 
  done();
});

This all looks to be correct according to the documentation.

Steps to Reproduce

See attached code above.

Expected Behavior

To not receive the deprecation error message..

🐛 Bug Report

Using this plugin with fastify version 3.9.2 I get the the warning from the following warning:

[FSTDEP006] FastifyDeprecation: You are decorating Request/Reply with a reference type.
This reference is shared amongst all requests. Use onRequest hook instead. Property: requestContext

❓ Possible Fix

I found a temporary solution mentioned in another plugin:
file: lib/requestContextPlugin.js

13:  fastify.decorateRequest('requestContext', requestContext)

replaced with:

13:  fastify.decorateRequest('requestContext', { getter: () => requestContext })

To Reproduce

• fastify v3.9.2
• fastify-request-context v2.1.2

Your Environment

• node version: 15.2.1
• fastify version: 3.9.2
• fastify request context version: v2.1.2
• os: Linux

🚀 Feature Proposal

Expose request context (at least the CLS part of it) as a global object instead of (or as an alternative to) asking users to access it through req/app.

Motivation

The whole purpose of any CLS is to avoid having to pass a context object through application modules. Exposing request context through the req object kinda kills the whole idea. Having it available on app makes things a bit better as it could provide isolation guarantees for multiple Fastify instances running in a single node process, but it looks like the underlying asynchronous-local-storage library has a single global AsyncLocalStorage instance, which means no isolation as such.

Example

const { ctx } = require('fastify-request-context')

// Later in route handler
const foo = ctx.get('bar')

Prerequisites

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

🚀 Feature Proposal

• pass AsyncResource factory
• pass AsyncResource.type

index.js

fastify.addHook(hook, (req, res, done) => {
    const defaultStoreValues = hasDefaultStoreValuesFactory
      ? opts.defaultStoreValues(req)
      : opts.defaultStoreValues

    asyncLocalStorage.run({ ...defaultStoreValues }, () => {
      const asyncResource =
        opts.createResource != null
          ? opts.createResource(req, requestContext)
          : new AsyncResource(
              opts.resourceType != null ? opts.resourceType : 'fastify-request-context',
            )
      req[asyncResourceSymbol] = asyncResource
      asyncResource.runInAsyncScope(done, req.raw)
    })
  })

Motivation

When using TypeORM or Prisma, logging can be customized: there is a way to use the triggerId of the AsyncContext to find the AsyncResource in order to pass the request.id to the logger. This would require either changing the type of the AsyncResource, or a factory that allows developers to create AsyncResources in whatever form the developers want.

Example

// async_hooks.createHook
watcher
      .setupInitHook()
      .setupBeforeHook()
      .setupAfterHook()
      .setupDestroyHook()
      .setupPromiseResolveHook()
      .start()
      .on(
        'INIT',
        ({
          asyncId,
          type,
          resource,
          triggerAsyncId,
        }: {
          asyncId: number;
          type: string;
          triggerAsyncId: number;
          executionAsyncId: number;
          resource: object;
        }) => {
          idMap.set(asyncId, triggerAsyncId);

          // customized AsyncResource.type
          if (type === 'fastify-request-context' || type === 'tid-async-resource') {
            resourceMap.set(asyncId, resource);
          }
        },
      )
      
// getStore
getStore<T = AsyncResource>(asyncId: number) {
    let resource: object | undefined = resourceMap.get(asyncId);

    if (resource != null) {
      return resource as T;
    }

    let id = idMap.get(asyncId);
    let sentinel = 0;

    while (id != null && sentinel < 1000) {
      resource = resourceMap.get(id);

      if (resource != null) {
        return resource as T;
      }

      id = idMap.get(id);
      sentinel += 1;
    }

    return undefined;
  }

Prerequisites

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

Issue

https://github.com/fastify/fastify-request-context/blob/v3.1.1/package.json#L38

Is it possible to publish 3.1.1 to npm?

https://www.npmjs.com/package/@fastify/request-context

3.1.0 resolves to fastify-plugin v4.

Thank you!

Encountering this issue while trying to use fastify-request-context with fastify 3.x
Error: fastify-plugin: fastify-request-context - expected '2.x' fastify version, '3.1.1' is installed

Prerequisites

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

Fastify version

4.15.0

Plugin version

4.2.0

Node.js version

18.14.0

Operating system

macOS

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

11.3

Description

when register multipart after context, the router frequently lost context info

Steps to Reproduce

const Fastify = require('fastify').default
const MultipartSupport = require('@fastify/multipart')
const { requestContext } = require('@fastify/request-context')
const RequestContextSupport = require('@fastify/request-context')

const server = Fastify({ logger: true })


server.register(RequestContextSupport, {
  defaultStoreValues: {
    user: { id: 0 },
  },
})

server.addHook('onRequest', async (req, reply) => {
  req.requestContext.set('user', { id: 1 })
})

server.register(MultipartSupport, {
  addToBody: true,
})

server.post('/', async (req, reply) => {
  console.log(req.requestContext.get('user'))

  return { message: 'ok' }
})

server.listen({ host: '0.0.0.0', port: 3002 })

use postman to send any file to http://127.0.0.1/,
console.log(req.requestContext.get('user')) sometimes are undefined

Expected Behavior

No response

Prerequisites

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

Fastify version

3.x.x

Node.js version

16.2.0

Operating system

Windows

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

Linux, Windows, doesn't matter

Description

All tests are failing when being run on Node 16.2.0. Seems like all stored values are lost and are being resolved to undefined.

Considering that tests for https://github.com/kibertoad/asynchronous-local-storage are still passing with 16.2.0, looks like this particular plugin is doing something wrong.

Steps to Reproduce

Execute existing test suite on Node 16.2.0

Expected Behavior

Test suite passes.

https://github.com/vicanso/async-local-storage is archived.

Working Example with the native AsyncLocalStorage:

// ctx.ts
import { AsyncLocalStorage } from "async_hooks";

const asyncLocalStorage = new AsyncLocalStorage<Map<string, unknown>>();

export const setInCtx = <T = unknown>(key: string, value: T): void => {
  asyncLocalStorage.getStore()?.set(key, value);
}

export const getFromCtx = <T = unknown>(key: string): T | undefined => asyncLocalStorage.getStore()?.get(key) as T;

export const initReqContext = (req: any, _res: any, next: any) => {
  asyncLocalStorage.run(new Map(), () => {
    const store = asyncLocalStorage.getStore();
    if (store){
      store.set("key",  "value");
    }
    next();
  });
}

// server.ts / index.ts 

import { initReqContext } from './ctx';

// this only works with preHandler hooks
server.addHook("preHandler", initReqContext)

you can start using get and set function to access the store

After the migration from TS to JS, the package.json files array should be updated to publish the module:

https://github.com/kibertoad/fastify-request-context/blob/b7e443bcff6d2008272ff73479fbd0d61c861d5e/package.json#L70-L73

Prerequisites

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

Issue

Hi,

When using a default value:

  fastify.register(fastifyRequestContextPlugin, 
  {
    hook: 'onRequest', 
    defaultStoreValues: {
      myDefault: {}
    }
  })

and later in the request handler doing this:

requestContext.get('myDefault').persistentData = "something between requests"

That persistentData property will "leak" between requests, I know this is not the intended use but if it is possible to prevent that, it could prevent a bug or exploitation.