lukeautry / tsoa

Build OpenAPI-compliant REST APIs using TypeScript and Node
MIT License
3.54k stars 499 forks source link

Don't attempt to json encode string types #1479

Closed keslerm closed 10 months ago

keslerm commented 1 year ago

All Submissions:

Closing issues close #1394

keslerm commented 1 year ago

I'm only really familiar with Express but reviewing the code for the other two types of servers they don't appear to have the same issue Express is having here.

WoH commented 1 year ago

Hi there, can you please add integration tests to show we have a consistent behavior?

keslerm commented 1 year ago

Hi there, can you please add integration tests to show we have a consistent behavior?

I'd love to, I looked around a bit at the tests and it's a lot to digest - could you give me a ballpark spot to look at that I can use as a starting point?

WoH commented 1 year ago

Sure, you can add a new controller method here: https://github.com/lukeautry/tsoa/blob/master/tests/fixtures/controllers/getController.ts#L313

that returns a string.

Then, call them: https://github.com/lukeautry/tsoa/blob/master/tests/integration/hapi-server.spec.ts https://github.com/lukeautry/tsoa/blob/master/tests/integration/koa-server.spec.ts https://github.com/lukeautry/tsoa/blob/master/tests/integration/express-server.spec.ts

Then, verify the response headers.

hpx7 commented 1 year ago

We are running into this issue as well. Is this fine to merge?

keslerm commented 1 year ago

@WoH I got side tracked by work, is there anything I need to do on this still?

WoH commented 1 year ago

Can you check the content type headers in the tests? The body could've been correct before, now we want text/plain not application/json

keslerm commented 1 year ago

@WoH I added some appropriate calls for each server type (hapi and express required an extra step, koa seemed to do it correctly from the git go)

There are a couple of tests failing as a result of this change however. I looked at them a bit and I'm not entirely sure what is ultimately causing them to fail

tsoa-tests:   3 failing
tsoa-tests:   1) Express Server
tsoa-tests:        Sub resource
tsoa-tests:          parses path parameters from the controller description:
tsoa-tests:      Uncaught AssertionError: expected {} to equal 'main-123'
tsoa-tests:       at /home/rridley/projects/aegiszero/tsoa/tests/integration/express-server.spec.ts:1450:29
tsoa-tests:       at Test.<anonymous> (integration/express-server.spec.ts:1565:11)
tsoa-tests:       at Test.assert (/home/rridley/projects/aegiszero/tsoa/node_modules/supertest/lib/test.js:172:8)
tsoa-tests:       at Server.localAssert (/home/rridley/projects/aegiszero/tsoa/node_modules/supertest/lib/test.js:120:14)
tsoa-tests:       at Object.onceWrapper (node:events:628:28)
tsoa-tests:       at Server.emit (node:events:514:28)
tsoa-tests:       at Server.emit (node:domain:489:12)
tsoa-tests:       at emitCloseNT (node:net:2148:8)
tsoa-tests:       at processTicksAndRejections (node:internal/process/task_queues:81:21)
tsoa-tests: 
tsoa-tests:   2) Express Server
tsoa-tests:        Sub resource
tsoa-tests:          parses path parameters from the controller description and method description:
tsoa-tests:      Uncaught AssertionError: expected {} to equal 'main-123:sub-456'
tsoa-tests:       at /home/rridley/projects/aegiszero/tsoa/tests/integration/express-server.spec.ts:1459:29
tsoa-tests:       at Test.<anonymous> (integration/express-server.spec.ts:1565:11)
tsoa-tests:       at Test.assert (/home/rridley/projects/aegiszero/tsoa/node_modules/supertest/lib/test.js:172:8)
tsoa-tests:       at Server.localAssert (/home/rridley/projects/aegiszero/tsoa/node_modules/supertest/lib/test.js:120:14)
tsoa-tests:       at Object.onceWrapper (node:events:628:28)
tsoa-tests:       at Server.emit (node:events:514:28)
tsoa-tests:       at Server.emit (node:domain:489:12)
tsoa-tests:       at emitCloseNT (node:net:2148:8)
tsoa-tests:       at processTicksAndRejections (node:internal/process/task_queues:81:21)
tsoa-tests: 
tsoa-tests:   3) Inversify Express Server Dynamic Container
tsoa-tests:        can handle get request with no path argument:
tsoa-tests:      Uncaught AssertionError: expected {} to equal '/v1/ManagedTest'
tsoa-tests:       at /home/rridley/projects/aegiszero/tsoa/tests/integration/inversify-dynamic-container.spec.ts:13:24
tsoa-tests:       at Test.<anonymous> (integration/inversify-dynamic-container.spec.ts:41:11)
tsoa-tests:       at Test.assert (/home/rridley/projects/aegiszero/tsoa/node_modules/supertest/lib/test.js:172:8)
tsoa-tests:       at Server.localAssert (/home/rridley/projects/aegiszero/tsoa/node_modules/supertest/lib/test.js:120:14)
tsoa-tests:       at Object.onceWrapper (node:events:628:28)
tsoa-tests:       at Server.emit (node:events:514:28)
tsoa-tests:       at Server.emit (node:domain:489:12)
tsoa-tests:       at emitCloseNT (node:net:2148:8)
tsoa-tests:       at processTicksAndRejections (node:internal/process/task_queues:81:21)
WoH commented 1 year ago

I'll take a look, first guess is the data is now on res.body, and it's checking/parsing res.data in the tests

github-actions[bot] commented 11 months ago

This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days

github-actions[bot] commented 10 months ago

This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days

david-loe commented 8 months ago

Any plans on reopening this? @WoH '' could create a 204 in my opinion

xseman commented 3 weeks ago

@WoH Is this still being worked on? I'd love to help if it's still planned.