nacx commented on this pull request.
> +package org.jclouds.azurecompute.arm.domain;
+
+import com.google.auto.value.AutoValue;
+import com.google.common.collect.ImmutableList;
+import org.jclouds.javax.annotation.Nullable;
+import org.jclouds.json.SerializedNames;
+
+import java.util.List;
+
+@AutoValue
+public abstract class ExtensionProfileSettings {
+
+ /**
+ * The fileUris reference of the extension profile settings
+ */
+ @Nullable
>If the nullable is removed the tests fail as there aren't any value in this
>case (as real response) null is appropriate. It is not really the anti-pattern
>case for this because null is not used to handle exceptions.
I've built your branch and removed the annotation and tests don't fail.
A jclouds API is **not** an exact mapping to existing REST APIs. We go one step
further to provide a meaningful experience when consuming an API from Java. We
take care of properly mapping response error codes to appropriate exceptions,
building good transfer objects, etc. Part of this work is helping users use
clean APIs (from a Java perspective), and that means reducing the changes where
users can make mistakes. Having `nullable` lists is an anti-pattern that should
be avoided, as empty lists are a perfect substitute and make much more sense.
My point is that **your code is already enforcing that**. The code in the
`create` method already enforces an empty list when a `null` value (or absent
value) cames in the response. With the code in this factory method, **there is
no way** an instance of an `ExtensionProfileSettings` object can have a null
value in this field, thus, the `@Nullable` annotation is simply not true, as
users will never get a `null` value in these objects. Please, remove it and
apply this pattern in all the mentioned domain objects.
As I said, the rule of thumb for a consistent implementation and declaration is:
* If your constructor/factory methods enforce presence (as we do in most
cases), the `@Nullable` annotation does not apply and should not be there.
* If there is a good reason to not enforce presence in the constructor/factory
method (like in tags), then let's not enforce presence and annotate the field
accordingly.
Here is a diff you can apply to verify that there are no
serialization/deserialization or other issues if you remove the annotation,
even if the response does not contain a value for that field. Tests keep
passing.
```diff
diff --git
a/azurecompute-arm/src/main/java/org/jclouds/azurecompute/arm/domain/ExtensionProfileSettings.java
b/azurecompute-arm/src/main/java/org/jclouds/azurecompute/arm/domain/ExtensionProfileSettings.java
index 0d89fda3e..61d50d910 100644
---
a/azurecompute-arm/src/main/java/org/jclouds/azurecompute/arm/domain/ExtensionProfileSettings.java
+++
b/azurecompute-arm/src/main/java/org/jclouds/azurecompute/arm/domain/ExtensionProfileSettings.java
@@ -16,12 +16,12 @@
*/
package org.jclouds.azurecompute.arm.domain;
-import com.google.auto.value.AutoValue;
-import com.google.common.collect.ImmutableList;
-import org.jclouds.javax.annotation.Nullable;
+import java.util.List;
+
import org.jclouds.json.SerializedNames;
-import java.util.List;
+import com.google.auto.value.AutoValue;
+import com.google.common.collect.ImmutableList;
@AutoValue
public abstract class ExtensionProfileSettings {
@@ -29,7 +29,6 @@ public abstract class ExtensionProfileSettings {
/**
* The fileUris reference of the extension profile settings
*/
- @Nullable
public abstract List<String> fileUris();
/**
diff --git
a/azurecompute-arm/src/test/resources/createvirtualmachinescalesetresponse.json
b/azurecompute-arm/src/test/resources/createvirtualmachinescalesetresponse.json
index ff2971d40..4b118c50a 100644
---
a/azurecompute-arm/src/test/resources/createvirtualmachinescalesetresponse.json
+++
b/azurecompute-arm/src/test/resources/createvirtualmachinescalesetresponse.json
@@ -42,7 +42,7 @@
"type": "CustomScriptExtension",
"typeHandlerVersion": "1.1",
"autoUpgradeMinorVersion": false,
- "settings":
{"fileUris":["https://mystorage1.blob.core.windows.net/winvmextekfacnt/SampleCmd_1.cmd"],"commandToExecute":"SampleCmd_1.cmd"}
+ "settings": {"commandToExecute":"SampleCmd_1.cmd"}
},
"name": "extensionName"
}
diff --git a/azurecompute-arm/src/test/resources/virtualmachinescaleset.json
b/azurecompute-arm/src/test/resources/virtualmachinescaleset.json
index c99eb39a5..720f71ff8 100644
--- a/azurecompute-arm/src/test/resources/virtualmachinescaleset.json
+++ b/azurecompute-arm/src/test/resources/virtualmachinescaleset.json
@@ -42,7 +42,7 @@
"type": "CustomScriptExtension",
"typeHandlerVersion": "1.1",
"autoUpgradeMinorVersion": false,
- "settings":
{"fileUris":["https://mystorage1.blob.core.windows.net/winvmextekfacnt/SampleCmd_1.cmd"],"commandToExecute":"SampleCmd_1.cmd"}
+ "settings": {"commandToExecute":"SampleCmd_1.cmd"}
},
"name": "extensionName"
}
```
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/412#discussion_r147912687
