kafka-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From gwens...@apache.org
Subject [kafka] branch trunk updated: KAFKA-8644; The Kafka protocol generator should allow null defaults for bytes and array fields
Date Thu, 11 Jul 2019 16:52:38 GMT
This is an automated email from the ASF dual-hosted git repository.

gwenshap pushed a commit to branch trunk
in repository https://gitbox.apache.org/repos/asf/kafka.git


The following commit(s) were added to refs/heads/trunk by this push:
     new d70fe5e  KAFKA-8644; The Kafka protocol generator should allow null defaults for
bytes and array fields
d70fe5e is described below

commit d70fe5ed2d25af432ec6ce02ccfeaa40bcd2f295
Author: Colin P. Mccabe <cmccabe@confluent.io>
AuthorDate: Thu Jul 11 09:52:01 2019 -0700

    KAFKA-8644; The Kafka protocol generator should allow null defaults for bytes and array
fields
    
    Author: Colin P. Mccabe <cmccabe@confluent.io>
    
    Reviewers: Stanislav Kozlovski, Gwen Shapira
    
    Closes #7059 from cmccabe/KAFKA-8644
---
 .../apache/kafka/message/MessageDataGenerator.java | 42 +++++++---
 .../kafka/message/MessageDataGeneratorTest.java    | 93 ++++++++++++++++++++++
 2 files changed, 124 insertions(+), 11 deletions(-)

diff --git a/generator/src/main/java/org/apache/kafka/message/MessageDataGenerator.java b/generator/src/main/java/org/apache/kafka/message/MessageDataGenerator.java
index ee0c3df..d0e5044 100644
--- a/generator/src/main/java/org/apache/kafka/message/MessageDataGenerator.java
+++ b/generator/src/main/java/org/apache/kafka/message/MessageDataGenerator.java
@@ -937,9 +937,17 @@ public final class MessageDataGenerator {
         buffer.incrementIndent();
         headerGenerator.addImport(MessageGenerator.UNSUPPORTED_VERSION_EXCEPTION_CLASS);
         if (field.type().isArray()) {
-            buffer.printf("if (!%s.isEmpty()) {%n", field.camelCaseName());
+            if (fieldDefault(field).equals("null")) {
+                buffer.printf("if (%s != null) {%n", field.camelCaseName());
+            } else {
+                buffer.printf("if (!%s.isEmpty()) {%n", field.camelCaseName());
+            }
         } else if (field.type().isBytes()) {
-            buffer.printf("if (%s.length != 0) {%n", field.camelCaseName());
+            if (fieldDefault(field).equals("null")) {
+                buffer.printf("if (%s != null) {%n", field.camelCaseName());
+            } else {
+                buffer.printf("if (%s.length != 0) {%n", field.camelCaseName());
+            }
         } else if (field.type().isString()) {
             if (fieldDefault(field).equals("null")) {
                 buffer.printf("if (%s != null) {%n", field.camelCaseName());
@@ -1221,19 +1229,19 @@ public final class MessageDataGenerator {
             }
         } else if (field.type() instanceof FieldType.StringFieldType) {
             if (field.defaultString().equals("null")) {
-                if (!(field.nullableVersions().contains(field.versions()))) {
-                    throw new RuntimeException("null cannot be the default for field " +
-                        field.name() + ", because not all versions of this field are " +
-                        "nullable.");
-                }
+                validateNullDefault(field);
                 return "null";
             } else {
                 return "\"" + field.defaultString() + "\"";
             }
         } else if (field.type().isBytes()) {
-            if (!field.defaultString().isEmpty()) {
+            if (field.defaultString().equals("null")) {
+                validateNullDefault(field);
+                return "null";
+            } else if (!field.defaultString().isEmpty()) {
                 throw new RuntimeException("Invalid default for bytes field " +
-                    field.name() + ": custom defaults are not supported for bytes fields.");
+                        field.name() + ".  The only valid default for a bytes field " +
+                        "is empty or null.");
             }
             headerGenerator.addImport(MessageGenerator.BYTES_CLASS);
             return "Bytes.EMPTY";
@@ -1244,9 +1252,13 @@ public final class MessageDataGenerator {
             }
             return "new " + field.type().toString() + "()";
         } else if (field.type().isArray()) {
-            if (!field.defaultString().isEmpty()) {
+            if (field.defaultString().equals("null")) {
+                validateNullDefault(field);
+                return "null";
+            } else if (!field.defaultString().isEmpty()) {
                 throw new RuntimeException("Invalid default for array field " +
-                    field.name() + ": custom defaults are not supported for array fields.");
+                    field.name() + ".  The only valid default for an array field " +
+                        "is the empty array or null.");
             }
             FieldType.ArrayType arrayType = (FieldType.ArrayType) field.type();
             if (structRegistry.isStructArrayWithKeys(field)) {
@@ -1260,6 +1272,14 @@ public final class MessageDataGenerator {
         }
     }
 
+    private void validateNullDefault(FieldSpec field) {
+        if (!(field.nullableVersions().contains(field.versions()))) {
+            throw new RuntimeException("null cannot be the default for field " +
+                    field.name() + ", because not all versions of this field are " +
+                    "nullable.");
+        }
+    }
+
     private void generateFieldAccessor(FieldSpec field) {
         buffer.printf("%n");
         generateAccessor(fieldAbstractJavaType(field), field.camelCaseName(),
diff --git a/generator/src/test/java/org/apache/kafka/message/MessageDataGeneratorTest.java
b/generator/src/test/java/org/apache/kafka/message/MessageDataGeneratorTest.java
new file mode 100644
index 0000000..e8fcaf2
--- /dev/null
+++ b/generator/src/test/java/org/apache/kafka/message/MessageDataGeneratorTest.java
@@ -0,0 +1,93 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.kafka.message;
+
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.Timeout;
+
+import java.util.Arrays;
+
+import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
+
+public class MessageDataGeneratorTest {
+    @Rule
+    final public Timeout globalTimeout = Timeout.millis(120000);
+
+    @Test
+    public void testNullDefaults() throws Exception {
+        MessageSpec testMessageSpec = MessageGenerator.JSON_SERDE.readValue(String.join("",
Arrays.asList(
+                "{",
+                "  \"type\": \"request\",",
+                "  \"name\": \"FooBar\",",
+                "  \"validVersions\": \"0-2\",",
+                "  \"fields\": [",
+                "    { \"name\": \"field1\", \"type\": \"int32\", \"versions\": \"0+\" },",
+                "    { \"name\": \"field2\", \"type\": \"[]TestStruct\", \"versions\": \"1+\",
",
+                "    \"nullableVersions\": \"1+\", \"default\": \"null\", \"fields\": [",
+                "      { \"name\": \"field1\", \"type\": \"int32\", \"versions\": \"0+\"
}",
+                "    ]},",
+                "    { \"name\": \"field3\", \"type\": \"bytes\", \"versions\": \"2+\", ",
+                "      \"nullableVersions\": \"2+\", \"default\": \"null\" }",
+                "  ]",
+                "}")), MessageSpec.class);
+        new MessageDataGenerator().generate(testMessageSpec);
+    }
+
+    @Test
+    public void testInvalidNullDefaultForInt() throws Exception {
+        MessageSpec testMessageSpec = MessageGenerator.JSON_SERDE.readValue(String.join("",
Arrays.asList(
+            "{",
+            "  \"type\": \"request\",",
+            "  \"name\": \"FooBar\",",
+            "  \"validVersions\": \"0-2\",",
+            "  \"fields\": [",
+            "    { \"name\": \"field1\", \"type\": \"int32\", \"versions\": \"0+\", \"default\":
\"null\" }",
+            "  ]",
+            "}")), MessageSpec.class);
+        try {
+            new MessageDataGenerator().generate(testMessageSpec);
+            fail("Expected MessageDataGenerator#generate to fail");
+        } catch (Throwable e) {
+            assertTrue("Invalid error message: " + e.getMessage(),
+                    e.getMessage().contains("Invalid default for int32"));
+        }
+    }
+
+    @Test
+    public void testInvalidNullDefaultForPotentiallyNonNullableArray() throws Exception {
+        MessageSpec testMessageSpec = MessageGenerator.JSON_SERDE.readValue(String.join("",
Arrays.asList(
+                "{",
+                "  \"type\": \"request\",",
+                "  \"name\": \"FooBar\",",
+                "  \"validVersions\": \"0-2\",",
+                "  \"fields\": [",
+                "    { \"name\": \"field1\", \"type\": \"[]int32\", \"versions\": \"0+\",
\"nullableVersions\": \"1+\", ",
+                "    \"default\": \"null\" }",
+                "  ]",
+                "}")), MessageSpec.class);
+        try {
+            new MessageDataGenerator().generate(testMessageSpec);
+            fail("Expected MessageDataGenerator#generate to fail");
+        } catch (RuntimeException e) {
+            assertTrue("Invalid error message: " + e.getMessage(),
+                    e.getMessage().contains("not all versions of this field are nullable"));
+        }
+    }
+}


Mime
View raw message