rkundam commented on code in PR #373:
URL: https://github.com/apache/atlas/pull/373#discussion_r2199713260
##########
webapp/src/main/java/org/apache/atlas/web/rest/DiscoveryREST.java:
##########
@@ -892,27 +892,47 @@ 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 (checkEmptyFilterCriteria(entityFilter) ) {
+ validateNestedCriteria(entityFilter);
}
- if (entityFilter.getCriterion() != null &&
- !entityFilter.getCriterion().isEmpty()) {
- if (entityFilter.getCondition() == null ||
StringUtils.isEmpty(entityFilter.getCondition().toString())) {
+ if (checkEmptyFilterCriteria(tagFilter)) {
+ validateNestedCriteria(tagFilter);
+ }
+ }
+
+ private boolean checkEmptyFilterCriteria(FilterCriteria criteria) {
Review Comment:
Thanks for the explanation — the reasoning around handling empty {} filters
for backward compatibility makes sense.
However, my main concern is that checkEmptyFilterCriteria() is only used at
the top level, which means we might miss validating leaf filters that appear
deeper in the nested structure.
For example, if a leaf filter is nested inside a criterion list, and it’s
not empty but still invalid (like missing an operator), it won’t go through
checkEmptyFilterCriteria() — and won’t be validated at all unless it happens to
be part of a composite filter that triggers validateNestedCriteria().
What I’m suggesting is:
Instead of calling checkEmptyFilterCriteria() just once at the top,
We should move that logic inside validateNestedCriteria() so every filter —
whether it’s top-level or nested — is consistently checked and validated.
This keeps the empty filter check (for {} support), but also ensures all
filters with actual content get validated properly.
Code Reference:
private void validateEntityFilter(SearchParameters parameters) throws
AtlasBaseException {
FilterCriteria entityFilter = parameters.getEntityFilters();
FilterCriteria tagFilter = parameters.getTagFilters();
validateNestedCriteria(entityFilter);
validateNestedCriteria(tagFilter);
}
private void validateNestedCriteria(SearchParameters.FilterCriteria
criteria) throws AtlasBaseException {
if (criteria == null) {
return;
}
boolean hasComposite = criteria.getCriterion() != null &&
!criteria.getCriterion().isEmpty();
boolean hasLeaf = StringUtils.isNotEmpty(criteria.getAttributeName())
|| criteria.getOperator() != null
|| StringUtils.isNotEmpty(criteria.getAttributeValue());
if (hasComposite) {
if (criteria.getCondition() == null ||
StringUtils.isEmpty(criteria.getCondition().toString())) {
throw new AtlasBaseException("Condition (AND/OR) must be
specified when using multiple filters.");
}
for (FilterCriteria filterCriteria : criteria.getCriterion()) {
if (filterCriteria != null) {
validateNestedCriteria(filterCriteria);
}
}
}
if (hasLeaf) {
validateLeafFilterCriteria(criteria);
}
}
--
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]
