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

Jump to bottom

fix(rest): remove trailing slash for routing match #2355

Closed
wants to merge 1 commit into from
Closed

fix(rest): remove trailing slash for routing match #2355

wants to merge 1 commit into from

Conversation

raymondfeng
Copy link
Contributor

@raymondfeng raymondfeng commented Feb 8, 2019 •
edited
Loading

Handle trailing slash for urls.

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

@raymondfeng raymondfeng requested a review from bajtos as a code owner February 8, 2019 00:08
@raymondfeng raymondfeng mentioned this pull request Feb 8, 2019
Closed
@raymondfeng
Copy link
Contributor Author

Please note the explorer redirect test is failing as we now treat /explorer/ the same as /explorer. What's the best practice for trailing /? Google seems to have this view - https://webmasters.googleblog.com/2010/04/to-slash-or-not-to-slash.html.

What do you think? Should we fix the explorer redirect for /explorer/ as it's not needed now?

@raymondfeng
Copy link
Contributor Author

I added commit 402a80d to make sure all tests are passing.

bajtos
bajtos suggested changes Feb 8, 2019
View reviewed changes
Copy link
Member

@bajtos bajtos left a comment

Choose a reason for hiding this comment

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

Please revert the changes in REST API Explorer.

The rest of the patch looks good to me.

packages/rest-explorer/src/__tests__/acceptance/rest-explorer.acceptance.ts Outdated
@@ -36,11 +36,12 @@ describe('API Explorer (acceptance)', () => {
.expect(/<title>LoopBack API Explorer/);
});

it('redirects from "/explorer" to "/explorer/"', async () => {
it('exposes API Explorer at "/explorer"', async () => {
Copy link
Member

Choose a reason for hiding this comment

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

This redirect is important because the way how relative paths are resolved in the browser. When the SPA UI is served from /explorer, a path like src="swagger-ui.js" is resolved to /swagger-ui.js instead of /explorer/swagger-ui.js.

I don't see how your patch is fixing this problem.

Can you revert the changes in REST Explorer please?

Copy link
Contributor Author

@raymondfeng raymondfeng Feb 8, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Instead of depending on redirect, I updated index.html.ejs to honor explorerPath and it works now for both http://localhost:3000/explorer and http://localhost:3000/explorer/.

packages/rest/src/router/router-base.ts Outdated Show resolved Hide resolved
@raymondfeng
Copy link
Contributor Author

@bajtos Without 402a80d, CI is failing. See https://travis-ci.org/strongloop/loopback-next/jobs/490404103.

After we normalizing the key for route matching, /explorer matches /explorer/ and vice versa. As a result, /explorer returns 200.

@raymondfeng raymondfeng force-pushed the fix-2188 branch 2 times, most recently from d1f4243 to 0eeae5b Compare February 8, 2019 16:49
@raymondfeng
Copy link
Contributor Author

@bajtos I improved rest-explorer to honor explorerPath in index.html. PTAL.

@raymondfeng
Copy link
Contributor Author

@bajtos PTAL

@bajtos
Copy link
Member

bajtos commented Feb 12, 2019 •
edited
Loading

@bajtos Without 402a80d, CI is failing. See https://travis-ci.org/strongloop/loopback-next/jobs/490404103.

After we normalizing the key for route matching, /explorer matches /explorer/ and vice versa. As a result, /explorer returns 200.

That looks like a breaking change to me!

I agree that /explorer should match /explorer/ and vice versa but only when there is no handler for the other path registered.

I am concerned that the behavior you are proposing here will be confusing to our users: they register handlers for two different paths, the framework happily accepts them both and then only one handler is invoked for both paths, the other handler being silently ignored.

bajtos
bajtos reviewed Feb 12, 2019
View reviewed changes
Copy link
Member

@bajtos bajtos left a comment

Choose a reason for hiding this comment

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

I am concerned that we don't have any automated test coverage to verify that API Explorer is using correct paths to front-end assets.

Ideally, we can use a tool like https://www.npmjs.com/package/jsdom to parse the index HTML and verify the links. (Using a headless Chrome would be even better, but that feels like an overkill to me.)

At minimum, we should do a regexp-based check to verify that src attributes are using the right relative path.

packages/rest-explorer/templates/index.html.ejs Outdated
<link rel="stylesheet" type="text/css" href="./swagger-ui.css">
<link rel="icon" type="image/png" href="./favicon-32x32.png" sizes="32x32" />
<link rel="icon" type="image/png" href="./favicon-16x16.png" sizes="16x16" />
<link rel="stylesheet" type="text/css" href="<%- explorerPath %>/swagger-ui.css">
Copy link
Member

Choose a reason for hiding this comment

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

Are you sure this is going to work when the app is running behind a reverse proxy like nginx?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's the same deal with or without this change. See #2293.

@bajtos bajtos added bug REST Issues related to @loopback/rest package and REST transport in general labels Feb 12, 2019
@raymondfeng
Copy link
Contributor Author

FYI: Express has the following setting to control the trialing / :

strict routing Boolean Enable strict routing. When enabled, the router treats "/foo" and "/foo/" as different. Otherwise, the router treats "/foo" and "/foo/" as the same.NOTE: Sub-apps will inherit the value of this setting. N/A (undefined)

@hacksparrow
Copy link
Contributor

I think we should have an option to configure this behavior.

Btw, the the setting is simply strict in case of a Router - http://expressjs.com/en/4x/api.html#express.router.

@raymondfeng raymondfeng mentioned this pull request Feb 12, 2019
Merged
7 tasks
@raymondfeng
Copy link
Contributor Author

@bajtos @hacksparrow I extracted the fix for #2188 into a new PR - #2375 and leave the trailing slash handling in this PR, which may require more discussions.

@raymondfeng raymondfeng force-pushed the fix-2188 branch 2 times, most recently from 14a215e to 086c5fc Compare February 15, 2019 23:00
@raymondfeng raymondfeng mentioned this pull request Feb 18, 2019
Merged
7 tasks
@raymondfeng
Copy link
Contributor Author

Superseded by #2419

@raymondfeng raymondfeng deleted the fix-2188 branch February 18, 2019 18:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bajtos bajtos bajtos requested changes

Assignees
No one assigned
Labels
bug REST Issues related to @loopback/rest package and REST transport in general
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@raymondfeng @bajtos @hacksparrow