From d05d0c6240217da9cc72c07110ed273f37b0fa21 Mon Sep 17 00:00:00 2001 From: Denes Arvay Date: Sat, 17 Oct 2020 21:31:37 +0200 Subject: [PATCH] NIFI-7925: ValidateRecord reports false positive for avro arrays with null elements --- flow.xml.gz | Bin 0 -> 2050 bytes .../serialization/record/RecordFieldType.java | 30 +++++++++++++- .../record/type/ArrayDataType.java | 37 +++++++++++------- .../record/type/MapDataType.java | 37 +++++++++++------- .../org/apache/nifi/avro/AvroTypeUtil.java | 7 +++- .../validation/StandardSchemaValidator.java | 18 +++++++-- .../standard/TestValidateRecord.java | 34 ++++++++++++++++ .../array-and-map-with-null-element.avro | Bin 0 -> 281 bytes 8 files changed, 125 insertions(+), 38 deletions(-) create mode 100644 flow.xml.gz create mode 100644 nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/test/resources/TestValidateRecord/array-and-map-with-null-element.avro diff --git a/flow.xml.gz b/flow.xml.gz new file mode 100644 index 0000000000000000000000000000000000000000..5f4d61a57f7563513612b01e253a4da01deaf14b GIT binary patch literal 2050 zcmV+d2>tgTiwFP!000000PR~_bK*J@exG0AO6{xh1!J2khEpVv8O}+71v2xP5>}h| zjIk(LWb*t6=b*p7se>^UHX$3U&NbvbWtEm?f1P;Mo zF#mjUH@%ka3n^qMz{n>7e7*?C#UJfYE!QVMuSmcs@qM5>o{Od%T-f(8i>D1_ddFxfe3(L8!#-C8B5n45*MA9$DH= z31MF-iI$?p+a7M~rU{y58nUWc4OvrFRYq<@mtDs;^qCl`yDf!JOeg}rY?fv(!y}^CwTc>~1xxDFZ*4dh!icE33BbiRPoZK4N7#u=zQ7>DX(Tz+uhEdQCpB1vTpPpZZ zzC?$Q!mv%~yoQ#@d4O8rxn3>bLs&~Jm*U!%FbQF{%#3Tn&xPY1^VlM`)@>>CjiJQT z5M`c=9M%@Ya>h#z5N#>>*k*d_8*Qzo)oZFe8FWUIufu6e$p$icl(Im=4e~>0QxyIP zQ~TfU%@xOG#*xn*as<>PxUEVdI4xzTKR*u$kncSqaVF_dJ*BA_%Ba>J_&Dv;lG(+s zje0poFfX`)75MG%o$)~Iu-KQ)^hp>_2?rF!z1T&52rZ@5x3xjQ6NVUYOCOF#!bi6H zcl9x0ln>0;?P>qEH@@$V``>zlqGIw187(}56N$|a2!?j&=BA~TyRwNi3c3-E^)x^M z3ELdrcKULKQ?lgtxGtDS>>xBYP+4FZ&@HD?wuzowU_egMsH0yO<#88NBraldASuJ^dJau;%&Q7{DDGaR3AIG{-R z+oLE{2L0>4Y&G@9GyZ-+2;n3W|7QoKashsN>>=PsC@<04CkWTjijvbAju3mOWcY$L zE)jDcq>1+gdt?@Q?WqF(1+bJRB%)5Nn0+jcfY}2~cor1V5Tr>o!>Ivy=OFqah4cFX zH+HGdlWxiv*~27-fK$2j=K-N0ef2Q6)L3tCvzuEl@D|Y`<{)+E`%dS~-Q1$b16)oO z_kh8w(&Z~jyUXPC=nsz3Gz-zc4yGzws*5$%)m&RQcBB6{9zZZh)o1SQ36+WP_rO` z+B8hLVO!X>?O8)J&wzF>SP6HPU+3y1j&^}$ctIYXOB)bAF-X=c~1xJU;Y`p}T zi|qX1zWpYQR!L~#9xh{G$n!%PG0uDzF*tN`8@khZFOq;SSIFQ)FlP^Y(_#^_UsxJ& zqaY|?b(|T@R2$)9G1Ag3g9DMN>sZ5<0eUGjD8_wFSM4C-SM?iMfA@tW_+_!LpNqm{P^#iY%6tqk|A3 zWpg#W9gVr&lvot!rT)F`(g%FJ>1svG(i$$vR@1R?6X`ZIcV9YQ;^P_zitOfL*e`K% z^~1@3Unh5Yc#J4~IQg&QnxeTaFB-*#xX>PF>qAA6B`z^2%O%#FOSP=%q(}|MCCx(!ySja?~)2 z>oOm%`j5M6`n}i$=*n}Tn$QJeCrc|n zo_~!sLWUO0a~B#>U~DaosHlVdIT;T2C@_6wKiOQOWt?lu;={0g4|xHs#G7h5^>k(= zV#|ohF<|e1%}Zw?O40g^__|4=Ww+ue3V0fvp}!&null if this RecordFieldType * is not the ARRAY type. */ public DataType getArrayDataType(final DataType elementType) { + return getArrayDataType(elementType, ArrayDataType.DEFAULT_NULLABLE); + } + + /** + * Returns a Data Type that represents an "ARRAY" type with the given element type. + * + * @param elementType the type of the arrays in the element + * @param elementsNullable indicates whether the array can contain null elements + * @return a DataType that represents an Array with the given element type, or null if this RecordFieldType + * is not the ARRAY type. + */ + public DataType getArrayDataType(final DataType elementType, final boolean elementsNullable) { if (this != ARRAY) { return null; } - return new ArrayDataType(elementType); + return new ArrayDataType(elementType, elementsNullable); } @@ -341,17 +354,30 @@ public enum RecordFieldType { /** * Returns a Data Type that represents a "MAP" type with the given value type. + * The returned map data type can't contain null values. * * @param valueDataType the type of the values in the map * @return a DataType that represents a Map with the given value type, or null if this RecordFieldType * is not the MAP type. */ public DataType getMapDataType(final DataType valueDataType) { + return getMapDataType(valueDataType, MapDataType.DEFAULT_NULLABLE); + } + + /** + * Returns a Data Type that represents a "MAP" type with the given value type. + * + * @param valueDataType the type of the values in the map + * @param valuesNullable indicates whether the map can contain null values + * @return a DataType that represents a Map with the given value type, or null if this RecordFieldType + * is not the MAP type. + */ + public DataType getMapDataType(final DataType valueDataType, boolean valuesNullable) { if (this != MAP) { return null; } - return new MapDataType(valueDataType); + return new MapDataType(valueDataType, valuesNullable); } /** diff --git a/nifi-commons/nifi-record/src/main/java/org/apache/nifi/serialization/record/type/ArrayDataType.java b/nifi-commons/nifi-record/src/main/java/org/apache/nifi/serialization/record/type/ArrayDataType.java index f9f2569c2c..26f491654a 100644 --- a/nifi-commons/nifi-record/src/main/java/org/apache/nifi/serialization/record/type/ArrayDataType.java +++ b/nifi-commons/nifi-record/src/main/java/org/apache/nifi/serialization/record/type/ArrayDataType.java @@ -23,11 +23,20 @@ import org.apache.nifi.serialization.record.RecordFieldType; import java.util.Objects; public class ArrayDataType extends DataType { + + public static final boolean DEFAULT_NULLABLE = false; + private final DataType elementType; + private final boolean elementsNullable; public ArrayDataType(final DataType elementType) { + this(elementType, DEFAULT_NULLABLE); + } + + public ArrayDataType(final DataType elementType, boolean elementsNullable) { super(RecordFieldType.ARRAY, null); this.elementType = elementType; + this.elementsNullable = elementsNullable; } public DataType getElementType() { @@ -39,25 +48,23 @@ public class ArrayDataType extends DataType { return RecordFieldType.ARRAY; } - @Override - public int hashCode() { - return 31 + 41 * getFieldType().hashCode() + 41 * (elementType == null ? 0 : elementType.hashCode()); + public boolean isElementsNullable() { + return elementsNullable; } @Override - public boolean equals(final Object obj) { - if (obj == this) { - return true; - } - if (obj == null) { - return false; - } - if (!(obj instanceof ArrayDataType)) { - return false; - } + public boolean equals(Object o) { + if (this == o) return true; + if (!(o instanceof ArrayDataType)) return false; + if (!super.equals(o)) return false; + ArrayDataType that = (ArrayDataType) o; + return isElementsNullable() == that.isElementsNullable() + && Objects.equals(getElementType(), that.getElementType()); + } - final ArrayDataType other = (ArrayDataType) obj; - return Objects.equals(elementType, other.elementType); + @Override + public int hashCode() { + return Objects.hash(super.hashCode(), getElementType(), isElementsNullable()); } @Override diff --git a/nifi-commons/nifi-record/src/main/java/org/apache/nifi/serialization/record/type/MapDataType.java b/nifi-commons/nifi-record/src/main/java/org/apache/nifi/serialization/record/type/MapDataType.java index 6435195c4b..74c068325e 100644 --- a/nifi-commons/nifi-record/src/main/java/org/apache/nifi/serialization/record/type/MapDataType.java +++ b/nifi-commons/nifi-record/src/main/java/org/apache/nifi/serialization/record/type/MapDataType.java @@ -23,41 +23,48 @@ import org.apache.nifi.serialization.record.RecordFieldType; import java.util.Objects; public class MapDataType extends DataType { + + public static final boolean DEFAULT_NULLABLE = false; + private final DataType valueType; + private final boolean valuesNullable; public MapDataType(final DataType elementType) { + this(elementType, DEFAULT_NULLABLE); + } + + public MapDataType(final DataType elementType, boolean valuesNullable) { super(RecordFieldType.MAP, null); this.valueType = elementType; + this.valuesNullable = valuesNullable; } public DataType getValueType() { return valueType; } + public boolean isValuesNullable() { + return valuesNullable; + } + @Override public RecordFieldType getFieldType() { return RecordFieldType.MAP; } @Override - public int hashCode() { - return 31 + 41 * getFieldType().hashCode() + 41 * (valueType == null ? 0 : valueType.hashCode()); + public boolean equals(Object o) { + if (this == o) return true; + if (!(o instanceof MapDataType)) return false; + if (!super.equals(o)) return false; + MapDataType that = (MapDataType) o; + return valuesNullable == that.valuesNullable + && Objects.equals(getValueType(), that.getValueType()); } @Override - public boolean equals(final Object obj) { - if (obj == this) { - return true; - } - if (obj == null) { - return false; - } - if (!(obj instanceof MapDataType)) { - return false; - } - - final MapDataType other = (MapDataType) obj; - return Objects.equals(valueType, other.valueType); + public int hashCode() { + return Objects.hash(super.hashCode(), getValueType(), valuesNullable); } @Override diff --git a/nifi-nar-bundles/nifi-extension-utils/nifi-record-utils/nifi-avro-record-utils/src/main/java/org/apache/nifi/avro/AvroTypeUtil.java b/nifi-nar-bundles/nifi-extension-utils/nifi-record-utils/nifi-avro-record-utils/src/main/java/org/apache/nifi/avro/AvroTypeUtil.java index 3919efaf61..6d3157c121 100644 --- a/nifi-nar-bundles/nifi-extension-utils/nifi-record-utils/nifi-avro-record-utils/src/main/java/org/apache/nifi/avro/AvroTypeUtil.java +++ b/nifi-nar-bundles/nifi-extension-utils/nifi-record-utils/nifi-avro-record-utils/src/main/java/org/apache/nifi/avro/AvroTypeUtil.java @@ -354,7 +354,9 @@ public class AvroTypeUtil { switch (avroType) { case ARRAY: - return RecordFieldType.ARRAY.getArrayDataType(determineDataType(avroSchema.getElementType(), knownRecordTypes)); + final DataType elementType = determineDataType(avroSchema.getElementType(), knownRecordTypes); + final boolean elementsNullable = isNullable(avroSchema.getElementType()); + return RecordFieldType.ARRAY.getArrayDataType(elementType, elementsNullable); case BYTES: case FIXED: return RecordFieldType.ARRAY.getArrayDataType(RecordFieldType.BYTE.getDataType()); @@ -403,7 +405,8 @@ public class AvroTypeUtil { case MAP: final Schema valueSchema = avroSchema.getValueType(); final DataType valueType = determineDataType(valueSchema, knownRecordTypes); - return RecordFieldType.MAP.getMapDataType(valueType); + final boolean valuesNullable = isNullable(valueSchema); + return RecordFieldType.MAP.getMapDataType(valueType, valuesNullable); case UNION: { final List nonNullSubSchemas = getNonNullSubSchemas(avroSchema); diff --git a/nifi-nar-bundles/nifi-extension-utils/nifi-record-utils/nifi-standard-record-utils/src/main/java/org/apache/nifi/schema/validation/StandardSchemaValidator.java b/nifi-nar-bundles/nifi-extension-utils/nifi-record-utils/nifi-standard-record-utils/src/main/java/org/apache/nifi/schema/validation/StandardSchemaValidator.java index 18b7c7a5ec..0f5384b690 100644 --- a/nifi-nar-bundles/nifi-extension-utils/nifi-record-utils/nifi-standard-record-utils/src/main/java/org/apache/nifi/schema/validation/StandardSchemaValidator.java +++ b/nifi-nar-bundles/nifi-extension-utils/nifi-record-utils/nifi-standard-record-utils/src/main/java/org/apache/nifi/schema/validation/StandardSchemaValidator.java @@ -69,7 +69,7 @@ public class StandardSchemaValidator implements RecordSchemaValidator { if (validationContext.isStrictTypeChecking()) { if (!isTypeCorrect(rawValue, dataType)) { result.addValidationError(new StandardValidationError(concat(fieldPrefix, field), rawValue, ValidationErrorType.INVALID_FIELD, - "Value is of type " + rawValue.getClass().getName() + " but was expected to be of type " + dataType)); + "Value is of type " + classNameOrNull(rawValue) + " but was expected to be of type " + dataType)); continue; } @@ -78,7 +78,7 @@ public class StandardSchemaValidator implements RecordSchemaValidator { // but will be false if the value is "123" and should be an Array or Record. if (!DataTypeUtils.isCompatibleDataType(rawValue, dataType)) { result.addValidationError(new StandardValidationError(concat(fieldPrefix, field), rawValue, ValidationErrorType.INVALID_FIELD, - "Value is of type " + rawValue.getClass().getName() + " but was expected to be of type " + dataType)); + "Value is of type " + classNameOrNull(rawValue) + " but was expected to be of type " + dataType)); continue; } @@ -140,7 +140,7 @@ public class StandardSchemaValidator implements RecordSchemaValidator { if (canonicalDataType == null) { result.addValidationError(new StandardValidationError(concat(fieldPrefix, field), rawValue, ValidationErrorType.INVALID_FIELD, - "Value is of type " + rawValue.getClass().getName() + " but was expected to be of type " + dataType)); + "Value is of type " + classNameOrNull(rawValue) + " but was expected to be of type " + dataType)); return null; } @@ -157,7 +157,7 @@ public class StandardSchemaValidator implements RecordSchemaValidator { if (canonicalDataType.getFieldType() == RecordFieldType.RECORD) { if (!(rawValue instanceof Record)) { // sanity check result.addValidationError(new StandardValidationError(concat(fieldPrefix, field), rawValue, ValidationErrorType.INVALID_FIELD, - "Value is of type " + rawValue.getClass().getName() + " but was expected to be of type " + expectedDataType)); + "Value is of type " + classNameOrNull(rawValue) + " but was expected to be of type " + expectedDataType)); return; } @@ -189,6 +189,9 @@ public class StandardSchemaValidator implements RecordSchemaValidator { final Object[] array = (Object[]) value; for (final Object arrayVal : array) { + if (arrayVal == null && arrayDataType.isElementsNullable()) { + continue; + } if (!isTypeCorrect(arrayVal, elementType)) { return false; } @@ -202,6 +205,9 @@ public class StandardSchemaValidator implements RecordSchemaValidator { final Map map = (Map) value; for (final Object mapValue : map.values()) { + if (mapValue == null && mapDataType.isValuesNullable()) { + continue; + } if (!isTypeCorrect(mapValue, valueDataType)) { return false; } @@ -287,4 +293,8 @@ public class StandardSchemaValidator implements RecordSchemaValidator { private String concat(final String fieldPrefix, final RecordField field) { return fieldPrefix + "/" + field.getFieldName(); } + + private String classNameOrNull(Object value) { + return value == null ? "null" : value.getClass().getName(); + } } diff --git a/nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/test/java/org/apache/nifi/processors/standard/TestValidateRecord.java b/nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/test/java/org/apache/nifi/processors/standard/TestValidateRecord.java index 895e3de309..dfcb41a185 100644 --- a/nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/test/java/org/apache/nifi/processors/standard/TestValidateRecord.java +++ b/nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/test/java/org/apache/nifi/processors/standard/TestValidateRecord.java @@ -640,4 +640,38 @@ public class TestValidateRecord { + "The following 1 fields had values whose type did not match the schema: [/id]"); } + @Test + public void testValidationForNullElementArrayAndMap() throws Exception { + AvroReader avroReader = new AvroReader(); + runner.addControllerService("reader", avroReader); + runner.enableControllerService(avroReader); + + + final MockRecordWriter validWriter = new MockRecordWriter("valid", false); + runner.addControllerService("writer", validWriter); + runner.enableControllerService(validWriter); + + final MockRecordWriter invalidWriter = new MockRecordWriter("invalid", true); + runner.addControllerService("invalid-writer", invalidWriter); + runner.enableControllerService(invalidWriter); + + runner.setProperty(ValidateRecord.RECORD_READER, "reader"); + runner.setProperty(ValidateRecord.RECORD_WRITER, "writer"); + runner.setProperty(ValidateRecord.INVALID_RECORD_WRITER, "invalid-writer"); + runner.setProperty(ValidateRecord.ALLOW_EXTRA_FIELDS, "false"); + runner.setProperty(ValidateRecord.MAX_VALIDATION_DETAILS_LENGTH, "150"); + runner.setProperty(ValidateRecord.VALIDATION_DETAILS_ATTRIBUTE_NAME, "valDetails"); + + runner.enqueue(Paths.get("src/test/resources/TestValidateRecord/array-and-map-with-null-element.avro")); + runner.run(); + + runner.assertTransferCount(ValidateRecord.REL_INVALID, 0); + runner.assertTransferCount(ValidateRecord.REL_FAILURE, 0); + runner.assertTransferCount(ValidateRecord.REL_VALID, 1); + + final MockFlowFile validFlowFile = runner.getFlowFilesForRelationship(ValidateRecord.REL_VALID).get(0); + validFlowFile.assertAttributeEquals("record.count", "1"); + validFlowFile.assertContentEquals("valid\n[text, null],{key=null}\n"); + } + } diff --git a/nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/test/resources/TestValidateRecord/array-and-map-with-null-element.avro b/nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/test/resources/TestValidateRecord/array-and-map-with-null-element.avro new file mode 100644 index 0000000000000000000000000000000000000000..b9eb1d587c50d7b75fe883e414fc852707c3dc73 GIT binary patch literal 281 zcmeZI%3@>@ODrqO*DFrWNX<=bW3E;zsVqoUvQjEaP0lY$QPNS$OUwoFOHzwVfV{NK z)SQ%JC9CLam_%YxQDP-f2BHtHA10hxl9~%ruasAslLJ&%TvC*om#!3BtD}^XpA0lu zsa6MWR&HVerb!UNvc#OyRBQ$T%}Gs5EX^rVvZ}7Ft&Ob}0edJpKP5Gp17s;fQ{Jrj khztEoPdQb5>Dc`(ib;WmiK8U7qJ)8gi7h*|k^w^n0KRKsbN~PV literal 0 HcmV?d00001