feat(rest): add redirect route

[ghost]

Fixes #2022

Implement RedirectRoute to redirect request route , beside SwaggerRoute that extend from RedirectRoute to redirect swagger ui including some refactoring of rest server to support the new swagger route instead of express middleware
The implementation, including 12 (unit/integration/acceptance) tests.

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

[ghost]

@bajtos @raymondfeng

[ghost]

@bajtos review please

[bajtos]

Thank you @sanadHaj for the pull request.

I love the idea of app.redirect 👍 It will help us to simplify the implementation of @loopback/rest and get rid of indexRedirect, see

https://github.com/strongloop/loopback-next/blob/edbbe88a634df1326d0b8c1b54114d53100f75d4/packages/rest-explorer/src/rest-explorer.controller.ts#L35-L37

https://github.com/strongloop/loopback-next/blob/edbbe88a634df1326d0b8c1b54114d53100f75d4/packages/rest-explorer/src/rest-explorer.component.ts#L27

I don't like the idea of SwaggerUIRedirect route. These redirects are preserved for backwards compatibility, we can remove them in the next semver-major version. Adding extra complexity just to support this route seems like too much maintenance overhead.

I would like to propose a different solution that's more generic and can be useful to more consumers.

The built-in redirect needs to obtain the full URL (including protocol, hostname and port) before the redirect URL can be constructed. Inside @loopback/rest-explorer, we want the path portion of the URL. I feel these two uses cases may be pretty common and it would be great to support them as first-class features.

I am proposing to modify app.redirect to and introduce a new variant that will accept a function instead of a string as the "target" argument.

// simple redirect for ease of use
app.redirect('/foo', '/bar');

// the built-in redirect
app.redirect('/explorer', ({protocol, host, basePath} => {
  const baseUrl = protocol === 'http' ? config.httpUrl : config.url
  // host is "hostname:port"
  const openApiUrl = `${protocol}://${host}${basePath}/openapi.json`;
  const fullUrl = `${baseUrl}?url=${openApiUrl}`;
});

// api-explorer redirect, "urlPath" is the requested path (URL)
app.redirect('/explorer', {urlPath} => urlPath + '/');
// or even simpler - depending on how basePath is applied
app.redirect('/explorer', '/explorer/');

Besides the comment above and the comments below, I'd like to discuss the impact of basePath on redirects. Should we apply basePath to source, target or both? I'd like us to find a solution that will be least confusing to our users. Personally, I am leaning towards applying basePath to both of them. Obviously, when the target is a full URL like https://explorer.loopback.io/..., then basePath must not be applied.

[bajtos]

I find this surprising. Why are we prepending this._basePath to target, but not to source? I would expect basePath to be prepended to both or to none. What am I missing?

[ghost]

I find this surprising. Why are we prepending this._basePath to target, but not to source? I would expect basePath to be prepended to both or to none. What am I missing?

1. adding the the base path to the source will cause to double the base path so the result will be
   something like basePath/basePath/redirectRoute.
2. the reason of adding base path to source to avoid 404 not found cause the location header will not hold the base path so when the request come back during the redirect process express/lb4 router will return 404.

eventually i wanna to achieve to redirect
route if not exist this will help developers and give them some flexibility of using redirect api

lets assume i have configured the lb4 app like this ;

    restApp.basePath('/api');
    await restApp.controller(barController).lock();
    await restApp.controller(fooController).lock();
    restApp.redirect('/foo','/bar');
    restApp.redirect('/notExistRoute','/bar');
    await restApp.start();

in the above example the user will not figure out what happening behind the scene , removing the base path to be added as target /api/bar a litile bit confusing me .

the result of requesting /api/foo and /api/notExistRoute it will be the bar controller response.
even when the api/notExistRoute missing as route .

[ghost]

lets discuss high level first .

I would like to propose a different solution that's more generic and can be useful to more consumers.
The built-in redirect needs to obtain the full URL (including protocol, hostname and port) before the redirect URL can be constructed. Inside @loopback/rest-explorer, we want the path portion of the URL. I feel these two uses cases may be pretty common and it would be great to support them as first-class features.

I am proposing to modify app.redirect to and introduce a new variant that will accept a function instead of a string as the "target" argument.

how you intend to get the host and protocol , currently we extract them from request .
may we need to take the (base path + traget) as target to bind the route , and when invoking the invokeHandler to redirect we can add host and protocol to the target then redirect to the full url . its a little bit confusing to provide function to redirect only for customizing url .
the user only should provide the api path and the full url should be generated by lb4 .
disagree with this line of code the base bath should not be set as part of openapi.json
to prevent => Not Found http://127.0.0.1:8080/api/openapi.json if baseUrl /api
cause the openapi.json path under root
e.g http://127.0.0.1:8080/openapi.json
for more further detail check this issue #2329 ,

  const openApiUrl = `${protocol}://${host}${basePath}/openapi.json`;

it should be

  const openApiUrl = `${protocol}://${host}/openapi.json`;

please see the above comment for more information.

Besides the comment above and the comments below, I'd like to discuss the impact of basePath on redirects. Should we apply basePath to source, target or both? I'd like us to find a solution that will be least confusing to our users. Personally, I am leaning towards applying basePath to both of them. Obviously, when the target is a full URL like https://explorer.loopback.io/..., then basePath must not be applied.

[bajtos]

Conflicting files

packages/rest/test/acceptance/routing/routing.acceptance.ts
packages/rest/test/integration/rest.application.integration.ts
packages/rest/test/integration/rest.server.integration.ts
packages/rest/test/unit/rest.application/rest.application.unit.ts

In #2330, we have moved test files from test to src/__tests__. Please follow the steps in How to rebase your branch to resolve the merge conflict. Please do not run git merge, it's important to use git rebase master.

Sorry for the inconvenience.

[bajtos]

Good discussion!

I am proposing to modify app.redirect to and introduce a new variant that will accept a function instead of a string as the "target" argument.

how you intend to get the host and protocol , currently we extract them from request .

As I am envisioning this functionality, it will be the responsibility of LB4 framework to obtain the host and protocol from the incoming request before calling user-provided function to build the target path/url. This way we can keep the complex logic for obtaining the right host/protocol/etc. inside the framework, individual redirects don't have to repeat this logic and will avoid accidental errors.

may we need to take the (base path + traget) as target to bind the route , and when invoking the invokeHandler to redirect we can add host and protocol to the target then redirect to the full url
(...)
the user only should provide the api path and the full url should be generated by lb4 .

I think this is not going to work for the redirecting to externally-hosted swagger-ui, where the target is pointing to that external URL and using the location of the LB4 app as a query string parameter.

Different applications will have different needs when it comes to target URLs. For example, when running behind a url-rewriting reverse proxy, it's probably easiest to redirect to a path only (e.g. Location: /base-path/target) and let the reverse proxy figure out the actual URL the client should see.

That's why I proposed that LB4 runtime should provide {protocol, host, basePath, urlPath} to the handler function and let the handler to decide how exactly it want to construct the redirect target.

disagree with this line of code the base bath should not be set as part of openapi.json
to prevent => Not Found http://127.0.0.1:8080/api/openapi.json if baseUrl /api
cause the openapi.json path under root
e.g http://127.0.0.1:8080/openapi.json
for more further detail check this issue #2329 ,

I am of a different opinion, I think the openapi endpoint should honor basePath setting. Let's discuss this aspect in #2329 please and keep this pull request focused on implementing a redirect route.

To move this work forward, I am proposing to split this pull request into multiple smaller steps:

1. Implement simple redirect accepting source & target strings only: app.redirect('/from', '/to'). I think this is a useful addition on its own. Please open a new PR for that.
2. Once that first PR is landed, let's rebase this PR on top of the new master and resume the discussion about supporting more advanced use cases.

Thoughts?

/cc @strongloop/loopback-maintainers

[bajtos]

To move this work forward, I am proposing to split this pull request into multiple smaller steps:

1. Implement simple redirect accepting source & target strings only: app.redirect('/from', '/to'). I think this is a useful addition on its own. Please open a new PR for that.
2. Once that first PR is landed, let's rebase this PR on top of the new master and resume the discussion about supporting more advanced use cases.

I see that you already took the opposite approach and simplified this pull request to implement only string-based redirect route. That's fine with me too, I am going to review the code changes now.

[bajtos]

The patch looks pretty good now!

Besides the comments below, please add test cases to verify the following scenarios:

• A custom status code provided to app.redirect()
• A custom status code provided to server.redirect()
• What happens when basePath is set - how is the target URL constructed. Must have: server-level test. Nice to have: acceptance-level test against an applicaition.
• Priority of a redirect route vs. a static assets route.

[bajtos]

I have rebased the patch on top of the latest master, squashed commits into a single one, updated the commit message and also made minor improvements in tests & doc comments.

Let's wait for CI results before landing.

[bajtos]

Hmm, I am not able to push into your feature branch :( I'll have to open a new PR to get this landed.

[bajtos]

Closing in favor of #2512.