[REQ][CSHARP][GENERICHOST] Support equal by value

[alec-petersen]

Fixed #21646

There was a dependency on readOnlyVars where you had to have some readOnlyVars in order for your model to inherit IEquatable. I removed that dependency and used vars in order to create the .Equals and .GetHashCode method.

PR checklist

• Read the contribution guidelines.
• Pull Request title clearly describes the work in the pull request and Pull Request description provides details about how to validate the work. Missing information here may result in delayed response from the community.
• Run the following to build the project and update samples:

  ./mvnw clean package || exit
  ./bin/generate-samples.sh ./bin/configs/*.yaml || exit
  ./bin/utils/export_docs_generators.sh || exit
  

  (For Windows users, please run the script in WSL)
  Commit all changed files.
  This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master.
  These must match the expectations made by your contribution.
  You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example ./bin/generate-samples.sh bin/configs/java*.
  IMPORTANT: Do NOT purge/delete any folders/files (e.g. tests) when regenerating the samples as manually written tests may be removed.
• File the PR against the correct branch: master (upcoming 7.x.0 minor release - breaking changes with fallbacks), 8.0.x (breaking changes without fallbacks)
• If your PR solves a reported issue, reference it using GitHub's linking syntax (e.g., having "fixes #123" present in the PR description)
• If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.

[alec-petersen]

@mandrean @shibayan @Blackclaws @lucamazzanti @iBicha

[devhl-labs]

I don't think this is necessary. Only readonly properties should be considered. See here https://learn.microsoft.com/en-us/dotnet/standard/design-guidelines/equality-operators#equality-operators-on-reference-types

[alec-petersen]

@devhl-labs I'll admit I don't fully understand the logic in the article you linked (I should say that I still find it useful to have these equality operators), but I think to maintain consistency with the previous default RestSharp generator, it may be good to at least provide an option for this. Thoughts?

[alec-petersen]

May be an OpenAPI best practices misunderstanding but if I have an object that is both a request for a PUT or POST endpoint and a response to a GET endpoint (because its the same properties both ways), I can't mark those properties as writeOnly and readOnly, so I'd need to add a redundant object. (I actually didn't know readOnly and writeOnly were options until this PR)

[alec-petersen]

Its particularly useful for testing, where you are using Moq to verify a request/response was provided correctly. Otherwise, we'd need to go in and compare each property to the expected request/response, which is a lot haha

[devhl-labs]

I don't believe RestSharp receives a lot of maintenance these days, so staying in parity with it is not a high priority for me. Also, I don't think any library should be overriding Equals or GetHashCode, especially by default as it used to. Nothing in the api spec tells us how to compare objects. For example, if you have an employee class, is the comparison done by name, employee id, or social security number? For this reason I'd really like to just remove the feature entirely, but its not something I'm pushing for.

(experts, feel free to correct/clarify me here, but please go easy!) The problem with the overriding Equals and GetHashCode with properties that can change is that if you add an instance of a class to a dictionary as a key, then update one of those settable properties, then retrieve from the dictionary using the same class instance, you wont get anything back because your class changed. Essentially your dictionary is corrupted.

My recommendation to solve your problem is to disable the equatable option and write your own Equatable and GetHashCode methods. The classes are partial so it might be easy to do, depending on what you do with your generated library. There may be other options too, if there is demand for better way to compare objects I think we can come up with something.

[alec-petersen]

@devhl-labs I definitely see your point. Thoughts on a new generated method like ValueEquals or similar?

[devhl-labs]

I dont think that helps because you probably want to implement the interface. Speaking of which, writing your own Equals and GetHashCode wouldnt work either. What problem are you trying to solve? Why do you want settable properties in these methods?

[alec-petersen]

We'll reconsider our use case and see if a generator solution makes sense or if we need to refactor our code. I think readOnly may make sense for us. Thanks for the discussion!

[alec-petersen]

Alrighty, so I can concisely state our problem as this.

We have a schema like this (using a simplified example):

SleepSetting:
  type: object
  properties:
    value:
      type: string
  required:
    - value

We often use schemas like this as both the request to a POST to update the setting and as the response in a POST/GET to retrieve or show the updated setting.

Now if we set the property value as readOnly: true, that means we can't construct the request object with our value. The constructor becomes internal and there is no setter.

To my knowledge, the only solution then is to make two schemas, one with readOnly and one writeOnly (or just blank).

The reason we want the .Equals method is primarily so in a test we can do something like this:

_mockConfigApi.Verify(c => c.SetSleepSetting(expectedRequest, It.IsAny<CancellationToken>()), Times.Once);

// or with an alternative method like .ValueEquals

_mockConfigApi.Verify(c => c.SetSleepSetting(It.Is<SleepSetting>(s => s.ValueEquals(expectedRequest)), It.IsAny<CancellationToken>(), Times.Once)

instead of:

_mockConfigApi.Verify(c => c.SetSleepSetting(It.Is<SleepSetting>(s => s.Value == expectedRequest.Value), It.IsAny<CancellationToken>(), Times.Once)

which becomes more of an ugly pain the more properties stack up. So implementing IEquatable<> isn't strictly necessary for our usecase, just a nicety. We may also use this outside of testing, this is just the main usecase.

And this may come down to poor practice on our part, curious to hear thoughts.