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

[REQ] Standardize vendor extension naming conventions #4976

Closed
jimschubert opened this issue Jan 11, 2020 · 7 comments · Fixed by #5192 or #6060
Closed

[REQ] Standardize vendor extension naming conventions #4976

jimschubert opened this issue Jan 11, 2020 · 7 comments · Fixed by #5192 or #6060

Comments

@jimschubert
Copy link
Member

jimschubert commented Jan 11, 2020

Is your feature request related to a problem? Please describe.

Vendor extensions are not intuitive, and are case sensitive. They're also not fully documented on the wiki at https://github.com/OpenAPITools/openapi-generator/wiki/Vendor-Extensions

Describe the solution you'd like

  • We should standardize this to avoid confusion, probably kebab lower case (x-objc-operationid rather than x-objc-operationId).
  • We may want to require that all words are separated by hyphen (x-objc-operation-id rather than x-objc-operationid).

Describe alternatives you've considered

none

Additional context

The issue is apparent in a vendor extension such as x-mysqlSchema. If someone mistakenly defines this as x-mysql-schema or x-mysqlschema, the configuration will not be accepted. However, if we normalize from the current x-mysqlSchema to kebab-cased then lower-case this, the result is x-mysql-schema.

Test examples:

@Test
public void testVendorExtensionConventionConversion(){
    String input = "x-mysqlSchema";
    CaseFormat formatter = CaseFormat.LOWER_CAMEL;
    String output = formatter.converterTo(CaseFormat.LOWER_HYPHEN).convert(input);
    Assert.assertEquals(output, "x-mysql-schema");
}
@Test
public void testVendorExtensionConventionNoop(){
    String input = "x-mysql-schema";
    CaseFormat formatter = CaseFormat.LOWER_CAMEL;
    String output = formatter.converterTo(CaseFormat.LOWER_HYPHEN).convert(input);
    Assert.assertEquals(output, "x-mysql-schema");
}
@Test
public void testVendorExtensionUnknown(){
    String input = "x-mySQLSchema";
    List<String> supportedVendorExtensions = Arrays.asList("x-mysql-schema");
    CaseFormat formatter = CaseFormat.LOWER_CAMEL;
    String output = formatter.converterTo(CaseFormat.LOWER_HYPHEN).convert(input);
    // In implementation, we'd want to log a warning.
    Assert.assertFalse(supportedVendorExtensions.contains(output), "Invalid vendor extension should fail.");
}

We could easily make the change in 5.0 and maintain currently documented extensions. We'd want to wait for 5.0, though, because templates will need to be updated to lower cased vendor extensions.

@jimschubert jimschubert added this to the 4.3.0 milestone Jan 11, 2020
@jimschubert jimschubert changed the title [REQ][docs] Standardize vendor extension naming conventions [REQ] Standardize vendor extension naming conventions Jan 11, 2020
@jimschubert jimschubert modified the milestones: 4.3.0, 5.0.0 Jan 11, 2020
@wing328
Copy link
Member

wing328 commented Jan 13, 2020

We mention the naming convention in https://github.com/OpenAPITools/openapi-generator/blob/master/CONTRIBUTING.md#style-guide

For Vendor Extensions, please follow the naming convention below:

For general vendor extension, use lower case and hyphen. e.g. x-is-unique, x-content-type
For language-specified vendor extension, put it in the form of x-{lang}-{extension-name}. e.g. x-objc-operation-id, x-java-feign-retry-limit
For a list of existing vendor extensions in use, please refer to https://github.com/openapitools/openapi-generator/wiki/Vendor-Extensions. If you've added new vendor extensions as part of your PR, please update the wiki page.

Maybe we can write a script to match these vendor extensions and make sure it's following kebab-case as part of the CI.

@jimschubert
Copy link
Member Author

I agree with the script, but there are a couple other points I mentioned which I think need clarification:

  • Would we require all words be separated by hyphen?
  • The wiki page is out of sync with actual extensions

I wonder if it might be possible to store vendor extensions and descriptions in a map, and allow dumping this map by a --dry-run option on generate? I don't think there's a good way to gather all the vendor extensions until runtime, because many are available only given certain options to generators. If these we documented in code, we could unit test the convention and generate the docs.

@wing328
Copy link
Member

wing328 commented Jan 14, 2020

Would we require all words be separated by hyphen?

I think that's what we suggest at the moment e.g. x-objc-operation-id, x-java-feign-retry-limit

The wiki page is out of sync with actual extensions

Yes, I don't think the users have kept it up-to-date.

allow dumping this map by a --dry-run option on generate?

Don't prefer another option. What about showing it in config-help instead.

I wonder if it might be possible to store vendor extensions and descriptions in a map

Instead of documenting these extensions in the wiki, we can of course store it somewhere else such as a map as you've pointed. I think the key challenge is who's going to keep it up-to-date??

Did you get complaints from users about vendor extensions not documented properly?

@jimschubert
Copy link
Member Author

@wing328 In the linked issue where I referenced this one, I reviewed our wiki before proposing the change to x-oneOfName. I proposed x-oneOf-name because our guidelines aren't clear (even the contributing docs don't clarify the questions I raised, at least for me).

Regarding putting the extensions on config-help, that would only be possible if the extensions are available as a populated field or after the constructor call. Many of our vendor extensions are only known after processOpts, see the Java client and Finch server generators for examples.

@sebastien-rosset
Copy link
Contributor

Instead of documenting these extensions in the wiki, we can of course store it somewhere else such as a map as you've pointed. I think the key challenge is who's going to keep it up-to-date??

Did you get complaints from users about vendor extensions not documented properly?

In my case, it wasn't a "complaint". I was evaluating which tool to use (oss and commercial). The limited documentation of open API tools was definitely a struggle, including lack of vendor extensions, though in the end I did end up using it.

@jimschubert
Copy link
Member Author

You've probably already found this in documentation, but you can currently get details about what's bound to templates using the debug system properties (e.g. -DdebugOpenAPI). More details about each are in the asides of https://openapi-generator.tech/docs/templating#operations

I'm working on https://github.com/OpenAPITools/openapi-generator/projects/5 which is aimed at decoupling much of our code, documentation, and spec support. At it's core, that project is aiming to reduce ramp-up time and make community contributions easier. This is why I've also been working to improve documentation, make info more discoverable, and remove confusion in some areas.

I might rename the project and assign more of the closed PRs to the project to make the process more transparent.

@wing328 wing328 modified the milestones: 5.0.0, 4.3.0 Jan 31, 2020
@jimschubert jimschubert linked a pull request Feb 7, 2020 that will close this issue
5 tasks
@jimschubert
Copy link
Member Author

jimschubert commented Feb 7, 2020

Three parts to this:

@jimschubert jimschubert self-assigned this Feb 14, 2020
@wing328 wing328 modified the milestones: 4.3.0, 4.3.1 Mar 27, 2020
@wing328 wing328 modified the milestones: 4.3.1, 5.0.0 May 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment