Skip to content

fix: hasRequestDecorator/hasReplyDecorator misses constructor-assigned built-in properties#6753

Merged
Eomm merged 2 commits into
fastify:mainfrom
LeSingh1:fix/decorator-instance-property-check
Jun 28, 2026
Merged

fix: hasRequestDecorator/hasReplyDecorator misses constructor-assigned built-in properties#6753
Eomm merged 2 commits into
fastify:mainfrom
LeSingh1:fix/decorator-instance-property-check

Conversation

@LeSingh1

@LeSingh1 LeSingh1 commented Jun 4, 2026

Copy link
Copy Markdown
Contributor

What

hasRequestDecorator and hasReplyDecorator only check the constructor prototype and the decorator props list. But several core properties — req.id, req.params, req.raw, req.query, req.log, req.body, reply.raw, reply.request, reply.log — are assigned directly in the Request and Reply constructors, not on their prototypes.

This means:

fastify.hasRequestDecorator('id')   // returns false (wrong)
fastify.decorateRequest('id', null) // silently succeeds (wrong)
// then at request time:
// TypeError: Cannot set property id... or req.id is null

Root cause

checkRequestExistence and decorateConstructor in lib/decorate.js use Object.hasOwn(prototype, name) and name in prototype. Properties set in the constructor body are never on the prototype, so these checks miss them.

Fix

Add an instanceProperties set to Request and Reply listing their constructor-assigned string-keyed properties. decorateConstructor and the checkRequest/ReplyExistence helpers now also check this set (walking the parent chain for derived constructors).

After the fix:

fastify.hasRequestDecorator('id')     // true
fastify.hasReplyDecorator('raw')      // true
fastify.decorateRequest('id', null)   // throws FST_ERR_DEC_ALREADY_PRESENT
fastify.decorateReply('raw', null)    // throws FST_ERR_DEC_ALREADY_PRESENT
fastify.decorateRequest('myExtra', null) // still works fine

All 52 existing decorator tests pass. Six new node:test tests cover the bug and the fix.

…-assigned built-in properties

Properties like req.id, req.params, req.body, reply.raw, and reply.log
are set directly in the Request and Reply constructors rather than on
their prototypes. Because of this, hasRequestDecorator('id') returned
false and decorateRequest('id', null) silently succeeded, overwriting
the built-in value and causing a TypeError at request time.

This adds an instanceProperties set to Request and Reply listing those
constructor-assigned string-keyed properties, and teaches
decorateConstructor and the existence helpers to check that set.
hasRequestDecorator('id') now returns true, and decorateRequest('id')
now throws FST_ERR_DEC_ALREADY_PRESENT as expected.
Comment thread lib/decorate.js

@mcollina mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes incorrect decorator-existence checks for core Request/Reply properties that are assigned in their constructors (rather than on prototypes), ensuring hasRequestDecorator / hasReplyDecorator and decoration collision detection behave correctly for built-in fields like req.id and reply.raw.

Changes:

  • Add explicit instanceProperties sets to lib/request.js and lib/reply.js to enumerate constructor-assigned built-in properties.
  • Extend lib/decorate.js checks to treat those instance properties as already-present when calling has*Decorator and when decorating request/reply constructors.
  • Add new tests covering has*Decorator correctness and duplicate-decoration protection for these built-ins.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File Description
test/decorator-instance-properties.test.js Adds regression tests for built-in constructor-assigned request/reply properties and decoration collision behavior.
lib/request.js Introduces Request.instanceProperties to list built-in constructor-assigned request fields.
lib/reply.js Introduces Reply.instanceProperties to list built-in constructor-assigned reply fields.
lib/decorate.js Updates decoration/existence logic to consider constructor-assigned instance properties (including walking parent constructors).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@Eomm Eomm added the bugfix Issue or PR that should land as semver patch label Jun 15, 2026
@metcoder95

Copy link
Copy Markdown
Member

ping @jean-michelet

@jean-michelet

Copy link
Copy Markdown
Member

I didn't block it, you can merge.

@Eomm Eomm merged commit cc8d9a3 into fastify:main Jun 28, 2026
40 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bugfix Issue or PR that should land as semver patch

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants