Skip serialization for json string #1937

YouCanKeepSilence · 2019-11-12T13:19:16Z

Checklist

run npm run test and npm run benchmark
tests and/or benchmarks are included
documentation is changed or added
commit message and code follows Code of conduct

lib/validation.js

YouCanKeepSilence · 2019-11-12T14:13:43Z

I'll try to add this as explicit param.

jsumners

This seems like a fair solution for people that know their output does not need to be serialized. However, the documentation will need to be updated.

lib/reply.js

docs/Reply.md

Co-Authored-By: James Sumners <james@sumners.email>

RafaelGSS · 2019-11-13T13:44:04Z

test/internals/reply.test.js

@@ -544,6 +544,30 @@ test('plain string with content type application/json should be serialized as js
  })
 })

+test('string with content type application/json and skipSerialization() called should NOT be serialized as json', t => {


Could add another test to JSON.stringfy({ test: true })? Just to prevent something

What you mean? Simple send serialized via JSON.stringify data with skipSerialization()?

YouCanKeepSilence · 2019-11-13T14:15:43Z

Don't know the reason why checks fails? Can't find the error log. At local machine 'test', 'test:ci', 'benchmark' scripts run correctly

RafaelGSS · 2019-11-13T14:29:13Z

Don't know the reason why checks fails? Can't find the error log. At local machine 'test', 'test:ci', 'benchmark' scripts run correctly

I think that is a little bug inside our CI, timeout or something like it. Don't worry.

YouCanKeepSilence · 2019-11-13T15:14:32Z

Don't know the reason why checks fails? Can't find the error log. At local machine 'test', 'test:ci', 'benchmark' scripts run correctly

I think that is a little bug inside our CI, timeout or something like it. Don't worry.

So i should just wait?

mcollina · 2019-11-13T16:03:51Z

lib/reply.js

@@ -195,6 +197,12 @@ Reply.prototype.headers = function (headers) {
  return this
 }

+Reply.prototype.skipSerialization = function () {
+  this[kReplyskipSerialization] = true
+  this.context[kReplyskipSerialization] = true


Unfortunately this won't work. The context is shared between multiple execution of the handler and multiple reply objects. Can you remove it? The alternative is to move this to the Context itself, and set this option during the route creation.

You told about this.context? What about just this? Will it work If i pass from this[kReplyskipSerialization] to serialize() method as optional params?

I would actually change the serialize function to receive the Reply object instead.

Changed it to pass this param via this of reply object. Added test to check that skipSerialization call doesn't affect next responses.

…am. Updated doc. Actualized tests

mcollina

LGTM

YouCanKeepSilence · 2019-11-14T10:40:25Z

So what about checks? What is incorrect?

mcollina · 2019-11-14T11:13:41Z

It's just flaky CI

YouCanKeepSilence · 2019-11-14T11:15:02Z

It's just flaky CI

So it'll be approved later? Or you just merge it with ignore this checks?

mcollina · 2019-11-14T11:16:01Z

Let's wait another review (cc @jsumners), then it can land.

jsumners

LGTM

test/internals/reply.test.js

RafaelGSS

LGTM

YouCanKeepSilence · 2019-11-18T22:25:23Z

What do you think?

I agree with @Eomm, so as i think more acceptable solution now is add explicit flag, and later change behaviour of this functionality. So if we talking about explicit flag i made it in current PR:)

YouCanKeepSilence · 2019-11-22T13:49:53Z

So, what about merge this fix? Or you need changes?

mcollina · 2019-11-22T17:57:08Z

@delvedor and myself had a quick chat about this, and we came to the conclusion that this is a bug and that test is wrong.
This was added to fix #540 and #541 (a long time ago). None had the intention of implementing the behavior that this PR is fixing.
I think changing that serialization is skipped when the content-type is specified is what we intended.
Therefore, we propose to treat that as a bugfix and release it in v2.

YouCanKeepSilence · 2019-11-22T18:56:54Z

@delvedor and myself had a quick chat about this, and we came to the conclusion that this is a bug and that test is wrong.
This was added to fix #540 and #541 (a long time ago). None had the intention of implementing the behavior that this PR is fixing.
I think changing that serialization is skipped when the content-type is specified is what we intended.
Therefore, we propose to treat that as a bugfix and release it in v2.

So you'll make it by yourself and my PR is not needed?(

delvedor · 2019-11-23T16:22:14Z

@YouCanKeepSilence you can update this pr and follow the suggestions I wrote in #1937 (comment).
Otherwise, you can open a new pr and reference this one :)

…it removed). Now if content-type is application/json and typeof payload is string it will not be serialize

YouCanKeepSilence · 2019-12-05T09:21:09Z

I still don't know why checks fails, all test and benchs were ok.

delvedor

@YouCanKeepSilence since the payload is already serialized, there is no need to call the preSerialization hook.
You can follow what I wrote in #1937 (comment), and if we detect that the content-type is configured, and the payload is a string already, we should call the onSend hook directly, similar to what we are doing for the text-plain handling.

delvedor

LGTM

mcollina

LGTM

adrai · 2020-02-19T07:49:44Z

Is it intended that with this fix also such a response now comes different with v2.12?

example:

reply.type('application/json')
reply.send('5d4a5dcf-f265-499d-94fe-644144c323dd')

response with v2.12: 5d4a5dcf-f265-499d-94fe-644144c323dd
response with v2.11: "5d4a5dcf-f265-499d-94fe-644144c323dd"

in v2.11 it was a json and in v2.12 it is now a string

jsumners · 2020-02-19T12:51:47Z

That definitely seems like an out of spec regression.

mcollina · 2020-02-19T14:42:34Z

@jsumners it’s exacty what this PR does. We might consider reverting it in v2 and applying it only in v3.

jsumners · 2020-02-19T14:47:19Z

@mcollina I thought the PR made it an option to skip serialization of a string.

Invalid JSON:

foo

Valid JSON:

"foo"

adrai · 2020-02-19T14:56:00Z

for me there is no problem... have a workaround...
imo this PR is ok for v2, but maybe there should also be a fix to handle a string correctly in case the content type is application/json

perhaps by testing the string with something like this:

function isJSON(str) {
  if (str === '') return false;
  str = str.replace(/\\(?:["\\\/bfnrt]|u[0-9a-fA-F]{4})/g, '@');
  str = str.replace(/"[^"\\\n\r]*"|true|false|null|-?\d+(?:\.\d*)?(?:[eE][+\-]?\d+)?/g, ']');
  str = str.replace(/(?:^|:|,)(?:\s*\[)+/g, '');
  return (/^[\],:{}\s]*$/).test(str);
}

"copied" from: https://github.com/prototypejs/prototype/blob/dee2f7d8611248abce81287e1be4156011953c90/src/prototype/lang/string.js#L715

github-actions · 2022-02-10T09:07:20Z

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

Fixed issue fastify#1936

cefd23a

YouCanKeepSilence mentioned this pull request Nov 12, 2019

Unnecessary json stringify by content-type #1936

Closed

RafaelGSS reviewed Nov 12, 2019

View reviewed changes

lib/validation.js Outdated Show resolved Hide resolved

jsumners reviewed Nov 12, 2019

View reviewed changes

lib/validation.js Outdated Show resolved Hide resolved

mcollina requested changes Nov 12, 2019

View reviewed changes

lib/validation.js Outdated Show resolved Hide resolved

Another fix for issue fastify#1936. Improved performance

512f0d0

YouCanKeepSilence requested a review from mcollina November 13, 2019 09:02

jsumners reviewed Nov 13, 2019

View reviewed changes

lib/reply.js Outdated Show resolved Hide resolved

Renamed method. Added docs

51dc138

jsumners reviewed Nov 13, 2019

View reviewed changes

docs/Reply.md Outdated Show resolved Hide resolved

docs/Reply.md Outdated Show resolved Hide resolved

Update docs/Reply.md

d74b342

Co-Authored-By: James Sumners <james@sumners.email>

RafaelGSS requested changes Nov 13, 2019

View reviewed changes

YouCanKeepSilence added 2 commits November 13, 2019 16:46

Update docs/Reply.md (strange bug with CI checks?)

cdbf738

Added JSON.stringify(object) with skipSerialization()

7e50287

mcollina reviewed Nov 13, 2019

View reviewed changes

Changed skipSerialization function. Now uses this context to pass par…

428e84e

…am. Updated doc. Actualized tests

YouCanKeepSilence requested a review from mcollina November 13, 2019 20:21

mcollina approved these changes Nov 14, 2019

View reviewed changes

jsumners approved these changes Nov 14, 2019

View reviewed changes

jsumners reviewed Nov 14, 2019

View reviewed changes

test/internals/reply.test.js Outdated Show resolved Hide resolved

removed forgotten t.setTimeout(0) at reply.test

eafafe2

RafaelGSS approved these changes Nov 14, 2019

View reviewed changes

Eomm added the bugfix Issue or PR that should land as semver patch label Nov 28, 2019

Update behaviour. Now don't need to call skipSerialization manually (…

b76b861

…it removed). Now if content-type is application/json and typeof payload is string it will not be serialize

YouCanKeepSilence requested a review from delvedor December 5, 2019 09:21

delvedor requested changes Dec 5, 2019

View reviewed changes

Changed place of check typeof.

888b913

YouCanKeepSilence requested a review from delvedor December 5, 2019 11:03

delvedor approved these changes Dec 9, 2019

View reviewed changes

delvedor requested a review from a team December 9, 2019 09:37

mcollina approved these changes Dec 9, 2019

View reviewed changes

Eomm merged commit c5d2820 into fastify:master Dec 10, 2019

theliuk pushed a commit to theliuk/fastify that referenced this pull request Dec 13, 2019

fix: skip serialization for json string (fastify#1937)

7a7366a

nokome mentioned this pull request Feb 25, 2020

fix(dependencies): update dependency fastify to ^2.13.1 stencila/executa#95

Merged

1 task

This was referenced Mar 20, 2020

[Snyk] Upgrade fastify from 2.11.0 to 2.12.0 lwojcik/sc2pte-panel-backend#53

Closed

[Snyk] Upgrade fastify from 2.12.0 to 2.12.1 mikaelvesavuori/node-simple-webserver#3

Merged

snyk-bot mentioned this pull request Mar 29, 2020

[Snyk] Upgrade fastify from 2.11.0 to 2.12.1 lwojcik/sc2pte-panel-backend#57

Closed

snyk-bot mentioned this pull request Apr 12, 2020

[Snyk] Upgrade fastify from 2.11.0 to 2.13.0 lwojcik/sc2pte-panel-backend#62

Closed

snyk-bot mentioned this pull request May 5, 2020

[Snyk] Upgrade fastify from 2.11.0 to 2.13.1 lwojcik/sc2pte-panel-backend#68

Closed

github-actions bot locked as resolved and limited conversation to collaborators Feb 10, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Skip serialization for json string #1937

Skip serialization for json string #1937

YouCanKeepSilence commented Nov 12, 2019 •

edited

YouCanKeepSilence commented Nov 12, 2019

jsumners left a comment

RafaelGSS Nov 13, 2019

YouCanKeepSilence Nov 13, 2019

RafaelGSS Nov 13, 2019

YouCanKeepSilence commented Nov 13, 2019

RafaelGSS commented Nov 13, 2019

YouCanKeepSilence commented Nov 13, 2019

mcollina Nov 13, 2019

YouCanKeepSilence Nov 13, 2019

mcollina Nov 13, 2019

YouCanKeepSilence Nov 13, 2019

mcollina left a comment

YouCanKeepSilence commented Nov 14, 2019

mcollina commented Nov 14, 2019

YouCanKeepSilence commented Nov 14, 2019

mcollina commented Nov 14, 2019

jsumners left a comment

RafaelGSS left a comment

YouCanKeepSilence commented Nov 18, 2019

YouCanKeepSilence commented Nov 22, 2019

mcollina commented Nov 22, 2019

YouCanKeepSilence commented Nov 22, 2019

delvedor commented Nov 23, 2019

YouCanKeepSilence commented Dec 5, 2019

delvedor left a comment

delvedor left a comment

mcollina left a comment

adrai commented Feb 19, 2020 •

edited

jsumners commented Feb 19, 2020

mcollina commented Feb 19, 2020

jsumners commented Feb 19, 2020

adrai commented Feb 19, 2020 •

edited

github-actions bot commented Feb 10, 2022

Skip serialization for json string #1937

Skip serialization for json string #1937

Conversation

YouCanKeepSilence commented Nov 12, 2019 • edited

Checklist

YouCanKeepSilence commented Nov 12, 2019

jsumners left a comment

Choose a reason for hiding this comment

RafaelGSS Nov 13, 2019

Choose a reason for hiding this comment

YouCanKeepSilence Nov 13, 2019

Choose a reason for hiding this comment

RafaelGSS Nov 13, 2019

Choose a reason for hiding this comment

YouCanKeepSilence commented Nov 13, 2019

RafaelGSS commented Nov 13, 2019

YouCanKeepSilence commented Nov 13, 2019

mcollina Nov 13, 2019

Choose a reason for hiding this comment

YouCanKeepSilence Nov 13, 2019

Choose a reason for hiding this comment

mcollina Nov 13, 2019

Choose a reason for hiding this comment

YouCanKeepSilence Nov 13, 2019

Choose a reason for hiding this comment

mcollina left a comment

Choose a reason for hiding this comment

YouCanKeepSilence commented Nov 14, 2019

mcollina commented Nov 14, 2019

YouCanKeepSilence commented Nov 14, 2019

mcollina commented Nov 14, 2019

jsumners left a comment

Choose a reason for hiding this comment

RafaelGSS left a comment

Choose a reason for hiding this comment

YouCanKeepSilence commented Nov 18, 2019

YouCanKeepSilence commented Nov 22, 2019

mcollina commented Nov 22, 2019

YouCanKeepSilence commented Nov 22, 2019

delvedor commented Nov 23, 2019

YouCanKeepSilence commented Dec 5, 2019

delvedor left a comment

Choose a reason for hiding this comment

delvedor left a comment

Choose a reason for hiding this comment

mcollina left a comment

Choose a reason for hiding this comment

adrai commented Feb 19, 2020 • edited

jsumners commented Feb 19, 2020

mcollina commented Feb 19, 2020

jsumners commented Feb 19, 2020

adrai commented Feb 19, 2020 • edited

github-actions bot commented Feb 10, 2022

YouCanKeepSilence commented Nov 12, 2019 •

edited

adrai commented Feb 19, 2020 •

edited

adrai commented Feb 19, 2020 •

edited