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

Extensible enums should use case-sensitive comparisons (with possible opt-out) #2826

Open
heaths opened this issue Nov 1, 2022 · 5 comments
Assignees
Labels
CodeGen Issues that relate to code generation DPG/RLC v2.1 Post Gallium work DPG Mgmt This issue is related to a management-plane library.

Comments

@heaths
Copy link
Member

heaths commented Nov 1, 2022

Originating from Azure/azure-sdk-for-net#29141 and after discussions with @KrzysztofCwalina we should actually be using case-sensitive comparisons for extensible enums as exemplified in our guidelines but with the stipulation from Azure/azure-sdk#1251:

Type of value comparison should be selected on per-service basis, if the service is inconsistent with how string values are returned the case-insensitive comparison is allowed.

Thus, the generate should default to case-sensitive comparisons but is currently using InvariantCultureIgnoreCase. It should be Ordinal as documented for performance reasons; however, we do need a way to opt into case-insensitive comparisons like needed for Azure/azure-sdk-for-net#29141.

This is also consistent with our Azure REST API guidelines:

✅ DO treat JSON field values with case-sensitivity. There may be some exceptions (e.g. GUIDs) but avoid if at all possible.

@heaths heaths added CodeGen Issues that relate to code generation DPG labels Nov 1, 2022
@heaths
Copy link
Member Author

heaths commented Nov 1, 2022

For comparison, it seems languages are inconsistent:

  1. Python uses case-insensitive comparisons (example)
  2. Java uses case-sensitive comparisons (example)
  3. JavaScript uses case-sensitive comparisons (confirmed by @bterlson)
  4. C++ recommends whatever the service does, which is observed behavior - not necessarily documented (guidelines)

Talking with @JeffreyRichter, perhaps we should relax the SDK guidelines (actually write a general guideline and remove language-specific recommendations) but keep REST API guidelines as-is (services should use case-sensitive comparisons, but could be case-insensitive if they desire).

To note, this is only about discriminators and LRO state. This is where it typically matters - where the SDKs themselves are doing comparisons.

@lirenhe
Copy link
Member

lirenhe commented Nov 4, 2022

@archerzz, please help to follow up

@archerzz
Copy link
Member

archerzz commented Nov 4, 2022

Given .Net SDK has a much larger user base, I'm not sure if we should do this.

I think the case-insensitive comparison works fine for happy path.

  • Assumption: There won't be enum values like Abc and abc defined for one enum. I think that can almost be guaranteed.
  • For serialization:
    • For predefined values: they are from swagger definition, and we don't change the case. If the service rejects the input, the service team should fix it.
    • For values created by customers: they must follow the back-end service requirement, e.g. case-sensitive or case-insensitive.
  • For de-serialization:
    • For services adopting case-insensitive, we just follow the rules.
    • For services adopting case-sensitive, case-insensitive comparison works fine.

However, case-insensitive comparison will provide better error tolerance when:

  • The back-end service return values with wrong case, e.g. a case-sensitive service returns value with wrong case.
  • The customers write values with wrong case.

On the other hand, case-insensitive comparison will also hide the errors. But I think it's a benign compromise. If we change to case-sensitive comparison, we could break customers' programs, which requires fixes in either their programs or in the back-end service. The latter could take more time. The impact could be non-trivial.

@ArthurMa1978 ArthurMa1978 added the Mgmt This issue is related to a management-plane library. label Nov 4, 2022
@ArthurMa1978
Copy link
Member

This is shared by all .net languages, since other languages are using case-sensitive comparisons it should be ok for .net, but not sure why it's insensitive now.

@heaths
Copy link
Member Author

heaths commented Nov 4, 2022

I'll add you two the thread I have with architects, who are (mostly) pushing for consistent case-sensitive comparisons across the board. Note that our guidelines for .NET do say to use case-sensitive comparisons, which @KrzysztofCwalina is maintaining should be our default.

@chunyu3 chunyu3 added the DPG/RLC v2.1 Post Gallium work label Sep 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CodeGen Issues that relate to code generation DPG/RLC v2.1 Post Gallium work DPG Mgmt This issue is related to a management-plane library.
Projects
None yet
Development

No branches or pull requests

5 participants