feat(serializers): avoid implicit sanitization

[gerardolima]

When serializers are defined for HTTP Request or HTTP Response, do not run the correspondent stdSerializers before calling the provided serializer functions.

ISSUE #1991

[gerardolima]

ok, I was running isolated tests and only now I noticed there were some assertions on the default serialisers; I am fixing them right now and soon the tests will be fixed; sorry for the silly mistake

[gerardolima]

hey, @mcollina, are these the changes you expected? is there something that needs improvement or still missing?

[gerardolima]

The issue mentioned deprecating the current implementation and switching to the new one in the next major. I don't see this being implemented in this PR. Have I missed it?

This changeset only modifies the following behviour: neither logged Request nor Response objects are not transformed by their corresponding stdSerializers unconditionally, at the start of LOG (lib/tools.js).

This change in behaviour is asserted in test/http.test.js, lines 86 and 240, with tests that create an arbitrary property that should be removed from them, if their standard serializers ran.

[jsumners]

This changeset only modifies the following behviour: neither logged Request nor Response objects are not transformed by their corresponding stdSerializers unconditionally, at the start of LOG (lib/tools.js).

This change in behaviour is asserted in test/http.test.js, lines 86 and 240, with tests that create an arbitrary property that should be removed from them, if their standard serializers ran.

This is a breaking change, correct?

[gerardolima]

This is a breaking change, correct?

Yes, this is correct. When custom serializers are provided and they depend on the changes made by the standard serializers on the target objects (ie: to sanitize them?), they will be impacted.

Though running standard serializers when custom serializers are given is not exaclty obvious behaviour -- and I think nor documented --, I understand there might be code in the wild that dependes on it (Hyrum's Law) and publishing this as breaking change should be the safest strategy.

[jsumners]

Yes, this is correct. When custom serializers are provided and they depend on the changes made by the standard serializers on the target objects (ie: to sanitize them?), they will be impacted.

Then we are missing the deprecation and opt-in for the current version.

[gerardolima]

Then we are missing the deprecation and opt-in for the current version.

I'm sorry, but I'm not familiar with the versioning process in Pino -- I've already checked CONTRIBUTING.md. Would it suffice updating major version in package.json? Could you, please help me?

[jsumners]

Either @mcollina or I will handle incrementing the version when we publish. What the original discussion called for, and what is missing here, is the deprecation of the current behavior so that people are alerted to the change in default behavior in the next major release. See the following examples:

https://github.com/pinojs/pino/blob/ba31fc11994893b03b104dbf36545503a4aa24dd/lib/tools.js#L416-L423

https://github.com/pinojs/pino/blob/ba31fc11994893b03b104dbf36545503a4aa24dd/lib/tools.js#L430-L434

We could introduce a PR that only adds the deprecation notice, issue a release with that, and then merge this one (with it removing the notice) and issue a new major. But we still haven't finished updating all of the org's maintained modules for v9. So I'm not sure we have much appetite for a new major so soon after v9 right now.

[gerardolima]

We could introduce a PR that only adds the deprecation notice, issue a release with that, and then merge this one (with it removing the notice) and issue a new major. But we still haven't finished updating all of the org's maintained modules for v9. So I'm not sure we have much appetite for a new major so soon after v9 right now.

hey, @jsumners, I'm sorry, but I still don't get it. No property has been deprecated nor renamed -- and I believe the explanation of the change in behaviour would be somewhat lengthy for a warning message. Also, I don't see any example of process.emitWarning nor warning. in the the actual codebase ... which should I use? which number should I use for PINODEPxxx?

[jsumners]

1. https://github.com/pinojs/pino/issues/1991#issuecomment-2243151339 suggested that the original functionality be deprecated. That means the current behavior does not change, but will generate a warning when the functionality is used.
2. This PR changes the published functionality, thus introducing a breaking change and necessitating a new major version to be published after it is merged.
3. We avoid needing to publish a new major version by deprecating the current functionality and providing an opt-in path to the functionality provided in this PR.
4. We utilize https://www.npmjs.com/package/process-warning to issue deprecation warnings. I provided links to historical code where we have issued deprecation warnings so that you can learn how to implement such a deprecation in this feature. A clearer example may be seen in https://github.com/pinojs/pino/commit/6c538159dc83b190889781f80051962138714bb8. The next available deprecation code is currently commented out in the deprecations source file (PINODEP010).

[gerardolima]

hey, @jsumners and @mcollina, I created the opts property future to allow consumers to chose these breaking changes as opt-in, and avoid bumping the major version right now. This increased the size of the changeset, but I think it was a sensible way for consumers of Pino to chose the between the new and old behaviour when creating the top-level Pino object. Maybe this feature should be subject of a PR of its own, to be merged before these changes in the serialization of requests/responses?

[gerardolima]

hey, @mcollina, this PR got stalled and outdated. I can rebase my branch and fix the conflicts, but first I'd like to understand whether you are still interested in changing the behaviour of stdSerializers for Request and Response. A no response is perfectly fine :)

[jsumners]

Sorry for the delay. I have it on my priority list to address Pino issues first when I am in a position to work on OSS things.

[gerardolima]

hey, @mcollina and @jsumners, I rebased the branch and resolved conflicts; this PR is ready for review again. Summary of changes:

1. Add property future to the constructor options. This property enables opt-in behaviour changes (all keys are false by default);
2. When logging a Request or Response object, depending on future.skipUnconditionalStdSerializers:
   • when false
     1. Behaviour is not changed (based on mapHttpRequest, and mapHttpResponse); and
     2. The warning PINODEP010 is emmitted;
   • when true
     1. Behaviour is changed and value is serialized either by the corresponding stdSerializer or by a serializer provided in constructor options (neither mapHttpRequest, nor mapHttpResponse is executed).

[mcollina]

Can you drop the future prefix?

[gerardolima]

Can you drop the future prefix?

sure, @mcollina, but how should I guard this breaking change not to force bumping a major version, as @jsumners pointed? Is it just a matter of naming? Should I use, some "opt-in", instead of "future" option?

[gerardolima]

hey, @mcollina and @jsumners, just a friendly reminder that this PR is half-an-year old, already; please let me know whether this is still relevant to you and whether you want any change 🙏🏼

[gerardolima]

hello again, @mcollina and @jsumners, it's been a while since we've last spoken; I understand that you folks are quite busy, but I wonder whether it still makes sense to keep this PR open.