API review of Steeltoe.Configuration

[bart-vmware]

Description

This PR is the result of me reviewing the public API surface of the Steeltoe.Configuration namespace, based on this guidance. Notable changes:

• Seal public types that don't seem (properly) designed for inheritance (please indicate where I'm wrong)
• Replace optional parameters with overloads in publicly exposed methods
• Add input validation (null checks) on the parameters of publicly exposed methods
• Change publicly exposed fields into properties, unless they are constants
• Naming changes to comply with .NET Naming Conventions and Capitalization Conventions guidance from Microsoft
• Correct or add doc-comments, as far as I understand the purpose of things
• Prefer exposing interfaces to (read-only) collections, unless impossible due to lack of support in System.Text.Json
• Remove logic that is conditional to the full .NET Framework

To facilitate the process, I've made several changes that are scoped to the Steeltoe.Configuration projects:

• Activate Microsoft.CodeAnalysis.PublicApiAnalyzers to track breaking changes. To see effective changes, diff PublicAPI.Unshipped.txt between the first and latest commit.
• Activate the next Sonar rules:
  • Address SA1401: Fields should be private #928
  • Address S1168: Empty arrays and collections should be returned instead of null #916
  • Address S2360: Optional parameters should not be used #929
  • Address S3872: Parameter names should not duplicate the names of their methods #936
  • Address S3874: "out" and "ref" parameters should not be used #937
  • Address S3898: Value types should implement "IEquatable<T>" #938
  • S3900: Arguments of public methods should be validated against null
  • Address S3956: "Generic.List" instances should not be part of public APIs #941
  • Address S4023: Interfaces should not be empty #947
  • Address S4059: Property names should not match get methods #950
  • Address S4487: Unread "private" fields should be removed #954
  • Address S4004: Collection properties should be readonly #946

Open questions:

• I've left all public types public, because I can't determine whether they should be. Although I don't understand why most types are public in the first place. The odd thing is that types in Steeltoe.Extensions.Configuration.Kubernetes are an exception to this: most types are internal there. What's the intended visibility of the types in the Steeltoe.Configuration namespace? Should we consider pubternal anywhere? In hindsight, this should have been the first step in the API review process.
• Why do we provide a custom command-line configuration provider, given that Microsoft.Extensions.Configuration.CommandLine.CommandLineConfigurationProvider is already provided by ASP.NET?
• What's the point of providing a random value provider? And why is it not generating cryptographically strong values, but instead relies on the deterministic (thus insecure) random generator?
• Should we remove '"Extensions" in the "Steeltoe.Extensions.Configuration" namespace? The reason Microsoft puts Extensions in the namespace is to distinguish them from the core .NET APIs. But that reasoning does not apply to Steeltoe, so I think we should drop it.
• Using the term Abstract in type names is uncommon in the .NET world. The typical convention is to prefix or suffix with Base. Should we rename types such as AbstractOptions and AbstractServiceOptions accordingly?

Fixes #899.

Quality checklist

• Your code complies with our Coding Style.
• You've updated unit and/or integration tests for your change, where applicable.
• You've updated documentation for your change, where applicable.
  If your change affects other repositories, such as Documentation, Samples and/or MainSite, add linked PRs here.
• There's an open issue for the PR that you are making. If you'd like to propose a new feature or change, please open an issue to discuss the change or find an existing issue.
• You've added required license files and/or file headers (explaining where the code came from with proper attribution), where code is copied from StackOverflow, a blog, or OSS.

[bart-vmware]

/azp run Steeltoe.All

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[bart-vmware]

/cc @ccheetham

[TimHess]

@hananiel can you address this one:

Why do we provide a custom command-line configuration provider, given that Microsoft.Extensions.Configuration.CommandLine.CommandLineConfigurationProvider is already provided by ASP.NET?

If we are talking about the SpringBootCmdprovider, it expands Cmd Line '.' delimited configuration key/value pairs that start with "spring." to .NET compatible form. Specifically it helps Steeltoe stream apps play in Dataflow There are other use cases as well where this would be useful - where the platform uses a java friendly convention.

Given other use cases, should this feature be detached from the command line and/or move into Placeholder (since the purpose seems to align as another config transform)?

[bart-vmware]

@hananiel can you address this one:

Why do we provide a custom command-line configuration provider, given that Microsoft.Extensions.Configuration.CommandLine.CommandLineConfigurationProvider is already provided by ASP.NET?

If we are talking about the SpringBootCmdprovider, it expands Cmd Line '.' delimited configuration key/value pairs that start with "spring." to .NET compatible form. Specifically it helps Steeltoe stream apps play in Dataflow There are other use cases as well where this would be useful - where the platform uses a java friendly convention.

Given other use cases, should this feature be detached from the command line and/or move into Placeholder (since the purpose seems to align as another config transform)?

Thanks for the clarification. I was mislead by the type name, thinking it would read command-line values. But what it actually does is rename already-loaded keys, regardless of where they came from. I understand this is a useful feature to keep. While it has functional parity with Placeholder, this implementation is a lot simpler, so I'd prefer to keep them separate. I'm trying to come up with a better name that reflects what it does, but can't think of any. The current name indicates its purpose, which is handling Spring Boot command-line key syntax conversion. We could clarify the documentation a bit, though:

- Configuration provider that expands command-line '.' delimited configuration key/value pairs that start with "spring." to .NET compatible form.
+ Configuration provider that expands '.' delimited configuration keys that start with "spring." (typically originating from Spring Boot command-line parameters) to .NET compatible form.

and:

- Initializes a new instance of the <see cref="SpringBootCommandLineProvider" /> class. The <see cref="Configuration" /> will be created from the <see cref="CommandLineConfigurationProvider" />.
+ Initializes a new instance of the <see cref="SpringBootCommandLineProvider" /> class. The <see cref="IConfiguration" /> is assumed to contain configuration keys originating from the <see cref="CommandLineConfigurationProvider" />.

[bart-vmware]

This PR seems to be so much more than just an API review of the Steeltoe.Configuration components... the changes touch everywhere. So many of the changes seem to have nothing to do with the API surface area of the Configuration components ... and more to do with resharper/analyzer/coding related changes ....

We should probably rename this PR to reflect what it really is ...

I don't think that's fair. Aside from a few small things (such as making test classes sealed, intentionally in a separate commit so it can be glanced over quickly), nearly all changes stem from the guidance here. The subset of sonar rules turned on in this PR exclusively concerns public API surface. This is unrelated to Resharper/StyleCop or code quality, it is about the exposed signatures. And where they changed, I've updated unit tests accordingly. I'm aware that this PR is large, but I've tried very hard to produce commits that target a single purpose, to make code review as easy as possible.

At this point, I guess I'm kind of with Hananiel ... lets merge it and start to work with the changes and see what happens

I disagree, there are still quite some open questions we haven't explored, discussed and decided on. The most important one being: What should be public and what should not be? Based on the outcome, I expect more changes regarding optional parameters and overloads. Merging now would not fulfill the purpose of this PR, which is to rationalize the public API surface and confirm this is what we want to expose.

Reviewing which types should be public may not be as hard as it seems, assuming a basic understanding of the interaction between types. We have 7 projects, each containing 10-15 files on average. We no longer have multiple types per .cs file, so going over the files per project should be doable. Another approach is to look at the PublicAPI.Unshipped.txt files. At a high level: everything in Kubernetes is internal, except for extension methods. In the other projects, nearly everything is public. I'd like to propose to make everything internal everywhere, except for extension methods. From there, we can decide which changes need to be reverted because we intend to expose those types. Would that work?

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[bart-vmware]

We've discussed the open questions in our team meeting and agreed to the following:

• Public types: Make all types internal, except for extension methods. @bart-vmware
• Random: Need to better understand its use cases first. @dtillman
• Namespace: Remove "Extensions" from the namespace. @bart-vmware
• Abstract: Drop in favor of Base prefix/suffix (investigate common patterns in BCL). @bart-vmware

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[sonarcloud]

SonarCloud Quality Gate failed.   

0 Bugs
0 Vulnerabilities
3 Security Hotspots
15 Code Smells

0.0% Coverage
0.2% Duplication

[TimHess]

LGTM