Change C# Equals() overload to allow reference type properties to be properly compared

[Louis-PhilippeGentile]

Why do we need this improvement?

We need this improvement because the current Equals() overload is functionally pointless. It only works if the properties of a given model are all value types or if the model and the object it is being compared with reference the same object in memory.

With the current behavior, the moment a JSON object is deserialized and typecasted to its Modelina model, it will always fail its comparison with an identical object if it contains a reference type which does not overload the "==" operator (which includes other models generated by Modelina).

How will this change help?

This change would allow the Equals() function to be applicable in more situations.

This does not fix Equals() completely, as both the current implementation and this implementation proposed below would still fail to return the proper result in the case of Enumerable properties such as Array or List.

How could it be implemented/designed?

By instead using the Equals(Object, Object) function, we can:

• Implicitly perform a ReferenceEquals() on the compared properties.
• Return true if both properties are null (which may be the case if the properties are nullable).
• Fallback on the Equals() function of the compared properties.

🚧 Breaking changes

Yes

👀 Have you checked for similar open issues?

I checked and didn't find a similar issue.

🏢 Have you read the Contributing Guidelines?

I have read the Contributing Guidelines

Are you willing to work on this issue?

Yes, I can work on this issue.

[github-actions[bot]]

Welcome to AsyncAPI. Thanks a lot for reporting your first issue. Please check out our contributors guide and the instructions about a basic recommended setup useful for opening a pull request.
Keep in mind there are also other channels you can use to interact with AsyncAPI community. For more details check out this issue.

[jonaslagoni]

@Louis-PhilippeGentile thanks for the issue! Do you mind giving an example model that is currently being generated, and then how you would like it to be implemented instead?

[jonaslagoni]

cc @RowlandBanks @JFCote (since you both recently made changes to the C# generator)

[Louis-PhilippeGentile]

@Louis-PhilippeGentile thanks for the issue! Do you mind giving an example model that is currently being generated, and then how you would like it to be implemented instead?

Taken straight from the test snapshots, here is the current Equals() function being generated:

public override bool Equals(object obj)
  {
    if(obj is Test model)
    {
      if(ReferenceEquals(this, model)) { return true; }
      return StringProp == model.StringProp && 
      NumberProp == model.NumberProp && 
      ObjectProp == model.ObjectProp && 
      AdditionalProperties == model.AdditionalProperties;
    }

    return false;
  }

Here is the generated Equals() function after my proposed changes:

public override bool Equals(object obj)
  {
    if(obj is Test model)
    {
      if(ReferenceEquals(this, model)) { return true; }
      return Object.Equals(StringProp, model.StringProp) && 
      Object.Equals(NumberProp, model.NumberProp) && 
      Object.Equals(ObjectProp, model.ObjectProp) && 
      Object.Equals(AdditionalProperties, model.AdditionalProperties);
    }

    return false;
  }

[RowlandBanks]

One issue you will face is that Equals on a dictionary (such as AdditionalProperties) is basically just a reference equality check, so:

var a = new Dictionary<string, dynamic>();
var b = new Dictionary<string, dynamic>();

Console.WriteLine(a.Equals(b));

will print false, so your proposed change will generally evaluate to false as well.

You could extend the code to start doing a deep comparison but that is quite a hard thing to do (with various edge-cases like circular references). Or, you could confirm the dictionary length is the same, then loop through the keys on the dictionary and call .Equals on each value in turn - trusting that the value implements equality correctly.

However, I think that the value cannot be trusted. For example, the System.Text.Json serializer will turn dynamic properties into JsonElement types, and those do not guarantee equality afaik. E.g. the following prints false:

var json = """
{
  "name": "Bob"
}
""";
var a = JsonObject.Parse(json);
var b = JsonObject.Parse(json);
Console.WriteLine(a.Equals(b));

My gut feeling is that implementing equality tests for generated models is a hard problem to solve and that it might be better to leave the implementation up to the programmer (who can implement a partial class to override ==/Equals, use a library like CompareNETObjects, or something else that makes sense to them).

[JFCote]

My gut feeling is that implementing equality tests for generated models is a hard problem to solve and that it might be better to leave the implementation up to the programmer (who can implement a partial class to override ==/Equals, use a library like CompareNETObjects, or something else that makes sense to them).

I agree with @RowlandBanks. Since I'm coming from OpenAPI Generator project, I would be curious to see what they chose to do in the C# client / Aspdotnet server generator. Most IDE are able to generate Equals/hashCode method using minimal input from the user so I think it we could base our logic on what IDEs do and iterate on this.

I don't have time at the moment to check but would gladly review PRs if required.

[Louis-PhilippeGentile]

One issue you will face is that Equals on a dictionary (such as AdditionalProperties) is basically just a reference equality check, so:

var a = new Dictionary<string, dynamic>();
var b = new Dictionary<string, dynamic>();

Console.WriteLine(a.Equals(b));

will print false, so your proposed change will generally evaluate to false as well.

You could extend the code to start doing a deep comparison but that is quite a hard thing to do (with various edge-cases like circular references). Or, you could confirm the dictionary length is the same, then loop through the keys on the dictionary and call .Equals on each value in turn - trusting that the value implements equality correctly.

However, I think that the value cannot be trusted. For example, the System.Text.Json serializer will turn dynamic properties into JsonElement types, and those do not guarantee equality afaik. E.g. the following prints false:

var json = """
{
  "name": "Bob"
}
""";
var a = JsonObject.Parse(json);
var b = JsonObject.Parse(json);
Console.WriteLine(a.Equals(b));

My gut feeling is that implementing equality tests for generated models is a hard problem to solve and that it might be better to leave the implementation up to the programmer (who can implement a partial class to override ==/Equals, use a library like CompareNETObjects, or something else that makes sense to them).

I agree with you that my proposed changes would result in AdditionalProperties generally returning false, and other Collections. However, this was also the behavior of the current implementation which uses "==", therefore always performing a reference equality check when interacting with reference type variables.

I don't intend to address that particular case or any case involving objects implementing ICollection, since I am of the opinion that the definition of equality for a collection (be it a dictionary or otherwise) depends too much on its implementation for us to make an educated guess to resolve equality properly.

My proposal was to address the specific case between the interaction of two generated models.

Suppose, for example that you have two generated models, lets call them Schema A and Schema B. As part of its definition, Schema A has a property which is a reference to Schema B (which I will call property B from now on), and all remaining properties of schema A and B are of basic types implementing .Equals() such as String, int, bool, etc. (No dictionary or other collections).

With the current implementation, when .Equals() is called on a Schema A object, property B would be resolved using "==", returning false in all cases where property B refers to a different object in memory, even if its underlying properties are all equal to the compared property's and should therefore be interpreted as being equal. However, with my implementation, resolving property B would be delegated to calling property B's .Equal() function, which would resolve properly, even if the two initial objects do not reference the same object in memory.

In short, my proposal is intended as an improvement over the current .Equals() override, fixing a particular use case that I feel would be somewhat common, but I do not pretend that this will fix the inherent issues with checking for equality in generated models.

[RowlandBanks]

Ah, apologies. I understand, and agree this would be a step in the right direction 🙂.

[jonaslagoni]

Sounds good 👍 Just document the limitations in the docs and all good from my side 👌

[Louis-PhilippeGentile]

Sounds good 👍 Just document the limitations in the docs and all good from my side 👌

Would that be under modelina/docs/languages/Csharp.md?

[jonaslagoni]

Yes, more specifically I would document it under here: https://github.com/asyncapi/modelina/blob/master/docs/languages/Csharp.md#generate-models-with-equals-and-gethashcode-methods

[asyncapi-bot]

:tada: This issue has been resolved in version 3.4.1 :tada:

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot :package::rocket:

[asyncapi-bot]

:tada: This issue has been resolved in version 4.0.0-next.14 :tada:

The release is available on:

• npm package (@next dist-tag)
• GitHub release

Your semantic-release bot :package::rocket: