Skip to content

Fix binary incompatibility introduced by 4.4.0#582

Merged
zhiyuanliang-ms merged 1 commit intomicrosoft:mainfrom
richardszalay:fix/ConfigurationFeatureDefinitionProvider-bincompat
Mar 4, 2026
Merged

Fix binary incompatibility introduced by 4.4.0#582
zhiyuanliang-ms merged 1 commit intomicrosoft:mainfrom
richardszalay:fix/ConfigurationFeatureDefinitionProvider-bincompat

Conversation

@richardszalay
Copy link
Contributor

@richardszalay richardszalay commented Mar 2, 2026

Why this PR?

4.4.0 introduced a change to ConfigurationFeatureDefinitionProvider, adding a nullable argument to the existing constructor. This was source compatible, but not binary incompatible and so breaks (via MissingMethodException) any libraries that depend on it directly.

Visible Changes

The ConfigurationFeatureDefinitionProviderOptions constructor argument is separated into an overloaded constructor, restoring the < 4.4.0 constructor signature.

@zhiyuanliang-ms
Copy link
Member

zhiyuanliang-ms commented Mar 2, 2026

Hey, @richardszalay Thanks for pointing this out. This is an oversight. Sorry for that.

public ConfigurationFeatureDefinitionProvider(
IConfiguration configuration,
ConfigurationFeatureDefinitionProviderOptions options = null)
ConfigurationFeatureDefinitionProviderOptions options)
Copy link
Member

@zhiyuanliang-ms zhiyuanliang-ms Mar 3, 2026

Choose a reason for hiding this comment

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

Could you please add a test to verify that DI will pick the ctor with most parameters?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think that's necessary, for two reasons:

  1. The library doesn't rely on DI (ref) to construct ConfigurationFeatureDefinitionProviderOptions - it always new's it up inline
  2. The MS DI Constructor selection rules are quite clear: the constructor with the most arguments is used

Copy link
Member

Choose a reason for hiding this comment

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

Make sense!

Copy link
Member

Choose a reason for hiding this comment

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

Referencing implementation shouldn't be used to justify tests. Otherwise, we wouldn't need tests, we could just say look at the code, it works.

The thing that needs coverage is if ConfigurationFeatureDefinitionProviderOptions are registered, then they are respected.

@zhiyuanliang-ms , I believe that's covered here, can you confirm?

Copy link
Member

Choose a reason for hiding this comment

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

confirmed

@zhiyuanliang-ms zhiyuanliang-ms merged commit ed9860d into microsoft:main Mar 4, 2026
4 checks passed
@zhiyuanliang-ms
Copy link
Member

Thank you for your contribution! @richardszalay

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.

3 participants