feat: add computed RequestContext properties like `requestedBaseUrl` #2547

bajtos · 2019-03-07T11:38:12Z

Move the code calculating the requested protocol, base path and base URL from RestServer private methods into RequestContext's public API. This allows Route implementations to access these calculated properties too. For example, it simplifies building target location for HTTP redirect response (see #2022 (comment)) and makes it possible to move the implementation of /openapi.json route from Express route into regular LB4 RouteEntry implementation (see #2434 (comment) and a PoC in 2b5be22).

This pull request has three parts:

The first commit improves givenHttpServerConfig to support creation of HTTPS servers by filling-in dummy key and cert values.
The second commit tightens type checks in RestServer config and fixes a problem discovered along the way.
The third commit introduces new RequestContext properties and refactors RestServer to use these new properties instead of private helper methods.

Checklist

👉 Read and sign the CLA (Contributor License Agreement) 👈

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

packages/rest/src/request-context.ts

raymondfeng · 2019-03-07T17:18:28Z

packages/rest/src/rest.server.ts

-export interface RestServerOptions {
+export type RestServerOptions = Partial<RestServerResolvedOptions>;
+
+export interface RestServerResolvedOptions {


We can leave RestServerOptions unchanged and declare resolved options as Required<RestServerOptions>. See https://www.typescriptlang.org/docs/handbook/release-notes/typescript-2-8.html.

Or event make it ReadonlyRequired:

type ReadonlyRequired<T> = { +readonly [P in keyof T]-?: T[P] }; // Add `readonly` and remove `?`

Nice, I was not aware of Required type.

Please note that not all fields are required in the resolved config. The following properties are remaining optional: basePath, sequence, requestBodyParser.

Conceptually, it makes more sense to me to use the resolved config as the source of truth:

RestServer defines a type describing configuration it uses at runtime: the shape of the config object as required by various implementation bits.

Then we define another interface describing options accepted by the server. This interface uses optional properties for fields that have a reasonable default.

raymondfeng · 2019-03-07T17:24:42Z

Please fix formatting issue:

[email protected] prettier:cli /home/travis/build/strongloop/loopback-next
node packages/build/bin/run-prettier "/*.ts" "/.js" "**/.md" "-l"
packages/rest/src/tests/integration/http-handler.integration.ts

bajtos · 2019-03-11T07:30:30Z

@raymondfeng @hacksparrow LGTY now?

packages/rest/src/__tests__/unit/request-context.unit.ts

bajtos · 2019-03-12T09:34:59Z

Reworked new tests from unit-style to integration-style.

@raymondfeng @hacksparrow LGTY now?

raymondfeng · 2019-03-13T14:25:20Z

packages/rest/src/__tests__/integration/request-context.integration.ts

+      lbApp.handler(contextObservingHandler);
+
+      const expressApp = express();
+      expressApp.use('/api', lbApp.requestHandler);


What's going to happen if lbApp.basePath('/v1') is called?

We probably need to have a test to cover the case above.

I agree it's a use case we should cover by our test suite and possibly fix.

At the same time, I feel it's out of scope of this pull request. My intention is to move certain computed properties from RestServer to RequestContext while preserving their behavior.

I'll open a new PR to take a look at this.

raymondfeng · 2019-03-13T14:28:28Z

packages/testlab/src/http-server-config.ts

+const DUMMY_TLS_CONFIG = {
+  key: readFileSync(path.join(FIXTURES, 'key.pem')),
+  cert: readFileSync(path.join(FIXTURES, 'cert.pem')),
+};


Simplify setup of HTTPS servers in tests by enhancing the helper `givenHttpServerConfig` to automatically add dummy TLS key & cert data when the protocol is set to `https`.

Provide more type information to allow the compiler to understand when we are expecting configuration with optional properties and when consumers are relying on the optional config properties to have been already resolved with defaults. This change allows us to remove several usages of `!` operator.

Move the code calculating the requested protocol, base path and base URL from RestServer private methods into RequestContext's public API.

bajtos added refactor feature REST Issues related to @loopback/rest package and REST transport in general labels Mar 7, 2019

bajtos self-assigned this Mar 7, 2019

bajtos requested review from raymondfeng, hacksparrow and jannyHou March 7, 2019 11:38

This was referenced Mar 7, 2019

[WIP] feat(rest) : redirect route accept function #2529

Closed

[RFC] Introduce express middleware extension point #2434

Closed

bajtos requested a review from a user March 7, 2019 11:45

hacksparrow reviewed Mar 7, 2019

View reviewed changes

packages/rest/src/request-context.ts Show resolved Hide resolved

hacksparrow reviewed Mar 7, 2019

View reviewed changes

packages/rest/src/request-context.ts Show resolved Hide resolved

hacksparrow reviewed Mar 7, 2019

View reviewed changes

packages/rest/src/request-context.ts Show resolved Hide resolved

hacksparrow reviewed Mar 7, 2019

View reviewed changes

packages/rest/src/request-context.ts Show resolved Hide resolved

raymondfeng reviewed Mar 7, 2019

View reviewed changes

bajtos requested review from raymondfeng and hacksparrow and removed request for raymondfeng March 11, 2019 07:30

bajtos force-pushed the feat/request-context-properties branch from f8bd1d5 to 46b4322 Compare March 11, 2019 07:30

hacksparrow reviewed Mar 11, 2019

View reviewed changes

packages/rest/src/__tests__/unit/request-context.unit.ts Outdated Show resolved Hide resolved

bajtos force-pushed the feat/request-context-properties branch from 46b4322 to 8a31a92 Compare March 12, 2019 09:31

bajtos requested a review from hacksparrow March 12, 2019 09:35

hacksparrow approved these changes Mar 13, 2019

View reviewed changes

raymondfeng reviewed Mar 13, 2019

View reviewed changes

feat(testlab): add dummy HTTPS config

7d616d6

Simplify setup of HTTPS servers in tests by enhancing the helper `givenHttpServerConfig` to automatically add dummy TLS key & cert data when the protocol is set to `https`.

bajtos added 2 commits March 14, 2019 08:46

feat(rest): add requestedBaseUrl API to RequestContext

0bb9b5a

Move the code calculating the requested protocol, base path and base URL from RestServer private methods into RequestContext's public API.

bajtos force-pushed the feat/request-context-properties branch from 8a31a92 to 0bb9b5a Compare March 14, 2019 07:50

bajtos merged commit 912bece into master Mar 14, 2019

bajtos deleted the feat/request-context-properties branch March 14, 2019 08:13

bajtos mentioned this pull request Mar 14, 2019

test(testlab): verify how RequestContext combines urlBase and basePath #2589

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add computed RequestContext properties like `requestedBaseUrl` #2547

feat: add computed RequestContext properties like `requestedBaseUrl` #2547

bajtos commented Mar 7, 2019 •

edited

Loading

raymondfeng Mar 7, 2019

bajtos Mar 8, 2019 •

edited

Loading

raymondfeng commented Mar 7, 2019

bajtos commented Mar 11, 2019

bajtos commented Mar 12, 2019

raymondfeng Mar 13, 2019

raymondfeng Mar 13, 2019

bajtos Mar 14, 2019

bajtos Mar 14, 2019

raymondfeng Mar 13, 2019

feat: add computed RequestContext properties like requestedBaseUrl #2547

feat: add computed RequestContext properties like requestedBaseUrl #2547

Conversation

bajtos commented Mar 7, 2019 • edited Loading

Checklist

raymondfeng Mar 7, 2019

Choose a reason for hiding this comment

bajtos Mar 8, 2019 • edited Loading

Choose a reason for hiding this comment

raymondfeng commented Mar 7, 2019

bajtos commented Mar 11, 2019

bajtos commented Mar 12, 2019

raymondfeng Mar 13, 2019

Choose a reason for hiding this comment

raymondfeng Mar 13, 2019

Choose a reason for hiding this comment

bajtos Mar 14, 2019

Choose a reason for hiding this comment

bajtos Mar 14, 2019

Choose a reason for hiding this comment

raymondfeng Mar 13, 2019

Choose a reason for hiding this comment

feat: add computed RequestContext properties like `requestedBaseUrl` #2547

feat: add computed RequestContext properties like `requestedBaseUrl` #2547

bajtos commented Mar 7, 2019 •

edited

Loading

bajtos Mar 8, 2019 •

edited

Loading