Skip to content

[release/9.0-rc1] Fallback to treating as object if not collection #106517

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

Merged
merged 4 commits into from
Aug 16, 2024

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Aug 16, 2024

Backport of #106378 to release/9.0-rc1

/cc @ericstj

Customer Impact

  • Customer reported
  • Found internally

Configuration Binder source generator cannot bind objects that implement IEnumerable but not ICollection<> or IDictionary<,>. IOW an enumerable type that's not treated as a collection.

Apache Kafka has a package with configuration objects like this that expose their strongly named properties as an IEnumerable<KeyValuePair<string,string>>: https://github.com/confluentinc/confluent-kafka-dotnet/blob/49f8188eeff4e6a77f9c1bcef68c504e773359f6/src/Confluent.Kafka/Config.cs#L28

This type binds fine with the reflection binder, but fails when using the source generator.

Regression

  • No

Not a regression, but a blocker for folks wanting to enable the source generator / aot.

Testing

Unit tests added, baseline tests evaluated. No regressions, no changes to existing emitted code. New scenarios added to cover these types. Customer type validated fixed with the change.

Risk

Medium - it's enabling binding of types that were previously considered "not supported". This shouldn't break existing successful usage, but it could cause problems when we start to see types we didn't see before. For this reason we'd like to get this out in RC1 and give a chance for customer feedback.

Risk to release is low. This shouldn't risk the RC1 build / stabilization.

The reflection binder will fallback if a type does not meet collection
heuristics, but the source generator did not.
This matches what the refelction binder does, and fixes the baseline
diffs (and diagnostics changes) we were seeing for unsupported
collection types.
Copy link
Contributor

Tagging subscribers to this area: @dotnet/area-extensions-configuration
See info in area-owners.md if you want to be subscribed.

@carlossanlop
Copy link
Contributor

carlossanlop commented Aug 16, 2024

Has this been Tactics approved?

@dotnet/area-extensions-configuration @tarekgh please review.

@tarekgh
Copy link
Member

tarekgh commented Aug 16, 2024

please review.

@carlossanlop review what?

@carlossanlop
Copy link
Contributor

I see your sign off. Sorry.

@ericstj ericstj added the Servicing-consider Issue for next servicing release review label Aug 16, 2024
@ericstj
Copy link
Member

ericstj commented Aug 16, 2024

Has this been Tactics approved?

@dotnet/area-extensions-configuration @tarekgh please review.

Not yet, I just filled in the template and sent the request.

@carlossanlop carlossanlop added api-needs-work API needs work before it is approved, it is NOT ready for implementation Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review api-needs-work API needs work before it is approved, it is NOT ready for implementation labels Aug 16, 2024
@carlossanlop carlossanlop merged commit 63c9565 into release/9.0-rc1 Aug 16, 2024
87 of 91 checks passed
@carlossanlop carlossanlop deleted the backport/pr-106378-to-release/9.0-rc1 branch August 16, 2024 23:04
@github-actions github-actions bot locked and limited conversation to collaborators Sep 16, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants