----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72172/#review219736 -----------------------------------------------------------
intg/src/main/java/org/apache/atlas/type/AtlasNamespaceType.java Line 103 (original), 104 (patched) <https://reviews.apache.org/r/72172/#comment307939> Instead of hardcoding "array<string>", I suggest the following approach: AtlasType attrType = attribute.getAttributeType(); if (attrType instanceof AtlasArrayType) { attrType = ((AtlasArrayType) attrType).getElementType(); } if (attrType instanceof AtlasBuiltInTypes.AtlasStringType) { Integer maxStringLength = attribute.getOptionInt(ATTR_MAX_STRING_LENGTH); ... } intg/src/main/java/org/apache/atlas/type/AtlasNamespaceType.java Lines 178 (patched) <https://reviews.apache.org/r/72172/#comment307941> Avoid using string "contains". Instead of hardcoding "array<string>", I suggest the following approach: public boolean isValidLength(Object value) { boolean ret = true; if (value != null) { AtlasType attrType = attribute.getAttributeType(); if (attrType instanceof AtlasBuiltInTypes.AtlasStringType) { ret = isValidString(value); } else if (attrType instanceof AtlasArrayType) { attrType = ((AtlasArrayType) attrType).getElementType(); if (attrType instanceof AtlasBuiltInTypes.AtlasStringType) { ret = isValidArrayValue(value); } } } return ret; } private boolean isValidStringValue(Object obj) { return obj == null || String.valueOf(obj).length() <= this.maxStringLength; } // similar to AtlasArrayType.isValidValue() private boolean isValidArrayValue(Object obj) { if (obj == null) { return true; } else if (obj instanceof List || obj instanceof Set) { Collection objList = (Collection) obj; for (Object element : objList) { if (!isValidStringValue(element)) { return false; } } } else if (obj.getClass().isArray()) { int arrayLen = Array.getLength(obj); for (int i = 0; i < arrayLen; i++) { if (!isValidStringValue(Array.get(obj, i))) { return false; } } } return true; } repository/src/main/java/org/apache/atlas/repository/store/graph/v2/AtlasEntityStoreV2.java Lines 1500 (patched) <https://reviews.apache.org/r/72172/#comment307938> it shouldn't be necessary to send nsAttribute.getMaxStringLength() as parameter. Its already available to nsAttribute instance. Please review and update. - Madhan Neethiraj On March 3, 2020, 3:04 p.m., Mandar Ambawane wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72172/ > ----------------------------------------------------------- > > (Updated March 3, 2020, 3:04 p.m.) > > > Review request for atlas, Ashutosh Mestry, Madhan Neethiraj, Nixon Rodrigues, > and Sarath Subramanian. > > > Bugs: ATLAS-3632 > https://issues.apache.org/jira/browse/ATLAS-3632 > > > Repository: atlas > > > Description > ------- > > Added check for length limit of String value > > > Diffs > ----- > > intg/src/main/java/org/apache/atlas/type/AtlasNamespaceType.java ede8443 > > repository/src/main/java/org/apache/atlas/repository/store/graph/v2/AtlasEntityStoreV2.java > 30f5e5a > > repository/src/test/java/org/apache/atlas/repository/store/graph/v2/AtlasNamespaceDefStoreV2Test.java > e2f5c16 > > > Diff: https://reviews.apache.org/r/72172/diff/2/ > > > Testing > ------- > > Pre-commit build: > https://builds.apache.org/job/PreCommit-ATLAS-Build-Test/1698/console > > Manul testing done by adding values beyong maximum length for String > namespace attributes. > Done same for String multi-valued namespace attributes. > > > Thanks, > > Mandar Ambawane > >
