Better basePath support

[dustinbarnes]

Currently, you can set a spec.basePath at the top-level of your spec, but it does not cascade down as expected to all the endpoints in your app. To get a basePath to have an effect, we must pass in a partial ControllerSpec for each controller, negating the benefits of describing something with a base path. I'd expect each endpoint under the primary document to inherit this base path.

In addition, no amount of config will seem to let us put the swagger docs under this basePath as well, since it's hardcoded in rest-server.ts to only serve from the root.

The basePath at the top-level openapi spec seems like a good candidate for defining this, but also maybe providing a general "mount path" for the application, that it's aware it's not at the context root.

The only workaround we see currently is to have a rewriting proxy server that will modify requests coming in to remove a context root, and to modify outgoing requests to add a context root, but this seems ... inefficient :)

[kjdelisle]

This is going to be a very wordy reply, so I apologize in advance!

Originally, our goal was to support both top-down and bottom-up development of a REST application in LoopBack 4. In the case of top-down, we're referring to either providing the whole spec via server.api(...) or by providing fragments of your specification as outlined in Defining and validating the API

As we continued this approach, some community members (both externally and within IBM) commented on how they found the top-down approach to be cumbersome and ugly.

"I don't want to deal with the Swagger; I like how LoopBack 3 generates this for me without needing to build it myself."

Given that feedback, combined with the issue of having far fewer full-time developers on LoopBack as we did last year, we had to begin prioritizing support for features with the goal of an MVP in mind. As a part of our MVP push, we're focusing on bottom-up support instead, though updating the existing documentation is a part of our backlog.

With respect to the basePath issue, I've done a quick poke around in the rest package, and it looks like we need to add support for setting the basePath as an option/injectable for the RestServer.

[kjdelisle]

@strongloop/loopback-devs What's everyone's take on the current documentation? We could:

• Remove it entirely and replace it with new bottom-up approach docs
• Mark it in some way so that users know it doesn't currently apply
• Something else I'm not thinking of?

[jannyHou]

I think the controller class decorator @api takes some of restServer's jobs, to be more specifically, restServer's config's jobs.

People now can specify a basePath in @api, which is not the expected place, IMO. As the discussion above said, basePath should be provided in restServer's config.

And maybe we should:

• rename the controller class decorator to @tag or @paths, to clarify what specs are generated by them.
• separate schema-spec-gen functions from file openapi-v2/src/controller-spec.ts, also talked to @shimks about it (out of the scope of this issue though).

Then the basePath of an app is provided by restServer's config, each controller's relative base path is specified in controller decorator @tag(or @path)

By relative base path I mean:

/appBasePath/Foos
/appBasePath/Foos/{id}
/appBasePath/Foos/{id}/Bars

here /appBasePath is basePath, /Foo is the relative base path for controller Foo

[kjdelisle]

I think we should create the fix for the base path first and then see about creating controller-specific base paths, but there's no reason why we can't create an issue for it now.

With respect to the current @api decorator, if we're going to ditch the documentation for the top-down approach, then I would also say this decorator should be tossed as well.

We've already got a significant case of doc-rot going on here; the last thing I want is to leave in decorators that don't do anything, or worse, do things in an incomplete/wrong fashion.

[jannyHou]

@kjdelisle I am already doing some change to the basePath in PR:
#916

I can address the issue you created in #918, I just don't think we need another PR for it.

but there's no reason why we can't create an issue for it now

Did I say anything that prevents you from creating new issue lol ? I agreed with you: "As the discussion above said, basePath should be provided in restServer's config.", just I believe some code in the restServer accepts basePath from @api, they should be replaced by taking basePath from config. And this results in limiting a controller decorator's job as a completed solution.

[jannyHou]

oh ok, I didn't notice that is a PR, I thought it's just an issue..

[bajtos]

What's everyone's take on the current documentation? We could:

• Remove it entirely and replace it with new bottom-up approach docs
• Mark it in some way so that users know it doesn't currently apply
• Something else I'm not thinking of?

Last time we were discussing top-down approach, we agreed to add a note to related doc pages. See #796 Docs: add a note about top-down approach not being ready yet

[dustinbarnes]

Thanks for taking this so seriously! It's been great to see.

One more thing to note -- when requesting the swagger/openapi documents, we would like those to follow the basepath as well. So if we're rooted at /api (as our current app is trying to do), then a GET /api/swagger.json would return the generated swagger JSON.

[kjdelisle]

For now, we'll have to delay the fix to post-MVP given the current timeline we've set. @jannyHou will be landing a PR in the near-future that will alter how it's working under the hood so I've closed my own PR.

As an alternative, if you'd like to make a pull request once the OpenAPI spec changes land, I could review/land it.

[dhmlau]

related to #918

[dhmlau]

@nabdelgadir , could you please help with the grooming? Thanks!

[nabdelgadir]

Current state:

• For only one controller to have a basePath, the following works:

  @api({basePath: '/api', paths: {}})
  export class MyController {/* ... */}`

• When defining the basePath at the RestApplication/RestServer level, it works for the explorer's path e.g. http://[::1]:3000/api/explorer and all the REST endpoints.

  • /openapi.json remains the same.
  • The explorer still uses the old endpoints as they're without the basePath in openapi.json

Acceptance Criteria

• When basePath is set via app.basePath(), it propagates down to the REST API in openapi.json.
  • Update /openapi.json to include the basePath in the paths e.g. "/api/{Model}/count"
• Verify the explorer uses the correct path when making a request.

[bajtos]

@nabdelgadir

The explorer still uses the old endpoints as they're without the basePath in openapi.json

I believe this is a regression introduced by 912bece.

The code generating openapi.json is taking basePath into account by including it in the server URL, see https://github.com/strongloop/loopback-next/blob/c50bedd60cb799b62e9af54e41f67b1c3f10ec85/packages/rest/src/rest.server.ts#L407-L415

My commit changed the way how basePath is obtained and introduced the bug.

[bajtos]

I believe this is a regression introduced by 912bece.

I opened a pull request to fix the problem - see #3266