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

[core] Normalizing vendor extension naming #5192

Merged

Conversation

jimschubert
Copy link
Member

According to OAS 2.0 and OAS 3.0 Specifications:

Allows extensions to the OpenAPI Schema. The field name MUST begin with x-,
for example, x-internal-id. The value can be null, a primitive, an array or an object.
Can have any valid JSON format value.

This commit attempts to define a clear identifier design format of maintaining lower-kebab casing and following the x- prefix defined by OAI Specification.

Following a convention that matches that used by others (see autorest), we will remove any confusion about naming strategies for template authors and customizers. Following the lower-kebab convention will allow us to convert from camelCase and missing prefixes to the desired format. For example, these conversions are simple to make for template consistency:

  • customValue => x-custom-value
  • x-customValue => x-custom-value
  • x-custom-value => x-custom-value

This convention also allows us to define a single standard for use across all generators. This means no occurrence of x-operationId in one generator and x-operation-id in another.

Included in this PR is a logger which will log a warning message only once. This could be used for ModelUtils and UrlUtils to reduce stdout/stderr noise. The LoggerWrapper implementation is supported by an in-memory (caffeine) cache. I've added support for the following system properties so consumers with more advanced use cases can tweak/disable the caching OnceLogger:

  • org.openapitools.codegen.utils.oncelogger.enabled: set to false to disable the caching OnceLogger. Set to true to enable (the default).
  • org.openapitools.codegen.utils.oncelogger.cachesize: modify the cache size from the default of 200 messages (this is log message, not message count)
  • org.openapitools.codegen.utils.oncelogger.expiry: set the expiration time in millis to control how long messages remain in-memory. The default is 2000 (2 seconds), but could be made lower for high traffic consumers. For one-off use cases such as CLI, Maven Plugin, and Gradle Plugin, changing this property may not be useful.

NOTE This will require additional template work and removal of older extensions in the 5.0 branch. We'll need document the change(s) and upgrade path for the 5.0 release as well.

cc @OpenAPITools/generator-core-team @OpenAPITools/openapi-generator-collaborators

PR checklist

  • Read the contribution guidelines.
  • If contributing template-only or documentation-only changes which will change sample output, build the project before.
  • Run the shell script(s) under ./bin/ (or Windows batch scripts under.\bin\windows) to update Petstore samples related to your fix. This is important, as CI jobs will verify all generator outputs of your HEAD commit, and these must match the expectations made by your contribution. You only need to run ./bin/{LANG}-petstore.sh, ./bin/openapi3/{LANG}-petstore.sh if updating the code or mustache templates for a language ({LANG}) (e.g. php, ruby, python, etc).
  • File the PR against the correct branch: master, 4.3.x, 5.0.x. Default: master.
  • Copy the technical committee to review the pull request if your PR is targeting a particular programming language.

According to [OAS 2.0][1] and [OAS 3.0][2] Specifications:

> Allows extensions to the OpenAPI Schema. The field name MUST begin with x-,
>  for example, x-internal-id. The value can be null, a primitive, an array or an object.
>  Can have any valid JSON format value.

This commit attempts to define a [clear identifier design format][3] of
maintaining lower-kebab casing and following the x- prefix defined by
OAI Specification.

Following a convention that matches that used by others (see [autorest][4]), we will remove
any confusion about naming strategies for template authors and
customizers. Following the lower-kebab convention will allow us to
convert from camelCase and missing prefixes to the desired format. For
example, these conversions are simple to make for template consistency:

* customValue => x-custom-value
* x-customValue => x-custom-value
* x-custom-value => x-custom-value

This convention also allows us to define a single standard for use
across all generators. This means no occurrence of x-operationId in one
generator and x-operation-id in another.

[1]: https://github.com/OAI/OpenAPI-Specification/blob/master/versions/2.0.md#patterned-objects
[2]: https://github.com/OAI/OpenAPI-Specification/blob/master/versions/3.0.2.md#specificationExtensions
[3]: https://tools.ietf.org/html/draft-wilde-registries-01#section-3.4
[4]: https://github.com/Azure/autorest/tree/master/docs/extensions
@jimschubert jimschubert added this to the 4.3.0 milestone Feb 2, 2020
@auto-labeler
Copy link

auto-labeler bot commented Feb 2, 2020

👍 Thanks for opening this issue!
🏷 I have applied any labels matching special text in your issue.

The team will review the labels and make any necessary changes.

Copy link
Contributor

@ybelenko ybelenko left a comment

Choose a reason for hiding this comment

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

That's why I prefer dashed on underscored names over camelCased. Simplest example is when you have schema with unique sms id value. Some coders may call it uniqueSMSId or uniqueSmsID or even uniqueSMSID. This name is not convertible back and forward when we need camelCase and kebab/snake case vars within a project. If humans cannot say for sure how to convert that kind of variable, so machine scripts can produce something even weirder uniqueSMSID -> unique-s-m-s-i-d.

if (shouldLog(msg)) super.trace(MARKER, msg);
}

private boolean shouldLog(final String msg) {
Copy link

@ben-manes ben-manes Feb 2, 2020

Choose a reason for hiding this comment

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

maybe a single operation if you want to avoid races?
messageCountCache.asMap().merge(msg, 1, count -> count + 1)

or an atomic counter if you want to avoid locking except on the initial mapping?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the quick and helpful review, Ben! I decided to go the AtomicInteger route because it's not critical functionality for this method and doesn't need repeated locking, I'm just trying to remove noise for users.

Copy link

@ben-manes ben-manes Feb 3, 2020

Choose a reason for hiding this comment

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

I think that's best choice, too!

However, it no longer writes into the map on every shouldLog call, causing the expireAfterWrite to evict based on the entry's creation time (as no updates). The old behavior was to reset the expiration each time the method was called, so it requires no occurrences for the expiration duration in order to evict. That could be done now using expireAfterAccess to reset on every read or write, which is fine as this is the sole usage.

I suspect you don't want to suppress an ongoing error for good once the threshold is reached, as that makes it appear to be over, and simply want to debounce over a time interval. This new expiration behavior might be more correct, but I'm unsure. You probably want to think a bit about exactly what expiration behavior you want and maybe write a quick test to verify that.

Copy link
Member Author

Choose a reason for hiding this comment

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

You're correct that my intent was to debounce over a time internal. I think I mistakenly took expireAfterWrite to mean initial writes only, but looking at it after your comment I see that understanding wouldn't have made sense if I'd looked at the method a little more closely.

I'll definitely need to add a comment about intention on the cache member, and write a handful of tests. I'll have to think about usage for errors and other levels a little more. The way I wrote this, it'll require that someone explicitly decorates a Logger instance and I may just throw an error if it's used to log error messages, or just skip the cache evaluation for that method.

@wing328 wing328 merged commit 8d6286d into OpenAPITools:master Feb 6, 2020
@jimschubert jimschubert deleted the normalizing-vendor-extensions branch February 6, 2020 15:14
@jimschubert jimschubert linked an issue Feb 7, 2020 that may be closed by this pull request
MikailBag pushed a commit to MikailBag/openapi-generator that referenced this pull request Mar 23, 2020
* [core] Normalizing vendor extension naming

According to [OAS 2.0][1] and [OAS 3.0][2] Specifications:

> Allows extensions to the OpenAPI Schema. The field name MUST begin with x-,
>  for example, x-internal-id. The value can be null, a primitive, an array or an object.
>  Can have any valid JSON format value.

This commit attempts to define a [clear identifier design format][3] of
maintaining lower-kebab casing and following the x- prefix defined by
OAI Specification.

Following a convention that matches that used by others (see [autorest][4]), we will remove
any confusion about naming strategies for template authors and
customizers. Following the lower-kebab convention will allow us to
convert from camelCase and missing prefixes to the desired format. For
example, these conversions are simple to make for template consistency:

* customValue => x-custom-value
* x-customValue => x-custom-value
* x-custom-value => x-custom-value

This convention also allows us to define a single standard for use
across all generators. This means no occurrence of x-operationId in one
generator and x-operation-id in another.

[1]: https://github.com/OAI/OpenAPI-Specification/blob/master/versions/2.0.md#patterned-objects
[2]: https://github.com/OAI/OpenAPI-Specification/blob/master/versions/3.0.2.md#specificationExtensions
[3]: https://tools.ietf.org/html/draft-wilde-registries-01#section-3.4
[4]: https://github.com/Azure/autorest/tree/master/docs/extensions

* Incorporate feedback to avoid race/blocking in OnceLogger

* Remove unnecessary additional log config

* Add tests,comments for OnceLogger

* Test caffeine cache with FakeTicker
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[REQ] Standardize vendor extension naming conventions
4 participants