[GitHub] [kafka] C0urante commented on a diff in pull request #13433: KAFKA-12694, KAFKA-3910: Add cyclic schema support, fix default struct values

["C0urante (via GitHub)" <gi...@apache.org>]

C0urante commented on code in PR #13433:
URL: https://github.com/apache/kafka/pull/13433#discussion_r1214775964


##########
connect/api/src/test/java/org/apache/kafka/connect/data/SchemaBuilderTest.java:
##########
@@ -36,6 +38,255 @@ public class SchemaBuilderTest {
     private static final String DOC = "doc";
     private static final Map<String, String> NO_PARAMS = null;
 
+    @Test
+    public void testDefaultValueStructSchema() {
+        SchemaBuilder builder = SchemaBuilder.struct()
+                .field("f1", Schema.BOOLEAN_SCHEMA);
+
+        Struct defaultValue = new Struct(builder.build()); // the Struct receives a schema, not a builder
+        defaultValue.put("f1", true);
+
+        builder.defaultValue(defaultValue)
+                .build();
+    }
+
+    @Test
+    public void testDefaultValueStructSchemaBuilder() {
+        SchemaBuilder builder = SchemaBuilder.struct()
+                .field("f1", Schema.BOOLEAN_SCHEMA);
+
+        Struct defaultValue = new Struct(builder);
+        defaultValue.put("f1", true);
+
+        builder.defaultValue(defaultValue).build();

Review Comment:
   > I think we should stick to one way of setting up struct defaults, so the end result of building a schema can be consistent
   
   The more I think of it, the more I agree with this. The inconsistencies aren't great, and allowing a default value to be used even though its schema is not equal to the schema it's defined on seems like a great chance for a footgun that burns people later.
   
   I think we should revert the changes in this PR that cause the `SchemaBuilder::testDefaultValueStructSchema` test case to pass and actively assert that that test cases throws an exception instead.
   
   We can also add a note to the `SchemaBuilder::defaultValue` Javadocs on how to use `Struct` instances as default values.
   
   Thoughts?



-- 
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: jira-unsubscribe@kafka.apache.org

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