Skip to content

Conversation

@steveharter
Copy link
Contributor

@steveharter steveharter commented Sep 11, 2023

Address #86333 (RC2 candidate as well)

  • Call the setter for value type properties (with the default value for the property type) if the value is not in the config, unless it is during a Bind (not "Get"). This requires a adding a bool arg to the BindCore() method.
  • Always check for a null value or missing section for properties and if null\missing (this was not done in all cases previously) then don't call the setter with the null. This addresses Config generator overwrites existing properties when provided with empty config section #91380.

Note that this PR matches known semantics regarding the above with the goal of maintaining compat with reflection; this is the case even for non-intuitive cases including:

  • Only value type properties get their setter called; not reference types or Nullable<T>.
  • Also, properties on objects that are nested objects or those added to a collection are also not defaulted through the setter (only properties on root objects are).

We could create an issue for these non-intuitive cases, but would need to consider the benefit:risk including breaking changes.

@ghost
Copy link

ghost commented Sep 11, 2023

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

Issue Details

null

Author: steveharter
Assignees: steveharter
Labels:

area-Extensions-Configuration

Milestone: -

@tarekgh
Copy link
Member

tarekgh commented Sep 12, 2023

@steveharter there are some failures that seem related to the change. Could you please have a look?

@steveharter steveharter requested a review from tarekgh September 13, 2023 01:01
@steveharter steveharter added the source-generator Indicates an issue with a source generator feature label Sep 13, 2023
@steveharter steveharter marked this pull request as ready for review September 13, 2023 01:56
@steveharter steveharter modified the milestone: 8.0.0 Sep 14, 2023
Copy link
Contributor

@layomia layomia left a comment

Choose a reason for hiding this comment

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

Thanks!

@steveharter steveharter merged commit 650eec9 into dotnet:main Sep 14, 2023
@steveharter steveharter deleted the GenPropertySet branch September 14, 2023 21:33
@steveharter
Copy link
Contributor Author

Need to get in at least this before backport merge can be successful: #91717

@steveharter
Copy link
Contributor Author

/backport to release/8.0

@github-actions
Copy link
Contributor

Started backporting to release/8.0: https://github.com/dotnet/runtime/actions/runs/6191249484

@github-actions
Copy link
Contributor

@steveharter backporting to release/8.0 failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch

Applying: Make src gen for property setters consistent with reflection
.git/rebase-apply/patch:218: trailing whitespace.
            Assert.Null(options.OtherCodeUri);            
warning: 1 line adds whitespace errors.
Using index info to reconstruct a base tree...
A	src/libraries/Microsoft.Extensions.Configuration.Binder/gen/Emitter/CoreBindingHelpers.cs
A	src/libraries/Microsoft.Extensions.Configuration.Binder/gen/Specs/Types/SimpleTypeSpec.cs
M	src/libraries/Microsoft.Extensions.Configuration.Binder/tests/Common/ConfigurationBinderTests.TestClasses.cs
M	src/libraries/Microsoft.Extensions.Configuration.Binder/tests/Common/ConfigurationBinderTests.cs
Falling back to patching base and 3-way merge...
Auto-merging src/libraries/Microsoft.Extensions.Configuration.Binder/tests/Common/ConfigurationBinderTests.cs
Auto-merging src/libraries/Microsoft.Extensions.Configuration.Binder/tests/Common/ConfigurationBinderTests.TestClasses.cs
Auto-merging src/libraries/Microsoft.Extensions.Configuration.Binder/gen/Model/ParsableFromStringSpec.cs
Auto-merging src/libraries/Microsoft.Extensions.Configuration.Binder/gen/Helpers/Emitter/CoreBindingHelpers.cs
CONFLICT (content): Merge conflict in src/libraries/Microsoft.Extensions.Configuration.Binder/gen/Helpers/Emitter/CoreBindingHelpers.cs
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Make src gen for property setters consistent with reflection
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

@github-actions
Copy link
Contributor

@steveharter an error occurred while backporting to release/8.0, please check the run log for details!

Error: git am failed, most likely due to a merge conflict.

@ericstj
Copy link
Member

ericstj commented Sep 16, 2023

/backport to release/8.0

@github-actions
Copy link
Contributor

Started backporting to release/8.0: https://github.com/dotnet/runtime/actions/runs/6204180109

@ghost ghost locked as resolved and limited conversation to collaborators Oct 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-Extensions-Configuration source-generator Indicates an issue with a source generator feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants