sheetalshah1007 commented on code in PR #326:
URL: https://github.com/apache/atlas/pull/326#discussion_r2039332431
##########
webapp/src/main/java/org/apache/atlas/web/rest/DiscoveryREST.java:
##########
@@ -885,7 +885,54 @@ private void validateSearchParameters(SearchParameters
parameters) throws AtlasB
if (StringUtils.isNotEmpty(parameters.getQuery()) &&
parameters.getQuery().length() > maxFullTextQueryLength) {
throw new
AtlasBaseException(AtlasErrorCode.INVALID_QUERY_LENGTH,
Constants.MAX_FULLTEXT_QUERY_STR_LENGTH);
}
+
+ validateEntityFilter(parameters);
+ }
+ }
+
+ private void validateEntityFilter(SearchParameters parameters) throws
AtlasBaseException {
+ if (parameters.getEntityFilters() == null) {
Review Comment:
Repeated call to `parameters.getEntityFilters()` makes the code verbose and
harder to read
Consider defining `FilterCriteria entityFilter =
parameters.getEntityFilters();`
and reuse `entityFilter` throughout the method.
##########
intg/src/main/java/org/apache/atlas/AtlasErrorCode.java:
##########
@@ -179,6 +179,8 @@ public enum AtlasErrorCode {
LINEAGE_ON_DEMAND_NOT_ENABLED(400, "ATLAS-400-00-100", "Lineage on demand
config: {0} is not enabled"),
INVALID_TOPIC_NAME(400, "ATLAS-400-00-101", "Unsupported topic name :
{0}"),
UNSUPPORTED_TYPE_NAME(400, "ATLAS-400-00-102", "Unsupported {0} name.
Names such as 'description','version','options','name','servicetype' are not
supported"),
+ INVALID_OPERATOR(400, "ATLAS-400-00-103", "Invalid Operator Passed: for
attribute name: {0}"),
+ BLANK_ATTRIBUTES(400, "ATLAS-400-00-104", "Attribute Name and Attribute
Value can't be empty!"),
Review Comment:
consider splitting attribute `name` and `value` into separate error codes;
as it is, the respective checks are also handled in different code blocks
##########
intg/src/main/java/org/apache/atlas/AtlasErrorCode.java:
##########
@@ -179,6 +179,8 @@ public enum AtlasErrorCode {
LINEAGE_ON_DEMAND_NOT_ENABLED(400, "ATLAS-400-00-100", "Lineage on demand
config: {0} is not enabled"),
INVALID_TOPIC_NAME(400, "ATLAS-400-00-101", "Unsupported topic name :
{0}"),
UNSUPPORTED_TYPE_NAME(400, "ATLAS-400-00-102", "Unsupported {0} name.
Names such as 'description','version','options','name','servicetype' are not
supported"),
+ INVALID_OPERATOR(400, "ATLAS-400-00-103", "Invalid Operator Passed: for
attribute name: {0}"),
Review Comment:
Suggested correction for the error message : "`Invalid operator specified
for attribute: {0}`"
##########
webapp/src/test/java/org/apache/atlas/web/integration/BasicSearchIT.java:
##########
@@ -224,6 +240,50 @@ public void testSavedSearch(String jsonFile) {
}
}
+ @Test(dataProvider = "attributeSearchJSONNamesInvalidOperator")
+ public void testAttributeSearchInvalidOperator(String jsonFile) {
+ try {
+ BasicSearchParametersWithExpectation[] testExpectations =
TestResourceFileUtils.readObjectFromJson(jsonFile,
BasicSearchParametersWithExpectation[].class);
+ assertNotNull(testExpectations);
+ Arrays
+ .stream(testExpectations)
+ .map(testExpectation ->
testExpectation.getSearchParameters())
+ .filter(params -> params.getEntityFilters() != null &&
params.getEntityFilters().getOperator() != null)
+ .forEach(params -> {
+ try {
+ atlasClientV2.facetedSearch(params);
+ }
+ catch (AtlasServiceException e) {
+
assertTrue(e.getMessage().contains("ATLAS-400-00-103"));
+ }
+ });
+ } catch (IOException e) {
+ fail(e.getMessage());
+ }
+ }
+
+ @Test(dataProvider = "attributeSearchJSONNamesEmptyAttribute")
Review Comment:
Consider using slightly clearer name like `emptyAttributeTestFiles` instead
of `attributeSearchJSONNamesEmptyAttribute`
##########
webapp/src/test/java/org/apache/atlas/web/integration/BasicSearchIT.java:
##########
@@ -224,6 +240,50 @@ public void testSavedSearch(String jsonFile) {
}
}
+ @Test(dataProvider = "attributeSearchJSONNamesInvalidOperator")
+ public void testAttributeSearchInvalidOperator(String jsonFile) {
+ try {
+ BasicSearchParametersWithExpectation[] testExpectations =
TestResourceFileUtils.readObjectFromJson(jsonFile,
BasicSearchParametersWithExpectation[].class);
+ assertNotNull(testExpectations);
+ Arrays
+ .stream(testExpectations)
+ .map(testExpectation ->
testExpectation.getSearchParameters())
+ .filter(params -> params.getEntityFilters() != null &&
params.getEntityFilters().getOperator() != null)
+ .forEach(params -> {
+ try {
+ atlasClientV2.facetedSearch(params);
+ }
+ catch (AtlasServiceException e) {
+
assertTrue(e.getMessage().contains("ATLAS-400-00-103"));
+ }
+ });
+ } catch (IOException e) {
+ fail(e.getMessage());
+ }
+ }
+
+ @Test(dataProvider = "attributeSearchJSONNamesEmptyAttribute")
+ public void testAttributeSearchEmptyAttribute(String jsonFile) {
+ try {
+ BasicSearchParametersWithExpectation[] testExpectations =
TestResourceFileUtils.readObjectFromJson(jsonFile,
BasicSearchParametersWithExpectation[].class);
+ assertNotNull(testExpectations);
+ Arrays
+ .stream(testExpectations)
+ .map(testExpectation ->
testExpectation.getSearchParameters())
+ .filter(params -> params.getEntityFilters() != null &&
params.getEntityFilters().getAttributeName() != null)
+ .forEach(params -> {
+ try {
+ atlasClientV2.facetedSearch(params);
Review Comment:
Currently, if `facetedSearch() `does not throw an exception, the test will
silently succeed. Add an explicit fail() if the method does not throw
##########
webapp/src/test/java/org/apache/atlas/web/integration/BasicSearchIT.java:
##########
@@ -224,6 +240,50 @@ public void testSavedSearch(String jsonFile) {
}
}
+ @Test(dataProvider = "attributeSearchJSONNamesInvalidOperator")
+ public void testAttributeSearchInvalidOperator(String jsonFile) {
+ try {
+ BasicSearchParametersWithExpectation[] testExpectations =
TestResourceFileUtils.readObjectFromJson(jsonFile,
BasicSearchParametersWithExpectation[].class);
+ assertNotNull(testExpectations);
+ Arrays
+ .stream(testExpectations)
+ .map(testExpectation ->
testExpectation.getSearchParameters())
+ .filter(params -> params.getEntityFilters() != null &&
params.getEntityFilters().getOperator() != null)
+ .forEach(params -> {
+ try {
+ atlasClientV2.facetedSearch(params);
+ }
+ catch (AtlasServiceException e) {
+
assertTrue(e.getMessage().contains("ATLAS-400-00-103"));
Review Comment:
Use `assertTrue` with a failure message like below to provide a clear
context when a test case fails:
```
assertTrue(e.getMessage().contains("ATLAS-400-00-103"),
"Expected error code ATLAS-400-00-103 in exception message: " +
e.getMessage());
```
##########
webapp/src/test/java/org/apache/atlas/web/integration/BasicSearchIT.java:
##########
@@ -224,6 +240,50 @@ public void testSavedSearch(String jsonFile) {
}
}
+ @Test(dataProvider = "attributeSearchJSONNamesInvalidOperator")
+ public void testAttributeSearchInvalidOperator(String jsonFile) {
+ try {
+ BasicSearchParametersWithExpectation[] testExpectations =
TestResourceFileUtils.readObjectFromJson(jsonFile,
BasicSearchParametersWithExpectation[].class);
+ assertNotNull(testExpectations);
+ Arrays
+ .stream(testExpectations)
+ .map(testExpectation ->
testExpectation.getSearchParameters())
+ .filter(params -> params.getEntityFilters() != null &&
params.getEntityFilters().getOperator() != null)
+ .forEach(params -> {
+ try {
+ atlasClientV2.facetedSearch(params);
Review Comment:
Currently, if `facetedSearch() `does not throw an exception, the test will
silently succeed. Add an explicit fail() if the method does not throw
##########
webapp/src/main/java/org/apache/atlas/web/rest/DiscoveryREST.java:
##########
@@ -885,7 +885,54 @@ private void validateSearchParameters(SearchParameters
parameters) throws AtlasB
if (StringUtils.isNotEmpty(parameters.getQuery()) &&
parameters.getQuery().length() > maxFullTextQueryLength) {
throw new
AtlasBaseException(AtlasErrorCode.INVALID_QUERY_LENGTH,
Constants.MAX_FULLTEXT_QUERY_STR_LENGTH);
}
+
+ validateEntityFilter(parameters);
+ }
+ }
+
+ private void validateEntityFilter(SearchParameters parameters) throws
AtlasBaseException {
+ if (parameters.getEntityFilters() == null) {
+ return;
+ }
+
+ if (parameters.getEntityFilters().getCriterion() != null &&
+ !parameters.getEntityFilters().getCriterion().isEmpty()) {
+ if
(StringUtils.isEmpty(parameters.getEntityFilters().getCondition().toString())) {
Review Comment:
This check is not safe: if `getCondition()` returns null, `toString()` will
throw a `NullPointerException`
##########
webapp/src/test/java/org/apache/atlas/web/integration/BasicSearchIT.java:
##########
@@ -224,6 +240,50 @@ public void testSavedSearch(String jsonFile) {
}
}
+ @Test(dataProvider = "attributeSearchJSONNamesInvalidOperator")
Review Comment:
Consider using slightly clearer name like `invalidOperatorTestFiles` instead
of `attributeSearchJSONNamesInvalidOperator`
##########
webapp/src/test/java/org/apache/atlas/web/integration/BasicSearchIT.java:
##########
@@ -224,6 +240,50 @@ public void testSavedSearch(String jsonFile) {
}
}
+ @Test(dataProvider = "attributeSearchJSONNamesInvalidOperator")
+ public void testAttributeSearchInvalidOperator(String jsonFile) {
+ try {
+ BasicSearchParametersWithExpectation[] testExpectations =
TestResourceFileUtils.readObjectFromJson(jsonFile,
BasicSearchParametersWithExpectation[].class);
+ assertNotNull(testExpectations);
+ Arrays
+ .stream(testExpectations)
+ .map(testExpectation ->
testExpectation.getSearchParameters())
+ .filter(params -> params.getEntityFilters() != null &&
params.getEntityFilters().getOperator() != null)
+ .forEach(params -> {
+ try {
+ atlasClientV2.facetedSearch(params);
+ }
+ catch (AtlasServiceException e) {
+
assertTrue(e.getMessage().contains("ATLAS-400-00-103"));
+ }
+ });
+ } catch (IOException e) {
+ fail(e.getMessage());
+ }
+ }
+
+ @Test(dataProvider = "attributeSearchJSONNamesEmptyAttribute")
+ public void testAttributeSearchEmptyAttribute(String jsonFile) {
+ try {
+ BasicSearchParametersWithExpectation[] testExpectations =
TestResourceFileUtils.readObjectFromJson(jsonFile,
BasicSearchParametersWithExpectation[].class);
+ assertNotNull(testExpectations);
+ Arrays
+ .stream(testExpectations)
+ .map(testExpectation ->
testExpectation.getSearchParameters())
+ .filter(params -> params.getEntityFilters() != null &&
params.getEntityFilters().getAttributeName() != null)
+ .forEach(params -> {
+ try {
+ atlasClientV2.facetedSearch(params);
+ }
+ catch (AtlasServiceException e) {
+
assertTrue(e.getMessage().contains("ATLAS-400-00-104"));
Review Comment:
Use `assertTrue` with a failure message like below to provide a clear
context when a test case fails:
```
assertTrue(e.getMessage().contains("ATLAS-400-00-104"),
"Expected error code ATLAS-400-00-103 in exception message: " +
e.getMessage());
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
