Don't attempt to json encode string types #1479

keslerm · 2023-09-05T19:19:53Z

All Submissions:

Have you followed the guidelines in our Contributing document?
Have you checked to ensure there aren't other open Pull Requests for the same update/change?
Have you written unit tests?
Have you written unit tests that cover the negative cases (i.e.: if bad data is submitted, does the library respond properly)?
This PR is associated with an existing issue?

Closing issues
close #1394

github-actions

Hello there keslerm 👋

Thank you and congrats 🎉 for opening your first PR on this project.✨

We will review the following PR soon! 👀

keslerm · 2023-09-05T19:23:13Z

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 · 2023-09-08T13:45:45Z

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

keslerm · 2023-09-08T14:14:07Z

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 · 2023-09-08T14:48:45Z

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 · 2023-10-03T03:11:17Z

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

keslerm · 2023-10-03T17:54:58Z

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

WoH · 2023-10-03T18:07:37Z

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 · 2023-10-04T00:46:24Z

@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 · 2023-10-04T06:18:32Z

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

WoH · 2023-10-04T06:24:56Z

packages/cli/src/routeGeneration/templates/express.hbs

@@ -208,6 +208,9 @@ export function RegisterRoutes(app: Router) {
        if (data && typeof data.pipe === 'function' && data.readable && typeof data._read === 'function') {
            response.status(statusCode || 200)
            data.pipe(response);
+        } else if (data && typeof data === 'string') {


data could be '' which is nullish, do we want this to fall through to 204 no content?

github-actions · 2023-11-04T00:06:32Z

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 · 2023-12-11T00:07:16Z

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 · 2024-02-23T06:50:24Z

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

xseman · 2024-10-06T08:18:04Z

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

github-actions bot reviewed Sep 5, 2023

View reviewed changes

keslerm force-pushed the fix/handle-string-return-types branch from 185dc8f to 5688f98 Compare September 8, 2023 16:05

keslerm added 3 commits October 3, 2023 19:00

Don't attempt to json encode string types

a3ee02a

Add integration tests

df72c6a

Add text/plain headers where needed + tests

306b854

keslerm force-pushed the fix/handle-string-return-types branch from 5688f98 to 306b854 Compare October 4, 2023 00:40

WoH reviewed Oct 4, 2023

View reviewed changes

github-actions bot added the Stale label Nov 4, 2023

github-actions bot closed this Nov 10, 2023

WoH reopened this Nov 10, 2023

WoH removed the Stale label Nov 10, 2023

github-actions bot added the Stale label Dec 11, 2023

github-actions bot closed this Dec 17, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't attempt to json encode string types #1479

Don't attempt to json encode string types #1479

keslerm commented Sep 5, 2023

github-actions bot left a comment

keslerm commented Sep 5, 2023

WoH commented Sep 8, 2023

keslerm commented Sep 8, 2023

WoH commented Sep 8, 2023

hpx7 commented Oct 3, 2023

keslerm commented Oct 3, 2023

WoH commented Oct 3, 2023 •

edited

Loading

keslerm commented Oct 4, 2023

WoH commented Oct 4, 2023 •

edited

Loading

WoH Oct 4, 2023 •

edited

Loading

github-actions bot commented Nov 4, 2023

github-actions bot commented Dec 11, 2023

david-loe commented Feb 23, 2024 •

edited

Loading

xseman commented Oct 6, 2024

Don't attempt to json encode string types #1479

Don't attempt to json encode string types #1479

Conversation

keslerm commented Sep 5, 2023

All Submissions:

github-actions bot left a comment

Choose a reason for hiding this comment

keslerm commented Sep 5, 2023

WoH commented Sep 8, 2023

keslerm commented Sep 8, 2023

WoH commented Sep 8, 2023

hpx7 commented Oct 3, 2023

keslerm commented Oct 3, 2023

WoH commented Oct 3, 2023 • edited Loading

keslerm commented Oct 4, 2023

WoH commented Oct 4, 2023 • edited Loading

WoH Oct 4, 2023 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Nov 4, 2023

github-actions bot commented Dec 11, 2023

david-loe commented Feb 23, 2024 • edited Loading

xseman commented Oct 6, 2024

WoH commented Oct 3, 2023 •

edited

Loading

WoH commented Oct 4, 2023 •

edited

Loading

WoH Oct 4, 2023 •

edited

Loading

david-loe commented Feb 23, 2024 •

edited

Loading