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

Configuration annotation processor. #3250

Merged
merged 10 commits into from
Oct 4, 2021

Conversation

tomas-langer
Copy link
Member

@tomas-langer tomas-langer commented Aug 5, 2021

Signed-off-by: Tomas Langer tomas.langer@oracle.com

Documentation is now here: https://github.com/oracle/helidon/pull/3250/files#diff-fb1435883a8ac748d3e2ac8dd8e416d4ff9b32912b3595631479d5f2e3d90f19

To see what can be done - please check the new example. Just add a dependency that has metadata on the classpath, and run it (I would recommend helidon-microprofile bundle, as that contains most of the types that were created as part of this PR.

Annotations are in module helidon-config-metadata
The annotation processor is in module helidon-config-metadata-processor.

There are a few new annotations:

  1. io.helidon.config.metadata.Configured - marks a Builder or a class as configurable through configuration
  2. io.helidon.config.metadata.ConfiguredOption (repeatable) - defines a single configuration option
  3. io.helidon.config.metadata.ConfiguredValue - possible values of an option (such as enum values)

Configured has the following properties:

  • root() - root configured classes can be configured in configuration (such as application.yaml) as standalone components. Example: server, security, tracing
  • prefix() - the configuration prefix used for this class (such as server for web server, abac for ABAC security provider), empty for most nested configurations, as they use the key defined on owning type
  • ignoreBuildMethod() - treat this type as an abstract class/interface and not as a builder type (for cases where the builder is used as a parameter to another builder that builds the target type. This is not according to our guidelines, but used for example in KeyConfig for PEM and Keystore builders
  • provides() - interfaces/abstract classes implemented by this type that are used as providers to another configuration. Example: SecurityProvider SPI in Security, where each provider has its own configuration

ConfigureOption has the following properties:

  • key() - defines key in configuration. Guessed from method name when on builder method (failOnError -> fail-on-error)
  • type() - type of the configuration value. May be another @Configured type. In such a case that type can be used as a subtree of the key of this property. Guessed from method parameter when on builder method (primitive types are boxed)
  • description() - description, created from Javadoc when on builder method
  • required() - set to true for required options, false by default
  • value() - what is the default value of this property, optional (if not defined and not required, default is defined in documentation)
  • experimental() - marks an experimental property
  • kind() - can be configured to LIST, MAP or VALUE, VALUE is the default
  • provider() - set to true if this is provided by a provider that may be in another module
  • allowedValues() - values with description for possible options of a property. Guessed from enum and javadoc for enumerations.
  • deprecated() - deprecated option - description should describe how to change this
  • mergeWithParent() - whether to ignore the key of this property and use all keys in the nested object instead (only for nested types, defaults to false)

Annotations are processed and a META-INF/helidon/config-metadata-json is created for each module
See the readme mentioned above that describes the JSON structure. If you build helidon from this PR, a few metadata files get generated (see security/security, webserver/webserver and some security providers (oidc, http-basic-auth, abac).

Required follow up - I will create these issues once this PR is merged, as working on them would not be benefitial now:

  1. Add metadata to all Helidon modules
  2. Use generated metadata in documentation
  3. Use generated metadata in javadoc (maybe just a reference to a generated document from javadoc, or some more complex solutin)
  4. Consider creating a validator plugin that would check a configuration file and report wrong keys/values based on current classpath
  5. Consider creating a plugin that would generate the sample application.yaml from current project (as the example in this PR does), or that would generate a document with the same information (HTML?)
  6. Consider generating an XML schema for the current classpath + a config source capable of processing configuration generated from that schema

@danielkec
Copy link
Contributor

Awesome! 👏

@tomas-langer tomas-langer marked this pull request as ready for review September 10, 2021 11:46
@tomas-langer tomas-langer linked an issue Sep 23, 2021 that may be closed by this pull request
danielkec
danielkec previously approved these changes Sep 30, 2021
Copy link
Member

@tjquinno tjquinno left a comment

Choose a reason for hiding this comment

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

I've made some detailed comments attached to lines.

Would the build-tools repo, rather than the main repo, be a better fit for the new modules? All the interesting work--or all the work?--happens at build-time.

docs-internal/config-metadata.md Outdated Show resolved Hide resolved
docs-internal/config-metadata.md Outdated Show resolved Hide resolved
docs-internal/config-metadata.md Outdated Show resolved Hide resolved
docs-internal/config-metadata.md Outdated Show resolved Hide resolved
@romain-grecourt
Copy link
Contributor

@tjquinno The processor depends on the new metadata module, It has a high coupling with config overall. Having it co-located with config is a nice convenience. If it were located in build-tools it would make things quite complicated.

@tjquinno
Copy link
Member

@romain-grecourt Neither config/metadata/pom.xml nor config/metadata-processor/pom.xml contains any explicit or transitive dependencies on actual config, do they?

For sure the processor is tightly coupled to the metadata, but I am not seeing the high coupling you are from either of those to config itself.

And now that I look at the poms again, config/config/pom.xml adds a provided optional dependency on the new metadata module but I do not see other files in the change list from config/config that would trigger that added dependency.

@romain-grecourt
Copy link
Contributor

@tjquinno

(not doing a quote reply as this is the very next comment)

config/metadata is a new config module and is used through-out the repository to annotate config related code.
One may have to update the metadata and processor when making certain kind of changes to the config API.

If you look at the processor implementation you can see it visits config code. E.g.

configType = elementUtils.getTypeElement("io.helidon.config.Config").asType();

@tjquinno
Copy link
Member

tjquinno commented Sep 30, 2021

I did get that config/metadata is new and used from lots of modules. That by itself doesn't imply the metadata module should be in the main repo.

On the other hand, configType and builderType in ConfigMetadataProcessor do create the binding between metadata and config/config you alluded to earlier. It looks as if the configType is used in only a few places and maybe those spots could have used String comparisons instead of the (admittedly nicer) type comparisons. But buildType is referenced from many places involving erasures so that would have been more difficult to avoid.

I do get how messy it can get if a module in one repo depends on one from another repo and each has its own release cadence, etc. and I was never arguing for that. It just seems a shame that build-time stuff drifts into the main repo. Always trade-offs.

@tomas-langer
Copy link
Member Author

I've made some detailed comments attached to lines.

Would the build-tools repo, rather than the main repo, be a better fit for the new modules? All the interesting work--or all the work?--happens at build-time.

I think the annotations are part of Helidon public API, so these should be part of Helidon itself. The annotation processor could be part of build tools, but then we would have a cyclic dependency (even if a "soft" one). If we did any change in the annotations, we would need to release new build tools, then upgrade to them in Helidon to be able to update the annotations themself.

Signed-off-by: Tomas Langer <tomas.langer@oracle.com>
Signed-off-by: Tomas Langer <tomas.langer@oracle.com>
Signed-off-by: Tomas Langer <tomas.langer@oracle.com>
Documentation.

Signed-off-by: Tomas Langer <tomas.langer@oracle.com>
…now value

Signed-off-by: Tomas Langer <tomas.langer@oracle.com>
Signed-off-by: Tomas Langer <tomas.langer@oracle.com>
Signed-off-by: Tomas Langer <tomas.langer@oracle.com>
Signed-off-by: Tomas Langer <tomas.langer@oracle.com>
Signed-off-by: Tomas Langer <tomas.langer@oracle.com>
Signed-off-by: Tomas Langer <tomas.langer@oracle.com>
Copy link
Member

@tjquinno tjquinno left a comment

Choose a reason for hiding this comment

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

LGTM.

@tomas-langer tomas-langer merged commit 3cb5c99 into helidon-io:master Oct 4, 2021
@tomas-langer tomas-langer deleted the 1519-config-metadata branch October 4, 2021 15:58
@tomas-langer
Copy link
Member Author

Follow up issue: #3463

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

Successfully merging this pull request may close these issues.

Add configuration metadata
4 participants