elysiajs / elysia

Ergonomic Framework for Humans
https://elysiajs.com
MIT License
10.63k stars 226 forks source link

`normalize: true` mutates objects used in responses #897

Closed crishoj closed 1 week ago

crishoj commented 1 month ago

What version of Elysia is running?

1.1.23

What platform is your computer?

macOS

What steps can reproduce the bug?

With normalize true (the default), Elysia will mutate objects used in responses.

Consider this example with a long-lived object people:

import {Elysia, t} from 'elysia'

const people = new Map<number, object>([
    [1, {name: 'Jane Doe', age: 80}],
])

const app = new Elysia()
    .get('/names/:id', ({params: {id}}) => {
        return people.get(id)
    }, {
        params: t.Object({id: t.Number()}),
        response: t.Object({name: t.String()})
    })

console.log('people before request:', people)
await app.handle(new Request('http://localhost/names/1'))
console.log('people after request:', people)

What is the expected behavior?

people after request: Map(1) {
  1: {
    name: "Jane Doe",
    age: 80,
  },
}

From a developer perspective, I expect Elysia to not directly mutate objects used in a response body.

If response normalization is active, I expect cleaning to happen on a (deep) clone of the object passed as response body.

What do you see instead?

people after request: Map(1) {
  1: {
    name: "Jane Doe",
    // ⚠️ Notice that the `age` property is now missing from the long-lived object
  },
}

Objects used in responses are directly mutated — additional properties are deleted by the Typebox cleaner:

if (IsSchema(additionalProperties) && Check(additionalProperties, references, value[key])) {
    value[key] = Visit(additionalProperties, references, value[key]);
    continue;
}
delete value[key];

Additional information

This caught me off guard and led to subsequent errors in my app where properties in a long-lived object were suddenly undefined.

For improved DX, please consider doing a structuredClone() of objects before normalization.

Have you try removing the node_modules and bun.lockb and try again yet?

Yes

crishoj commented 1 month ago

To be clear: Developers likely don't expect side-effects from using objects as response bodies. This can lead to subtle and surprising errors with long-lived objects, and kind of breaks strong typings in the app codebase.

I would suggest:

crishoj commented 1 month ago

@SaltyAom — what do you think about the proposed change https://github.com/crishoj/elysia/commit/d759a6f59bf01ca3e7486375a2533cbdf327af3c?

Also removed redundant logic — all three cases do exactly the same thing.

crishoj commented 4 weeks ago

Updated with example to reproduce.

kravetsone commented 1 week ago

I think we shouldn't do it by default. It as typebox do And structuredClone is perfomance killer

crishoj commented 1 week ago

I would be happy with an option as well.

Perhaps:

kravetsone commented 1 week ago

I would be happy with an option as well.

Perhaps:

  • clean → mutates
  • normalize → clone then clean (doesn't mutate)
  • none → no normalization, extra attributes in responses causes validation error

Yeah it's interesting Something like union type But clean should be by default

If u want to use clean and no mutate something u can just place structuredClone in u handler/hook

Perfomance is a important goal of elysia (like JIT code-generation at runtime) so no need to call structuredClone on everything

crishoj commented 1 week ago

I would argue that DX should also be an important goal. Frankly speaking, seeing long-lived objects referenced in responses being mutated by Elysia/Typebox was a terrifying gotcha for me.

Perhaps a reasonable default could be to error in case responses don't match the type sanctioned by the schema.

Then, let the developer make an explicit choice:

kravetsone commented 1 week ago

I would argue that DX should also be an important goal. Frankly speaking, seeing long-lived objects referenced in responses being mutated by Elysia/Typebox was a terrifying gotcha for me.

Perhaps a reasonable default could be to error in case responses don't match the type sanctioned by the schema.

Then, let the developer make an explicit choice:

  • Use mutating cleaning to minimize response time.
  • Use clone+clean to maintain type safety.

Long-lived objects is not so often case If u need to prevent mutation just clone it But i agree that it should be more warned in the docs