microsoft / kiota

OpenAPI based HTTP Client code generator
https://aka.ms/kiota/docs
MIT License
2.95k stars 207 forks source link

Add support for numerical enum values #5165

Open jhancock-taxa opened 2 months ago

jhancock-taxa commented 2 months ago

Is your feature request related to a problem? Please describe the problem.

The assertion made at the bottom of this article is false: https://learn.microsoft.com/en-us/openapi/kiota/models

Supporting proper enums does add lots of value. And x-enumNames like nswag does and the oneof syntax for enums that is fully supported in OpenApi 3.0 absolutely is of value.

Client library/SDK language

None

Describe the solution you'd like

i.e. in typescript, this should define an enum with a name that is defined in the openapi document AND the number value and when the client sets the enum, it's setting the number in the payload, while allowing the use of the fully defined enum

That means that it should support both x-enuNames and the properly defined syntax from OpenAPI 3.0 and generate, for all clients, full enums that map through if that client supports them.

Additional context

nswag client generator and countless other tools support this without issue out of the box.

Here's a verbose version that should support every permutation for all client generation tools that kiota should be able to pick from and generate enums. (and it should support all 3, especially the openapi 3.0 spec version using oneof)

      "RuleTypes": {
        "type": "integer",
        "description": "Rule Types\n            ",
        "x-enumNames": [
          "Validation",
          "Sum",
          "Average",
          "Max",
          "Min"
        ],
        "enum": [
          1,
          2,
          3,
          4,
          5
        ],
        "oneOf": [
          {
            "type": "integer",
            "enum": [
              1
            ],
            "title": "Validation"
          },
          {
            "type": "integer",
            "enum": [
              2
            ],
            "title": "Sum"
          },
          {
            "type": "integer",
            "enum": [
              3
            ],
            "title": "Average"
          },
          {
            "type": "integer",
            "enum": [
              4
            ],
            "title": "Max"
          },
          {
            "type": "integer",
            "enum": [
              5
            ],
            "title": "Min"
          }
        ],
        "x-ms-enum": {
          "name": "RuleTypes",
          "modelAsString": false,
          "values": [
            {
              "name": "Validation",
              "value": 1,
              "description": ""
            },
            {
              "name": "Sum",
              "value": 2,
              "description": ""
            },
            {
              "name": "Average",
              "value": 3,
              "description": ""
            },
            {
              "name": "Max",
              "value": 4,
              "description": ""
            },
            {
              "name": "Min",
              "value": 5,
              "description": ""
            }
          ]
        }
      },

This should generate in typescript as an example something like this:

/**
 * Rule Types
 */
enum RuleTypes {
    /**
     * Validation
     */
    Validation = 1,

    /**
     * Sum
     */
    Sum = 2,

    /**
     * Average
     */
    Average = 3,

    /**
     * Max
     */
    Max = 4,

    /**
     * Min
     */
    Min = 5
}

And C# this:

public enum RuleTypes
{
    Validation = 1,

    Sum = 2,

    Average = 3,

    Max = 4,

    Min = 5
}

And Java this:

/**
 * Rule Types
 */
public enum RuleTypes {
    /**
     * Validation
     */
    VALIDATION(1),

    /**
     * Sum
     */
    SUM(2),

    /**
     * Average
     */
    AVERAGE(3),

    /**
     * Max
     */
    MAX(4),

    /**
     * Min
     */
    MIN(5);

    private final int value;

    RuleTypes(int value) {
        this.value = value;
    }

    public int getValue() {
        return value;
    }
}

And Go:

package main

// RuleTypes represents the different types of rules
type RuleTypes int

const (
    // ValidationRule represents a validation rule
    ValidationRule RuleTypes = iota + 1
    // SumRule represents a sum rule
    SumRule
    // AverageRule represents an average rule
    AverageRule
    // MaxRule represents a max rule
    MaxRule
    // MinRule represents a min rule
    MinRule
)

// String returns the string representation of RuleTypes
func (r RuleTypes) String() string {
    switch r {
    case ValidationRule:
        return "Validation"
    case SumRule:
        return "Sum"
    case AverageRule:
        return "Average"
    case MaxRule:
        return "Max"
    case MinRule:
        return "Min"
    default:
        return "Unknown"
    }
}

// Value returns the integer value of RuleTypes
func (r RuleTypes) Value() int {
    return int(r)
}

Etc.

And any language that doesn't support numeric assignment of enums, would fall back to the current behavior.

baywet commented 2 months ago

Hi @jhancock-taxa , Thank you for using kiota and for reaching out. I think there could be multiple asks here and I'd like to clarify which one you meant before further discussions.

Support x-enumNames

This is not implemented today but could be added. Kiota leverages x-ms-enum instead. Leveraging the same pattern, this also could be supported. Then one question becomes: what should happen when both are present?

Support declaring enums with one of

I've not come across this pattern before. And I think this assumes support for the const keyword which was only added in OAS3.1. See #3914

Support numerical enums

Unfortunately I don't think there's a way to support this without a source breaking change. The serialization writer interface only supports passing the Enum with the member attribute which only support a string value

jhancock-taxa commented 2 months ago

x-ms-enum's entire point is to support key/value pairs. I.e. C# enums where the number is serialized. That's what it was created for in the first place.

Please strongly consider fixing this so that it x-ms-enum works as intended and allows key/value pair mapping for enums with enums being created in every language correctly and being passed as properties. (real enums, not consts)

All languages that support key/value enums (i.e. all of them that you support right now) should generate real enums in the language with string OR numeric values. And x-ms-enum is the perfect test case, because its entire purpose is to represent C# enums correctly, and thus should generate perfect C# enums as a result of this work with numerical values.

And of course, since OpenAPI as you stated now supports enums properly in 3.1, Kiota should also support generating enums correctly in all languages according to that standard (and Microsoft should be enabling the new OpenAPI support in .net 9 to support OpenAPI 3.1 enum definitions too)

baywet commented 2 months ago

Thank you for the additional information.

x-ms-enum is supported today, just from one "string" (symbol) to another one (the value). If I'm understanding your last reply properly the main request here is to support number values for enums?

jhancock-taxa commented 2 months ago

Since C# doesn't support enums with string values, x-ms-enum can't be supported as you're suggesting right now becasue x-ms-enum is specifically for number/value not string/string since "enum" in OpenAPI already supports this. I'm not sure why x-ms-enum, with the current support in kiota would even be needed since it adds nothing.

I'm asking for x-ms-enum to be properly supported with number/string values per the spec that was originally created for it to specifically map C# enums. I'm also asking that "enum" in OpenAPI earlier versions be properly supported if x-ms-enum is not available in the definition, and that OpenApI 3.1 enums be supported too if available and those preferentially to x-ms-enum and enum and in all cases support both string/string and number/string. And in all cases they should result in the target language native enum functionality used whenever possible.

Order of importance and preference of what is generated:

  1. Open API 3.1 enum with one of functionality supporting number/string
  2. x-ms-enum supporting number/string
  3. enum support with string/string
baywet commented 2 months ago

Thank you for the additional information.

I'm not sure why x-ms-enum, with the current support in kiota would even be needed since it adds nothing.

It allows to map a different code symbol to the string value (and descriptions). This is a good example.

  1. already tracked with #3914, let's leave it out of scope for the current issue.
  2. most likely source breaking, but let's keep the discussion going to see if we can find a non-source breaking design.
  3. already implemented, except for the other extension we talked about earlier. Since this additional extension doesn't seem to be the main ask here, let's leave it out of scope of this issue.

Adding support for numerical enum values.

Before looking at the generation, I think we should look at serialization libraries to see if the change can be implemented without a source breaking change.

Serialization

We could probably add a bunch of special cases here to call WriteIntValue when the representation is in fact an int. https://github.com/microsoft/kiota-dotnet/blob/c198508ec738bc780e5816c33847880ddd5981ff/src/serialization/json/JsonSerializationWriter.cs#L305

Deserialization

Likewise, we could probably special case things here if we get a number

https://github.com/microsoft/kiota-dotnet/blob/c198508ec738bc780e5816c33847880ddd5981ff/src/serialization/json/JsonParseNode.cs#L217

Is this something you'd like to submit a pull request for provided some guidance? Once we have a clear implementation with unit tests in dotnet, we can probably start replicating in other languages. And once the serialization libraries support the number values, we'll have clarity on required generation changes, if any.

Let us know if you have any additional comments or questions.

jhancock-taxa commented 2 months ago

The example ignores the original point to AutoREST: the enum: section is supposed to be the numerical representation and the value in x-ms-enum is supposed to be the same and the description is the text representation that the enum in the language is supposed to have.

(I know, I was there when the standard was created BEFORE Azure's documentation on the subject existed.)

My point to all of this, is that Kiota started with a false premise that number/string doesn't add value (it obviously does) and needs to fix its implementation of x-ms-enum to follow the original intent, which means fixing generating number/value which is the base case for all enums in all programming languages. In fact, only a few natively support string/string without extensions (typescript does, Java needs extended enums to make it work, default enums do not, etc.)

I don't have time to fix this false premise so that it follows the original intent of x-ms-enum, and why you'd want to do this anyhow. I'm asking the maintainers of Kiota to fix their mistake.