Browse Source

Address review findings for OR key filter conditions

- Mark force-added entity-field filter mappings as ignored under OR so
  EntityDataAdapter keeps the response shape identical to AND (applied
  to both findEntityDataByQuery and validateEntityCountQuery paths).
- Skip the ON-clause filter in toLatestJoin when forceLeftJoin=true to
  avoid duplicating the predicate that buildQuery already emits at the
  middle layer under OR (different bound parameter names, same filter).
- Document the empty-predicate drop in buildQuery (safe under AND,
  narrowing under OR) and the defensive vacuously-true return in
  RepositoryUtils.checkKeyFilters.
- Explain why validateEntityCountQuery uses the nullable
  getKeyFiltersOperation() rather than the OrDefault helper.
- Add regression test asserting latest[ENTITY_FIELD] under OR only
  contains keys declared in entityFields.
pull/15286/head
Viacheslav Klimov 1 month ago
parent
commit
686b0a3be4
Failed to extract signature
  1. 27
      application/src/test/java/org/thingsboard/server/controller/EntityQueryControllerTest.java
  2. 2
      common/edqs/src/main/java/org/thingsboard/server/edqs/util/RepositoryUtils.java
  3. 2
      dao/src/main/java/org/thingsboard/server/dao/entity/BaseEntityService.java
  4. 10
      dao/src/main/java/org/thingsboard/server/dao/sql/query/DefaultEntityQueryRepository.java
  5. 8
      dao/src/main/java/org/thingsboard/server/dao/sql/query/EntityKeyMapping.java

27
application/src/test/java/org/thingsboard/server/controller/EntityQueryControllerTest.java

@ -1864,6 +1864,33 @@ public class EntityQueryControllerTest extends AbstractControllerTest {
assertThat(extractNames(result)).containsExactlyInAnyOrder("OrDataDeviceX", "OrDataDeviceY");
}
@Test
public void testFindEntityDataWithOrDoesNotLeakFilterOnlyEntityFields() throws Exception {
// Regression test: under OR, an entity-field filter (e.g. label) that isn't declared in
// entityFields must not leak its value into EntityData.latest[ENTITY_FIELD].
String type = "orLeakGuardType";
Device d1 = createDeviceWithSharedAttributes("OrLeakDeviceA", type, "{\"status\":\"active\"}");
d1.setLabel("leak-label-A");
doPost("/api/device", d1, Device.class);
Device d2 = createDeviceWithSharedAttributes("OrLeakDeviceB", type, null);
d2.setLabel("leak-label-B");
doPost("/api/device", d2, Device.class);
List<KeyFilter> keyFilters = List.of(
stringAttributeKeyFilter("status", StringFilterPredicate.StringOperation.EQUAL, "active"),
buildStringKeyFilter(EntityKeyType.ENTITY_FIELD, "label", StringFilterPredicate.StringOperation.EQUAL, "leak-label-B"));
EntityDataQuery orQuery = new EntityDataQuery(deviceTypeFilter(type), pageLinkSortedByName(10, 0, null),
nameEntityField(), null, keyFilters, ComplexOperation.OR);
await().atMost(TIMEOUT, TimeUnit.SECONDS).untilAsserted(() -> findByQueryAndCheck(orQuery, 2));
PageData<EntityData> result = findByQueryAndCheck(orQuery, 2);
assertThat(extractNames(result)).containsExactlyInAnyOrder("OrLeakDeviceA", "OrLeakDeviceB");
for (EntityData entity : result.getData()) {
assertThat(entity.getLatest().get(EntityKeyType.ENTITY_FIELD)).containsOnlyKeys("name");
}
}
@Test
public void testFindEntityDataWithOrSameKeyFilters() throws Exception {
String type = "orSameKeyType";

2
common/edqs/src/main/java/org/thingsboard/server/edqs/util/RepositoryUtils.java

@ -210,6 +210,8 @@ public class RepositoryUtils {
return true;
}
}
// Vacuously true when OR is called with an empty filter list. Unreachable via checkFilters
// (which guards on isHasKeyFilters()), but kept defensive for any future direct caller.
return keyFilters.isEmpty();
} else {
for (EdqsFilter keyFilter : keyFilters) {

2
dao/src/main/java/org/thingsboard/server/dao/entity/BaseEntityService.java

@ -347,6 +347,8 @@ public class BaseEntityService extends AbstractEntityService implements EntitySe
} else if (query.getEntityFilter().getType().equals(ENTITY_NAME)) {
validateEntityNameQuery((EntityNameFilter) query.getEntityFilter());
}
// Intentionally using the nullable getKeyFiltersOperation() (not getKeyFiltersOperationOrDefault()):
// a null value encodes "classic AND" and must pass this guard even when the OR feature flag is off.
if (!keyFiltersOrConditionsEnabled && query.getKeyFiltersOperation() == ComplexOperation.OR) {
throw new IncorrectParameterException("OR conditions between key filters are disabled by the system administrator.");
}

10
dao/src/main/java/org/thingsboard/server/dao/sql/query/DefaultEntityQueryRepository.java

@ -365,10 +365,13 @@ public class DefaultEntityQueryRepository implements EntityQueryRepository {
List<EntityKeyMapping> entityFieldsFiltersMapping = filterMapping.stream().filter(mapping -> !mapping.isLatest() && mapping.getEntityKeyColumn() != null)
.collect(Collectors.toList());
// Under OR: entity field filter columns must be in inner SELECT for outer WHERE reference
// Under OR: entity field filter columns must be in inner SELECT for outer WHERE reference.
// Mirror the ignore=true fix from findEntityDataByQuery so the inner subquery still emits the
// extra column but downstream response shape (benign for count) stays symmetric with the data path.
if (isOr) {
for (EntityKeyMapping m : entityFieldsFiltersMapping) {
if (!selectionMapping.contains(m)) {
m.setIgnore(true);
selectionMapping.add(m);
}
}
@ -464,10 +467,13 @@ public class DefaultEntityQueryRepository implements EntityQueryRepository {
List<EntityKeyMapping> entityFieldsFiltersMapping = filterMapping.stream().filter(mapping -> !mapping.isLatest() && mapping.getEntityKeyColumn() != null)
.collect(Collectors.toList());
// Under OR: entity field filter columns must be in inner SELECT for outer WHERE reference
// Under OR: entity field filter columns must be in inner SELECT for outer WHERE reference.
// Mark force-added filter-only mappings as ignored so EntityDataAdapter does not expose
// them in EntityData.latest — keeps the response shape identical to AND.
if (isOr) {
for (EntityKeyMapping m : entityFieldsFiltersMapping) {
if (!selectionMapping.contains(m)) {
m.setIgnore(true);
selectionMapping.add(m);
}
}

8
dao/src/main/java/org/thingsboard/server/dao/sql/query/EntityKeyMapping.java

@ -324,7 +324,10 @@ public class EntityKeyMapping {
entityTypeStr = "'" + entityType.name() + "'";
}
ctx.addStringParameter(getKeyId(), entityKey.getKey());
String filterQuery = toQueries(ctx, entityFilter.getType())
// Under OR (forceLeftJoin=true) the filter predicate is re-emitted by buildQuery at the middle
// layer with disjunction semantics. Inlining it in the ON clause here would duplicate the
// predicate with a different bound parameter name and double-filter the LEFT JOIN.
String filterQuery = forceLeftJoin ? "" : toQueries(ctx, entityFilter.getType())
.filter(StringUtils::isNotEmpty)
.collect(Collectors.joining(" and "));
if (StringUtils.isNotEmpty(filterQuery)) {
@ -393,6 +396,9 @@ public class EntityKeyMapping {
public static String buildQuery(SqlQueryContext ctx, List<EntityKeyMapping> mappings,
EntityFilterType filterType, ComplexOperation operation) {
String joiner = (operation == ComplexOperation.OR) ? " OR " : " AND ";
// Vacuously-true predicates (e.g. a ComplexFilterPredicate with zero nested predicates producing
// an empty string) are dropped here. Under AND this is safe — TRUE is the identity for AND — but
// under OR it silently narrows the disjunction. Callers must not emit meaningful empty predicates.
return mappings.stream()
.flatMap(mapping -> mapping.toQueries(ctx, filterType, operation == ComplexOperation.OR))
.filter(StringUtils::isNotEmpty)

Loading…
Cancel
Save