You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@atlas.apache.org by Mandar Ambawane <ma...@freestoneinfotech.com> on 2020/03/03 15:04:28 UTC
Re: Review Request 72172: ATLAS-3632 (Namespace) String attribute length limit does not work during association
-----------------------------------------------------------
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.
Changes
-------
Moved isValidLength() in AtlasNamespaceAttribute.
Added Max length check for Multi-valued String Namespace attributes.
Changes in AtlasNamespaceDefStoreV2Test to handle mandatory field maxStrLength empty/null value
Bugs: ATLAS-3632
https://issues.apache.org/jira/browse/ATLAS-3632
Repository: atlas
Description
-------
Added check for length limit of String value
Diffs (updated)
-----
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/
Changes: https://reviews.apache.org/r/72172/diff/1-2/
Testing (updated)
-------
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
Re: Review Request 72172: ATLAS-3632 (Namespace) String attribute length limit does not work during association
Posted by Madhan Neethiraj <ma...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72172/#review219765
-----------------------------------------------------------
repository/src/main/java/org/apache/atlas/repository/store/graph/v2/AtlasEntityStoreV2.java
Line 49 (original), 49 (patched)
<https://reviews.apache.org/r/72172/#comment307965>
Please avoid collapse/expand changes to existing 'import' statements.
repository/src/main/java/org/apache/atlas/repository/store/graph/v2/AtlasEntityStoreV2.java
Lines 1501 (patched)
<https://reviews.apache.org/r/72172/#comment307964>
Please add unit tests to exercise both valid and invalid length cases.
- Madhan Neethiraj
On March 4, 2020, 12:29 p.m., Mandar Ambawane wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72172/
> -----------------------------------------------------------
>
> (Updated March 4, 2020, 12:29 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/3/
>
>
> 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
>
>
Re: Review Request 72172: ATLAS-3632 (Namespace) String attribute length limit does not work during association
Posted by Mandar Ambawane <ma...@freestoneinfotech.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72172/
-----------------------------------------------------------
(Updated March 4, 2020, 12:29 p.m.)
Review request for atlas, Ashutosh Mestry, Madhan Neethiraj, Nixon Rodrigues, and Sarath Subramanian.
Changes
-------
Addressed review comments.
Manual testing is done for both String and Multivalued-String namespace attributes
Pre-commit build: https://builds.apache.org/job/PreCommit-ATLAS-Build-Test/1704/console
Bugs: ATLAS-3632
https://issues.apache.org/jira/browse/ATLAS-3632
Repository: atlas
Description
-------
Added check for length limit of String value
Diffs (updated)
-----
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/3/
Changes: https://reviews.apache.org/r/72172/diff/2-3/
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
Re: Review Request 72172: ATLAS-3632 (Namespace) String attribute length limit does not work during association
Posted by Madhan Neethiraj <ma...@apache.org>.
-----------------------------------------------------------
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
>
>