Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] feat(rest): allow bypassing http response writing and custom content type #1753

Closed
wants to merge 1 commit into from
Closed

[WIP] feat(rest): allow bypassing http response writing and custom content type #1753

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 49 additions & 0 deletions docs/site/Controllers.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,6 +214,55 @@ export class HelloController {
- `@param.query.number` specifies in the spec being generated that the route
takes a parameter via query which will be a number.

## Custom Response Writing

By default, LoopBack serializes the return value from the controller methods
into HTTP response based on the data type. For example, a string return value
will be written to HTTP response as follows:

status code: `200` Content-Type header: `text/plain` body: `the string value`

In some cases, it's desirable for a controller method to customize how to
produce the HTTP response. For example, we want to set the content type as
`text/html`.

```ts
import {get} from '@loopback/openapi-v3';
import {inject} from '@loopback/context';
import {RestBindings, Response} from '@loopback/rest';

export class HomePageController {
// ...
constructor(@inject(RestBindings.Http.RESPONSE) private res: Response) {
// ...
}
@get('/')
homePage() {
const homePage = '<html><body><h1>Hello</h1></body></html>';
this.res
.status(200)
.contentType('html')
.send(homePage);
// Return the `response` object itself to bypass further writing to HTTP
return this.res;
}
}
```

Alternatively, the controller method can set status code and content type but
leave the body to be populated by the LoopBack HTTP response writer.

```ts
@get('/')
homePage() {
const homePage = '<html><body><h1>Hello</h1></body></html>';
this.res
.status(200)
.contentType('html');
return homePage;
}
```

## Handling Errors in Controllers

In order to specify errors for controller methods to throw, the class
Expand Down
7 changes: 6 additions & 1 deletion packages/rest/src/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,8 +51,13 @@ export type InvokeMethod = (
*
* @param response The response the response to send to.
* @param result The operation result to send.
* @param contentType The content type to send
*/
export type Send = (response: Response, result: OperationRetval) => void;
export type Send = (
response: Response,
result: OperationRetval,
contentType?: string,
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand the purpose of this new parameter. I don't see it used anywhere in production code, only in unit tests.

send is typically invoked from a Sequence. There is usually the same sequence of actions implemented for all endpoints. IMO, the decision which content-type to send back should be made in individual controller methods (or routes), not in the Sequence.

I am proposing to revert this change.

) => void;

/**
* Reject the request with an error.
Expand Down
30 changes: 23 additions & 7 deletions packages/rest/src/writer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,21 +12,37 @@ import {Readable} from 'stream';
*
* @param response HTTP Response
* @param result Result from the API to write into HTTP Response
* @param contentType Optional content type for the response
*/
export function writeResultToResponse(
// not needed and responsibility should be in the sequence.send
response: Response,
// result returned back from invoking controller method
result: OperationRetval,
// content type for the response
contentType?: string,
Copy link
Member

Choose a reason for hiding this comment

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

Similarly here, let's revert this change please.

): void {
if (!result) {
response.statusCode = 204;
// Check if response writing should be bypassed. When the return value
// is the response object itself, no writing should happen.
if (result === response) return;

if (result === undefined) {
response.status(204);
response.end();
return;
}

if (result instanceof Readable || typeof result.pipe === 'function') {
response.setHeader('Content-Type', 'application/octet-stream');
function setContentType(defaultType: string = 'application/json') {
Copy link
Contributor

Choose a reason for hiding this comment

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

the variable name should be changed to type as it can be overridden.

if (response.getHeader('Content-Type') == null) {
Copy link
Member

@bajtos bajtos Sep 25, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Personally, I prefer to revert this change.

Ideally, there should be only two ways how a content-type is determined:

  1. The controller method/route returns data to be serialized to the response. The content-type is determined by the framework depending on the type of data (object vs. string vs. stream or buffer), request headers like Accepts and endpoint OpenAPI spec. Right now, we have a very limited version of this algorithm implemented in the current writeResultToResponse code (objects are sent as JSON, streams as binary, strings as text).

  2. The controller wants to set an explicit content type, status code or change any other aspect of the HTTP response. It returns an object describing response properties (status code, headers, body) and the send action applies these instructions to the response object. This will be enabled by Configurable response types/formats #436 in the future.

The change proposed here adds a third mode that makes it difficult to reason about the HTTP responses when reading code, because different aspects of the response (content type vs. actual response body) are produced by different pieces of code.

  1. The controller (or a sequence action in general) explicitly sets the content type, but it still expects the framework to convert the value returned by the controller method (or route handler) into HTTP response body.

There is of course also the fourth mode being added here, I think it should be the only scope of this pull request. It goes against the two modes I described above, but I ok with it as a temporary workaround:

  1. The controller (or a sequence action in general) takes full control of the response sent: it sets the status code & headers and writes the response body. The send action becomes a no-op.

response.setHeader('Content-Type', contentType || defaultType);
}
}

if (
result instanceof Readable ||
typeof (result && result.pipe) === 'function'
Copy link
Member

Choose a reason for hiding this comment

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

https://loopback.io/doc/en/contrib/style-guide.html#indentation-of-multi-line-expressions-in-if

const isStream = result instanceof Readable || 
  typeof (result && result.pipe) === 'function';

if (isStream) {
  // ...
}

) {
setContentType('application/octet-stream');
// Stream
result.pipe(response);
return;
Expand All @@ -37,17 +53,17 @@ export function writeResultToResponse(
case 'number':
if (Buffer.isBuffer(result)) {
// Buffer for binary data
response.setHeader('Content-Type', 'application/octet-stream');
setContentType('application/octet-stream');
} else {
// TODO(ritch) remove this, should be configurable
// See https://github.com/strongloop/loopback-next/issues/436
response.setHeader('Content-Type', 'application/json');
setContentType();
// TODO(bajtos) handle errors - JSON.stringify can throw
result = JSON.stringify(result);
}
break;
default:
response.setHeader('Content-Type', 'text/plain');
setContentType('text/plain');
result = result.toString();
break;
}
Expand Down
47 changes: 47 additions & 0 deletions packages/rest/test/unit/writer.unit.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,16 @@ describe('writer', () => {
expect(result.payload).to.equal('Joe');
});

it('writes string result to response as html', async () => {
writeResultToResponse(response, 'Joe', 'text/html');
const result = await observedResponse;

// content-type should be 'application/json' since it's set
// into the response in writer.writeResultToResponse()
expect(result.headers['content-type']).to.eql('text/html');
expect(result.payload).to.equal('Joe');
});

it('writes object result to response as JSON', async () => {
writeResultToResponse(response, {name: 'Joe'});
const result = await observedResponse;
Expand All @@ -31,6 +41,14 @@ describe('writer', () => {
expect(result.payload).to.equal('{"name":"Joe"}');
});

it('writes null object result to response as JSON', async () => {
writeResultToResponse(response, null);
const result = await observedResponse;

expect(result.headers['content-type']).to.eql('application/json');
expect(result.payload).to.equal('null');
});

it('writes boolean result to response as json', async () => {
writeResultToResponse(response, true);
const result = await observedResponse;
Expand Down Expand Up @@ -77,6 +95,35 @@ describe('writer', () => {
expect(result.payload).to.equal('');
});

it('skips writing when the return value is the response', async () => {
response
.status(200)
.contentType('text/html; charset=utf-8')
.send('<html><body>Hi</body></html>');
writeResultToResponse(response, response);
const result = await observedResponse;

expect(result.statusCode).to.equal(200);
expect(result.headers).to.have.property(
'content-type',
'text/html; charset=utf-8',
);
expect(result.payload).to.equal('<html><body>Hi</body></html>');
});

it('does not override content type', async () => {
response.status(200).contentType('text/html; charset=utf-8');
writeResultToResponse(response, '<html><body>Hi</body></html>');
const result = await observedResponse;

expect(result.statusCode).to.equal(200);
expect(result.headers).to.have.property(
'content-type',
'text/html; charset=utf-8',
);
expect(result.payload).to.equal('<html><body>Hi</body></html>');
});

function setupResponseMock() {
const responseMock = stubExpressContext();
response = responseMock.response;
Expand Down