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

Extend JsonSourceGenerationOptionsAttribute to have feature parity with JsonSerializerOptions. #88753

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,58 @@ namespace System.Text.Json.Serialization
#endif
sealed class JsonSourceGenerationOptionsAttribute : JsonAttribute
eiriktsarpalis marked this conversation as resolved.
Show resolved Hide resolved
{
/// <summary>
/// Constructs a new <see cref="JsonSourceGenerationOptionsAttribute"/> instance.
/// </summary>
public JsonSourceGenerationOptionsAttribute() { }

/// <summary>
/// Constructs a new <see cref="JsonSourceGenerationOptionsAttribute"/> instance with a predefined set of options determined by the specified <see cref="JsonSerializerDefaults"/>.
/// </summary>
/// <param name="defaults">The <see cref="JsonSerializerDefaults"/> to reason about.</param>
/// <exception cref="ArgumentOutOfRangeException">Invalid <paramref name="defaults"/> parameter.</exception>
public JsonSourceGenerationOptionsAttribute(JsonSerializerDefaults defaults)
stephentoub marked this conversation as resolved.
Show resolved Hide resolved
{
// Constructor kept in sync with equivalent overload in JsonSerializerOptions

if (defaults is JsonSerializerDefaults.Web)
{
PropertyNameCaseInsensitive = true;
PropertyNamingPolicy = JsonKnownNamingPolicy.CamelCase;
NumberHandling = JsonNumberHandling.AllowReadingFromString;
}
else if (defaults is not JsonSerializerDefaults.General)
{
throw new ArgumentOutOfRangeException(nameof(defaults));
}
}

/// <summary>
/// Defines whether an extra comma at the end of a list of JSON values in an object or array
/// is allowed (and ignored) within the JSON payload being deserialized.
/// </summary>
public bool AllowTrailingCommas { get; set; }

/// <summary>
/// Specifies a list of custom converter types to be used.
/// </summary>
public Type[]? Converters { get; set; }

/// <summary>
/// The default buffer size in bytes used when creating temporary buffers.
Copy link
Member

Choose a reason for hiding this comment

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

These comments on properties inconsistently start or don't start with "Gets or sets..."

Copy link
Member Author

Choose a reason for hiding this comment

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

I mostly just copied and pasted the wording in the corresponding JsonSerializerOptions properties. I can submit a follow-up PR that improves consistency between properties in this attribute.

/// </summary>
public int DefaultBufferSize { get; set; }
Comment on lines +55 to +58
Copy link
Member

Choose a reason for hiding this comment

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

By default this is going to be 0. Doesn't that differ from JsonSerializerOptions.DefaultBufferSize, which defaults to a positive value and throws an exception if it's set to 0?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because the attribute is only being read by Roslyn, the value of the property is only being consulted if explicitly set by the user in the attribute declaration. I briefly considered making this and other properties nullable, however that would be a departure from convention already used in properties shipped in this attribute, so went with consistency instead. The other alternative would be to explicitly set the properties to equal the JsonSerializerOptions default, however that would necessitate a bit of duplication or refactoring without providing any clear benefit.

Copy link
Member

Choose a reason for hiding this comment

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

Then shouldn't the XML comment call out what a value of 0 means, as is done below for MaxDepth?

Copy link
Member Author

Choose a reason for hiding this comment

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

I would probably opt for removing that reference in MaxDepth, making all of them deliberately ambiguous and just have them point to the corresponding JsonSerializerOptions properties.

Copy link
Member

Choose a reason for hiding this comment

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

We need to call out somewhere in the docs what it means to not provide a value for these.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll follow up with a PR once we conclude P7 work.


/// <summary>
/// Specifies the default ignore condition.
/// </summary>
public JsonIgnoreCondition DefaultIgnoreCondition { get; set; }

/// <summary>
/// Specifies the policy used to convert a dictionary key to another format, such as camel-casing.
/// </summary>
public JsonKnownNamingPolicy DictionaryKeyPolicy { get; set; }

/// <summary>
/// Specifies whether to ignore read-only fields.
/// </summary>
Expand All @@ -35,11 +82,47 @@ sealed class JsonSourceGenerationOptionsAttribute : JsonAttribute
/// </summary>
public bool IncludeFields { get; set; }

/// <summary>
/// Gets or sets the maximum depth allowed when serializing or deserializing JSON, with the default (i.e. 0) indicating a max depth of 64.
/// </summary>
public int MaxDepth { get; set; }

/// <summary>
/// Specifies how number types should be handled when serializing or deserializing.
/// </summary>
public JsonNumberHandling NumberHandling { get; set; }

/// <summary>
/// Specifies preferred object creation handling for properties when deserializing JSON.
/// </summary>
public JsonObjectCreationHandling PreferredObjectCreationHandling { get; set; }

/// <summary>
/// Determines whether a property name uses a case-insensitive comparison during deserialization.
/// </summary>
public bool PropertyNameCaseInsensitive { get; set; }

/// <summary>
/// Specifies a built-in naming polices to convert JSON property names with.
/// </summary>
public JsonKnownNamingPolicy PropertyNamingPolicy { get; set; }

/// <summary>
/// Defines how JSON comments are handled during deserialization.
/// </summary>
public JsonCommentHandling ReadCommentHandling { get; set; }

/// <summary>
/// Defines how deserializing a type declared as an <see cref="object"/> is handled during deserialization.
/// </summary>
public JsonUnknownTypeHandling UnknownTypeHandling { get; set; }

/// <summary>
/// Determines how <see cref="JsonSerializer"/> handles JSON properties that
/// cannot be mapped to a specific .NET member when deserializing object types.
/// </summary>
public JsonUnmappedMemberHandling UnmappedMemberHandling { get; set; }

/// <summary>
/// Specifies whether JSON output should be pretty-printed.
/// </summary>
Expand Down
Loading