dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.28k stars 4.73k forks source link

Improve error message in scenaria where `JsonIgnore` is combined with `required` members. #92330

Open grosch-intl opened 1 year ago

grosch-intl commented 1 year ago

Description

It's often necessary to use the required flag on a property that you don't want serialized via JSON. Adding the JsonIgnore attribute to such a property results in an InvalidOperationException.

Reproduction Steps

void Main() {
    var dto = new DTO {
        NotRequiredNumber = 1,
        Number = 2
    };

    JsonSerializer.Serialize<DTO>(dto);
}

class DTO {
    [JsonIgnore]
    public required int Number { get; init; }
    public int NotRequiredNumber { get; init; }
}

Expected behavior

Should be able to have the JSON generated without the ignored property.

Actual behavior

JsonPropertyInfo 'Number' defined in type 'UserQuery+DTO' is marked required but does not specify a setter.

Regression?

No

Known Workarounds

No response

Configuration

Other information

No response

ghost commented 1 year ago

Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis See info in area-owners.md if you want to be subscribed.

Issue Details
### Description It's often necessary to use the `required` flag on a property that you don't want serialized via JSON. Adding the `JsonIgnore` attribute to such a property results in an `InvalidOperationException`. ### Reproduction Steps ```cs void Main() { var dto = new DTO { NotRequiredNumber = 1, Number = 2 }; JsonSerializer.Serialize(dto); } class DTO { [JsonIgnore] public required int Number { get; init; } public int NotRequiredNumber { get; init; } } ``` ### Expected behavior Should be able to have the JSON generated without the ignored property. ### Actual behavior JsonPropertyInfo 'Number' defined in type 'UserQuery+DTO' is marked required but does not specify a setter. ### Regression? No ### Known Workarounds _No response_ ### Configuration - .NET 7 - Windows 10 - x64 - Not specific to this configuration. ### Other information _No response_
Author: grosch-intl
Assignees: -
Labels: `area-System.Text.Json`, `untriaged`
Milestone: -
gregsdennis commented 1 year ago

To be clear, are you only asking for this to be supported during serialization and not during deserialization?

eiriktsarpalis commented 1 year ago

are you only asking for this to be supported during serialization and not during deserialization?

Annotating a property with [JsonIgnore] means ignoring in both serialization and deserialization. As such the JsonIgnore annotation violates the contract of the required keyword, so failing in that case is in my opinion by-design behavior.

What could be improved is the error message though, we should add better testing validating how the two features interact, specifically when the new ignore conditions are implemented in https://github.com/dotnet/runtime/issues/66490.

grosch-intl commented 1 year ago

To be clear, are you only asking for this to be supported during serialization and not during deserialization?

That's correct, just during deserialization.

gregsdennis commented 1 year ago

During deserialization (going from JSON to C#)? (Your code shows serialization.)

I think you have a DTO design flaw then. You're saying that you want to ignore Number in the JSON but that it's required to make a C# object. I think this is just an invalid arrangement.

Why do you want Number to be required when creating this DTO from C#, but you don't want it populated when receiving it in JSON? That seems inconsistent.

I would split the DTO into two models: a domain model for populating in C# with required and a true DTO which is used solely for JSON serialization. Then you'll just need a mapper to go between them.

grosch-intl commented 1 year ago

Sorry @gregsdennis, I meant serialization as you assumed. While you're technically 100% correct, I was hoping to not have to have another object. This is already a temporary object that's populated via AutoMapper and ProjectTo. While I don't want the caller to get the value, I do need it for the next query that's done in the same method.

I was hoping to avoid having to do yet another mapping to strip out that one property.

Think of something like this obviously non-compiling code. Need the internal ID for the next query but don't want to send it back.

var items = await _context.SomeTable.ProjectTo<SomeObj>(...).ToArrayAsync();
var ids = items.Select(x => x.InternalIdentifier).ToArrayAsync();
var otherStuff = await _context.SomeOtherTable
    .Where(x => ids.Contains(x.SomeTableIdentifier))
    .ToArrayAsync();

var mapping = otherStuff
    .ToLookup(x => x.InternalIdentifier)
    .ToDictionary(x => x.Key, x => x.Select(...));

foreach (var item in items) {
    item.Stuff = mapping[item.InternalIdentifier];
gregsdennis commented 1 year ago

Sure, do what makes sense for you.

But I agree that this is by-design behavior.

AArnott commented 1 year ago

I hit this. In my case, I have a custom deserializer, but wanted to utilize the JsonSerializer for serialization. That's why it made sense for [JsonIgnore] and required to appear on the same property. But it rejects serialization even though it could do that.