fix(rest): parse query string even when there is no rest query param

[raymondfeng]

See #2088

Now we use Express body parsers instead of body module. I think there was a side effect of body module that parses request.query beyond Express.

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

[bajtos]

-1 from me, I have intentionally disabled query parsing when no query arguments are described in the operation spec.

@larsm11 @raymondfeng Let's discuss.

What is the use case for having query arguments not documented in the OpenAPI spec? I find the example provided in #2088 as too artificial. IMO, a real-world endpoint will want start and end query arguments described in the documentation so that API consumers are aware of them, client generators like swagger-codegen can include them in the client API, etc.

[bajtos]

Please inline the handler in this test case.

I find it difficult to understand what's going on in this test case when the query handler is defined elsewhere and the name does not explain what is the handler actually doing.

Alternatively, find a better name for this handler function, e.g. respondWithParsedQueryObject.

[raymondfeng]

-1 from me, I have intentionally disabled query parsing when no query arguments are described in the operation spec.

I think disabling query parsing is over-restrictive. Parsing query string is not a heavy operation. There are valid use cases that we want to have query string parsed regardless of being referenced in a controller method argument.

1. There are non-application specific query params, such as:

• access_token.
• https://github.com/expressjs/method-override
• _format to override/specify Accept header

2. We allow routes beyond ControllerRoute

[bajtos]

There is also the question of consistency. So far, LB4 applications were usually parsing the query by calling qs with default arguments, see
https://github.com/strongloop/loopback-next/blob/8156f9d32aac3a0461b617a01cde9c319a1cceca/packages/rest/src/parser.ts#L122

With your change in place, this code will be effectively never called, because the query will be always parsed by the Express layer.

If we agree that the query should be parsed inside Express, then please clean up the code in packages/rest/src/parser.ts as part of that change. I think you will need to update few tests as well.

There are non-application specific query params, such as access_token, ...
We allow routes beyond ControllerRoute

Agreed.

My main objection is that all of these query parameters should be included in the OpenAPI spec for documentation purposes and for consumption by client generators.

Once these params are documented, we will automatically parse the query as part of argument processing.

On the other hand, I see that the current implementation does not work when the parsed query needs to be accessed before the parameter parser did it job, e.g. from a middleware or a custom sequence action called early in the sequence flow.

I guess that means we do need to change the behavior as you are proposing here.

[raymondfeng]

If we agree that the query should be parsed inside Express, then please clean up the code in packages/rest/src/parser.ts as part of that change. I think you will need to update few tests as well.

I switch to express.query to parse query string for consistency.

[raymondfeng]

@bajtos PTAL

[raymondfeng]

@bajtos Good feedback! PTAL.

[bajtos]

LGTM, please consider addressing my comment below.

[bajtos]

Please note that givenHttpServerConfig sets the port to 0 by default. I think the following simpler version should work well too:

async function givenAServer(options: {rest?: RestServerConfig} = {}) {
   options.rest = givenHttpServerConfig(options.rest);
   // ...
}