[GitHub] [iceberg] rdblue commented on a diff in pull request #5968: Core: Make CreateNamespaceRequest immutable and use explicit JSON Parser

[GitBox <gi...@apache.org>]

rdblue commented on code in PR #5968:
URL: https://github.com/apache/iceberg/pull/5968#discussion_r993635346


##########
core/src/main/java/org/apache/iceberg/rest/requests/CreateNamespaceRequest.java:
##########
@@ -19,80 +19,24 @@
 package org.apache.iceberg.rest.requests;
 
 import java.util.Map;
-import java.util.Objects;
 import org.apache.iceberg.catalog.Namespace;
-import org.apache.iceberg.relocated.com.google.common.base.MoreObjects;
-import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
 import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
-import org.apache.iceberg.relocated.com.google.common.collect.Maps;
 import org.apache.iceberg.rest.RESTRequest;
+import org.immutables.value.Value;
 
 /** A REST request to create a namespace, with an optional set of properties. */
-public class CreateNamespaceRequest implements RESTRequest {
+@Value.Immutable
+public interface CreateNamespaceRequest extends RESTRequest {
 
-  private Namespace namespace;
-  private Map<String, String> properties;
+  Namespace namespace();
 
-  public CreateNamespaceRequest() {
-    // Needed for Jackson Deserialization.
-  }
-
-  private CreateNamespaceRequest(Namespace namespace, Map<String, String> properties) {
-    this.namespace = namespace;
-    this.properties = properties;
-    validate();
-  }
-
-  @Override
-  public void validate() {
-    Preconditions.checkArgument(namespace != null, "Invalid namespace: null");
-  }
-
-  public Namespace namespace() {
-    return namespace;
-  }
-
-  public Map<String, String> properties() {
-    return properties != null ? properties : ImmutableMap.of();
+  @Value.Default
+  default Map<String, String> properties() {
+    return ImmutableMap.of();
   }
 
   @Override
-  public String toString() {
-    return MoreObjects.toStringHelper(this)
-        .add("namespace", namespace)
-        .add("properties", properties)
-        .toString();
-  }
-
-  public static Builder builder() {
-    return new Builder();
-  }
-
-  public static class Builder {
-    private Namespace namespace;
-    private final ImmutableMap.Builder<String, String> properties = ImmutableMap.builder();
-
-    private Builder() {}
-
-    public Builder withNamespace(Namespace ns) {
-      Preconditions.checkNotNull(ns, "Invalid namespace: null");
-      this.namespace = ns;
-      return this;
-    }
-
-    public Builder setProperties(Map<String, String> props) {
-      Preconditions.checkNotNull(props, "Invalid collection of properties: null");
-      Preconditions.checkArgument(!props.containsKey(null), "Invalid property: null");
-      Preconditions.checkArgument(
-          !props.containsValue(null),
-          "Invalid value for properties %s: null",
-          Maps.filterValues(props, Objects::isNull).keySet());
-      properties.putAll(props);
-      return this;
-    }
-
-    public CreateNamespaceRequest build() {
-      return new CreateNamespaceRequest(namespace, properties.build());
-    }
+  default void validate() {
+    // nothing to do here as it's not possible to create an invalid CreateNamespaceRequest

Review Comment:
   Is this because a null namespace is caught in the parser?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org