From 730ee86b6b3304cf3ab723658eb904037978e359 Mon Sep 17 00:00:00 2001 From: Nandor Kollar Date: Mon, 2 Oct 2017 09:58:16 +0200 Subject: [PATCH 1/2] AVRO-2078: Avro does not enforce schema resolution rules for Decimal type --- .../org/apache/avro/SchemaCompatibility.java | 36 ++++++++++- .../io/parsing/ResolvingGrammarGenerator.java | 33 ++++++++++ ...tesSchemaCompatibilityDecimalMismatch.java | 64 +++++++++++++++++++ ...xedSchemaCompatibilityDecimalMismatch.java | 64 +++++++++++++++++++ .../apache/avro/TestSchemaCompatibility.java | 16 +++++ .../java/org/apache/avro/TestSchemas.java | 14 ++++ .../TestResolvingGrammarGenerator2.java | 52 +++++++++++++++ 7 files changed, 277 insertions(+), 2 deletions(-) create mode 100644 lang/java/avro/src/test/java/org/apache/avro/TestBytesSchemaCompatibilityDecimalMismatch.java create mode 100644 lang/java/avro/src/test/java/org/apache/avro/TestFixedSchemaCompatibilityDecimalMismatch.java diff --git a/lang/java/avro/src/main/java/org/apache/avro/SchemaCompatibility.java b/lang/java/avro/src/main/java/org/apache/avro/SchemaCompatibility.java index 4b454f6eb70..9a0298c6911 100644 --- a/lang/java/avro/src/main/java/org/apache/avro/SchemaCompatibility.java +++ b/lang/java/avro/src/main/java/org/apache/avro/SchemaCompatibility.java @@ -269,6 +269,8 @@ private SchemaCompatibilityResult calculateCompatibility( case FLOAT: case DOUBLE: case BYTES: + return isDecimal(reader, writer) ? + checkDecimalScaleAndPrecision(reader, writer) : SchemaCompatibilityResult.compatible(); case STRING: { return SchemaCompatibilityResult.compatible(); } @@ -283,7 +285,12 @@ private SchemaCompatibilityResult calculateCompatibility( if (nameCheck.getCompatibility() == SchemaCompatibilityType.INCOMPATIBLE) { return nameCheck; } - return checkFixedSize(reader, writer); + SchemaCompatibilityResult fixedCheck = checkFixedSize(reader, writer); + if (fixedCheck.getCompatibility() == SchemaCompatibilityType.INCOMPATIBLE) { + return fixedCheck; + } + return isDecimal(reader, writer) ? + checkDecimalScaleAndPrecision(reader, writer) : SchemaCompatibilityResult.compatible(); } case ENUM: { SchemaCompatibilityResult nameCheck = checkSchemaNames(reader, writer); @@ -393,6 +400,30 @@ private SchemaCompatibilityResult calculateCompatibility( } } + private SchemaCompatibilityResult checkDecimalScaleAndPrecision(Schema reader, Schema writer) { + LogicalTypes.Decimal readerLogicalType = (LogicalTypes.Decimal) reader.getLogicalType(); + LogicalTypes.Decimal writerLogicalType = (LogicalTypes.Decimal) writer.getLogicalType(); + if (readerLogicalType.getScale() == writerLogicalType.getScale() + && readerLogicalType.getPrecision() == writerLogicalType.getPrecision()) { + return SchemaCompatibilityResult.compatible(); + } + return SchemaCompatibilityResult.incompatible( + SchemaIncompatibilityType.DECIMAL_SCALE_OR_PRECISION_MISMATCH, + reader, writer, + String.format( + "Decimal (precision,scale) doesn't match for reader (%s,%s) and writer (%s,%s) schemas", + ((LogicalTypes.Decimal) reader.getLogicalType()).getPrecision(), + ((LogicalTypes.Decimal) reader.getLogicalType()).getScale(), + ((LogicalTypes.Decimal) writer.getLogicalType()).getPrecision(), + ((LogicalTypes.Decimal) writer.getLogicalType()).getScale()) + ); + } + + private boolean isDecimal(Schema reader, Schema writer) { + return reader.getLogicalType() instanceof LogicalTypes.Decimal + && writer.getLogicalType() instanceof LogicalTypes.Decimal; + } + private SchemaCompatibilityResult checkReaderWriterRecordFields(final Schema reader, final Schema writer) { // Check that each field in the reader record can be populated from the writer record: @@ -473,7 +504,8 @@ public enum SchemaIncompatibilityType { MISSING_ENUM_SYMBOLS, READER_FIELD_MISSING_DEFAULT_VALUE, TYPE_MISMATCH, - MISSING_UNION_BRANCH; + MISSING_UNION_BRANCH, + DECIMAL_SCALE_OR_PRECISION_MISMATCH } /** diff --git a/lang/java/avro/src/main/java/org/apache/avro/io/parsing/ResolvingGrammarGenerator.java b/lang/java/avro/src/main/java/org/apache/avro/io/parsing/ResolvingGrammarGenerator.java index 54073272fd8..730a5d3e03a 100644 --- a/lang/java/avro/src/main/java/org/apache/avro/io/parsing/ResolvingGrammarGenerator.java +++ b/lang/java/avro/src/main/java/org/apache/avro/io/parsing/ResolvingGrammarGenerator.java @@ -26,6 +26,7 @@ import java.util.Map; import org.apache.avro.AvroTypeException; +import org.apache.avro.LogicalTypes; import org.apache.avro.Schema; import org.apache.avro.Schema.Field; import org.apache.avro.io.Encoder; @@ -86,10 +87,22 @@ public Symbol generate(Schema writer, Schema reader, case STRING: return Symbol.STRING; case BYTES: + if (decimalReaderWriter(reader, writer) && !decimalScaleAndPrecisionMatch(reader, writer)) { + return scalePrecisionMismatchError(reader.getFullName(), + writer.getFullName(), + (LogicalTypes.Decimal) reader.getLogicalType(), + (LogicalTypes.Decimal) writer.getLogicalType()); + } return Symbol.BYTES; case FIXED: if (writer.getFullName().equals(reader.getFullName()) && writer.getFixedSize() == reader.getFixedSize()) { + if (decimalReaderWriter(reader, writer) && !decimalScaleAndPrecisionMatch(reader, writer)) { + return scalePrecisionMismatchError(reader.getFullName(), + writer.getFullName(), + (LogicalTypes.Decimal) reader.getLogicalType(), + (LogicalTypes.Decimal) writer.getLogicalType()); + } return Symbol.seq(Symbol.intCheckAction(writer.getFixedSize()), Symbol.FIXED); } @@ -189,6 +202,26 @@ public Symbol generate(Schema writer, Schema reader, + ", expecting " + reader.getFullName()); } + private boolean decimalReaderWriter(Schema reader, Schema writer) { + return reader.getLogicalType() instanceof LogicalTypes.Decimal + && writer.getLogicalType() instanceof LogicalTypes.Decimal; + } + + private boolean decimalScaleAndPrecisionMatch(Schema reader, Schema writer) { + LogicalTypes.Decimal readerLogicalType = (LogicalTypes.Decimal) reader.getLogicalType(); + LogicalTypes.Decimal writerLogicalType = (LogicalTypes.Decimal) writer.getLogicalType(); + return readerLogicalType.getPrecision() == writerLogicalType.getPrecision() + && readerLogicalType.getScale() == writerLogicalType.getScale(); + } + + private Symbol scalePrecisionMismatchError(String readerFullName, String writerFullName, + LogicalTypes.Decimal readerLogicalType, LogicalTypes.Decimal writerLogicalType) { + return Symbol.error("Found " + writerFullName + + " with decimal logical type (" + writerLogicalType.getPrecision() + "," + writerLogicalType.getScale() + ")" + + ", expecting " + readerFullName + + " with decimal logical type (" + readerLogicalType.getPrecision() + "," + readerLogicalType.getScale() + ")" + ); + } private Symbol resolveUnion(Schema writer, Schema reader, Map seen) throws IOException { List alts = writer.getTypes(); diff --git a/lang/java/avro/src/test/java/org/apache/avro/TestBytesSchemaCompatibilityDecimalMismatch.java b/lang/java/avro/src/test/java/org/apache/avro/TestBytesSchemaCompatibilityDecimalMismatch.java new file mode 100644 index 00000000000..2c6e07237b5 --- /dev/null +++ b/lang/java/avro/src/test/java/org/apache/avro/TestBytesSchemaCompatibilityDecimalMismatch.java @@ -0,0 +1,64 @@ +/** + * 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.avro; + +import org.apache.avro.SchemaCompatibility.SchemaIncompatibilityType; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.Parameterized; +import org.junit.runners.Parameterized.Parameter; +import org.junit.runners.Parameterized.Parameters; + +import java.util.ArrayList; +import java.util.List; + +import static org.apache.avro.TestSchemaCompatibility.validateIncompatibleSchemas; +import static org.apache.avro.TestSchemas.BYTES_DECIMAL_3_2; +import static org.apache.avro.TestSchemas.BYTES_DECIMAL_3_3; +import static org.apache.avro.TestSchemas.BYTES_DECIMAL_4_3; + +@RunWith(Parameterized.class) +public class TestBytesSchemaCompatibilityDecimalMismatch { + + @Parameters(name = "r: {0} | w: {1}") + public static Iterable data() { + Object[][] fields = { + { BYTES_DECIMAL_3_3 , BYTES_DECIMAL_3_2, + "Decimal (precision,scale) doesn't match for reader (3,3) and writer (3,2) schemas" }, + { BYTES_DECIMAL_3_3 , BYTES_DECIMAL_4_3, + "Decimal (precision,scale) doesn't match for reader (3,3) and writer (4,3) schemas" } + }; + List list = new ArrayList<>(fields.length); + for (Object[] schemas : fields) { + list.add(schemas); + } + return list; + } + + @Parameter(0) + public Schema reader; + @Parameter(1) + public Schema writer; + @Parameter(2) + public String details; + + @Test + public void testTypeMismatchSchemas() throws Exception { + validateIncompatibleSchemas(reader, writer, SchemaIncompatibilityType.DECIMAL_SCALE_OR_PRECISION_MISMATCH, details); + } +} diff --git a/lang/java/avro/src/test/java/org/apache/avro/TestFixedSchemaCompatibilityDecimalMismatch.java b/lang/java/avro/src/test/java/org/apache/avro/TestFixedSchemaCompatibilityDecimalMismatch.java new file mode 100644 index 00000000000..db33e507f2a --- /dev/null +++ b/lang/java/avro/src/test/java/org/apache/avro/TestFixedSchemaCompatibilityDecimalMismatch.java @@ -0,0 +1,64 @@ +/** + * 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.avro; + +import org.apache.avro.SchemaCompatibility.SchemaIncompatibilityType; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.Parameterized; +import org.junit.runners.Parameterized.Parameter; +import org.junit.runners.Parameterized.Parameters; + +import java.util.ArrayList; +import java.util.List; + +import static org.apache.avro.TestSchemaCompatibility.validateIncompatibleSchemas; +import static org.apache.avro.TestSchemas.FIXED_DECIMAL_3_2; +import static org.apache.avro.TestSchemas.FIXED_DECIMAL_3_3; +import static org.apache.avro.TestSchemas.FIXED_DECIMAL_4_3; + +@RunWith(Parameterized.class) +public class TestFixedSchemaCompatibilityDecimalMismatch { + + @Parameters(name = "r: {0} | w: {1}") + public static Iterable data() { + Object[][] fields = { + { FIXED_DECIMAL_3_3 , FIXED_DECIMAL_3_2, + "Decimal (precision,scale) doesn't match for reader (3,3) and writer (3,2) schemas" }, + { FIXED_DECIMAL_3_3 , FIXED_DECIMAL_4_3, + "Decimal (precision,scale) doesn't match for reader (3,3) and writer (4,3) schemas" } + }; + List list = new ArrayList<>(fields.length); + for (Object[] schemas : fields) { + list.add(schemas); + } + return list; + } + + @Parameter(0) + public Schema reader; + @Parameter(1) + public Schema writer; + @Parameter(2) + public String details; + + @Test + public void testTypeMismatchSchemas() throws Exception { + validateIncompatibleSchemas(reader, writer, SchemaIncompatibilityType.DECIMAL_SCALE_OR_PRECISION_MISMATCH, details); + } +} diff --git a/lang/java/avro/src/test/java/org/apache/avro/TestSchemaCompatibility.java b/lang/java/avro/src/test/java/org/apache/avro/TestSchemaCompatibility.java index 6b38cbd02e4..6ba834f37cd 100644 --- a/lang/java/avro/src/test/java/org/apache/avro/TestSchemaCompatibility.java +++ b/lang/java/avro/src/test/java/org/apache/avro/TestSchemaCompatibility.java @@ -25,6 +25,7 @@ import static org.apache.avro.TestSchemas.A_INT_RECORD1; import static org.apache.avro.TestSchemas.A_LONG_RECORD1; import static org.apache.avro.TestSchemas.BOOLEAN_SCHEMA; +import static org.apache.avro.TestSchemas.BYTES_DECIMAL_3_3; import static org.apache.avro.TestSchemas.BYTES_SCHEMA; import static org.apache.avro.TestSchemas.BYTES_UNION_SCHEMA; import static org.apache.avro.TestSchemas.DOUBLE_SCHEMA; @@ -35,6 +36,7 @@ import static org.apache.avro.TestSchemas.ENUM1_AB_SCHEMA; import static org.apache.avro.TestSchemas.ENUM1_BC_SCHEMA; import static org.apache.avro.TestSchemas.FIXED_4_BYTES; +import static org.apache.avro.TestSchemas.FIXED_DECIMAL_3_3; import static org.apache.avro.TestSchemas.FLOAT_SCHEMA; import static org.apache.avro.TestSchemas.FLOAT_UNION_SCHEMA; import static org.apache.avro.TestSchemas.INT_ARRAY_SCHEMA; @@ -515,4 +517,18 @@ public void testReaderWriterDecodingCompatibility() throws Exception { expectedDecodedDatum, decodedDatum); } } + + @Test + public void testBytesDecimalWithSameScaleAndPrecision() { + final SchemaPairCompatibility result = + checkReaderWriterCompatibility(BYTES_DECIMAL_3_3, BYTES_DECIMAL_3_3); + assertEquals(SchemaCompatibilityType.COMPATIBLE, result.getType()); + } + + @Test + public void testFixedDecimalWithSameScaleAndPrecision() { + final SchemaPairCompatibility result = + checkReaderWriterCompatibility(FIXED_DECIMAL_3_3, FIXED_DECIMAL_3_3); + assertEquals(SchemaCompatibilityType.COMPATIBLE, result.getType()); + } } diff --git a/lang/java/avro/src/test/java/org/apache/avro/TestSchemas.java b/lang/java/avro/src/test/java/org/apache/avro/TestSchemas.java index 264e7e64b22..28a1c1eb581 100644 --- a/lang/java/avro/src/test/java/org/apache/avro/TestSchemas.java +++ b/lang/java/avro/src/test/java/org/apache/avro/TestSchemas.java @@ -77,6 +77,20 @@ public class TestSchemas { static final Schema FIXED_4_BYTES = Schema.createFixed("Fixed", null, null, 4); static final Schema FIXED_8_BYTES = Schema.createFixed("Fixed", null, null, 8); + static final Schema BYTES_DECIMAL_3_3 = + LogicalTypes.decimal(3, 3).addToSchema(Schema.create(Schema.Type.BYTES)); + static final Schema BYTES_DECIMAL_3_2 = + LogicalTypes.decimal(3, 2).addToSchema(Schema.create(Schema.Type.BYTES)); + static final Schema BYTES_DECIMAL_4_3 = + LogicalTypes.decimal(4, 3).addToSchema(Schema.create(Schema.Type.BYTES)); + + static final Schema FIXED_DECIMAL_3_3 = + LogicalTypes.decimal(3, 3).addToSchema(Schema.createFixed("Fixed", null, null, 8)); + static final Schema FIXED_DECIMAL_3_2 = + LogicalTypes.decimal(3, 2).addToSchema(Schema.createFixed("Fixed", null, null, 8)); + static final Schema FIXED_DECIMAL_4_3 = + LogicalTypes.decimal(4, 3).addToSchema(Schema.createFixed("Fixed", null, null, 8)); + static { EMPTY_RECORD1.setFields(Collections.emptyList()); EMPTY_RECORD2.setFields(Collections.emptyList()); diff --git a/lang/java/avro/src/test/java/org/apache/avro/io/parsing/TestResolvingGrammarGenerator2.java b/lang/java/avro/src/test/java/org/apache/avro/io/parsing/TestResolvingGrammarGenerator2.java index ea9ed1a5f63..a597cb52292 100644 --- a/lang/java/avro/src/test/java/org/apache/avro/io/parsing/TestResolvingGrammarGenerator2.java +++ b/lang/java/avro/src/test/java/org/apache/avro/io/parsing/TestResolvingGrammarGenerator2.java @@ -18,6 +18,8 @@ package org.apache.avro.io.parsing; import java.util.Arrays; + +import org.apache.avro.LogicalTypes; import org.apache.avro.SchemaBuilder; import org.apache.avro.SchemaValidationException; import org.apache.avro.SchemaValidatorBuilder; @@ -66,6 +68,20 @@ public class TestResolvingGrammarGenerator2 { .name("z").type().doubleType().doubleDefault(0.0) .endRecord(); + private static final Schema FIXED_DECIMAL_3_3_SCHEMA = + LogicalTypes.decimal(3, 3).addToSchema(Schema.createFixed("decimal", "", "", 5)); + private static final Schema FIXED_DECIMAL_4_3_SCHEMA = + LogicalTypes.decimal(4, 3).addToSchema(Schema.createFixed("decimal", "", "", 5)); + private static final Schema FIXED_DECIMAL_3_2_SCHEMA = + LogicalTypes.decimal(3, 2).addToSchema(Schema.createFixed("decimal", "", "", 5)); + + private static final Schema BYTES_DECIMAL_3_3_SCHEMA = + LogicalTypes.decimal(3, 3).addToSchema(Schema.create(Schema.Type.BYTES)); + private static final Schema BYTES_DECIMAL_4_3_SCHEMA = + LogicalTypes.decimal(4, 3).addToSchema(Schema.create(Schema.Type.BYTES)); + private static final Schema BYTES_DECIMAL_3_2_SCHEMA = + LogicalTypes.decimal(3, 2).addToSchema(Schema.create(Schema.Type.BYTES)); + @Test(expected=SchemaValidationException.class) public void testUnionResolutionNoStructureMatch() throws Exception { // there is a short name match, but the structure does not match @@ -140,4 +156,40 @@ public void testUnionResolutionFullNameMatch() throws Exception { grammar.production[1]; Assert.assertEquals(4, action.rindex); } + + @Test(expected=SchemaValidationException.class) + public void testFixedDecimalWithDifferentPrecision() throws Exception { + new SchemaValidatorBuilder().canBeReadStrategy().validateAll() + .validate(FIXED_DECIMAL_3_3_SCHEMA, Arrays.asList(FIXED_DECIMAL_4_3_SCHEMA)); + } + + @Test(expected=SchemaValidationException.class) + public void testFixedDecimalWithDifferentWithDifferentScale() throws Exception { + new SchemaValidatorBuilder().canBeReadStrategy().validateAll() + .validate(FIXED_DECIMAL_3_3_SCHEMA, Arrays.asList(FIXED_DECIMAL_3_2_SCHEMA)); + } + + @Test + public void testFixedDecimalWithSameScaleAndPrecision() throws Exception { + new SchemaValidatorBuilder().canBeReadStrategy().validateAll() + .validate(FIXED_DECIMAL_3_3_SCHEMA, Arrays.asList(FIXED_DECIMAL_3_3_SCHEMA)); + } + + @Test(expected=SchemaValidationException.class) + public void testBytesDecimalWithDifferentPrecision() throws Exception { + new SchemaValidatorBuilder().canBeReadStrategy().validateAll() + .validate(BYTES_DECIMAL_3_3_SCHEMA, Arrays.asList(BYTES_DECIMAL_4_3_SCHEMA)); + } + + @Test(expected=SchemaValidationException.class) + public void testBytesDecimalWithDifferentWithDifferentScale() throws Exception { + new SchemaValidatorBuilder().canBeReadStrategy().validateAll() + .validate(BYTES_DECIMAL_3_3_SCHEMA, Arrays.asList(BYTES_DECIMAL_3_2_SCHEMA)); + } + + @Test + public void testBytesDecimalWithSameScaleAndPrecision() throws Exception { + new SchemaValidatorBuilder().canBeReadStrategy().validateAll() + .validate(BYTES_DECIMAL_3_3_SCHEMA, Arrays.asList(BYTES_DECIMAL_3_3_SCHEMA)); + } } From 73783c48eaf90b919bcafdbdc91926ff75acb5d3 Mon Sep 17 00:00:00 2001 From: Nandor Kollar Date: Tue, 12 Dec 2017 12:45:53 +0100 Subject: [PATCH 2/2] Remove unused method --- .../test/java/org/apache/avro/TestSchemaCompatibility.java | 7 ------- 1 file changed, 7 deletions(-) diff --git a/lang/java/avro/src/test/java/org/apache/avro/TestSchemaCompatibility.java b/lang/java/avro/src/test/java/org/apache/avro/TestSchemaCompatibility.java index ba31454a910..734cb43a7c1 100644 --- a/lang/java/avro/src/test/java/org/apache/avro/TestSchemaCompatibility.java +++ b/lang/java/avro/src/test/java/org/apache/avro/TestSchemaCompatibility.java @@ -565,11 +565,4 @@ public void testFixedDecimalWithSameScaleAndPrecision() { assertEquals(SchemaCompatibilityType.COMPATIBLE, result.getType()); } - Deque asDeqeue(String... args) { - Deque dq = new ArrayDeque(); - List x = Arrays.asList(args); - Collections.reverse(x); - dq.addAll(x); - return dq; - } }