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

Support property ordering on POCOs #54748

Closed
steveharter opened this issue Jun 25, 2021 · 13 comments · Fixed by #55586
Closed

Support property ordering on POCOs #54748

steveharter opened this issue Jun 25, 2021 · 13 comments · Fixed by #55586
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Text.Json blocking Marks issues that we want to fast track in order to unblock other important work code-analyzer Marks an issue that suggests a Roslyn analyzer
Milestone

Comments

@steveharter
Copy link
Member

steveharter commented Jun 25, 2021

Background and Motivation

As part of making more extensible POCO converters and community asks, a low-risk feature for specifying ordering of properties\fields on a POCO for serialization is possible in the V6 timeframe. Other extensibility POCO features are no longer feasible for V6 since they need 1-2 preview releases to be properly vetted.

Today the serializer depends on reflection order for serializing object properties, and does base classes afterwards. Reflection is currently not deterministic and sometimes control over base class ordering is desired such as specifying base properties first.

The simplest way to implement this is through an attribute that specifies the order. However, this only works for owned POCOs. For V7+ once the JsonPropertyInfo is exposed publicly, that will allow for non-owned POCOs to also have this behavior.

Note that Newtonsoft has a similar approach with JsonPropertyAttribute.Order.

The source-generator should also leverage this attribute.

Proposed API

namespace System.Text.Json.Serialization
{
+  [AttributeUsage(AttributeTargets.Property | AttributeTargets.Field, AllowMultiple = false)]
+  public sealed class JsonPropertyOrderAttribute : JsonAttribute
+  {
+    public JsonPropertyNameAttribute(int order);
+    public int Order { get; }
+  }
}

Usage Examples

public class Person
{
    [JsonPropertyOrder(-1)] // Always appear first
    public int Id { get; set; }

    [JsonPropertyOrder(1)] // appear before LastName
    public string FirstName { get; set; }

    [JsonPropertyOrder(2)]
    public string LastName{ get; set; }

    public string City{ get; set; } // No order defined (has a order value of 0)
}

If there is a "tie" in the values, the reflection order is used. This is also consistent with Newtsonsoft.

Alternative Designs

It is possible to add a "strategy" pattern here, like we do for property naming. However, as mentioned in the introduction, in V7+ allowing write access to the JsonPropertyInfo.Order property will likely be the most natural mechanism once JsonPropertyInfo is exposed publicly. This would be coupled with a mechanism to retrieve all properties, allowing, for example, properties to be sorted by base class first and\or sorted alphabetically without using [JsonPropertyOrder].

@steveharter steveharter added api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Text.Json labels Jun 25, 2021
@steveharter steveharter added this to the 6.0.0 milestone Jun 25, 2021
@steveharter steveharter self-assigned this Jun 25, 2021
@ghost
Copy link

ghost commented Jun 25, 2021

Tagging subscribers to this area: @eiriktsarpalis, @layomia
See info in area-owners.md if you want to be subscribed.

Issue Details

Background and Motivation

As part of making more extensible POCO converters and community asks, a low-risk feature for specifying ordering of properties\fields on a POCO for serialization is possible in the V6 timeframe. Other extensibility POCO features are no longer feasible for V6 since they need 1-2 preview releases to be properly vetted.

Today the serializer depends on reflection order for properties, and does base classes afterwards. Reflection is currently not deterministic and control over base class ordering is desired.

The simplest way to implement this is through an attribute that specifies the order. However, this only works for owned POCOs. For V7+ once the JsonPropertyInfo is exposed publicly, that will allow for non-owned POCOs to also have this behavior.

Note that Newtonsoft has a similar approach with JsonPropertyAttribute.Order.

The source-generator should also leverage this attribute.

Proposed API

namespace System.Text.Json.Serialization
{
+  [AttributeUsage(AttributeTargets.Property | AttributeTargets.Field, AllowMultiple = false)]
+  public sealed class JsonPropertyOrderAttribute : JsonAttribute
+  {
+    public JsonPropertyNameAttribute(int order);
+    public int Order { get; }
+  }
}

Usage Examples

public class Person
{
    [JsonPropertyOrder(-1)] // Always appear first
    public int Id { get; set; }

    [JsonPropertyOrder(1)] // appear before LastName
    public string FirstName { get; set; }

    [JsonPropertyOrder(2)]
    public string LastName{ get; set; }

    public string City{ get; set; } // No order defined (has a order value of 0)
}

Alternative Designs

It is possible to add a "strategy" pattern here, like we do for property naming. However, as mentioned in the introduction, in V7+ allowing write access to the JsonPropertyInfo.Order property will likely be the most natural mechanism once JsonPropertyInfo is exposed publicly. This would be coupled with a mechanism to retreive all properties, allowing, for example, properties to be sorted alphabetically automatically without using [JsonPropertyOrder].

Author: steveharter
Assignees: steveharter
Labels:

api-suggestion, area-System.Text.Json

Milestone: 6.0.0

@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Jun 25, 2021
@steveharter steveharter added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed untriaged New issue has not been triaged by the area owner api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Jun 26, 2021
@bugproof
Copy link

I think if someone wants to serialize base properties first it shouldn't be that tiresome as adding [JsonPropertyOrder] attribute to every single property. I think users should be able to implement their own sorting strategies themselves or this behaviour should be a default one. E.g. to preserve the order it's possible to order by MetadataToken. Attributes are good but aren't very flexible. JsonSerializerOptions could have some kind of sorting strategy that would be used here

foreach (PropertyInfo propertyInfo in currentType.GetProperties(bindingFlags))

@eiriktsarpalis
Copy link
Member

[JsonPropertyOrder(-1)] // Always appear first

Trying to understand the comment: Is -1 a special value that makes the field appear first or is it using the natural ordering of integers? (i.e. a member with [JsonPropertyOrder(-2)] would always appear before the above)

@layomia layomia self-assigned this Jun 28, 2021
@layomia layomia added the blocking Marks issues that we want to fast track in order to unblock other important work label Jun 28, 2021
@zcsizmadia
Copy link

zcsizmadia commented Jun 28, 2021

in 728 I suggested to use a similar property ordering what Newtonsoft.Json is using, where -1 is the default value for the order if attribute is not explicitly defined. I think what @steveharter is proposing a +1 what Newtonsoft.json is doing, so 0 is the default (instead of -1), -1 is to force to be first (instead of -2).

It might make sense to define a JsonPropertyFirst() attribute , instead of using a "weird" looking negative number

@layomia
Copy link
Contributor

layomia commented Jun 29, 2021

Following offline discussion between @eiriktsarpalis and I, the proposal is to place no limitation on the integer values passed into the attribute ctor. If there are duplicates/ties, they'd be resolved by the serializer falling back to the reflection order (which would write props on derived classes first, before base). So props with order -3 would be written before -1, etc.

@layomia
Copy link
Contributor

layomia commented Jun 29, 2021

I think users should be able to implement their own sorting strategies themselves or this behaviour should be a default one. E.g. to preserve the order it's possible to order by MetadataToken. Attributes are good but aren't very flexible.

We eventually want to allow this sort of imperative implementation, where STJ users can access the list of serializable properties and sort them themselves, using whatever sorting scheme they wish. This work isn't in scope for .NET 6.0, but is generally tracked by #34456.

@zcsizmadia
Copy link

zcsizmadia commented Jun 29, 2021

@layomia What do you think about a class Attribute to sort the props by name without individually setting the order value for each of them? props name sorting in this case should use the JsonPropertyNameAttribute for a prop if set.

namespace System.Text.Json.Serialization
{
    /// <summary>
    /// Specifies the property order in JSON when serializing.
    /// </summary>
    [AttributeUsage(AttributeTargets.Class | AttributeTargets.Struct, AllowMultiple = false)]
    public sealed class JsonPropertyOrderByNameAttribute : JsonAttribute
    {
        /// <summary>
        /// Initializes a new instance of <see cref="JsonPropertyOrderByNameAttribute"/> with the specified property order.
        /// </summary>
        /// <param name="stringComparison">Property name comparison type.</param>
        public JsonPropertyOrderByNameAttribute(StringComparison stringComparison)
        {
            StringComparison = stringComparison;
        }

        /// <summary>
        /// Initializes a new instance of <see cref="JsonPropertyOrderByNameAttribute"/> with the specified property order.
        /// </summary>
        public JsonPropertyOrderByNameAttribute()
            : this(StringComparison.Ordinal)
        {
        }

        /// <summary>
        /// Property name string comparison type.
        /// </summary>
        public StringComparison StringComparison { get; }
    }
}

@terrajobst terrajobst added api-approved API was approved in API review, it can be implemented code-analyzer Marks an issue that suggests a Roslyn analyzer and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Jun 29, 2021
@terrajobst
Copy link
Member

  • It seems the default value shouldn't be zero, but a high enough number (e.g. int.MaxValue / 2) so that people can force fields to be at the beginning or the end without having to apply the attribute to all properties
  • We should consider having an analyzer that checks that no duplicates exist
    • However, properties/fields without the attribute should be ignored
  • The sort order should be across the hierarchy, e.g. a derived type should be able to inject a member between members coming from the base
namespace System.Text.Json.Serialization
{
    [AttributeUsage(AttributeTargets.Property | AttributeTargets.Field, AllowMultiple = false)]
    public sealed class JsonPropertyOrderAttribute : JsonAttribute
    {
        public JsonPropertyNameAttribute(int order);
        public int Order { get; }
    }
}

@svick
Copy link
Contributor

svick commented Jun 29, 2021

@terrajobst

It seems the default value shouldn't be zero, but a high enough number (e.g. int.MaxValue / 2) so that people can force fields to be at the beginning or the end without having to apply the attribute to all properties

I'm not sure I understand. Are you saying that the example from the original post will be written like this?

public class Person
{
    [JsonPropertyOrder(0)] // Always appear first
    public int Id { get; set; }

    [JsonPropertyOrder(int.MaxValue - 1)] // appear before LastName
    public string FirstName { get; set; }

    [JsonPropertyOrder(int.MaxValue)]
    public string LastName{ get; set; }

    public string City { get; set; } // No order defined
}

To me, using -1, 1 and 2 (as originally proposed) is much clearer and it also doesn't require applying the attribute to all properties.

@terrajobst
Copy link
Member

terrajobst commented Jun 29, 2021

The idea is: people generally don't care about the order of all the properties. For the subset they care, they might want to push them all the way to the top or all the way way to the end. Using 0 as the default would mean that organizing the sort order of all fields at the beginning means the user has to use negative numbers, which isn't very intuitive. If we use int.MaxValue as the default, then pushing values to the end isn't very intuitive because the have to walk back from int.MaxValue, which is akin to using negative numbers at the beginning.

By using a number that is neither 0 nor int.MaxValue but "high enough" the user can always assign values in ascending order:

public class AtTheBeginning
{
    [JsonPropertyOrder(0)] 
    public int Id { get; set; }

    [JsonPropertyOrder(1)]
    public string FirstName { get; set; }

    [JsonPropertyOrder(2)]
    public string LastName{ get; set; }

    // A bunch of properties which order I don't care for
}

public class AtTheEnd
{
    // A bunch of properties which are sort of relevant, but I'm OK
    // with alphabetic search

    // Keep those two last:

    [JsonPropertyOrder(ushort.MaxValue + 1)]
    public string[] CustomKeys { get; set; }

    [JsonPropertyOrder(ushort.MaxValue + 2)]
    public string[] CustomValue { get; set; }
}

Keeping things last is a bit of a fringe scenario, so having to know the magic default value seems fine.

@svick
Copy link
Contributor

svick commented Jun 29, 2021

@terrajobst I think I understand now. With default 0, if you want to specify 1st, 2nd and 3rd item from the start, you would write them as -3, -2, -1 and that indeed isn't very intuitive, especially if that is the most common scenario.

The magic default value could be fixed by introducing a separate constant for it, but that's probably not worth it.

@ladeak
Copy link
Contributor

ladeak commented Jun 30, 2021

Here is a suggestion I have not heard being discussed during API review.

  • 0 is default
  • positive numbers give an ordering at the beginning
  • negative numbers give an order viewed from the end
public class SomeClass
{
    [JsonPropertyOrder(1)]
    public string FirstName { get; set; } // Appears first

    [JsonPropertyOrder(2)]
    public string LastName{ get; set; } // Appears after first

    // A bunch of properties which order we don't care for (default values)

    [JsonPropertyOrder(0)] 
    public int Id { get; set; } // Same as default or not having the attribute

    [JsonPropertyOrder(-2)]
    public string Address{ get; set; } // Appears before the last one

    [JsonPropertyOrder(-1)]
    public string City{ get; set; } // Appears as the last one
}

If a type has 5 properties and someone uses values 3 and -3 is a question, but should be able to settle in different ways.

@steveharter
Copy link
Member Author

For the comments on the ordering, the intent was to allow all integer values (including negative) and sort the properties by order (e.g. from Int32.MinValue to Int32.MaxValue). This is also what Newtonsoft does. It allows ordering of "before" and "after" the default value of 0.

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jul 13, 2021
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jul 13, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Aug 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Text.Json blocking Marks issues that we want to fast track in order to unblock other important work code-analyzer Marks an issue that suggests a Roslyn analyzer
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants