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

feat: openapi spec contributor extension point #4258

Merged
merged 1 commit into from
Dec 19, 2019
Merged

Conversation

jannyHou
Copy link
Contributor

@jannyHou jannyHou commented Dec 4, 2019

connect to #3854

  • Created a service as extension point for OpenAPI spec enhancers under folder src/enhancers.

    • the extension point is created in file spec-enhancer.service.ts
    • types.ts describes the extension interface
    • keys.ts gives the extensionpoint a key
  • Added two extensions in test fixtures, they are also the demo example for extension contributors

    • add info spec
    • add security spec
  • Added unit test for service utility functions like findEnhancerByName, applyEnhancerByName, generateSpec

  • Feedback addressed:

    • renamed the extension point to be enhancer instead of contributor(the name is still open for discussion, opinions are welcomed.)
    • based on discussion in comment, I added a default spec merge function leveraging json-merge-patch, it's also used by module ajv.
    • added function findEnhancerByName and applyEnhancerByName to allow people do the merge with a single enhancer, I will enable loading the enhancers by group names in a separate PR.
  • TBD: apply the extensionpoint to rest and other related modules, create corresponding extensions like controller spec builder, security spec builder, etc...in follow-up stories.

  • TBD: load the enhancers by group names in a separate PR.

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

👉 Check out how to submit a PR 👈

@jannyHou jannyHou changed the title [WIP, earlier feedback are welcomed]feat: openapi spec contributor extension point [WIP, early feedback are welcomed]feat: openapi spec contributor extension point Dec 5, 2019

async main() {}

async getSpecService() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The async keyword is not needed here.

import {SPEC_SERVICE} from './keys';
import {SpecService} from './spec-contributor.service';

export class SpecComponent implements Component {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it's overkill to have a SpecComponent. We can register the SpecService as part of RestComponent.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point agreed.

@@ -0,0 +1,12 @@
import {createBindingFromClass} from '@loopback/context';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's use a different directory name for extension-point, which is generic.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, any suggestion? cuz I feel extension-point is generic and spec-enhancer is more specific

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe enhancers

* Find contributors for a given field
* @param fieldName - The field name
*/
async findContributors(): Promise<OAISpecContributor[] | undefined> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method can be removed to use this.getContributors() directly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right. created it for filtering by field, not needed any more.

*/
async generateSpec(options = {}): Promise<OpenApiSpec> {
const contributors = await this.findContributors();
if (!contributors) return this._spec;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll get [] if no contributors are found.

get spec(): OpenApiSpec {
return this._spec;
}
set spec(value: OpenApiSpec) {
Copy link
Contributor Author

@jannyHou jannyHou Dec 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copied from chat with @raymondfeng

when openapi spec is generated, we first build it from controller inspections, then call enhancers to update the spec

I add a setter for the _spec here so people can pass in an existing spec to enhance by the service.

@@ -0,0 +1,23 @@
import {Application, createBindingFromClass} from '@loopback/core';
import {OAISpecEnhancerService, OAISPEC_ENHANCER_SERVICE} from '../../../../';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we use OAISpec instead of OpenAPISpec or OASpec? What would the I mean in the acronym? Isn't the I part of the other acronym API?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, double checked with https://github.com/OAI/OpenAPI-Specification/blob/master/versions/3.0.0.md#introduction, I think a more accurate abbreviation should be OAS, which stands for OpenAPI Specification.

I used OAI because it's their github organization name.

beforeEach(givenAppWithSpecComponent);
beforeEach(findSpecService);

it('greets by language', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

greets by language

Need to change

@@ -0,0 +1,69 @@
// Copyright IBM Corp. 2019. All Rights Reserved.
// Node module: @loopback/example-greeter-extension
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@loopback/example-greeter-extension

Need to change

title: 'LoopBack Application',
version: '1.0.0',
},
paths: {},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to set

servers: [{url: '/'}],

as well.

Please see : https://github.com/strongloop/loopback4-example-shopping/blob/master/packages/shopping/src/application.ts#L68

When we added the code to add security spec to the open api spec for this shopping example, we also had to set the servers url to '/'

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please note not all root level fields are required in a complete OpenAPI spec, see its interface

Like servers: [{url: '/'}], should be filled by our rest server component.

@emonddr
Copy link
Contributor

emonddr commented Dec 9, 2019

@jannyHou , great start :)

@@ -8,6 +8,7 @@
"dependencies": {
"@loopback/context": "^1.24.0",
"@loopback/repository-json-schema": "^1.11.2",
"@loopback/core": "^1.11.0",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you need things from core that are not part of context, then please remove the dependency on @loopback/context on L9 and rework imports to use @loopback/core instead.

modifySpec(spec: OpenApiSpec) {
spec.components = spec.components ?? {};
spec.components.securitySchemes = SECURITY_SCHEME_SPEC;
return spec;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am little bit concerned about the design where the spec-enhancer is modifying the spec directly. Every implementation has to deal with detecting whether optional objects like spec.compontents are set and be prepared to handle the case when they are not present.

Have you considered a different approach, where the enhancer provides new data to be added to the spec, and LB runtime ensures the data is cleverly merged together?

Take the InfoSpecEnhancer for an example. IIUC, it will discard any info fields provided by the app developer and replace them with info object containing only title and version. This problem is easy to miss when reviewing the code. I would prefer to design spec-enhancing infrastructure in a way that will make errors like these more difficult to make.

Copy link
Contributor Author

@jannyHou jannyHou Dec 9, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you considered a different approach, where the enhancer provides new data to be added to the spec, and LB runtime ensures the data is cleverly merged together?

Good question, I understand your concern and the risk to give enhancers the right to modify the entire spec, and actually for the original issue(add endpoint level security spec for auth automatically) we are addressing, WE CONTROL THE MERGING LOGIC is easier than my current approach in this PR.

While my concern is...for a fragment contributed by an arbitrary extension, how do we decide the merging logic?

I added a separate comment to explain how to add endpoint level security spec in #4258 (comment)

Copy link
Contributor Author

@jannyHou jannyHou Dec 9, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An explanation for the comment above

Add endpoint level security spec as follow:

paths:
  /something:
    get:
      security:
        - basicAuth:[]
  • Developer decorate endpoint as @authenticate('basicAuth')
  • Basic auth strategy component provide the security spec fragment and register the it as an OASEnhancer extension.(here the extension only provides the spec, but don't have merge logic)
  • In the rest module, we first generate the entire paths spec for the controller methods with auth metadata included, e.g. x-auth-strategy-name: 'basicAuth'. Then iterate through all the endpoints, retrieve the security spec fragment based on their auth metadata by calling OASenhancer.getSpecByName('basicAuth'), merge fragment into the path spec.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While my concern is...for a fragment contributed by an arbitrary extension, how do we decide the merging logic?

I am proposing to start with the simplest option that works for security schemas. We can look into more flexible & powerful merging strategies later, as we learn about new different use cases.

I think the initial merging strategy can do the following:

  • If the value is scalar (not object, not an array), then set it as-is.
  • If the value is an object, then recursively merge all properties.
  • If the value is an array, then append items provided by OASenhancer to the existing array.

A mock-up implementation:

function merge(current, enhancement) {
  const result = _.cloneDeep(current);
  for (key in enhancement) {
    const val = enhancement[key];
    if (Array.isArray(val)) {
      // TODO: verify that current[key] is either `undefined` or an array
      result[key] = [...current[key], ...val];
    } else if (typeof val === 'object') {
      // TODO: verify that current[key] is either `undefined` or an object
      result[key] = merge(current[key] ?? {}, val);
    } else {
      result[key] = val;
    }
  }
  return result;
}

Thoughts?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, based on our chat, I see that I misunderstood this thread. My comment above is proposing a merging rule for security schemes in /components/security, not for endpoint-level security requirements.

When it comes to security requirements at endpoint level, I feel it's important to have a somewhat generic tool provided by @loopback/rest, a tool that any custom authentication solution can leverage.

This will make it easy for any extensions to contribute endpoint-level security requirements. If somebody decides to build an alternative to @loopback/authentication extension, then they should have all tools available and should not need to re-implement contributions of endpoint security requirements.

The rest is just implementation details in a sense.


One option is to use the spec enhancer as proposed so far.

In https://github.com/strongloop/loopback-next/pull/4258/files#r357211992, I proposed to implement a helper function for merging OpenAPI spec fragments. If we decide to use the concept of OpenAPI spec enhancer as the tool for contributing endpoint security requirements, then I would like @loopback/rest to expose a function for setting (or adding) a new security requirement for the given endpoints.

For example (assuming a hard-coded security spec to keep the example simpler):

import {assignSecurityRequirement, /*...*/} from '@loopback/rest';

@bind(asSpecEnhancer)
export class BasicAuthSpecEnhancer implements OAISpecEnhancer {
  modifySpec(spec: OpenApiSpec) {
    return assignSecurityRequirement({
      spec, // the original spec
      verb: 'get', // http verb
      path: '/greet',  // http path
      name: 'petstore_auth', // name of the security schema
      scopes: ['read:pets'], // optional, defaults to []
   });
  }
}

On the second thought, maybe we don't need any new function and assignSpecFragment is actually all we need:

assignSpecFragment(spec, {
  paths: {
    [endpointPath]: {
      [endpointVerb]: {
        security: {
          petstore_auth: [
            'read:pets'
          ],
         }
       }
     }
  }
});

Another option is to introduce a new decorator. If you think about how we are defining endpoint metadata now, we have several decorators like @param and @requestBody that are contributing spec fragments that are later combined into a single operation object. I have an idea to implement a new decorator for defining security requirements.

Example usage:

import {get, security} from '@loopback/rest'

class GreetController {
  @security('petstore_auth', [
    "write:pets",
    "read:pets",
  ])
  @get('/hello')
   greet() {
     return 'hello world';
  }
}

The @authenticate decorator can then call @security under the hood to contribute the correct security requirement to the OpenAPI spec.

import {get} from '@loopback/rest';
import {authenticate} from '@loopbakc/authenticate';

class GreetController {
  @authenticate('basicAuth')
  // ^-- will call security('basicAuth') under the hood
  @get('/hello')
   greet() {
     return 'hello world';
  }
}

Anyhow. I don't know if the @security decorator is a viable solution that can support our current auth/auth design.

I am posting these ideas just for inspiration, please don't hold to them.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bajtos Good summary, based on all our discussions, I changed the implementation and design to be:

  • I still kept the modifySpec function in the interface to give people the max flexibility to modify the original spec:
  • As we chatted in the meeting, I introduced a default spec merge function(merge a patch spec into the original spec), and exported it for extension developers to use in the modifySpec function.
    • It leverages an existing json-merge-patch module to merge the spec. I chose it because ajv module also uses it. More details can be found in the API documentation.
  • I added two more util functions to the service getEnhancerByName and applyEnhancerByName, so people can apply a single enhancer in case they need, or doesn't want to automatically apply all the enhancers in generateSpec.
  • For the endpoint level security spec, I tend to agree with adding a new decorator @security(). Reasons are same as what you explained:
    • "When it comes to security requirements at endpoint level, I feel it's important to have a somewhat generic tool provided by @loopback/rest, a tool that any custom authentication solution can leverage."

* Generate OpenAPI spec from contributors
*/
async generateSpec(options = {}): Promise<OpenApiSpec> {
const contributors = await this.getContributors();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How are we going to decide in which order the contributors are applied? What if we have two contributors adding conflicting values for the same field in the spec - how is the user going to determine which contributor wins?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question * 2, I share your concern when implementing the extension point but haven't figured out a concrete plan, maybe use a combination of group and alphabet order? like how we load observers group by group in https://loopback.io/doc/en/lb4/Life-cycle.html#observer-groups

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moreover, if extension only contributes spec, and the app decides ALL the merge logic, then the order won't be a problem...

So the design here is related to our discussion in #4258 (comment).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe use a combination of group and alphabet order? like how we load observers group by group in https://loopback.io/doc/en/lb4/Life-cycle.html#observer-groups

Sounds good, I like the idea of using a mechanism we already have in place elsewhere in the framework. This way our users have less to learn.

I think it's ok to leave custom order out of scope of the initial implementation, as long as the design makes it possible to introduce custom order later.

@bajtos
Copy link
Member

bajtos commented Dec 9, 2019

TBD: rename the extensionpoint and related types/keys

  • OpenApiSpecEnhancer?
  • OpenApiSpecContributor?
  • other better name?

How about using the concept of a fragment from GraphQL? Each contributor can provide a fragment of the spec to be merged with other fragments. I can imagine a name like OpenApiFragmentProvider if we want to follow Provider<T> API, otherwise OpenApiFragmentContributor would be my preferred name.

version: '1.0.1',
};
return spec;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer the framework to provide a tool for merging OpenAPI spec fragments for us, so that enhancers don't have to (re)implement the logic.

@bind(asSpecEnhancer)
export class InfoSpecEnhancer implements OAISpecEnhancer {
  modifySpec(spec: OpenApiSpec) {
    return assignSpecFragment(spec, {
      info: {
        title: 'LoopBack Test Application',
        version: '1.0.1',
      },
    });
  }
}

@jannyHou jannyHou changed the title [WIP, early feedback are welcomed]feat: openapi spec contributor extension point feat: openapi spec contributor extension point Dec 16, 2019
@jannyHou jannyHou force-pushed the oaispec/extension branch 5 times, most recently from aee8540 to b859065 Compare December 16, 2019 21:14
info: {title: 'LoopBack Test Application', version: '1.0.1'},
paths: {},
};
const mergedSpec = await specService.applyEnhancerByName('info');
Copy link
Contributor

@agnes512 agnes512 Dec 17, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why SECURITY_SCHEME_SPEC is not part of the expected result? Does the enhancer only update the info part? Cause I thought this line merges the security spec to it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why SECURITY_SCHEME_SPEC is not part of the expected result? Does the enhancer only update the info part? Cause I thought (this line)[https://github.com//pull/4258/files#diff-dd8c9f9bac61f1da3ef811d81198fb9aR40] merges the security spec to it.

The service calls all contributors...including SecuritySpecEnhancer which applies the spec you are refering to.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@agnes512 @emonddr By calling applyEnhancerByName you ONLY APPLY A SINGLE ENHANCER, in this case is info enhancer, that's why the security spec won't be included.

Let me rename the generateSpec function to sth like applyAllEnhancers to be more clear.

Copy link
Contributor

@emonddr emonddr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work, Janny.
:)

}

/**
* Generate OpenAPI spec by apply ALL registered enhancers
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

by applying ALL

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jannyHou , please make the change I recommend above to
the existing comment:

* Generate OpenAPI spec by apply ALL registered enhancers

const enhancers = await this.getEnhancers();
if (_.isEmpty(enhancers)) return this._spec;
for (const e of enhancers) {
this._spec = e.modifySpec(this._spec);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why don't we call applyEnhancerByName here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

applyEnhancerByName() calls getEnhancerByName(), which loads all enhancers again by calling getEnhancers(), see its implementation:

/**
   * Find an enhancer by its name
   * @param name The name of the enhancer you want to find
   */
  async getEnhancerByName(name: string): Promise<OASEnhancer | undefined> {
    // Get the latest list of enhancers
    const enhancers = await this.getEnhancers();
    return enhancers.find(e => e.name === name);
  }

And applyEnhancerByName essentially calls enhancer's modifySpec function.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realize that. I just though we could make use of applyEnhancerByName from within the loop, but just noticed that the latter method performs a get on the enhancers and we won't want to be doing that twice. :) Looks good. disregard my question. :)


/**
* The default merge function to patch the current OpenAPI spec.
* It leverages module `json-merge-patch`'s merge API to merge two json object.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

two json objects.

*/
export const asSpecEnhancer: BindingTemplate = binding => {
extensionFor(OAS_ENHANCER_EXTENSION_POINT_NAME)(binding);
// is it ok to have a different namespace than the extension point name?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question. Raymond originally used the same value in both places when he created
this function to register authentication strategies. https://github.com/strongloop/loopback-next/blob/master/packages/authentication/src/types.ts#L91 .

export class SecuritySpecEnhancer implements OASEnhancer {
name = 'security';

modifySpec(spec: OpenApiSpec): OpenApiSpec {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Security stuff can be added at the top level, or for operation level. Does this method take this into account?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The operation level spec will be addressed in a new story, see all the discussion in #4258 (comment), me and @bajtos tend to introduce @security to provide the spec. So the extension in this PR only focus on merging the security scheme into components.

expect(mergedSpec).to.eql(EXPECTED_SPEC);
});

function givenAppWithSpecComponent() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

by component here, do we mean a LB4 component, or a openApiSpec document with a 'component' section?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch, no component any more.

expect(specService.spec).to.eql(EXPECTED_SPEC);
});

it('generateSpec - loads and create spec for ALL registered extensions', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

loads and creates spec

OR

loads and applies spec

Do we want to say create or apply? You have methods like ApplyEnhancer etc

@emonddr
Copy link
Contributor

emonddr commented Dec 17, 2019

@jannyHou, please update the Checklist section by checking the relevant checkboxes

@dhmlau
Copy link
Member

dhmlau commented Dec 18, 2019

@jannyHou, are you planning to add docs as a follow up PR of the task? Thanks.

@raymondfeng
Copy link
Contributor

@jannyHou I like the idea to separate modifySpec and mergeSpec. The mergeSpec is a helper but we should allow an enhancer to modify the spec including removing some part of the spec, which will be very difficult to achieve via merging. For the 1st version, modifySpec is the simplest form that doesn't require the extension point to be magic in merging.

@jannyHou
Copy link
Contributor Author

are you planning to add docs as a follow up PR of the task? Thanks.

Yep adding the doc :)

please update the Checklist section by checking the relevant checkboxes

Will update after adding doc :)

I like the idea to separate modifySpec and mergeSpec.
For the 1st version, modifySpec is the simplest form that doesn't require the extension point to be magic in merging.

Great 👍 I believe now team agrees on the plan.

}

/**
* Generate OpenAPI spec by apply ALL registered enhancers
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jannyHou , please make the change I recommend above to
the existing comment:

* Generate OpenAPI spec by apply ALL registered enhancers

const enhancers = await this.getEnhancers();
if (_.isEmpty(enhancers)) return this._spec;
for (const e of enhancers) {
this._spec = e.modifySpec(this._spec);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realize that. I just though we could make use of applyEnhancerByName from within the loop, but just noticed that the latter method performs a get on the enhancers and we won't want to be doing that twice. :) Looks good. disregard my question. :)

Copy link
Contributor

@agnes512 agnes512 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just a few comments.

docs/site/Extending-OpenAPI-specification.md Outdated Show resolved Hide resolved
docs/site/Extending-OpenAPI-specification.md Outdated Show resolved Hide resolved
docs/site/Extending-OpenAPI-specification.md Outdated Show resolved Hide resolved
docs/site/Extending-OpenAPI-specification.md Outdated Show resolved Hide resolved
## OAS Enhancer Service as Extension Point

The OAS enhancer extension point is created in package `@loopback/openapi-v3`.
It organizes the registered OAS enhancers, and as the first implementation, it
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does as the first implementation mean?

Copy link
Contributor Author

@jannyHou jannyHou Dec 19, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@agnes512 we will have follow-up stories to improve this feature.

docs/site/Extending-OpenAPI-specification.md Outdated Show resolved Hide resolved
Copy link
Contributor

@nabdelgadir nabdelgadir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly LGTM just some nitpicks 👍

Edit: sorry I didn't see @agnes512's comments before submitting

docs/site/Extending-OpenAPI-specification.md Outdated Show resolved Hide resolved
docs/site/Extending-OpenAPI-specification.md Outdated Show resolved Hide resolved
docs/site/Extending-OpenAPI-specification.md Outdated Show resolved Hide resolved
docs/site/Extending-OpenAPI-specification.md Outdated Show resolved Hide resolved
docs/site/Extending-OpenAPI-specification.md Outdated Show resolved Hide resolved
docs/site/Extending-OpenAPI-specification.md Outdated Show resolved Hide resolved
docs/site/Extending-OpenAPI-specification.md Outdated Show resolved Hide resolved
docs/site/Extending-OpenAPI-specification.md Outdated Show resolved Hide resolved
packages/openapi-v3/src/enhancers/index.ts Show resolved Hide resolved
* @param currentSpec The original spec
* @param patchSpec The patch spec to be merged into the original spec
*/
export function defaultMergeFn(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should rename it to something like patchOpenApiSpec or mergeOpenApiSpec. For a consumer, it is a bit strange to call defaultMergeFn(...).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed, mergeOpenApiSpec sounds better to me, will rename it.

@jannyHou jannyHou merged commit 9fee3f3 into master Dec 19, 2019
@jannyHou jannyHou deleted the oaispec/extension branch December 19, 2019 20:33
};

export const SECURITY_SCHEME_SPEC: SecuritySchemeObjects = {
bearerAuth: {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@emonddr Nitpick: Would it make sense to use jwt specifically rather than bearerAuth to be more descriptive?

      "jwt": {
        "type": "http",
        "scheme": "bearer",
        "bearerFormat": "jwt"
      }

It would make the openapi security object more informative IMO as JWT's are nearly always used as bearer tokens where are the former can vary.

  "security": [
    {
      "jwt": []
    }
  ],

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

Successfully merging this pull request may close these issues.

8 participants