Skip to content

Respect JsonSerializerOptions casing for property names in validation errors #62036

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

Open
wants to merge 21 commits into
base: main
Choose a base branch
from

Conversation

Copilot
Copy link
Contributor

@Copilot Copilot AI commented May 20, 2025

Summary

When configuring JSON serialization options with a custom property naming policy (like camelCase), validation error responses were not consistently following the same naming policy for property names. This created a disconnect between what clients send/expect and what the validation errors contained.

The implemented changes ensure validation errors respect the configured JSON naming policy, making them consistent with the rest of the API's JSON formatting.

Changes

  • Added a SerializerOptions property to ValidateContext to access JSON naming policies
  • Modified the ValidationEndpointFilterFactory to pass the JSON options from DI to the validation context
  • Implemented property formatting that respects PropertyNamingPolicy for error key names
  • Added comprehensive tests for different naming policies, nested properties, and array indices
  • Properly handles complex property paths with dots and array indices (items[0].productNameitems[0].productName)

Before

{
  "type": "https://tools.ietf.org/html/rfc9110#section-15.5.41",
  "title": "One or more validation errors occurred.",
  "status": 400,
  "errors": {
    "LastName": [
      "The LastName field is required."
    ]
  }
}

After

{
  "type": "https://tools.ietf.org/html/rfc9110#section-15.5.41",
  "title": "One or more validation errors occurred.",
  "status": 400,
  "errors": {
    "lastName": [
      "The LastName field is required."
    ]
  }
}

The implementation preserves the full compatibility with existing usage while ensuring that property names in validation errors now follow the configured naming policy.

Fixes #61764.

Warning

Firewall rules blocked me from connecting to one or more addresses

I tried to connect to the following addresses, but was blocked by firewall rules:

  • cdn.fwupd.org
    • Triggering command: /usr/bin/fwupdmgr refresh (dns block)

If you need me to access, download, or install something from one of these locations, you can either:


💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

@Copilot Copilot AI changed the title [WIP] Respect JsonSerializerOptions casing for property names in validation errors Respect JsonSerializerOptions casing for property names in validation errors May 20, 2025
@Copilot Copilot AI requested a review from captainsafia May 20, 2025 23:49
Copilot finished work on behalf of captainsafia May 20, 2025 23:49
Copy link
Member

@captainsafia captainsafia left a comment

Choose a reason for hiding this comment

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

Good start! Left some comments inline.

Copy link
Contributor Author

Copilot AI commented May 21, 2025

Good start! Left some comments inline.

I've addressed all the inline comments. Made the SerializerOptions property internal, removed the test file, and updated ValidationEndpointFilterFactory to use the internal property via reflection. All changes in commit 216406a.

Copilot finished work on behalf of captainsafia May 21, 2025 00:57
Copy link
Contributor Author

Copilot AI commented May 21, 2025

@captainsafia Unfortunately I hit an unexpected error while processing your comment. I've automatically reported this to GitHub.

You can ask me to try again later by mentioning me in a new comment.

If you want to contact GitHub about this error, please mention the following identifier so they can better serve you: 590bfda1-76e1-4d10-a5c3-2f6bb862c94c

Sorry for the inconvenience!

Copilot finished work on behalf of captainsafia May 21, 2025 01:27
@captainsafia
Copy link
Member

@copilot Let's try to address the review comments I left for you earlier. You were on the right track with making the SerializerOptions property public and setting it in the ValidationEndpointFilterFactory based on the way our assembly references are structured.

Copilot finished work on behalf of captainsafia May 21, 2025 15:47
Copy link
Member

@captainsafia captainsafia left a comment

Choose a reason for hiding this comment

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

We're almost there! One more comment. Also, be undo all the changes you made to the package.json files and remove the package.json.bak files.

@captainsafia
Copy link
Member

@copilot Revert any changes made to package.json and package-lock.json files to their state in main.

Copilot finished work on behalf of captainsafia May 21, 2025 16:21
@captainsafia
Copy link
Member

@copilot I've made some fixes to the implementation and want you to follow up on some things, including:

  • HasDisplayAttribute is called each type a type is validated. Can you update the implementation to call it once and cache it on the ValidatablePropertyInfo?
  • The performance of the FormatComplexKey method can probably be improved. Can we cache the JSON naming policy on the ValidateContext? Are there other improvements that we can make here?

Copy link
Contributor Author

Copilot AI commented May 29, 2025

@copilot I've made some fixes to the implementation and want you to follow up on some things, including:

  • HasDisplayAttribute is called each type a type is validated. Can you update the implemen...

I've implemented both performance improvements in commit 409bb34:

  1. HasDisplayAttribute caching: Added _hasDisplayAttribute field to cache reflection result in constructor, eliminating repeated calls during validation
  2. FormatComplexKey optimization: Cached the JSON naming policy in ValidateContext and made the method static with the naming policy as a parameter, reducing property access overhead and redundant null checks

Warning

Firewall rules blocked me from connecting to one or more addresses

I tried to connect to the following addresses, but was blocked by firewall rules:

If you need me to access, download, or install something from one of these locations, you can either:

Copilot finished work on behalf of captainsafia May 29, 2025 04:20
Copy link
Contributor Author

Copilot AI commented May 29, 2025

Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details.

Warning

Firewall rules blocked me from connecting to one or more addresses

I tried to connect to the following addresses, but was blocked by firewall rules:

If you need me to access, download, or install something from one of these locations, you can either:

Copilot finished work on behalf of captainsafia May 29, 2025 05:34
@captainsafia captainsafia force-pushed the copilot/fix-61764-2 branch from 4afa2bb to f035b35 Compare May 30, 2025 04:30
@captainsafia captainsafia marked this pull request as ready for review May 30, 2025 15:53
@captainsafia captainsafia requested a review from DeagleGross May 30, 2025 15:54
@captainsafia
Copy link
Member

@DeagleGross @BrennanConroy Would love your review n this PR. Outside of the prompting, I ended up making local changes on 59069f7 (#62036) and f035b35 (#62036).

Some things here:

  • We need to add JsonSerializerOptions as a property on the ValidateContext since we don't have access to the JsonOptions type in the Http.Abstractions assembly in order to be able to resolve it ourselves in the ValidatableTypes. I think that probably makes the most sense for frameworks who are building on top of the abstractions API anyways.
  • When I asked Copilot to add support for formatting the keys in the validation result, it used the strategy that I removed in this commit. I played around with why it chose this instead of relying on the fact that we format the names in the ValidatableTypes. The only impacted scenario was the IValidatableObject one so I created a specific one for tat.
  • The precedence ordering is [Display(Name = "")], then [JsonPropertyName] then the JSON naming policy.

Copy link
Contributor

@mikekistler mikekistler left a comment

Choose a reason for hiding this comment

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

This looks great! I can't say I understand how this works but just looking at the changes to the expected results of the tests, this is a very welcome change.

Copy link
Member

@halter73 halter73 left a comment

Choose a reason for hiding this comment

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

  • We need to add JsonSerializerOptions as a property on the ValidateContext since we don't have access to the JsonOptions type in the Http.Abstractions assembly in order to be able to resolve it ourselves in the ValidatableTypes. I think that probably makes the most sense for frameworks who are building on top of the abstractions API anyways.

Would this be better on ValidationOptions? We could use IPostConfigureOptions to set it once.

We should also create an API proposal issue for it. I bet copilot would do well at taking our issue template and this PR as inputs and creating an issue.

  • The precedence ordering is [Display(Name = "")], then [JsonPropertyName] then the JSON naming policy.

Does [Display(Name = "")] affect how the type gets serialized? And if not, shouldn't the validation errors match what gets serialized?

Also, do we have a test that verifies what happens when you have both [Display(Name = "")] and [JsonPropertyName] on the same property?

@@ -106,9 +106,17 @@ public virtual async Task ValidateAsync(object? value, ValidateContext context,
// Create a validation error for each member name that is provided
foreach (var memberName in validationResult.MemberNames)
{
// Format the member name using JsonSerializerOptions naming policy if available
// Note: we don't respect [JsonPropertyName] here because we have no context of the property being validated.
Copy link
Member

@halter73 halter73 Jun 5, 2025

Choose a reason for hiding this comment

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

Is this true? It's not like we're asking for the one and only [JsonPropertyName] used for the type represented by the ValidatableTypeInfo here. That would be impossible. Instead, we're asking for the [JsonPropertyName] for each of its member properties, right? If we didn't have any context of the property being validated here, how are we getting the name of the property at all let alone the proper casing?

I think we should probably respect all the same attributes we do in ValidatablePropertyInfo, namely [Display] (if we keep doing that) and [JsonPropertyName].

Copy link
Member

@halter73 halter73 Jun 5, 2025

Choose a reason for hiding this comment

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

Is this just redundant? Could it be that memberName already formatted here? I haven't run it, so it might not be the case, but we should definitely add some tests with a camel cased PropertyNamingPolicy and a capitalized first letter in the [JsonPropertyName] attribute.

public string? LastName { get; set; }
}

[Fact]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
[Fact]
[Fact]


// Cache the HasDisplayAttribute result to avoid repeated reflection calls
var property = DeclaringType.GetProperty(Name);
_hasDisplayAttribute = property is not null && HasDisplayAttribute(property);
Copy link
Member

Choose a reason for hiding this comment

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

Feels weird to grab the attribute, but then never actually use the value from the attribute. At that point, why not just use string.IsNullOrEmpty(DisplayName)?

Copy link
Member

Choose a reason for hiding this comment

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

Good catch. I find it sometimes helps to ask copilot not to add any fields if you don't think it should be necessary.

/// When available, property names in validation errors will be formatted according to the
/// PropertyNamingPolicy and JsonPropertyName attributes.
/// </summary>
public JsonSerializerOptions? SerializerOptions { get; set; }
Copy link
Member

Choose a reason for hiding this comment

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

Putting it on the record that I'm not a fan of adding this to the ValidateContext. Especially if we're wanting to eventually move this API into dotnet/runtime.

Copy link
Member

Choose a reason for hiding this comment

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

What do you think about moving it to ValidationOptions as I suggest here?

Copilot finished work on behalf of BrennanConroy June 5, 2025 18:57

/// <summary>
/// Creates a new instance of <see cref="ValidatablePropertyInfo"/>.
/// </summary>
protected ValidatablePropertyInfo(
[param: DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicProperties)]
[param: DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicProperties | DynamicallyAccessedMemberTypes.PublicConstructors)]
Copy link
Member

Choose a reason for hiding this comment

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

Why is this necessary?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Respect JsonSerializerOptions casing for property names in validation errors
5 participants