rkundam commented on code in PR #373:
URL: https://github.com/apache/atlas/pull/373#discussion_r2195726113
##########
webapp/src/main/java/org/apache/atlas/web/rest/DiscoveryREST.java:
##########
@@ -892,27 +892,48 @@ private void validateSearchParameters(SearchParameters
parameters) throws AtlasB
private void validateEntityFilter(SearchParameters parameters) throws
AtlasBaseException {
FilterCriteria entityFilter = parameters.getEntityFilters();
+ FilterCriteria tagFilter = parameters.getTagFilters();
- if (entityFilter == null) {
- return;
+ if (isNotEmpty(entityFilter) ) {
+ validateNestedCriteria(entityFilter);
}
- if (entityFilter.getCriterion() != null &&
- !entityFilter.getCriterion().isEmpty()) {
- if (entityFilter.getCondition() == null ||
StringUtils.isEmpty(entityFilter.getCondition().toString())) {
+ if (isNotEmpty(tagFilter)) {
+ validateNestedCriteria(tagFilter);
+ }
+ }
+
+ private boolean isNotEmpty(FilterCriteria filter) {
+ if (filter == null) {
+ return false;
+ }
+
+ if (filter.getAttributeName() != null && filter.getOperator() != null
&& filter.getAttributeValue() != null) {
Review Comment:
attributeName, attributeValue, and operator are part of each FilterCriteria
inside criterion.
So, checking them directly at the top level might not make sense — this
condition might always be false unless we're supporting flat filters.
Do we have any use case where these fields exist directly under
FilterCriteria (outside criterion)? If not, probably safe to remove or revisit.
##########
webapp/src/main/java/org/apache/atlas/web/rest/DiscoveryREST.java:
##########
@@ -892,27 +892,48 @@ private void validateSearchParameters(SearchParameters
parameters) throws AtlasB
private void validateEntityFilter(SearchParameters parameters) throws
AtlasBaseException {
FilterCriteria entityFilter = parameters.getEntityFilters();
+ FilterCriteria tagFilter = parameters.getTagFilters();
- if (entityFilter == null) {
- return;
+ if (isNotEmpty(entityFilter) ) {
+ validateNestedCriteria(entityFilter);
}
- if (entityFilter.getCriterion() != null &&
- !entityFilter.getCriterion().isEmpty()) {
- if (entityFilter.getCondition() == null ||
StringUtils.isEmpty(entityFilter.getCondition().toString())) {
+ if (isNotEmpty(tagFilter)) {
+ validateNestedCriteria(tagFilter);
+ }
+ }
+
+ private boolean isNotEmpty(FilterCriteria filter) {
+ if (filter == null) {
+ return false;
+ }
+
+ if (filter.getAttributeName() != null && filter.getOperator() != null
&& filter.getAttributeValue() != null) {
+ return true;
+ }
+
+ return filter.getCriterion() != null &&
!filter.getCriterion().isEmpty();
+ }
+
+ private void validateNestedCriteria(SearchParameters.FilterCriteria
criteria) throws AtlasBaseException {
+ if (criteria.getCriterion() != null &&
Review Comment:
We’re checking condition when nested filters exist, but shouldn’t we also
validate attributeName, attributeValue, and operator for every FilterCriteria
in the list?
Right now, it looks like we only validate the nested structure and skip
actual field-level checks on each node.
Given the above two points, does it make sense to keep isNotEmpty() as a
separate method?
We're already going deep into the validation in validateNestedCriteria(),
and isNotEmpty() is mostly doing partial checks.
Maybe better to include those checks directly in the validation method to
keep all related logic in one place.
--
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: dev-unsubscr...@atlas.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
