Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

[FEATURE REQ] azure-core support for extensible enum of type Integer and Float #38839

Closed
2 tasks done
weidongxu-microsoft opened this issue Feb 20, 2024 · 4 comments · Fixed by #41417
Closed
2 tasks done
Assignees
Labels
Azure.Core azure-core feature-request This issue requires a new behavior in the product in order be resolved.

Comments

@weidongxu-microsoft
Copy link
Member

weidongxu-microsoft commented Feb 20, 2024

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

azure-core support for extensible enum of type Integer and Float.

Similar to String type https://github.com/Azure/azure-sdk-for-java/blob/main/sdk/core/azure-core/src/main/java/com/azure/core/util/ExpandableStringEnum.java

Describe the solution you'd like

Possibly a generic class ExpandableEnum<T> (or ExtensibleEnum<>?).

Describe alternatives you've considered

Class generated via codegen without azure-core class.

Additional context

Information Checklist
Kindly make sure that you have added all the following information above and checkoff the required fields otherwise we will treat the issuer as an incomplete report

  • Description Added
  • Expected solution specified
@github-actions github-actions bot added the needs-triage Workflow: This is a new issue that needs to be triaged to the appropriate team. label Feb 20, 2024
@srnagar
Copy link
Member

srnagar commented Feb 20, 2024

Do you have usecases for where Integer and Float are needed?

@joshfree joshfree added Azure.Core azure-core feature-request This issue requires a new behavior in the product in order be resolved. labels Feb 20, 2024
@github-actions github-actions bot removed the needs-triage Workflow: This is a new issue that needs to be triaged to the appropriate team. label Feb 20, 2024
@alzimmermsft
Copy link
Member

I believe @billwert has a case where long is being used in this scenario

@billwert
Copy link
Contributor

Yeah, we have an enum that looks like this in typespec:
https://github.com/Azure/azure-rest-api-specs/blob/c7b8df506c79231f08e1d878b1c4917a5abb1113/specification/eventgrid/Azure.Messaging.EventGrid/main.tsp#L132

  @doc("Supported delays for release operation.")
  enum ReleaseDelay {
    @doc("Release the event after 0 seconds.")
    By0Seconds: 0,

    @doc("Release the event after 10 seconds.")
    By10Seconds: 10,

    @doc("Release the event after 60 seconds.")
    By60Seconds: 60,

    @doc("Release the event after 600 seconds.")
    By600Seconds: 600,

    @doc("Release the event after 3600 seconds.")
    By3600Seconds: 3600,
  }

The codegen for this becomes an ExpandableStringEnum with a fromLong method on it that then initializes thus: https://github.com/billwert/azure-sdk-for-java/blob/12fb25c810ab61180d66c315d4e3a57b548d08ae/sdk/eventgrid/azure-messaging-eventgrid-namespaces-http/src/main/java/com/azure/messaging/eventgrid/namespaces/http/models/ReleaseDelay.java#L20

public final class ReleaseDelay extends ExpandableStringEnum<ReleaseDelay> {
    /**
     * Release the event after 0 seconds.
     */
    @Generated
    public static final ReleaseDelay BY0_SECONDS = fromLong(0L);

    /**
     * Release the event after 10 seconds.
     */
    @Generated
    public static final ReleaseDelay BY10_SECONDS = fromLong(10L);
<snip>

Also of interest is the code generator emitted this when the type was used:

requestOptions.addQueryParam("releaseDelayInSeconds", String.valueOf(releaseDelayInSeconds.toLong()),
                false)

However the type didn't define a toLong() so it became a build error. It turns out that we didn't want a long, really, anyway. I mention it here as there's seemingly already some confusion about what to do with numeric enums from typespec. That's maybe just an issue I should go open in the autorest.java repo but wanted to call it out here in the context of "what does support for numeric enums" look like. (Also why did it default to long and not int? so many questions..)

@weidongxu-microsoft
Copy link
Member Author

weidongxu-microsoft commented Feb 22, 2024

Hi Bill, thanks very much on the use case. I wasn't expecting to see the usage so soon (issue was mainly for parity with other codegen).

The half baked class is mostly due to the "fixed enum" as such
https://github.com/Azure/autorest.java/blob/ff03ca0c69955f5490da2a61e947ff91a563e0b5/typespec-tests/src/main/java/com/cadl/enumservice/models/Priority.java
(you can see it has fromLong and toLong)

Long is because the there is no way in typespec source specifying the data type (typespec only have number). It could even become a Double if add a non-integer value :-( e.g.

  @doc("Supported delays for release operation.")
  enum ReleaseDelay {
    @doc("Release the event after 0 seconds.")
    By0Seconds: 0,

    @doc("Release the event after 10 seconds.")
    By10Seconds: 10,

    @doc("Release the event after 60 seconds.")
    By60Seconds: 60,

    @doc("Release the event after 600 seconds.")
    By600Seconds: 600,

    @doc("Release the event after 3600 seconds.")
    By3600Seconds: 3600,

    @doc("Release the event after 0.5 seconds.")
    By05Seconds: 0.5,
  }

Though in future, the extensible enum would be written as

  @doc("Supported delays for release operation.")
  union ReleaseDelay {
    int32,

    @doc("Release the event after 0 seconds.")
    By0Seconds: 0,

    @doc("Release the event after 10 seconds.")
    By10Seconds: 10,

    @doc("Release the event after 60 seconds.")
    By60Seconds: 60,

    @doc("Release the event after 600 seconds.")
    By600Seconds: 600,

    @doc("Release the event after 3600 seconds.")
    By3600Seconds: 3600,
  }

where we would know the type on that int32 (basically it means, the value can be these specified value, or just an int32 -- an extensible enum of integer). But still no such on enum.


So far, we don't have a azure-core class for extensible enum as int/float. A formal support would depend on this issue about the decision on azure-core.

Meanwhile, since the lib is still preview, we may use this class for the EventGrid. And replace the class when azure-core be ready.

Maybe customize the API in client to be

requestOptions.addQueryParam("releaseDelayInSeconds", releaseDelayInSeconds.toString(), false)

It happens that we do not really need a Long as this is on query parameter, which could only be a string. But if this ReleaseDelay was used in a model, it had to serialize to / de-serialize from a JSON number. And then this ExpandableStringEnum would not work at all.


@srnagar Do let us know what you think we can do, before we had the azure-core class.

# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
Azure.Core azure-core feature-request This issue requires a new behavior in the product in order be resolved.
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

5 participants