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

CSharp | Enum values are invalid after being sanitized #2488

Closed
vadrsa opened this issue Mar 30, 2023 · 5 comments · Fixed by #2494
Closed

CSharp | Enum values are invalid after being sanitized #2488

vadrsa opened this issue Mar 30, 2023 · 5 comments · Fixed by #2494
Assignees
Labels
Csharp Pull requests that update .net code type:bug A broken experience WIP
Milestone

Comments

@vadrsa
Copy link

vadrsa commented Mar 30, 2023

First of all, thanks for such a great tool!

Enum values are being sanitized, but don't have an EnumMember Attribute on top of them to specify the original name from the spec.
Enum value sanitization is also a bit off in my opinion.
For example,
OpenAPI spec

"enum": [
    "DISABLED",
    "CONSIDERING",
    "UNKNOWN",
    "PLAN:EXECUTING:STOPPING",
    "PLAN:EXECUTING:STOP_FAILED",
    "PLAN:EXECUTING:STOPPED",
    "PLAN:EXECUTING:STOPPED:AWAITING_PRICES",
    "PLAN:EXECUTING:STARTING",
    "PLAN:EXECUTING:START_FAILED",
    "PLAN:EXECUTING:STARTED",
    "PLAN:EXECUTING:CHARGE_INTERRUPTED",
    "PLAN:EXECUTING:OVERRIDDEN",
    "PLAN:ENDED:FINISHED",
    "PLAN:ENDED:UNPLUGGED",
    "PLAN:ENDED:FAILED",
    "PLAN:ENDED:DISABLED",
    "PLAN:ENDED:DEADLINE_CHANGED"
],

is converted to

public enum EnumName{
    DISABLED,
    CONSIDERING,
    UNKNOWN,
    PLANEXECUTINGSTOPPING,
    PLANEXECUTINGSTOP_FAILED,
    PLANEXECUTINGSTOPPED,
    PLANEXECUTINGSTOPPEDAWAITING_PRICES,
    PLANEXECUTINGSTARTING,
    PLANEXECUTINGSTART_FAILED,
    PLANEXECUTINGSTARTED,
    PLANEXECUTINGCHARGE_INTERRUPTED,
    PLANEXECUTINGOVERRIDDEN,
    PLANENDEDFINISHED,
    PLANENDEDUNPLUGGED,
    PLANENDEDFAILED,
    PLANENDEDDISABLED,
    PLANENDEDDEADLINE_CHANGED,
}

Proposed enum in chsarp

public enum EnumName{
    [EnumMember(Value = "DISABLED")]
    DISABLED,
    [EnumMember(Value = "CONSIDERING")]
    CONSIDERING,
    [EnumMember(Value = "UNKNOWN")]
    UNKNOWN,
    [EnumMember(Value = "PLAN:EXECUTING:STOPPING")]
    PLAN_EXECUTING_STOPPING,
    [EnumMember(Value = "PLAN:EXECUTING:STOP_FAILED")]
    PLAN_EXECUTING_STOP_FAILED
    ///...
}

The naming is a bit subjective, but it would be cool if Kiota could be more extensible, so one could write some code to customize the generation or namings.

@baywet baywet self-assigned this Mar 30, 2023
@baywet baywet added type:bug A broken experience Csharp Pull requests that update .net code and removed Needs: Triage 🔍 labels Mar 30, 2023
@baywet baywet added this to Kiota Mar 30, 2023
@github-project-automation github-project-automation bot moved this to Todo in Kiota Mar 30, 2023
@baywet baywet added this to the Kiota v1.2 milestone Mar 30, 2023
@baywet
Copy link
Member

baywet commented Mar 30, 2023

Thanks for reporting this and for the praises! I believe the issue is multiple folds:

  1. The CSharp symbol is not normalized as expected by conventions PLAN:ENDED:DEADLINE_CHANGED should become PlanEndedDeadlineChanged
  2. The Serialization value is not caried over, leading to inaccurate parsing/serialization

I'm going to focus on the second aspect as it's a more pressing issue at the moment.
If this is something you'd like to help address with a couple of pull requests, here are some pointers:

The code that could write the enum member attribute lives here

writer.WriteLine($"{option.Name.ToFirstCharacterUpperCase()}{(codeElement.Flags ? " = " + GetEnumFlag(idx) : string.Empty)},");

It should rely on this information and only add an annotation if this property is true.

public bool IsNameEscaped

Unit tests can be added here.

public class CodeEnumWriterTests : IDisposable

Lastly the serialization will need to be updated to handle that case.
https://github.com/microsoft/kiota-serialization-json-dotnet/blob/e26135c0ab1edc8cad5a70cacd7964b16dddce3b/src/JsonSerializationWriter.cs#L241
https://github.com/microsoft/kiota-serialization-json-dotnet/blob/e26135c0ab1edc8cad5a70cacd7964b16dddce3b/src/JsonParseNode.cs#L157

Are you willing to submit a couple of pull requests to address this?

CC @andreaTP and @andrueastman for visibility.

@vadrsa
Copy link
Author

vadrsa commented Mar 30, 2023

Thanks for the quick response and the pointers @baywet, I will give it a try.

@andreaTP
Copy link
Contributor

Just skimmed through, but, if I understand this correctly, this is done in Java:

writer.WriteLine($"{enumOption.Name.ToFirstCharacterUpperCase()}(\"{enumOption.SerializationName}\"){(enumOption == lastEnumOption ? ";" : ",")}");

and the desired encoding for C# looks pretty much matching the current Java encoding.

vadrsa pushed a commit to vadrsa/kiota that referenced this issue Mar 30, 2023
…SerializationName is different from CodeEnumOption.Name
vadrsa pushed a commit to vadrsa/kiota that referenced this issue Mar 30, 2023
…SerializationName is different from CodeEnumOption.Name
vadrsa pushed a commit to vadrsa/kiota that referenced this issue Mar 30, 2023
…SerializationName is different from CodeEnumOption.Name
baywet added a commit that referenced this issue Mar 30, 2023
#2488 - CSharp | Add EnumMember Attribute if CodeEnumOption.Serializa…
vadrsa pushed a commit to vadrsa/kiota that referenced this issue Mar 31, 2023
@vadrsa
Copy link
Author

vadrsa commented Mar 31, 2023

@baywet I created a new pr with a small fix for my last pr, #2494

vadrsa pushed a commit to vadrsa/kiota that referenced this issue Mar 31, 2023
@baywet
Copy link
Member

baywet commented Mar 31, 2023

I can see you forked the naming convention aspect into #2495, so we'll keep this issue just for the enum member aspect in dotnet.

@baywet baywet linked a pull request Mar 31, 2023 that will close this issue
@baywet baywet modified the milestones: Kiota v1.2, Kiota v1.1 Mar 31, 2023
vadrsa pushed a commit to vadrsa/kiota that referenced this issue Mar 31, 2023
baywet added a commit that referenced this issue Mar 31, 2023
#2488 - Add using statement for EnumMember attribute
@github-project-automation github-project-automation bot moved this from Todo to Done in Kiota Mar 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Csharp Pull requests that update .net code type:bug A broken experience WIP
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants