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

feature: allow custom hook name #234

Merged
merged 12 commits into from
Nov 8, 2022

Conversation

giulianok
Copy link
Contributor

The goal with this PR is to be able to change the hook created by the plugin, for example, being able to execute the code on preHandler rather than onRequest.

I currently have a case where I need to build a redux store on preHandler, injected into the request object for later consuption, and use it in this plugin to perform some checks

Checklist

Copy link
Member

@climba03003 climba03003 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please do not change the default. Otherwise, it is a breaking change.

index.js Outdated Show resolved Hide resolved
index.js Outdated
}
})
}

function onRequest (fastify, options, req, reply, next) {
function handler (fastify, options, req, reply, next) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dont name it handler. Name it addCorsHeaders or so.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gonna call it addCorsHeadersHandler since there's a function already named addCorsHeaders, and the word handler is good to have since it's mainly called from a hook handler (like onRequest)

index.js Outdated
function handleCorsOptionsDelegator (optionsResolver, fastify) {
fastify.addHook('onRequest', function onRequestCors (req, reply, next) {
function handleCorsOptionsDelegator (optionsResolver, fastify, { hookName } = { hookName: defaultHookName }) {
fastify.addHook(hookName, function handleCors (req, reply, next) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Were do we define the variable hookName?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's being passed as parameter and defaults to defaultHookName

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dont see a const hookName= in your new code?!

README.md Outdated

const fastify = Fastify()
await fastify.register(cors, {
hookName: 'preHandler',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should call it not hookName but simply hook as we do it in other plugins, like in fastify-rate-limit

@giulianok giulianok requested review from climba03003 and Uzlopak and removed request for climba03003 and Uzlopak November 4, 2022 19:35
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Uzlopak
Copy link
Contributor

Uzlopak commented Nov 5, 2022

Something strange:

If I add a test case like this:

test('Should support custom hook with dynamic config', t => {
  t.plan(22)

  const configs = [{
    origin: 'example.com',
    methods: 'GET',
    credentials: true,
    exposedHeaders: ['foo', 'bar'],
    allowedHeaders: ['baz', 'woo'],
    maxAge: 123
  }, {
    origin: 'sample.com',
    methods: 'GET',
    credentials: true,
    exposedHeaders: ['zoo', 'bar'],
    allowedHeaders: ['baz', 'foo'],
    maxAge: 321
  }]

  const fastify = Fastify()
  let requestId = 0
  const configDelegation = function (req, cb) {
    // request should have id
    t.ok(req.id)
    // request should not have send
    t.notOk(req.send)
    const config = configs[requestId]
    requestId++
    if (config) {
      cb(null, config)
    } else {
      cb(new Error('ouch'))
    }
  }
  fastify.register(cors, {
    delegator: configDelegation
  })

  fastify.addHook('onRequest', (request, reply, done) => {
    t.equal(request[kFastifyContext].preHandler, null)
    t.equal(request[kFastifyContext].onRequest.length, 2)
    done()
  })

  fastify.get('/', (req, reply) => {
    reply.send('ok')
  })

  fastify.inject({
    method: 'GET',
    url: '/'
  }, (err, res) => {
    t.error(err)
    delete res.headers.date
    t.equal(res.statusCode, 200)
    t.equal(res.payload, 'ok')
    t.match(res.headers, {
      'access-control-allow-origin': 'example.com',
      vary: 'Origin',
      'access-control-allow-credentials': 'true',
      'access-control-expose-headers': 'foo, bar',
      'content-length': '2'
    })
  })

  fastify.inject({
    method: 'OPTIONS',
    url: '/',
    headers: {
      'access-control-request-method': 'GET',
      origin: 'example.com'
    }
  }, (err, res) => {
    t.error(err)
    delete res.headers.date
    t.equal(res.statusCode, 204)
    t.equal(res.payload, '')
    t.match(res.headers, {
      'access-control-allow-origin': 'sample.com',
      vary: 'Origin',
      'access-control-allow-credentials': 'true',
      'access-control-expose-headers': 'zoo, bar',
      'access-control-allow-methods': 'GET',
      'access-control-allow-headers': 'baz, foo',
      'access-control-max-age': '321',
      'content-length': '0'
    })
  })

  fastify.inject({
    method: 'GET',
    url: '/',
    headers: {
      'access-control-request-method': 'GET',
      origin: 'example.com'
    }
  }, (err, res) => {
    t.error(err)
    t.equal(res.statusCode, 500)
  })
})

then the unit tests break.

Uzlopak
Uzlopak previously requested changes Nov 5, 2022
Copy link
Contributor

@Uzlopak Uzlopak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please dont merge. I will have to review it.

@climba03003
Copy link
Member

the unit tests break.

Your OPTIONS assertion is wrong. It will not add CORS header when origin do not match. So, it will block by default.

@Uzlopak
Copy link
Contributor

Uzlopak commented Nov 7, 2022

Well. I reworked it to handle custom hooks more resilient. It is easy to oversee that some hooks also have payload as an parameter.

@climba03003
@mcollina
@Eomm
Please review again.

@Uzlopak Uzlopak dismissed their stale review November 7, 2022 15:16

I modified it heavily.

@Uzlopak Uzlopak requested a review from mcollina November 7, 2022 15:18
@Uzlopak Uzlopak requested review from Uzlopak and Eomm November 7, 2022 15:18
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

index.d.ts Outdated Show resolved Hide resolved
@Uzlopak Uzlopak merged commit ad759a7 into fastify:master Nov 8, 2022
@giulianok giulianok deleted the feature/allowCustomHookName branch November 15, 2022 03:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants