Skip to content

Commit

Permalink
Fix BaseController.ok on non string content (#423)
Browse files Browse the repository at this point in the history
* refactor: update JsonResult with no generic

* fix: update OkNegotiatedContentResult

update OkNegotiatedContentResult to rely on JsonContent when non string content is provided
  • Loading branch information
notaphplover authored Jan 30, 2025
1 parent 800f041 commit f0eb0de
Show file tree
Hide file tree
Showing 11 changed files with 84 additions and 45 deletions.
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
### Changed
- Updated `BaseMiddleware.handler` to allow async handlers.
- Updated `Middleware` to allow include any `ServiceIdentifier`.
- Updated `JsonContent` with no generic.

### Fixed
- Updated `BaseController.ok` to no longer return `text/plain` responses when non string content is passed.

## [6.4.10]

Expand Down
28 changes: 5 additions & 23 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@
"@types/supertest": "6.0.2",
"@typescript-eslint/eslint-plugin": "8.20.0",
"@typescript-eslint/parser": "8.20.0",
"body-parser": "1.20.3",
"cookie-parser": "1.4.7",
"eslint": "9.18.0",
"eslint-config-prettier": "10.0.1",
Expand Down
6 changes: 3 additions & 3 deletions src/base_http_controller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -79,10 +79,10 @@ export class BaseHttpController {
return new StatusCodeResult(statusCode);
}

protected json<T extends Record<string, unknown>>(
content: T | T[],
protected json(
content: unknown,
statusCode: number = StatusCodes.OK,
): JsonResult<T> {
): JsonResult {
return new JsonResult(content, statusCode);
}

Expand Down
5 changes: 1 addition & 4 deletions src/content/httpContent.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import type { OutgoingHttpHeaders } from 'node:http';
import type { Readable } from 'node:stream';

export abstract class HttpContent {
private readonly _headers: OutgoingHttpHeaders = {};
Expand All @@ -8,7 +7,5 @@ export abstract class HttpContent {
return this._headers;
}

public abstract readAsync(): Promise<
string | Record<string, unknown> | Record<string, unknown>[] | Readable
>;
public abstract readAsync(): Promise<unknown>;
}
8 changes: 3 additions & 5 deletions src/content/jsonContent.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,16 +2,14 @@ import { HttpContent } from './httpContent';

const DEFAULT_MEDIA_TYPE: string = 'application/json';

export class JsonContent<
T extends Record<string, unknown>,
> extends HttpContent {
constructor(private readonly content: T | T[]) {
export class JsonContent extends HttpContent {
constructor(private readonly content: unknown) {
super();

this.headers['content-type'] = DEFAULT_MEDIA_TYPE;
}

public async readAsync(): Promise<T | T[]> {
public async readAsync(): Promise<unknown> {
return this.content;
}
}
8 changes: 3 additions & 5 deletions src/results/JsonResult.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,19 +2,17 @@ import { JsonContent } from '../content/jsonContent';
import { HttpResponseMessage } from '../httpResponseMessage';
import type { IHttpActionResult } from '../interfaces';

export class JsonResult<T extends Record<string, unknown>>
implements IHttpActionResult
{
export class JsonResult implements IHttpActionResult {
constructor(
public readonly json: T | T[],
public readonly json: unknown,
public readonly statusCode: number,
) {}

public async executeAsync(): Promise<HttpResponseMessage> {
const response: HttpResponseMessage = new HttpResponseMessage(
this.statusCode,
);
response.content = new JsonContent<T>(this.json);
response.content = new JsonContent(this.json);

return response;
}
Expand Down
8 changes: 7 additions & 1 deletion src/results/OkNegotiatedContentResult.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { StatusCodes } from 'http-status-codes';

import { JsonContent } from '../content/jsonContent';
import { StringContent } from '../content/stringContent';
import { HttpResponseMessage } from '../httpResponseMessage';
import type { IHttpActionResult } from '../interfaces';
Expand All @@ -11,7 +12,12 @@ export class OkNegotiatedContentResult<T> implements IHttpActionResult {
const response: HttpResponseMessage = new HttpResponseMessage(
StatusCodes.OK,
);
response.content = new StringContent(JSON.stringify(this.content));

if (typeof this.content === 'string') {
response.content = new StringContent(this.content);
} else {
response.content = new JsonContent(this.content);
}

return response;
}
Expand Down
4 changes: 1 addition & 3 deletions src/test/action_result.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,9 +41,7 @@ describe('ActionResults', () => {
await actionResult.executeAsync();

expect(responseMessage.statusCode).toBe(StatusCodes.OK);
expect(await responseMessage.content.readAsync()).toBe(
JSON.stringify(content),
);
expect(await responseMessage.content.readAsync()).toStrictEqual(content);
});
});

Expand Down
2 changes: 1 addition & 1 deletion src/test/content/jsonContent.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import { JsonContent } from '../../content/jsonContent';

describe('JsonContent', () => {
it('should have application/json as the default media type', () => {
const content: JsonContent<Record<string, unknown>> = new JsonContent({});
const content: JsonContent = new JsonContent({});
expect(content.headers['content-type']).toBe('application/json');
});

Expand Down
57 changes: 57 additions & 0 deletions src/test/issues/issue_420.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
import { beforeEach, describe, expect, it } from '@jest/globals';
import * as bodyParser from 'body-parser';
import { Application, Request } from 'express';
import { Container } from 'inversify';
import supertest from 'supertest';
import TestAgent from 'supertest/lib/agent';

import { BaseHttpController } from '../../base_http_controller';
import { controller, httpPut, request } from '../../decorators';
import { InversifyExpressServer } from '../../server';
import { cleanUpMetadata } from '../../utils';

describe('Issue 420', () => {
beforeEach(() => {
cleanUpMetadata();
});

it('should work with no url params', async () => {
@controller('/controller')
// eslint-disable-next-line @typescript-eslint/no-unused-vars
class TestController extends BaseHttpController {
@httpPut('/test')
public async updateTest(
@request()
req: Request<unknown, unknown, { test: string }, void>,
) {
return this.ok({ message: req.body.test });
}
}

const container: Container = new Container();

const server: InversifyExpressServer = new InversifyExpressServer(
container,
);

server.setConfig((app: Application) => {
app.use(
bodyParser.urlencoded({
extended: true,
}),
);
app.use(bodyParser.json());
});

const agent: TestAgent<supertest.Test> = supertest(server.build());

const response: supertest.Response = await agent
.put('/controller/test')
.send({ test: 'test' })
.set('Content-Type', 'application/json')
.set('Accept', 'application/json');

expect(response.status).toBe(200);
expect(response.body).toStrictEqual({ message: 'test' });
});
});

0 comments on commit f0eb0de

Please sign in to comment.