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.

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

Sugar API for defining URL redirects #2022

Open
2 of 6 tasks
bajtos opened this issue Nov 13, 2018 · 2 comments
Open
2 of 6 tasks

Sugar API for defining URL redirects #2022

bajtos opened this issue Nov 13, 2018 · 2 comments
Labels
API Explorer developer-experience Issues affecting ease of use and overall experience of LB users feature good first issue REST Issues related to @loopback/rest package and REST transport in general

Comments

@bajtos
Copy link
Member

bajtos commented Nov 13, 2018

This is a follow-up for #2014.

When using LB4+ to serve HTML pages, it's useful to add a trailing slash to the URL when serving a folder, for example redirect /explorer to /explorer/. Without this redirect, relative URLs to assets like CSS & JS files are incorrectly resolved. For example, when served from /explorer, relative links like ./swagger-ui.css are resolved in the parent directory, e.g. /swagger-ui.css instead of /explorer/swagger-ui.css.

Right now, a redirect can be implemented using a controller route that's hidden from the documentation and uses HTTP response object to send back the redirect. Such solution requires a lot of code and feels a bit hacky.

Let's make redirects a first-class feature in LB4 and provide a high-level API that's easy to use.

For example:

restApp.redirect('/explorer', '/explorer/');
restServer.redirect('/explorer', '/explorer/');

Under the hood, this can be implemented as a new Route class, for example:

app.route(new RedirectRoute('/explorer', '/explorer/'));

A mock-up implementation of RedirectRoute:

export class RedirectRoute implements RouteEntry, ResolvedRoute {
  // ResolvedRoute API
  readonly pathParams: PathParameterValues = [];
  readonly schemas: SchemasObject = {};

  // RouteEntry implementation
  readonly verb: string = 'get';
  readonly get path(): string { return this.sourcePath; }
  // ...

  constructor(
    public readonly sourcePath: string, 
    public readonly targetPath: string, 
    public statusCode: number = 303,
  ) {
    this.path = sourcePath;
  }

  async invokeHandler(
    {response}: RequestContext,
    args: OperationArgs,
  ): Promise<OperationRetval> {
    response.redirect(this.statusCode, this.targetPath);
  }

  // ...
}

Acceptance criteria

#2512

  • The implementation, including unit/integration/acceptance tests
    • Most of the tests should be written for RestServer.
    • Add one or few tests for RestApplication at integration or acceptance level, just to ensure the new RestApplication API is covered.
  • Documentation

TODO

  • Redirect to dynamically computed location (see the discussion below)
  • Search for all places calling .redirect( and consider updating them to use the new route and/or the new RestServer/RestApplication sugar APIs. E.g. REST API Explorer.
    • Redirect to externally hosted swagger-ui
    • Redirect from /explorer to /explorer/
  • Redirect to a location that's full URL (http://example.com) instead of a local path (/home). The trick is to skip appending basePath.
  • Honor req.baseUrl when the LB4 app is mounted on an external Express application.
@bajtos bajtos added feature API Explorer good first issue REST Issues related to @loopback/rest package and REST transport in general developer-experience Issues affecting ease of use and overall experience of LB users labels Nov 13, 2018
@ghost
Copy link

ghost commented Jan 29, 2019

@bajtos
working on it :)

@ghost ghost mentioned this issue Jan 30, 2019
7 tasks
@bajtos bajtos assigned ghost Feb 8, 2019
@bajtos
Copy link
Member Author

bajtos commented Mar 1, 2019

#2314 and #2512 implement redirects to static targets, where the target location is known at the time when the redirect is registered.

To support redirects used by REST (redirect to externally hosted swagger-ui) and REST API Explorer (redirect from /explorer to /explorer/), we need a way how to build the target location dynamically from the incoming request.

I am re-posting my proposal from #2314 (review):

I would like to propose a 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/');

#2314 (comment)
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.

(...)

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Explorer developer-experience Issues affecting ease of use and overall experience of LB users feature good first issue REST Issues related to @loopback/rest package and REST transport in general
Projects
None yet
Development

No branches or pull requests

1 participant