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

[REST] Add basePath configuration #918

Closed
8 tasks
kjdelisle opened this issue Jan 26, 2018 · 17 comments
Closed
8 tasks

[REST] Add basePath configuration #918

kjdelisle opened this issue Jan 26, 2018 · 17 comments
Labels

Comments

@kjdelisle
Copy link
Contributor

kjdelisle commented Jan 26, 2018

linked to #914

Overview

Currently, we do not have proper basePath support in the RestServer for the supported bottom-up approach. Let's address that by allowing users to pass these values in via options OR inject via the appropriate key.

Acceptance Criteria

  • Add new BASE_PATH binding to RestBindings
  • Add option-based setting of the basePath inside of the RestServer module
    • Option-based setting should utilize the BASE_PATH binding under the hood
  • Set base path binding to a default value of / if not specified
  • Allow BASE_PATH to be registered through a basePath() function
  • Change router so that it utilizes the new RestBindings.BASE_PATH for all routes
  • Add in a controller decorator that would set the metadata to set the 'base path' for its routes (different from BASE_PATH binding)
  • Add TSDocs
  • Update loopback.io documentation to explain how to use the basePath when configuring your application from the bottom-up
@bajtos
Copy link
Member

bajtos commented Jan 29, 2018

Currently, we do not have proper basePath support in the RestServer for the supported bottom-up approach.

+1

Let's address that by allowing users to pass these values in via options OR inject via the appropriate key.

I am not convinced the base path should become a configuration aspect of the REST server. When doing top-down approach, shouldn't we pick it from the Swagger/OpenAPI spec? Also as was discussed in our Slack, we need to take into consideration different places where one may want to specify basePath, for example in the spec passed to controller's @api decorator.

@kjdelisle
Copy link
Contributor Author

I am not convinced the base path should become a configuration aspect of the REST server. When doing top-down approach, shouldn't we pick it from the Swagger/OpenAPI spec?

If we're building a bottom-up spec then we need a way to specify what that value is in a bottom-up way. We might want to read that value from the spec instance, but we don't want to make it exclusively configurable in the top-down approach.

@bajtos
Copy link
Member

bajtos commented Jan 30, 2018

If we're building a bottom-up spec then we need a way to specify what that value is in a bottom-up way.

I see, that makes sense. Isn't it enough to configure base path(s) on controller level, as we are already supporting now?

https://github.com/strongloop/loopback-next/blob/fed0524b5404e409cd198d266a44274679414b8e/packages/rest/test/acceptance/routing/routing.acceptance.ts#L412-L428

Since each HTTP/Rest server is owning it's entire path namespace, if we configure the base path at rest server level, then every URL served by this particular server will have to start with that base path. I.e. if our app has a RestServer listening on localhost:3000 with the base path set to /api, then all our URLs will start with http://localhost:3000/api. As a result, we won't be able to serve an URL like http://localhost:3000/explorer. I don't see how this would be useful. What am I missing?

@shimks
Copy link
Contributor

shimks commented Mar 19, 2018

@bajtos Is it possible for for the base paths to be used only on the routes that are registered through controllers?

@virkt25
Copy link
Contributor

virkt25 commented Mar 19, 2018

My understanding of a decorator for basePath would that it would only apply to the API's (and not static files / explorer) ... ideally explorer would have it's own config for the path in the future (or even having it enabled / disabled). Same goes for static files (it's own config).

I would also like to see basePath be capable of building up a nested path. For example:

// app.ts
app.basePath('/api')

// userController.ts
@basePath('/user')
class UserController {
  constructor()
  @get('/') { ... } // Maybe '/' can be optional? The route would be /api/user
  @get('/{id}') // The route would be /api/user/{id}

@raymondfeng
Copy link
Contributor

IMO, the basePath can be potentially configured at various level of the hierarchy, such as:

  • RestServer (http://host1:8080/api) (common base path for all controllers)
    • Controller: /customers (common base path for all controller methods)
      • Method: /{id}

I think we can add decorators to MyApplication to provide application/server level settings. For example:

@api(...) 
class MyApplication extends Application {
}

@dhmlau dhmlau removed their assignment Mar 19, 2018
@dhmlau dhmlau removed the non-DP3 label Mar 27, 2018
@dhmlau dhmlau added the DP3 label Apr 19, 2018
@bajtos
Copy link
Member

bajtos commented Apr 24, 2018

I agree with Raymond's comment that basePath can be configured at various levels of the hierarchy.

I am not sure if it's a good idea to allow the @api decorator to be used at application (and RestServer?) level. Right now, we provide RestServer#api() and RestApplication#api() methods for design-first (top-down) approach.

Another important thing to consider: OpenAPIv3 does not have anybasePath field, they have servers[].url instead. For controllers, we solve this issue by maintaining our interface for the object passed to @api decorator, see https://github.com/strongloop/loopback-next/blob/1ed79c9e5ab2f258f95cc4e539df87b58b790884/packages/openapi-v3/src/controller-spec.ts#L24-L41

Also in my view, design-first approach is mutually exclusive with basePath configuration. It does not make sense to me to provide both a full OpenAPIv3 object that contains all paths and possibly server urls (this would be typically exported from an OpenAPIv3 editor like API Connect), and at the same time provide basePath configuration that's used by our OpenAPIv3 spec generator - how would this basePath config be applied?

I think we should allow basePath configuration both at RestApplication and RestServer level. Implementation wise, this may be difficult to achieve via @api decorator. Right now, RestApplication is a thin wrapper around Application, adding a copy ofRestServer methods implemented by delegating the work to the single RestServer registered with the app.

What's more important, our current design is not forcing people to subclass application/server class, it's possible to build an app using vanilla RestApplication class. IMO, this won't be possible if basePath configuration required a decorator.

const app = new RestApplication();
app.controller(MyController);
app.start();

Based on the above, I am proposing to NOT allow @api decorator at app/server level and provide a regular method for configuring basePath. We can either modify the existing api method to accept basePath string only (to rule out the possibility of configuring both basePath and the full api spec) or add a new method or perhaps a property.

// direct usage
const app = new RestApplication();
app.basePath = '/api';
app.controller(MyController);
app.start();

// usage in an app subclass
class MyApp extends RestApplication {
  constructor() {
    this.basePath = '/api';
  }
}

Thoughts?

@bajtos bajtos removed their assignment Apr 24, 2018
@virkt25
Copy link
Contributor

virkt25 commented Apr 24, 2018

+1 for having a top-level property for configuring the basePath. Question though ... how does a controller customize its basePath?

@virkt25 virkt25 removed their assignment Apr 24, 2018
@raymondfeng
Copy link
Contributor

raymondfeng commented Apr 24, 2018

+1 to expose basePath property or a setter to configure server level base path.

To provide base path for a controller class, I see two options:

  1. Introduce @path to decorate a controller class:
@path('/customers')
export class CustomerController {
}
  1. Add x-base-path property to @api.

@virkt25
Copy link
Contributor

virkt25 commented Apr 24, 2018

I like option 1 -- @path decorator. With that I image we can even make @get() & friends only require a path for a nested path (/:id), otherwise just assume it's for /

@bajtos
Copy link
Member

bajtos commented Apr 26, 2018

Add x-base-path property to @api.

Right now, @api already supports basePath property. Are you proposing to rename this property to x-base-path?

Introduce @path to decorate a controller class:

+1 for this option. I personally prefer a different name - @basePath or perhaps @rootPath, because it allows us to use a consistent name at both controller and server levels. I am worried that if we used path at server level for consistency, then it would be too cryptic.

class MyApp extends RestApplication {
  constructor() {
    this.path = '/api';
  }
}

@shimks
Copy link
Contributor

shimks commented May 14, 2018

For clarification, if a user was to set the base path as /my-path and if the user was to have a route decorated with @get('/user/{id}'), would that endpoint be served at some-server/my-path/user/{id} or some-server/my-path/{id}?

@virkt25
Copy link
Contributor

virkt25 commented May 15, 2018

I image the following is how basePath would work:

app.basePath('/api') // => This doesn't apply to staticFiles and /swagger-ui endpoint.

@path('/users')
class MyController {
  // Resolves for: /api/users
  @get('/')
  getMethod() { ... }

  // Resolves for: /api/users/{id}
  @get('/{id}')
  byId() { ... }
}

class NoPathController {
  // Resolves for: /api/cart
  @get('/cart') 
  userCart() { ... }
}

@bajtos bajtos added the p2 label May 15, 2018
@bajtos
Copy link
Member

bajtos commented May 16, 2018

+1 for what TV wrote above.

@raymondfeng
Copy link
Contributor

I think we should move .basePath to RestServer and RestApplication:

  • http://localhost/<basePath1>/...
  • https://localhost/<basePath2>/...

@bajtos
Copy link
Member

bajtos commented Jul 30, 2018

Removed from 4.0 GA, moved to [EPIC] REST layer improvements (post-GA) #1452

@raymondfeng
Copy link
Contributor

Implemented by #2097

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

8 participants