feat: add 'x-visibility' extension property to OpenAPI spec #1896

hacksparrow · 2018-10-22T11:22:56Z

Add x-visibility extension property to OpenAPI spec.

Addresses #1874.

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

bajtos

Great start, let's polish the details now.

bajtos · 2018-10-22T12:30:28Z

packages/rest/src/router/routing-table.ts

-      if (!paths[route.path]) {
-        paths[route.path] = {};
-      }
+      if (!route.spec['x-internal']) {


Please use reverse logic + early return to make the code easier to follow and less nested.

if (route.spec['x-internal']) continue;

bajtos · 2018-10-22T12:31:54Z

packages/rest/test/unit/rest.server/rest.server.open-api-spec.unit.ts

+    function greet() {}
+    function meet() {}
+    server.route(
+      new Route(


Let's use the recently introduced sugar API instead of calling new Route.

server.route('get', '/greet', {'x-internal': true, /*...*/}, greet);

VSC is complaining No overload expects 4 arguments, but overloads do exist that expect either 1 or 6 arguments..

Did you mean something like this?

server.route('get', '/greet', spec, MyController, factory, 'greet');

We'll have to create MyController and factory, in that case. It adds more to the test code.

I opened a pull request to fix that problem, see #1898

packages/rest/test/unit/rest.server/rest.server.open-api-spec.unit.ts

bajtos · 2018-10-22T12:37:59Z

packages/rest/src/router/routing-table.ts

@@ -202,7 +208,7 @@ export interface RouteEntry {
  /**
   * OpenAPI operation spec
   */
-  readonly spec: OperationObject;
+  readonly spec: LB4OperationObject;


This change is not strictly necessary because OperationObject type already supports arbitrary extension (additional properties). Having said that, I see how LB4OperationObject can be useful because it allows IDEs like VisualStudio Code to offer auto-completion and also the compiler can check that developers don't assign a non-boolean value to this extension by a mistake.

So, if we want to keep LB4OperationObject, then please find other places in our public API that are accepting OperationObject and change them accordingly. Few examples to get you started:

@operation decorator and verb-based shortcuts like @get

server.route() method

Route and ControllerRoute classes (and their constructors)

Let's keep it and update the references.

Replacing OperationObject with LB4OperationObject in all instances is not easy, let's not do it in this PR. So, for not let's keep it as such. Reverting to readonly spec: OperationObject.

bajtos · 2018-10-22T14:25:26Z

packages/rest/src/router/routing-table.ts

      if (!paths[route.path]) {
        paths[route.path] = {};
      }
-


please preserve this empty line - it serves as a visual delimiter

bajtos · 2018-10-22T14:25:44Z

packages/rest/src/router/routing-table.ts

      paths[route.path][route.verb] = route.spec;
    }
-


please preserve this empty line - it serves as a visual delimiter to make the code easier to read (skim through)

bajtos · 2018-10-22T14:26:24Z

packages/rest/test/unit/rest.server/rest.server.open-api-spec.unit.ts

+        greet,
+      ),
+    );
+    server.route(new Route('get', '/meet', {responses: {}, spec: {}}, meet));


Please replace server.route(new Route(...)) to server.route(...), see #1898

raymondfeng · 2018-10-22T14:27:45Z

packages/rest/src/router/routing-table.ts

@@ -156,13 +156,12 @@ export class RoutingTable {
    const paths: PathObject = {};

    for (const route of this._router.list()) {
+      if (route.spec['x-internal']) continue;


I don't think x-internal reflects the purpose very well. Do we want to hide the route from OpenAPI spec or completely disable the route? I see a few options here:

private (hidden from the spec and reports 404 error for the route)

unlisted (hidden from the spec, but the route is still accessible)

We just want to hide the route from OpenAPI spec, so unlisted is what we want.

What purpose could a route that's hidden from the spec and 404s, serve?

@bajtos, thoughts on @raymondfeng's suggestions?

We want to hide the route from the generated OpenAPI spec, but still be available via the REST API for clients that know the route. Example use cases:

a health endpoint used for DevOps monitoring

an internal endpoint used by API Explorer to load dynamic configuration for a static-only front-end (we do this in LB 3.x now)

a catch-all route to serve static files - see fix: optimize serving static files #1848

Personally, I prefer x-hidden: true or x-documented: false most.

bajtos

Besides the alternative names offered in my comment in the discussion above, I guess I can live with x-unlisted or even x-internal too, although I find them less descriptive than my proposals.

The rest of the patch looks good to me.

hacksparrow · 2018-10-23T14:59:25Z

So @raymondfeng what about x-hidden: true or x-documented: false?

raymondfeng · 2018-10-23T15:02:15Z

Maybe something like x-visibility: unlisted or x-visibility: undocumented? This way, we can extend the values in the future.

bajtos · 2018-10-23T15:03:47Z

Let's go with x-visibility: undocumented then 👍

Add `x-visibility` extension property to OpenAPI spec.

hacksparrow self-assigned this Oct 22, 2018

hacksparrow requested review from bajtos and raymondfeng as code owners October 22, 2018 11:22

bajtos suggested changes Oct 22, 2018

View reviewed changes

bajtos reviewed Oct 22, 2018

View reviewed changes

bajtos mentioned this pull request Oct 22, 2018

feat(rest): push route(verb, path, spec, fn) down to RestServer #1898

Merged

4 tasks

raymondfeng reviewed Oct 22, 2018

View reviewed changes

This was referenced Oct 22, 2018

feat: add 'x-internal' extension property to OpenAPI spec #1897

Closed

feat: add 'x-internal' extension property to OpenAPI spec #1895

Closed

hacksparrow force-pushed the fix/#1874 branch 2 times, most recently from 87ed8cc to 1609efb Compare October 23, 2018 06:05

bajtos approved these changes Oct 23, 2018

View reviewed changes

feat: add 'x-visibility' extension property to OpenAPI spec

b9e34cd

Add `x-visibility` extension property to OpenAPI spec.

hacksparrow force-pushed the fix/#1874 branch from 1609efb to b9e34cd Compare October 24, 2018 05:11

hacksparrow changed the title ~~feat: add 'x-internal' extension property to OpenAPI spec~~ feat: add 'x-visibility' extension property to OpenAPI spec Oct 24, 2018

hacksparrow merged commit 5634e18 into master Oct 24, 2018

hacksparrow mentioned this pull request Oct 24, 2018

Add new extension property to hide controllers and routes from openapi.json #1874

Closed

hacksparrow deleted the fix/#1874 branch October 24, 2018 05:49

bajtos mentioned this pull request Sep 18, 2020

Add a new decorator @visibility('undocumented') #6392

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add 'x-visibility' extension property to OpenAPI spec #1896

feat: add 'x-visibility' extension property to OpenAPI spec #1896

hacksparrow commented Oct 22, 2018 •

edited

Loading

bajtos left a comment

bajtos Oct 22, 2018

bajtos Oct 22, 2018

hacksparrow Oct 22, 2018

bajtos Oct 22, 2018

bajtos Oct 22, 2018

hacksparrow Oct 22, 2018

hacksparrow Oct 22, 2018

bajtos Oct 22, 2018

bajtos Oct 22, 2018

bajtos Oct 22, 2018

raymondfeng Oct 22, 2018

hacksparrow Oct 23, 2018

bajtos Oct 23, 2018

bajtos left a comment

hacksparrow commented Oct 23, 2018

raymondfeng commented Oct 23, 2018

bajtos commented Oct 23, 2018

feat: add 'x-visibility' extension property to OpenAPI spec #1896

feat: add 'x-visibility' extension property to OpenAPI spec #1896

Conversation

hacksparrow commented Oct 22, 2018 • edited Loading

Checklist

bajtos left a 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

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 left a comment

Choose a reason for hiding this comment

hacksparrow commented Oct 23, 2018

raymondfeng commented Oct 23, 2018

bajtos commented Oct 23, 2018

hacksparrow commented Oct 22, 2018 •

edited

Loading