From 686b0a3be498be38e7a720d6e594eeef3ebf6699 Mon Sep 17 00:00:00 2001 From: Viacheslav Klimov Date: Tue, 21 Apr 2026 10:49:02 +0300 Subject: [PATCH] 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. --- .../controller/EntityQueryControllerTest.java | 27 +++++++++++++++++++ .../server/edqs/util/RepositoryUtils.java | 2 ++ .../server/dao/entity/BaseEntityService.java | 2 ++ .../query/DefaultEntityQueryRepository.java | 10 +++++-- .../dao/sql/query/EntityKeyMapping.java | 8 +++++- 5 files changed, 46 insertions(+), 3 deletions(-) diff --git a/application/src/test/java/org/thingsboard/server/controller/EntityQueryControllerTest.java b/application/src/test/java/org/thingsboard/server/controller/EntityQueryControllerTest.java index eff74778dc..5de4c9ffcf 100644 --- a/application/src/test/java/org/thingsboard/server/controller/EntityQueryControllerTest.java +++ b/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 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 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"; diff --git a/common/edqs/src/main/java/org/thingsboard/server/edqs/util/RepositoryUtils.java b/common/edqs/src/main/java/org/thingsboard/server/edqs/util/RepositoryUtils.java index f6da57e24c..6da25c260a 100644 --- a/common/edqs/src/main/java/org/thingsboard/server/edqs/util/RepositoryUtils.java +++ b/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) { diff --git a/dao/src/main/java/org/thingsboard/server/dao/entity/BaseEntityService.java b/dao/src/main/java/org/thingsboard/server/dao/entity/BaseEntityService.java index 740b629045..be49e64d10 100644 --- a/dao/src/main/java/org/thingsboard/server/dao/entity/BaseEntityService.java +++ b/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."); } diff --git a/dao/src/main/java/org/thingsboard/server/dao/sql/query/DefaultEntityQueryRepository.java b/dao/src/main/java/org/thingsboard/server/dao/sql/query/DefaultEntityQueryRepository.java index 140fe38af4..3417b81e25 100644 --- a/dao/src/main/java/org/thingsboard/server/dao/sql/query/DefaultEntityQueryRepository.java +++ b/dao/src/main/java/org/thingsboard/server/dao/sql/query/DefaultEntityQueryRepository.java @@ -365,10 +365,13 @@ public class DefaultEntityQueryRepository implements EntityQueryRepository { List 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 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); } } diff --git a/dao/src/main/java/org/thingsboard/server/dao/sql/query/EntityKeyMapping.java b/dao/src/main/java/org/thingsboard/server/dao/sql/query/EntityKeyMapping.java index c797d21041..ed4bb8eeb2 100644 --- a/dao/src/main/java/org/thingsboard/server/dao/sql/query/EntityKeyMapping.java +++ b/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 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)