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

Behavior changed during binding to collections in Net 8 #97558

Closed
bbenameur opened this issue Jan 19, 2024 · 19 comments
Closed

Behavior changed during binding to collections in Net 8 #97558

bbenameur opened this issue Jan 19, 2024 · 19 comments

Comments

@bbenameur
Copy link

bbenameur commented Jan 19, 2024

Description

Duplicated properties when deserialize array from appsetting.json only with .Net 8

Configuration

on the appsetting.json i have an object with this structure:

 "ArrayObject": [
   {  
     "Elements": 
       {
         "Type": "xxxx",
         "BaseUrl": "xxxxxxxx",
         "one": "",
         "xxxx": ""

       }
     
   }
 ]

There are a stranger behavior only with .Net 8
using

var builder = WebApplication.CreateBuilder(args);

var result = builder.Configuration.GetSection("ArrayObject").Get<List<ArrayObject>>();
 the result this case will be 

Regression?

Net8

As you see Elements should be null and empty but with Net 8 no it's take the number of properties on the elements objects.

Other information

It's ok with Net 6 see image
Net6

@eiriktsarpalis eiriktsarpalis transferred this issue from dotnet/core Jan 26, 2024
@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Jan 26, 2024
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Jan 26, 2024
@eiriktsarpalis eiriktsarpalis added area-Extensions-Configuration and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Jan 26, 2024
@ghost
Copy link

ghost commented Jan 26, 2024

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

Issue Details

Description

Duplicated properties when deserialize array from appsetting.json only with .Net 8

Configuration

on the appsetting.json i have an object with this structure:

 "ArrayObject": [
   {  
     "Elements": 
       {
         "Type": "xxxx",
         "BaseUrl": "xxxxxxxx",
         "one": "",
         "xxxx": ""

       }
     
   }
 ]

There are a stranger behavior only with .Net 8
using

var builder = WebApplication.CreateBuilder(args);

var result = builder.Configuration.GetSection("ArrayObject").Get<List<ArrayObject>>();
 the result this case will be 

Regression?

Net8

As you see Elements should be null and empty but with Net 8 no it's take the number of properties on the elements objects.

Other information

It's ok with Net 6 see image
Net6

Author: bbenameur
Assignees: -
Labels:

untriaged, area-Extensions-Configuration

Milestone: -

@bbenameur
Copy link
Author

@ericstj

@bbenameur
Copy link
Author

@eerhardt

@bbenameur
Copy link
Author

@davidfowl

@eerhardt
Copy link
Member

@bbenameur - can you post a complete repro? The code above doesn't compile since it doesn't define ArrayObject.

@bbenameur
Copy link
Author

Hello @eerhardt
this is the project
https://github.com/bbenameur/demo-project
debug please and see the result on program.cs
var result = builder.Configuration.GetSection("ArrayObject").Get<List>();

@ericstj ericstj added this to the 9.0.0 milestone Jan 26, 2024
@ghost ghost removed the untriaged New issue has not been triaged by the area owner label Jan 26, 2024
@ericstj
Copy link
Member

ericstj commented Jan 26, 2024

Confirmed the change in behavior, here's a simplified repro in console app. Having a look for the cause to determine if it's intentional or not.
binding-repro.zip

@bbenameur
Copy link
Author

Yes i confirm that the behavior is changed

@bbenameur
Copy link
Author

@ericstj Am I in the right group dotnet/runtime ?

@bbenameur bbenameur changed the title Bug: during deserialization Net 8 Behavior changed during deserialization Net 8 Jan 26, 2024
@ericstj
Copy link
Member

ericstj commented Jan 26, 2024

Yes this is the right group and area. I'm updating the issue as I understand more. Thank you for the report.

This is the cause: e7e9dbd#diff-2e6f75e1d4f577e1c8c1b6a32d158ce7812a38ad8fcd3b4d762157b92c03e890R420

I read through that PR and didn't see why we were adding another case here. Moreover - we'll only create the instance and never bind any of it's properties which seems wrong. If I change the type to have a constructor, then it will fail:

public class Element
{
	public Element(string type) { Type = type; }
	public string Type { get; set; }
}
var result = config.GetSection("ArrayObject").Get<List<ArrayObject>>(o => o.ErrorOnUnknownConfiguration = true);
System.InvalidOperationException: 'Cannot create instance of type 'Element' because parameter 'type' has no matching config. Each parameter in the constructor that does not have a default value must have a corresponding config entry.'

This is definitely a regression that we'll look into fixing. @bbenameur - is this blocking you? Seems like your JSON might have been set up in a way that didn't match your objects and you weren't noticing that until now. Can you fix either the JSON or your type shape to make things work correctly?

@bbenameur
Copy link
Author

Thanks @ericstj for your quick response,
We were able to bypass this regression with a piece of code (not clean), but no blocking for the moment,
please I would like to receive a notification when the problem is resolved

@tarekgh tarekgh modified the milestones: 9.0.0, 8.0.x Jan 26, 2024
@tarekgh
Copy link
Member

tarekgh commented Jan 26, 2024

Looks we fixed this in 9.0 #96737. We were watching to see if more reports on the issue before porting it to servicing. I'll confirm first the fix address this issue and then will port it to servicing.

@tarekgh
Copy link
Member

tarekgh commented Jan 26, 2024

Update:

#96737 doesn't address this issue. I'll look more here at the right fix.

@ericstj ericstj changed the title Behavior changed during deserialization Net 8 Behavior changed during binding to collections in Net 8 Jan 26, 2024
@ericstj
Copy link
Member

ericstj commented Jan 26, 2024

I originally misidentified the regressing change, it was actually #86485 which was fixing Configuration Binding a dictionary with a key that has empty value skips the key as well - that makes more sense.

It still seems wrong to create an instance when operating on a config section with a value, that value cannot be directly assigned, and there are no child sections. Probably the fix for the above didn't scope down the config input enough - it handled no-child values, but didn't check for no value. Maybe we only do this if the config.value is null as well.

@bbenameur
Copy link
Author

The behavior is ok only if the element class has a constructor and without o => o.ErrorOnUnknownConfiguration = true

public class Element
{
	public Element(string type) { Type = type; }
	public string Type { get; set; }
}

@tarekgh
Copy link
Member

tarekgh commented Jan 28, 2024

@bbenameur

The behavior is ok only if the element class has a constructor and without o => o.ErrorOnUnknownConfiguration = true

This is expected because the binder will not be able to create instance of the Element class. When public Element(string type) is there, the binder can create the instance using the default constructor provided by the compiler.

@bbenameur
Copy link
Author

Hello @tarekgh
So you confirm that it will be fixed on .Net 9?

@tarekgh
Copy link
Member

tarekgh commented Jan 30, 2024

@bbenameur For sure we are going to fix it in 9.0. We'll consider porting the fix back 8.0 if the changes are not risky and scoped. The fix for this is going to span the source generator too which I am currently looking at.

@tarekgh
Copy link
Member

tarekgh commented Feb 1, 2024

This is now fixed in 9.0. We'll watch if we'll get more reports and then consider porting the fix to 8.0.

@tarekgh tarekgh closed this as completed Feb 1, 2024
@tarekgh tarekgh modified the milestones: 8.0.x, 9.0.0 Feb 1, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Mar 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants