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

Bug Report: FastifyAdapter Middleware no run #7511

Closed
Zclhlmgqzc opened this issue Jul 10, 2021 · 7 comments
Closed

Bug Report: FastifyAdapter Middleware no run #7511

Zclhlmgqzc opened this issue Jul 10, 2021 · 7 comments

Comments

@Zclhlmgqzc
Copy link
Contributor

Zclhlmgqzc commented Jul 10, 2021

Bug Report

Current behavior

GlobalPrefix '/test'
Middleware forRoutes('/')

http /test/a

I want get the url '/a'
wildcards should not be used at this time

import { Injectable, NestMiddleware } from '@nestjs/common';
import { Request, Response, NextFunction } from 'express';

@Injectable()
export class LoggerMiddleware implements NestMiddleware {
  use(req: Request, res: Response, next: NextFunction) {
    // GlobalPrefix '/test'

    // 1. consumer.apply(LoggerMiddleware).forRoutes('/');

    // http://127.0.0.1:3000/test/a
    // express /test/a
    // !! BUG
    // !! fastify middleware no run -> FastifyAdapter if (!re.exec(pathname + '/') && normalizedPath) { return next(); }

    // http://127.0.0.1:3000/test/
    // express /test/
    // fastify /test/

    // http://127.0.0.1:3000/test
    // express /test
    // fastify /test

    // 2. consumer.apply(LoggerMiddleware).forRoutes  [express /*] [fastify /(.*)]

    // http://127.0.0.1:3000/test/a
    // express /test/a
    // fastify /test/a

    // http://127.0.0.1:3000/test/
    // express /test/
    // fastify /

    // http://127.0.0.1:3000/test
    // express /test
    // ok
    // fastify middleware no run the same as fastify.js

    console.log(req.originalUrl);

    // 1. consumer.apply(LoggerMiddleware).forRoutes('/');

    // http://127.0.0.1:3000/test/a
    // express /a
    // !! BUG
    // !! fastify middleware no run -> FastifyAdapter if (!re.exec(pathname + '/') && normalizedPath) { return next(); }

    // http://127.0.0.1:3000/test/
    // express /
    // fastify /

    // http://127.0.0.1:3000/test
    // express /
    // fastify /

    // 2. consumer.apply(LoggerMiddleware).forRoutes  [express /*] [fastify /(.*)]

    // http://127.0.0.1:3000/test/a
    // express /
    // fastify /

    // http://127.0.0.1:3000/test/
    // express /
    // fastify /

    // http://127.0.0.1:3000/test
    // express /
    // ok
    // fastify middleware no run the same as fastify.js
    console.log(req.url);
    next();
  }
}

Input Code

Expected behavior

Possible Solution

remove

// FastifyAdapter createMiddlewareFactory
  const re = pathToRegexp(path);

...

    const queryParamsIndex = req.originalUrl.indexOf('?');
    const pathname =
      queryParamsIndex >= 0
        ? req.originalUrl.slice(0, queryParamsIndex)
        : req.originalUrl;

    if (!re.exec(pathname + '/') && normalizedPath) {
      return next();
    }

because of fastify/middie

    if (url) {
      regexp = pathToRegexp(sanitizePrefixUrl(url), [], {
        end: false,
        strict: false
      })
    }

...

    if (regexp) {
      const result = regexp.exec(url)
      if (result) {
        req.url = req.url.replace(result[0], '')
        if (req.url.startsWith('/') === false) {
          req.url = '/' + req.url
        }
        fn(req, res, that.done)
      } else {
        that.done()
      }
    }

https://github.com/fastify/middie/blob/b3d4ef14248927ecad71f328bda9d858c69c47cf/engine.js#L101

Environment


Nest version: 7.x 8.x



@Zclhlmgqzc Zclhlmgqzc added the needs triage This issue has not been looked into label Jul 10, 2021
@Tony133
Copy link
Contributor

Tony133 commented Jul 10, 2021

Hi @Zclhlmgqzc, I advise you to leave a minimal reproduction of a clonable git repository so that the core team of NestJS can evaluate the problem you have reported.

@Zclhlmgqzc
Copy link
Contributor Author

Zclhlmgqzc commented Jul 10, 2021

Hi @Zclhlmgqzc, I advise you to leave a minimal reproduction of a clonable git repository so that the core team of NestJS can evaluate the problem you have reported.

main.ts

import {
  Injectable,
  NestMiddleware,
  NestModule,
  MiddlewareConsumer,
  Module,
} from '@nestjs/common';
import { NestFactory } from '@nestjs/core';
import {
  NestFastifyApplication,
  FastifyAdapter,
} from '@nestjs/platform-fastify';
import { Request, Response, NextFunction } from 'express';

@Injectable()
class LoggerMiddleware implements NestMiddleware {
  use(req: Request, res: Response, next: NextFunction) {
    console.log(req.originalUrl);
    console.log(req.url);
    next();
  }
}

@Module({})
class AppModule implements NestModule {
  configure(consumer: MiddlewareConsumer) {
    consumer.apply(LoggerMiddleware).forRoutes('/');
  }
}

async function bootstrap() {
  const app = await NestFactory.create<NestFastifyApplication>(
    AppModule,
    new FastifyAdapter(),
  );

  // const app = await NestFactory.create(AppModule);
  app.setGlobalPrefix('/test');
  await app.listen(3000);
}

bootstrap();

fastify

const Fastify = require('fastify');

async function main() {
  const fastify = Fastify()

  await fastify.register(require('middie'))

  const globalPrefix = '/test'

  // /test/a
  // /a
  const path = '/'

  // /test/a
  // /
  // const path = '/(.*)'

  fastify.use(globalPrefix + path, function (req, res, next) {
    console.log(req.originalUrl)
    console.log(req.url)
    next()
  })

  fastify.listen(3002)
}

main()

express

const express = require('express')
const app = express()

const globalPrefix = '/test'

// /test/a
// /a
const path = '/'

// /test/a
// /
// const path = '*'

// http://127.0.0.1:3001/test/a
app.use(globalPrefix + path, function (req, res, next) {
    console.log(req.originalUrl)
    console.log(req.url)
    next()
})

app.listen(3001)

@jmcdo29
Copy link
Member

jmcdo29 commented Jul 10, 2021

Please provide a minimum reproduction repository.

@kamilmysliwiec kamilmysliwiec added needs clarification and removed needs triage This issue has not been looked into labels Jul 12, 2021
@Zclhlmgqzc
Copy link
Contributor Author

Zclhlmgqzc commented Jul 12, 2021

@kamilmysliwiec
Copy link
Member

Would you like to create a PR for this issue @Zclhlmgqzc?

@Zclhlmgqzc
Copy link
Contributor Author

Zclhlmgqzc commented Jul 13, 2021

Would you like to create a PR for this issue @Zclhlmgqzc?

I wil PR as soon as possible

@kamilmysliwiec
Copy link
Member

Lets' track this here #7562

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants