feat(sdk/java) enums #9856
feat(sdk/java) enums #9856
Conversation
| .beginControlFlow("if (clazz.isEnum())") | ||
| .addStatement( | ||
| "$T valueOf = clazz.getMethod($S, $T.class)", | ||
| Method.class, | ||
| "valueOf", | ||
| String.class) | ||
| .addStatement( | ||
| "return clazz.cast(valueOf.invoke(null, jsonb.fromJson(json, $T.class)))", | ||
| String.class) | ||
| .endControlFlow() |
There was a problem hiding this comment.
Enum values are serialized using their name. But deserialization is not identical as this value can't be set.
See for details: https://jakarta.ee/specifications/jsonb/3.0/jakarta-jsonb-spec-3.0#enum
So here it first converts the JSON string to a Java String (jsonb.fromJson(json, String.class)) then use this value to call valueOf on the enum class.
There was a problem hiding this comment.
Good to be aware that we plan on changing this to use the member names instead of values. See:
If there's anything you can do to make the transition easier later that would be very helpful 🙂 Otherwise not a concern for now.
There was a problem hiding this comment.
Interesting, I'll have a look at that.
For now it's using name only, but I'll see to have the smoothest transition.
There was a problem hiding this comment.
🤔
That will be fun I think... The thing is in Java an enum member only have a value, not a name (or the other way around, but that's called value). And there's no strict way to define a specific value, only conventions. And an enum value can have any number of fields.
For instance:
public enum Severity {
INFO("info-severity", 1),
WARN("warn-severity", 2),
ERROR("error-severity", 3);
private final String severity;
private final int criticality;
Severity(String s, int c) {
this.severity = s;
this.criticality = c;
}
public String getSeverity() {
return severity;
}
public int getCriticality() {
return criticality;
}
}And of course, there's no standard way to get an enum value based on the field values.
In this case there's a Severity.valueOf(String) that will convert a name to an enum, but it only stick to the INFO, WARN, etc.
I'm not entirely sure how that will work, but I'll probably move with this first, as it just uses default, standard, java enums and that works great. But I'll keep an eye on the name/value question to see how we can support those changes too.
There was a problem hiding this comment.
For Java modules defining enums, I think it's ok not to be able to have a name different than a value. In GraphQL that destinction doesn't exist so we'll need to implement a custom directive to preserve the value if it's different from the name.1 Thus, in this case, the transition should be simple. Just need to update the API call for registering the enum, in the SDK.
Ideally everyone would use the same name and value when defining enums, but most SDKs allow setting them differently and some people want to take advantage of that. So, since a Java module can depend on a Go module that has different values and names, how does the Java codegen handle that?
Footnotes
-
For context, GraphQL enums have only values, but the values are "names", in the sense that they must be valid identifiers (as in variable name). ↩
There was a problem hiding this comment.
If I have that in Go:
type Severity string
const (
Unknown Severity = "UNKNOWN_SEVERITY"
Low Severity = "low_severity"
Medium Severity = "MEDIUM_SEVERITY"
High Severity = "HIGH_SEVERITY"
Critical Severity = "CRITICAL_SEVERITY"
)It will creates this in Java:
public enum MyEnumSeverity {
UNKNOWN_SEVERITY,
low_severity,
MEDIUM_SEVERITY,
HIGH_SEVERITY,
CRITICAL_SEVERITY
}So it uses the values. As the values are valid identifiers, like for GraphQL enums, that works well.
| enumInfos.put( | ||
| qName, | ||
| new EnumInfo( | ||
| element.getSimpleName().toString(), | ||
| element.getEnclosedElements().stream() | ||
| .filter(elt -> elt.getKind() == ElementKind.ENUM_CONSTANT) | ||
| .map(Element::getSimpleName) | ||
| .map(Name::toString) | ||
| .toArray(String[]::new))); |
There was a problem hiding this comment.
Store the list of possible values in the enum
0844572 to
cc7aba9
Compare
| .add("$L = (", parameterInfo.name()) | ||
| .add(paramType) | ||
| .add( | ||
| ") $T.fromJSON(inputArgs.get($S), ", | ||
| "$L = $T.fromJSON(inputArgs.get($S), ", | ||
| parameterInfo.name(), |
There was a problem hiding this comment.
This cast was useless as the type is defined
d28a86f to
4631255
Compare
40a8ed1 to
b0c8566
Compare
| List ints = null; | ||
| List<Integer> ints = null; | ||
| if (inputArgs.get("ints") != null) { | ||
| ints = (List) JsonConverter.fromJSON(inputArgs.get("ints"), List.class); | ||
| ints = Arrays.asList(JsonConverter.fromJSON(inputArgs.get("ints"), Integer[].class)); | ||
| } |
There was a problem hiding this comment.
A lot of great things here, and a good example of the improvements regarding types:
List ints replaced by List<Integer>: the inner type is now better handled. So we are dealing with typed list and not just generic List.
The input is a JSON array of integers, but the end variable is a List<Integer>. So we would like to deserialize the input into the list. But from the JSON perspective it doesn't matter, this is an array.
And on the Java side, Integer[].class is what is the closest to the JSON type. List.class is generic and it's hard to correctly deserialize the JSON array into this type.
So the JSON array is deserialized into an array of the expected type (Integer here) then converted into a List so that the List<Integer> type of the variable is respected.
There was a problem hiding this comment.
Looks Good! Very complete PR with doc addition to make it perfect 🚀
I left a couple of nit comment, feel free to ignore them if you think they are not relevant (I'm not a Java programmer and I'm not familiar with the standard rules)
I'm ready to approve it once my comments are resolved
| With(daggerCall("print", "--severity=FOO")). | ||
| Stdout(ctx) | ||
|
|
||
| require.Error(t, err) |
There was a problem hiding this comment.
Ideally, the error should include the list of possible values for the enums https://docs.dagger.io/api/enumerations/
Maybe you can test that behaviour here for consistency with other SDKs?
There was a problem hiding this comment.
Fixed, added the line after this one 👍
...-processor/src/main/java/io/dagger/annotation/processor/DaggerModuleAnnotationProcessor.java
Outdated
Show resolved
Hide resolved
...-processor/src/main/java/io/dagger/annotation/processor/DaggerModuleAnnotationProcessor.java
Show resolved
Hide resolved
...agger-java-annotation-processor/src/main/java/io/dagger/annotation/processor/DaggerType.java
Outdated
Show resolved
Hide resolved
663cd97 to
6a1b689
Compare
User defined enums must have the `@Enum` annotation. Then they can be used in modules. This annotation is required to know this is an enum during the annotation parsing phase, as the object might not yet be accessible. It also ensures only one single enum is defined with the same name. Technically this is done by updating the deserialization of JSON as enum values must use the `valueOf(String)` method and can't be deserialized straight away. See https://jakarta.ee/specifications/jsonb/3.0/jakarta-jsonb-spec-3.0#enum Signed-off-by: Yves Brissaud <gh@lgtd.io>
Signed-off-by: Yves Brissaud <gh@lgtd.io>
Signed-off-by: Yves Brissaud <gh@lgtd.io>
Rework the way we deal with types. Extract the type management in dedicated classes to add specific behavior depending on the types. This also fixes a lot of issues with List. In short, when you have a `List<Something>` and try to deserialize some JSON to it, it's not straightforward: `Something` disappears, so you just have `List` and `List` is an interface: - the fact `Something` disappear is an issue when the type is required to deserialize. For instance an enum is serialized as a string, but deserialized into a specific type. If we have `["low", "medium", "high"]` to deserialize and only the type `List` we don't know if it's a list of string or a list of the enum. And as the enums needs a special call to be set we can't do it. - `List` is an interface. We can't set something as an interface, it has to be a concrete type. For instance we have to use an `ArrayList`. But `ArrayList` is not the defined type. All in all, the idea is here, when we are deserializing, to not deserialize as `List` but always as arrays. Then to convert to a `List`. So for instance if the parameter type is `List<MyEnum>` we will deserialize the value into a `MyEnum[]` then call `Arrays.toList` on it. The usage of the array fixes the issues mentionned above, and from a JSON perspective list or arrays are identical. The new types exposes several methods, to know if it's a list or not, and to expose the different code blocks we need for the entrypoint generation. In particular a new method has been added when we want the class of a type instead of just adding `.class` in the code generation. Most of the `List` related behavior (to return or get a list) is covered in the `enums` integration tests. Signed-off-by: Yves Brissaud <gh@lgtd.io>
Signed-off-by: Yves Brissaud <gh@lgtd.io>
Signed-off-by: Yves Brissaud <gh@lgtd.io>
|
I'm going to merge it. We can continue the enum discussion when we'll add the name in addition to the value, but my understanding is Java is close to GraphQL so that shouldn't be a problem (and is not yet done). Especially this PR will improves a lot on List/array management. Thanks for the reviews 👍 |
Support Java enums! 🎉
Enums defined in the module must have the
@Enumannotation. This allows the annotation processor to find them and register them and their possible values to the Dagger engine.For instance:
Only a single enum can be defined with a specific name, whatever their fully qualified name. For instance it you have two enums
io.dagger.module.mymodule.Severityandio.dagger.module.foo.Severityan error will be raised.Enums can be defined as independent objects or can be nested inside the main module object.
For instance this is valid:
Enums requires a specific change in the way we deserialize the JSON. Java enums requires to call the
valueOf(String)method on the enum class. See https://jakarta.ee/specifications/jsonb/3.0/jakarta-jsonb-spec-3.0#enum for more details.Integration tests and docs have been added.
edit: new type management system
Rework the way we deal with types. Extract the type management in dedicated classes to add specific behavior depending on the types.
This also fixes a lot of issues with List. In short, when you have a
List<Something>and try to deserializesome JSON to it, it's not straightforward:
Somethingdisappears, so you just haveListandListis an interface:Somethingdisappear is an issue when the type is required to deserialize. For instance an enum is serialized as a string, but deserialized into a specific type. If we have["low", "medium", "high"]to deserialize and only the typeListwe don't know if it's a list of string or a list of the enum. And as the enums needs a special call to be set we can't do it.Listis an interface. We can't set something as an interface, it has to be a concrete type. For instance we have to use anArrayList. ButArrayListis not the defined type.All in all, the idea is here, when we are deserializing, to not deserialize as
Listbut always as arrays. Then to convert to aList.So for instance if the parameter type is
List<MyEnum>we will deserialize the value into aMyEnum[]then callArrays.toListon it. The usage of the array fixes the issues mentionned above, and from a JSONperspective list or arrays are identical.
The new types exposes several methods, to know if it's a list or not, and to expose the different code blocks we need for the entrypoint generation. In particular a new method has been added when we want the class of a type
instead of just adding
.classin the code generation.Most of the
Listrelated behavior (to return or get a list) is covered in theenumsintegration tests.