fix: fix static assets router blocking controller registration #1977

hacksparrow · 2018-11-05T11:41:47Z

This PR is to fix a regression, which prevented controllers and routes from being registered if app.static() was called before them.

app.static() was calling this.httpHandler.registerStaticAssets(path, rootDir, options);, which caused _setupHandlerIfNeeded() to be called and this._httpHandler being set. Once this._httpHandler is set, none of the bound controllers or routes were registered.

The fix is to collect the details of app.static() in an array this.staticAssets, and process them as part of _setupHandlerIfNeeded().

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

I think the overall direction is okay, let's improve the implementation details please.

packages/rest/src/rest.server.ts

bajtos · 2018-11-05T13:22:00Z

packages/rest/test/integration/rest.application.integration.ts

@@ -92,6 +127,19 @@ describe('RestApplication (integration)', () => {
      .expect('Hello');
  });

+  it('gives precedence to API controllers over static assets', async () => {


We already have a similar test in rest.server.integration.ts, see https://github.com/strongloop/loopback-next/blob/1bcdb5b69221501b3952bd70c0c78409b600d1ab/packages/rest/test/integration/rest.server.integration.ts#L150-L163

I think this test should be named differently: it('registers controllers defined later than static assets').

Also since all newly added tests in this file are related to RestServer functionality (they are not RestApplication specific), they should go to rest.server.integration.ts. IMO, there is no need to duplicate RestServer tests in RestApplication integration test suite again, unless I am missing a good reason you found?

... there is no need to duplicate RestServer tests in RestApplication integration test suite ...

If the implementation of app.static is broken (for whatever reason) and we don't have tests to determine app.static actually work, the problem will be found out only after trying to call app.static in a project.

True. On the other hand, app.static is implemented as a simple wrapper delegating all work to this.restServer.static. I agree it's important to have basic integration tests in place, but I don't think it's necessary to duplicate tests for all edge cases. Having too many tests is a maintenance burden.

👍 removed them.

packages/rest/test/integration/rest.application.integration.ts

packages/rest/test/integration/rest.server.integration.ts

bajtos · 2018-11-05T14:01:36Z

packages/rest/test/integration/rest.server.integration.ts

+
+  it('allows static assets to be mounted on the same path multiple times', async () => {
+    const root = FIXTURES;
+    const dirA = path.join(root, 'dir-a');


Let's find a more descriptive directory name than dir-a please.

I think it's time to reorganize the fixtures directory, I am proposing the following structure:

fixtures - cert.pem - key.pem - pfx.pfx - assets - index.html # moved from fixtures/index.html - other-assets - index.html

In the long term, I think it would be best to leverage TestSandbox to create temporary asset directories for the tests, but I guess that's out of scope of this pull request.

bajtos

Looks mostly good.

Please give @raymondfeng a chance to review too.

bajtos · 2018-11-05T15:29:49Z

packages/rest/src/rest.server.ts

@@ -614,7 +621,15 @@ export class RestServer extends Context implements Server, HttpServerLike {
   * @param options Options for serve-static
   */
  static(path: PathParams, rootDir: string, options?: ServeStaticOptions) {
-    this.httpHandler.registerStaticAssets(path, rootDir, options);
+    if (this._httpHandler) {
+      this.httpHandler.registerStaticAssets(path, rootDir, options);


I find this a bit weird - you are checking for the existence of this._httpHandler and then calling this.httpHandler.registerStaticAssets. Shouldn't you use the same property in both places?

bajtos · 2018-11-05T15:31:25Z

packages/rest/src/rest.server.ts

+
+    this.staticAssets.forEach(assetEntry => {
+      const {path, rootDir, options} = assetEntry;
+      this.httpHandler.registerStaticAssets(path, rootDir, options);


I think you should call this._httpHandler.registerStaticAssets here, instead of this.httpHandler....?

bajtos · 2018-11-05T15:35:50Z

packages/rest/src/rest.server.ts

+    if (this._httpHandler) {
+      this.httpHandler.registerStaticAssets(path, rootDir, options);
+    } else {
+      this.staticAssets.push({


I think you should always create a new entry in this.staticAssets, so that all static assets are preserved whenever the HTTP handler is rebuilt.

See #433 and
https://github.com/strongloop/loopback-next/blob/1bcdb5b69221501b3952bd70c0c78409b600d1ab/packages/rest/src/rest.server.ts#L262-L267

I think this is the simplest version of this method:

this.staticAssets.push(/*...*/); delete this._httpHandler;

bajtos · 2018-11-05T15:37:30Z

packages/rest/test/integration/rest.server.integration.ts

+      .expect(200, content);
+  });
+
+  it('allows static assets to be mounted on the same path multiple times', async () => {


Let's improve this test name:

merges different static asset directories when mounted on the same path

bajtos · 2018-11-05T15:41:51Z

packages/rest/test/integration/rest.server.integration.ts

+
+    let content = fs
+      .readFileSync(path.join(root, 'index.html'))
+      .toString('utf-8');


This can be simplified, see https://nodejs.org/docs/latest-v8.x/api/fs.html#fs_fs_readfilesync_path_options

let content = fs.readFileSync(path.join(root, 'index.html'), {encoding: 'utf-8'});

It would be even better to extract a shared helper function to avoid repetition.

let content = await readFileFromDirectory(root, 'index.html'); // impl const readFileAsync = util.promisify(fs.readFile); function readFileFromDirectory(dirname: string, filename: string): Promise<string> { return readFileAsync(path.join(dirname, filename), {encoding: 'utf-8'}); }

raymondfeng · 2018-11-05T15:44:33Z

@hacksparrow Is this a regression? Please add a bit more context in the PR.

hacksparrow · 2018-11-05T16:00:48Z

@raymondfeng updated:

This PR is to fix a regression, which prevented controllers and routes from being registered if app.static() was called before them.

app.static() was calling this.httpHandler.registerStaticAssets(path, rootDir, options);, which caused _setupHandlerIfNeeded() to be called and this._httpHandler being set. Once this._httpHandler is set, none of the bound controllers or routes were registered.

The fix is to collect the details of app.static() in an array this.staticAssets, and process them as part of _setupHandlerIfNeeded().

raymondfeng · 2018-11-05T16:19:38Z

packages/rest/src/rest.server.ts

+
+    this.staticAssets.forEach(assetEntry => {
+      const {path, rootDir, options} = assetEntry;
+      this._httpHandler.registerStaticAssets(path, rootDir, options);


Why don't we create an Express Router as the container so that registerStaticAssets can just add a new entry to the router without rebuilding the handler?

I am responding which some assumptions, please clarify if I misunderstood something.

We are ultimately using an Express Router - https://github.com/strongloop/loopback-next/blob/master/packages/rest/src/router/static-assets-route.ts#L35.

We have to register the static assets router with the Routing Table so that we have the ability to check the matching route for a request in the Routing Table, and return the static assets router, if none was matched.

If I misunderstood your statement, can you clarify what you mean by "container" and "rebuilding the handler".

I'm saying we always create an instance of StaticAssetsRoute as the place holder and use it to register static assets. This way we don't have to do the following:

this.staticAssets.push(/*...*/); delete this._httpHandler; // I assume it's used to remove the cached _httpHandler to pick up new static assets

I am still not getting it 😬.

One of the requirements of app.static() is that it should be able to add new routes even after the app has started. Will it work with what you are suggesting?

bajtos

I am not very happy about the fact that we are effectively moving the registration of static assets out of the Routing table into StaticAssetsRoute maintained by RestServer, I feel it's creating inconsistency with the way how controllers are registered. Having said that, I hope this is mostly an internal implementation detail that will be easy to fix in the future if/when needed.

LGTM.

bajtos · 2018-11-06T12:12:37Z

packages/rest/src/rest.server.ts

+/**
+ * Options for handling static assets
+ */
+export interface StaticAssetsEntry {


AFAICT, this interface is no longer used. Please remove.

Fix static assets router blocking registration of controllers

hacksparrow self-assigned this Nov 5, 2018

hacksparrow requested review from bajtos and raymondfeng as code owners November 5, 2018 11:41

hacksparrow force-pushed the fix/static-assets-router branch from cbdf1fa to 936abe2 Compare November 5, 2018 11:48

bajtos reviewed Nov 5, 2018

View reviewed changes

packages/rest/test/integration/rest.application.integration.ts Outdated Show resolved Hide resolved

bajtos reviewed Nov 5, 2018

View reviewed changes

packages/rest/test/integration/rest.server.integration.ts Show resolved Hide resolved

bajtos reviewed Nov 5, 2018

View reviewed changes

hacksparrow force-pushed the fix/static-assets-router branch from 8a95be2 to b6ae45a Compare November 5, 2018 14:04

bajtos approved these changes Nov 5, 2018

View reviewed changes

raymondfeng reviewed Nov 5, 2018

View reviewed changes

raymondfeng approved these changes Nov 6, 2018

View reviewed changes

bajtos approved these changes Nov 6, 2018

View reviewed changes

hacksparrow force-pushed the fix/static-assets-router branch 8 times, most recently from 69218f2 to fd143cc Compare November 7, 2018 19:49

fix: fix static assets router blocking controller registration

1b725b3

Fix static assets router blocking registration of controllers

hacksparrow force-pushed the fix/static-assets-router branch from fd143cc to 1b725b3 Compare November 8, 2018 03:57

hacksparrow merged commit 0e1b06f into master Nov 8, 2018

hacksparrow deleted the fix/static-assets-router branch November 8, 2018 10:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: fix static assets router blocking controller registration #1977

fix: fix static assets router blocking controller registration #1977

hacksparrow commented Nov 5, 2018 •

edited

Loading

bajtos left a comment

bajtos Nov 5, 2018

hacksparrow Nov 5, 2018

bajtos Nov 5, 2018

hacksparrow Nov 5, 2018

bajtos Nov 5, 2018

bajtos left a comment

bajtos Nov 5, 2018

bajtos Nov 5, 2018

bajtos Nov 5, 2018

bajtos Nov 5, 2018

bajtos Nov 5, 2018

raymondfeng commented Nov 5, 2018

hacksparrow commented Nov 5, 2018

raymondfeng Nov 5, 2018

hacksparrow Nov 5, 2018

raymondfeng Nov 5, 2018

hacksparrow Nov 5, 2018

bajtos left a comment

bajtos Nov 6, 2018

hacksparrow Nov 6, 2018

fix: fix static assets router blocking controller registration #1977

fix: fix static assets router blocking controller registration #1977

Conversation

hacksparrow commented Nov 5, 2018 • edited Loading

Checklist

bajtos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bajtos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raymondfeng commented Nov 5, 2018

hacksparrow commented Nov 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bajtos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hacksparrow commented Nov 5, 2018 •

edited

Loading