Create valid property identifiers when generating OData client

[gregwinterstein]

Modify the code generator to generate valid identifiers

This change resolves the following issues:

• 317

• 297 & #331

• 242

• 224

[gregwinterstein]

@microsoft-github-policy-service agree

[mvarie]

When using the ODataConnectedService extension in Visual Studio 2022 to generate C# classes from an unpatched Dynamics 365 10.0.29 OData ($metadata) url, @gregwinterstein 's fix has generated code that compiles correctly.

Note: by unpatched I mean that our Dynamics 365 has no hotfix for LCS 753226, nor it can be applied - if you have 10.0.30 or 10.0.31 you can apply it, while 10.0.32+ is supposed to incorporate the fix.

[SomeTroglodyte]

@gregwinterstein 's fix has generated code

I'd like to test with non-dynamics sources but am too lazy to compile from source - care to share a build?

[mvarie]

@SomeTroglodyte binaries here: https://www.dropbox.com/sh/n0qxpvijm3all52/AAAg-5UQ1WBT8J4rKj3Msz6ca?dl=0

[SomeTroglodyte]

Took me a while, but I have just now built @gregwinterstein 's "all-fixes" branch from source and I can confirm it generates a working connected service where the current public version fails miserably. A source feed with dashes in field names no longer lets the compiler stumble over all the invalid subtractions...

Sample excerpt

```csharp ///

/// There are no comments for Property _SMC_K in the schema. ///

[global::System.CodeDom.Compiler.GeneratedCodeAttribute("Microsoft.OData.Client.Design.T4", "#VersionNumber#")] [global::Microsoft.OData.Client.OriginalNameAttribute("_SMC-K")] public virtual string _SMC_K { get { return this.__SMC_K; } set { this.On_SMC_KChanging(value); this.__SMC_K = value; this.On_SMC_KChanged(); this.OnPropertyChanged("_SMC-K"); } } [global::System.CodeDom.Compiler.GeneratedCodeAttribute("Microsoft.OData.Client.Design.T4", "#VersionNumber#")] private string __SMC_K; partial void On_SMC_KChanging(string value); partial void On_SMC_KChanged(); ```

👍 👍 👍

[jackypowell]

My team really needs these changes. Can we please get them reviewed soon? Please and thank you! @ElizabethOkerio @KenitoInc @habbes @gathogojr

[habbes]

@gathogojr you had some thoughts around this issue. Could you check this PR?

[gathogojr]

Thank you @gregwinterstein for your contribution. Have you considered that this could be a breaking change for someone who's REGENERATING the proxy classes? Specifically for the case of the property named Context.

However, it might also not be a breaking change since the proxy classes that are generated in such a scenario do not compile meaning the affected customers are likely to be manually editing the proxy classes to mitigate the issue. Either way I think it'd be prudent to try and replicate the scenario by first generating the proxy using the extension before the fix and then regenerating it after the fix.

[SomeTroglodyte]

REGENERATING

I confirm: Regenerating is the most frequent use for me. Without this PR, each run requires 1-2 minutes of manual edits afterwards, per feed. This saves time especially then regenerating.

[gregwinterstein]

Have you considered that this could be a breaking change for someone who's REGENERATING the proxy classes? Specifically for the case of the property named Context.

@gathogojr, it has occurred to me that this pull request may cause some breaking changes because of the way valid identifiers are generated vs the way they were generated previously. I don't have a good solution for that.

My main issue is that I am unable to generate proxies that compile for either Dynamics F&O or Dynamics CRM (this doesn't entirely resolve all of the CRM issues due to inconsistent casing in the CRM metadata). This change generates proxies that compile for Dynmaics F&O and gets a lot closer for Dynamics CRM.

If backward compatibility is an issue, I'm open to suggestions.

[habbes]

@gregwinterstein I'm fine with the PR in general. I've left a few comments where I think the code is unnecessarily wasteful and could be improved.

[SomeTroglodyte]

unnecessarily wasteful

Don't forget source readability trumps performance. Here in this context. By far. No matter how wasteful a toString is, this will always be far less sluggish than, say, ~censored~. And overall performance is in my eyes controlled mostly by the ODATA service in question, the tool not so much. And tool runtime is only a fraction of resulting product runtime.

Of course, one of these proposals does read way better...

[habbes]

unnecessarily wasteful

Don't forget source readability trumps performance. Here in this context. By far. No matter how wasteful a toString is, this will always be far less sluggish than, say, ~censored~. And overall performance is in my eyes controlled mostly by the ODATA service in question, the tool not so much. And tool runtime is only a fraction of resulting product runtime.

Of course, one of these proposals does read way better...

I would argue that the proposed changes, in addition to being more performant, were as readable (if not more) than the original code. So, in the case where readability is similar, and the cost to create more performant code is low (didn't require a lot of refactoring), then there's no reason not to go for the more performant code.

For example, char.IsLetter(validateName[0]) is not any less readable than char.IsLetter(validName.ToString(), 0). I would argue that it's in fact more readable, it's clear that you're checking whether the first character is a letter. In the second snippet (the original code), it may not be immediately clear what the second argument of the char.IsLetter method refers to. The code change is straightforward and introduces virtually no risks compared to the original code. So in this case, there's no reason not to switch to the more performant code in my opinion.

[gathogojr]

Thank you @gregwinterstein for your contribution

[SomeTroglodyte]

~It might be nice to have some indication here on when this change might become available all the way through normal Visual Studio updates. 2025? 2026?~

Edit: This is already available through VS marketplace in 1.1.0:

ChangeLog:
1.1.0
Fix bug affecting excluding of schema types, operations, and operation imports.
Support omitting runtime version and code generation timestamp from generated files.
Fix bug affecting emitting of dynamic properties container property.
Support arm64 product architecture.
Support generation of valid property identifiers when service metadata does not strictly adhere to OData specification on property name.

I just got thrown off because that is not mirrored here on the releases page.