Skip to content

fix: return the correct serializer function when no content-type is defined#5229

Merged
Uzlopak merged 18 commits intofastify:mainfrom
DouglasdeMoura:make-it-return-function
Dec 28, 2023
Merged

fix: return the correct serializer function when no content-type is defined#5229
Uzlopak merged 18 commits intofastify:mainfrom
DouglasdeMoura:make-it-return-function

Conversation

@DouglasdeMoura
Copy link
Contributor

@DouglasdeMoura DouglasdeMoura commented Dec 22, 2023

Description

While I was creating am API and validating the response, I ran into Error: serializerFn is not a function. By investigating the issue, I've seen that, in my specific case, the serializerFn returned an object , as follows:

{
  'application/json': Function
}

Therefore, with this fix, I set the default content type to pick the correct serializer.

In this repo, you can find a minimal reproducible error. Start the project and send the following request:

curl --json '{"foo":"bar"}' http://localhost:3000/example/1

Checklist

@DouglasdeMoura DouglasdeMoura force-pushed the make-it-return-function branch from 86fe878 to dfcb7b2 Compare December 22, 2023 17:42
Copy link
Member

@jsumners jsumners left a comment

Choose a reason for hiding this comment

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

The "reproduction" is a lot of code. Please highlight what the actual reproduction is or narrow it down to a single snippet.

I suspect the error is due to improper usage of the API. If not, the suggested fix is rather fragile and at the very least needs a unit test to cover it.

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.

Thanks for opening a PR! Can you please add a unit test?

@DouglasdeMoura
Copy link
Contributor Author

DouglasdeMoura commented Dec 24, 2023

@jsumners the "reproduction" is the template generated by Fastify's CLI (I got this error while bootstrapping a new API with the CLI).

@mcollina will do!

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.

Can you add a test?

@DouglasdeMoura
Copy link
Contributor Author

Guys, I made a better fix for this issue. I realized that the serializers from context['response-schema'] for responses > 299 are returning inside an object (e.g. { 'application/json': Function }) when the reply[kReplyHeaders]['content-type'] returned undefined. Therefore I setted up a default content type from where the getSchemaSerializer can pick up the correct serializer function. Sorry for the delay, I didn't had time to work on this until today.

@DouglasdeMoura
Copy link
Contributor Author

@jsumners you'll be able to reproduce the error using the test I added. Here's the result running it on the main branch:

npx tap test/serializeResponse.test.js
​ FAIL ​ test/serializeResponse.test.js
 ✖ should be equal

  test/serializeResponse.test.js                 
  125 |     }, (err, response, body) => {        
  126 |       t.error(err)                       
> 127 |       t.equal(response.statusCode, 400)  
      | --------^                                
  128 |       t.same(body, {                     
  129 |         statusCode: 400,                 
  130 |         error: 'Bad Request',            

  --- expected    
  +++ actual      
  @@ -1,1 +1,1 @@ 
  -400            
  +500            

  test: test/serializeResponse.test.js serialize the response for a Bad
Request
    error, as defined on the schema
  stack: |
    test/serializeResponse.test.js:127:9
    node_modules/simple-get/index.js:98:7
    IncomingMessage.<anonymous>
(node_modules/simple-concat/index.js:8:13)

​ FAIL ​ test/serializeResponse.test.js
 ✖ should be equivalent

  test/serializeResponse.test.js                 
  126 |       t.error(err)                       
  127 |       t.equal(response.statusCode, 400)  
> 128 |       t.same(body, {                     
      | --------^                                
  129 |         statusCode: 400,                 
  130 |         error: 'Bad Request',            
  131 |         message: 'body must be object'   

  --- expected                                                         
  +++ actual                                                           
  @@ -1,5 +1,5 @@                                                      
   Object {                                                            
  -  "statusCode": 400,                                                
  -  "message": "body must be object",                                 
  -  "error": "Bad Request",                                           
  +  "statusCode": 500,                                                
  +  "code": "FST_ERR_FAILED_ERROR_SERIALIZATION",                     
  +  "message": "Failed to serialize an error. Error: serializerFn is not
 a function. Original error: body must be object",
   }                                                                   

  test: test/serializeResponse.test.js serialize the response for a Bad
Request
    error, as defined on the schema
  stack: |
    test/serializeResponse.test.js:128:9
    node_modules/simple-get/index.js:98:7
    IncomingMessage.<anonymous>
(node_modules/simple-concat/index.js:8:13)

​ FAIL ​ test/serializeResponse.test.js 2 failed of 16 114.562ms
 ✖ should be equal
 ✖ should be equivalent



                         
  🌈 SUMMARY RESULTS 🌈  
                         
​ FAIL ​ test/serializeResponse.test.js 2 failed of 16 114.562ms
 ✖ should be equal
 ✖ should be equivalent


Suites:   ​1 failed​, ​1 of 1 completed
Asserts:  ​​​2 failed​, ​14 passed​, ​of 16
​Time:​   ​394.496ms

Co-authored-by: Aras Abbasi <aras.abbasi@googlemail.com>
Signed-off-by: Douglas Moura <douglas.ademoura@gmail.com>
@github-actions
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 28, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants