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

[FEATURE] Represent enums which have aliases #593

Open
Xtansia opened this issue Oct 1, 2024 · 6 comments
Open

[FEATURE] Represent enums which have aliases #593

Xtansia opened this issue Oct 1, 2024 · 6 comments
Labels
enhancement New feature or request

Comments

@Xtansia
Copy link
Collaborator

Xtansia commented Oct 1, 2024

Is your feature request related to a problem?

There are enums that OpenSearch accepts where multiple string representations map to the same value, with one being the canonical representation (i.e. they're aliased).

Example:
Current spec of GeoOrientation:

GeoOrientation:
      type: string
      enum:
        - left
        - right

Implementation of Orientation in OpenSearch:

public static Orientation fromString(String orientation) {
            orientation = orientation.toLowerCase(Locale.ROOT);
            switch (orientation) {
                case "right":
                case "counterclockwise":
                case "ccw":
                    return Orientation.RIGHT;
                case "left":
                case "clockwise":
                case "cw":
                    return Orientation.LEFT;
                default:
                    throw new IllegalArgumentException("Unknown orientation [" + orientation + "]");
            }
        }

Existing implementation of GeoOrientation in opensearch-java (on serialization first item is used, all accepted for deserialization):

public enum GeoOrientation implements JsonEnum {
    Right("right", "RIGHT", "counterclockwise", "ccw"),

    Left("left", "LEFT", "clockwise", "cw");
}

Note that we also handle all-caps variants as if OpenSearch itself sets and returns its version it will be uppercase.

This means the final spec should be roughly equivalent to:

GeoOrientation:
      type: string
      enum:
        - left
        - LEFT
        - clockwise
        - cw
        - right
        - RIGHT
        - counterclockwise
        - ccw

But should also communicate that left and right are the canonical versions and others map onto them.

@Xtansia Xtansia added enhancement New feature or request untriaged labels Oct 1, 2024
@Xtansia
Copy link
Collaborator Author

Xtansia commented Oct 1, 2024

What are your thoughts @dblock @nhtruong?

@Xtansia Xtansia removed the untriaged label Oct 1, 2024
@Xtansia
Copy link
Collaborator Author

Xtansia commented Oct 1, 2024

We could potentially re-use the oneOf concept like was used for deprecated enums and combine it with the title property:

GeoOrientation:
  oneOf:
    - title: left
      type: string
      enum:
        - left
        - LEFT
        - clockwise
        - cw
    - title: right
      type: string
      enum:
        - right
        - RIGHT
        - counterclockwise
        - ccw

@dblock
Copy link
Member

dblock commented Oct 1, 2024

I like the proposal.

Is there a downside to treating these as separate values? Where do we need the semantic meaning of equivalence?

@Xtansia
Copy link
Collaborator Author

Xtansia commented Oct 1, 2024

I like the proposal.

Is there a downside to treating these as separate values? Where do we need the semantic meaning of equivalence?

Main downside from the Java client's side is that breaking them out into separate values would be a breaking change for users (i.e. if previously had if (orientation == GeoOrientation.Right) this would now not match if it was actually deserialized from "ccw"). A potential workaround is to just hard code these aliases in the java client codegen.

Splitting them out also may lead to confusion for users as they suddenly have new distinct options that are not actually distinct options. i.e. you put index with orientation(GeoOrientation.Ccw), but then you get index and OS has normalized it so it returns GeoOrientation.Right.

@nhtruong
Copy link
Collaborator

nhtruong commented Oct 2, 2024

We can also keep it as one big enum type with all the values and has a way to communicate with the user that certain values are equal (via the description property maybe?). This will also benefit the documentation website in the future.

(Both approaches will yield the same result in the TS generator, so this is only affecting Java.)

@Xtansia
Copy link
Collaborator Author

Xtansia commented Oct 2, 2024

While not yet generated, it is also currently represented in .NET: https:/opensearch-project/opensearch-net/blob/main/src/OpenSearch.Client/Mapping/Types/Geo/GeoShape/GeoOrientation.cs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants