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

ghost · 2019-03-04T15:50:24Z

Implements a part of #2022
This is a follow-up for #2512

change redirect method signature to accept function ,
the RedirectRoute class check if target is a function and call it else redirect the path .
with adding some functions to extract basePath,host,protocol from request .
delete basePath from server and added it later on Redirect-route to avoid if statement in redirect method on server class .

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

change redirect-route to accept function inside app and server with customizing the redirect-route class to extract the relvant info from request

ghost · 2019-03-04T15:55:45Z

@bajtos @raymondfeng @hacksparrow
this file will be removed https://github.com/strongloop/loopback-next/blob/35066c7b40c9e50889f5947f8cde9e9fb8cf6aeb/packages/rest/src/run.ts
its only for testing purpose . as well the import {Request} from "express";

let me know what do you think before start writing tests

bajtos · 2019-03-04T16:56:55Z

@sanadHaj thank you for the pull request. I took a quick look at the proposed changes, my first impression is that your changes are going in a wrong direction, unfortunately. I'll do a more thorough review later this week and post more details.

bajtos · 2019-03-05T14:11:06Z

packages/rest/src/rest.application.ts

@@ -244,7 +244,7 @@ export class RestApplication extends Application implements HttpServerLike {
   */
  redirect(
    fromPath: string,
-    toPathOrUrl: string,
+    toPathOrUrl: string | Function,


Please use a more specific type instead of Function, for example:

toPathOrUrl: string | LocationFactory

where LocationFactory can be defined as follows:

export type LocationFactory = (ctx: RedirectContext) => string; export interface RedirectContext { request: Request, // etc. }

On the second thought: let's not invent any new RedirectContext class/interface and use the existing RequestContext as defined here:

https://github.com/strongloop/loopback-next/blob/c8f1a71fc9ae7f2331f52fe11af43fab567ba4d1/packages/rest/src/request-context.ts#L15-L21

I feel the additional fields like basePath, etc. may be useful in other places besides redirection, therefore it make sense to define them directly on the request context.

it make sense to define them directly on the request context.

See #2547

bajtos · 2019-03-05T14:12:55Z

packages/rest/src/router/redirect-route.ts

+
+   private _getBasePath(request: Request) {
+        let basePath = '';
+        if (request.baseUrl && request.baseUrl !== '/') {


request.baseUrl is set by Express when a middleware is mounted via app.use(basePath, handler). It's a different concept from LB4's basePath option.

Eventually, we need to support both. Let's work in small increments please and leave baseUrl out of scope of this pull request.

bajtos · 2019-03-05T14:13:58Z

packages/rest/src/rest.server.ts

@@ -683,11 +683,11 @@ export class RestServer extends Context implements Server, HttpServerLike {
   */
  redirect(
    fromPath: string,
-    toPathOrUrl: string,
+    toPathOrUrl: string | Function,


Use LocationFactory instead of Function.

bajtos · 2019-03-05T14:14:32Z

packages/rest/src/rest.server.ts

    statusCode?: number,
  ): Binding {
    return this.route(
-      new RedirectRoute(fromPath, this._basePath + toPathOrUrl, statusCode),
+      new RedirectRoute(fromPath, toPathOrUrl, statusCode),


I think you should pass this._basePath to RedirectRoute constructor via a new argument.

Even better: wait until #2547 is landed and them modify RedirectRoute's invokeHandler function to obtain the base path from the request context via requestContext.basePath.

bajtos · 2019-03-05T14:14:37Z

packages/rest/src/router/redirect-route.ts

@@ -19,15 +19,22 @@ export class RedirectRoute implements RouteEntry, ResolvedRoute {

  constructor(
    private readonly sourcePath: string,
-    private readonly targetLocation: string,
+    private readonly targetLocation: string | Function,


Use LocationFactory instead of Function.

bajtos · 2019-03-05T14:18:53Z

packages/rest/src/router/redirect-route.ts

+    let targetLocation :string = typeof this.targetLocation === 'function'
+        ? this.targetLocation(
+          this._getProtocolForRequest(request),
+          this._getHost(request),


We should not be running this complex logic for simple redirects. Here is how I am envisioning the implementation:

class { async invokeHandler( {response,request}: RequestContext, args: OperationArgs, ): Promise<OperationRetval> { if (typeof this.targetLocation === 'string') { response.redirect(this.statusCode, this._basePath + this.targetLocation); return; } const ctx: RedirectContext = { request, basePath: this._basePath, // build additional fields like protocol, request, etc. }; const targetLocation = this.targetLocation(ctx); response.redirect(this.statusCode, targetLocation); } }

See #2022 (comment) for more details.

bajtos · 2019-03-05T15:49:13Z

I am proposing to make this pull request smaller and focus on introducing locationFactory only, leaving out any computed fields like the requested host, etc.

class {

  async invokeHandler(
    requestContext: RequestContext,
    args: OperationArgs,
  ): Promise<OperationRetval> {
    const target = typeof this.targetLocation === 'string'
      ? this._basePath + this.targetLocation
      : this.targetLocation(requestContext);
    requestContext.response.redirect(this.statusCode, target);
  }

}

Please make sure to include automated tests and also update API docs & regular documentation.

hacksparrow · 2019-03-07T12:17:19Z

packages/rest/src/run.ts

+        console.log(protocol+host+basePath);
+        return protocol+'://'+host+basePath+'/bye';
+    });
+    //restApp.redirect('/ping','/bye');


Please clean up irrelevant comments from PRs.

@hacksparrow this file for testing purpose .
this pull request seized until #2547 resolved .

hacksparrow · 2019-03-07T12:19:37Z

@sanadHaj thanks for the PR. Please run npm t to catch possible linting issues, apart checking implementation integrity.

bajtos · 2019-03-18T14:28:59Z

this pull request seized until #2547 resolved .

FWIW, #2547 has been landed.

bajtos · 2020-02-17T13:22:04Z

Hi @sanadHaj, what's the status of this pull request? Are you still keen to continue working on this feature?

stale · 2020-03-18T13:35:51Z

This issue has been closed due to continued inactivity. Thank you for your understanding. If you believe this to be in error, please contact one of the code owners, listed in the CODEOWNERS file at the top-level of this repository.

feat(rest) : redirect route accept function

35066c7

change redirect-route to accept function inside app and server with customizing the redirect-route class to extract the relvant info from request

ghost requested review from bajtos and raymondfeng as code owners March 4, 2019 15:50

bajtos added community-contribution REST Issues related to @loopback/rest package and REST transport in general labels Mar 4, 2019

bajtos self-assigned this Mar 4, 2019

bajtos suggested changes Mar 5, 2019

View reviewed changes

bajtos mentioned this pull request Mar 7, 2019

[RFC] Introduce express middleware extension point #2434

Closed

8 tasks

hacksparrow reviewed Mar 7, 2019

View reviewed changes

bajtos requested review from deepakrkris and emonddr February 17, 2020 13:22

bajtos added the stale label Feb 17, 2020

stale bot closed this Mar 18, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

ghost commented Mar 4, 2019 •

edited by bajtos

Loading

ghost commented Mar 4, 2019

bajtos commented Mar 4, 2019

bajtos Mar 5, 2019

bajtos Mar 5, 2019

bajtos Mar 7, 2019

bajtos Mar 5, 2019

bajtos Mar 5, 2019

bajtos Mar 5, 2019

bajtos Mar 7, 2019

bajtos Mar 5, 2019

bajtos Mar 5, 2019

bajtos commented Mar 5, 2019

hacksparrow Mar 7, 2019

ghost Mar 11, 2019

hacksparrow commented Mar 7, 2019

bajtos commented Mar 18, 2019 •

edited

Loading

bajtos commented Feb 17, 2020

stale bot commented Mar 18, 2020

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

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

Conversation

ghost commented Mar 4, 2019 • edited by bajtos Loading

Checklist

ghost commented Mar 4, 2019

bajtos commented Mar 4, 2019

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bajtos commented Mar 5, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hacksparrow commented Mar 7, 2019

bajtos commented Mar 18, 2019 • edited Loading

bajtos commented Feb 17, 2020

stale bot commented Mar 18, 2020

ghost commented Mar 4, 2019 •

edited by bajtos

Loading

bajtos commented Mar 18, 2019 •

edited

Loading