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

[raymondfeng]

We discover an issue to return html from a controller:
See loopbackio/loopback4-example-shopping#16

Checklist

• npm test passes on your machine
• New tests added or existing tests modified to cover all changes
• Code conforms with the style guide
• API Documentation in code was updated
• Documentation in /docs/site was updated
• Affected artifact templates in packages/cli were updated
• Affected example projects in examples/* were updated

[virkt25]

LGTM 🚀

[raymondfeng]

@virkt25 I refined the PR to check if a controller method returns the response object itself as the flag to skip writing. PTAL.

[virkt25]

I like the new bypass much better!! Should we add a section in the README showing how to just set the content type and let the writer write the response

[virkt25]

LoopBack will serialize
OR
LoopBack serializes the ...

[virkt25]

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

[bajtos]

I am rather unhappy about the design changes proposed in this pull request.

So far, the framework was offering pretty strong guarantees to LoopBack users: every HTTP response was produced either by send or reject. If an application or an extension wants to intercept or modify all responses, and it provides custom send & reject actions, then it can be pretty certain it has all cases covered.

The solution proposed by this pull request, where the controller sends the response directly via Express response API, is throwing all guarantees out.

I understand we need a quick workaround to enable our shopping-cart example to serve a home page (loopbackio/loopback4-example-shopping#16) and I am fine to add an undocumented fix as a short-term workaround.

I would personally use the following check to determine whether the response have been already produced:

• The controller method returned undefined AND
• The response headers have been already sent (see Node.js API response.headersSent)

What's wrong with letting the controller method return response? I don't like the fact that a controller method can return the response and yet don't send any data back. We will end up with a client connection hanging forever in such case.

For a longer term solution that will be documented, I'd like us to use the approach described in #436, #436 (comment) and #788 (comment), where the controller method can return an object describing the response to be produced, e.g. {statusCode: 201, headers: {/*...*/}, body: data}.

To move this pull request forward, I am asking for the following changes:

• Reduce the amount of functionality changes. Just add a check to send action to return immediately when the response have been already send by the controller.
• Either keep this feature undocumented, or else make it very clear that this feature is a short-term hack.

[bajtos]

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.

[bajtos]

Similarly here, let's revert this change please.

[bajtos]

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.

3. 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:

4. 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.

[bajtos]

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) {
  // ...
}

[raymondfeng]

@bajtos Thank you for the feedback. A few clarifications:

1. I introduced the extra contentType argument in the hope of being able to infer the value from the response object. But that seems to be a bit involved to implement at the moment. As you mentioned, the final content type can be a result of content negotiation.

2. I was thinking about to introduce a new HttpResponse return type with the fluent API so that a controller method can return a fully-populated response so that our writer can honor it. But I realized most of the APIs are already available on Response and I didn't want to reinvent the wheel.

3. There is a pull and push to write the http response.

• pull: The writer pulls data from the controller method return value and serialize them into the response
• push: The controller method serializes data into the response

I see use cases for both modes and would love to allow both flavors. We can treat the push mode as the controller method has its own writer.

The issues above need to be addressed in the long run. To enable our shopping example, I'll create a different (much simpler) PR to allow:

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.

[virkt25]

Isn't it better to allow the controller to just set the response type / response header? ... How much work is it for us to implement pulling the controller metadata to see what the response header should be and set it based on that?

[raymondfeng]

Close it to favor #1760