From 9fee9ea85e9baa36621ad1d0495f057e7edc0253 Mon Sep 17 00:00:00 2001 From: Ivan Khropachov Date: Fri, 7 Aug 2026 12:37:15 +0300 Subject: [PATCH 1/2] deviceFilters: stop computing six Pinot queries to answer one --- .../config/DeviceFilterExecutorConfig.java | 53 ++++++ .../api/dto/device/DeviceFilterFacet.java | 38 ++++ .../api/service/DeviceFilterService.java | 70 +++++++- .../api/service/DeviceFilterServiceTest.java | 125 +++++++++++++ .../api/datafetcher/DeviceDataFetcher.java | 38 +++- .../DeviceFiltersSelectionSetTest.java | 170 ++++++++++++++++++ 6 files changed, 483 insertions(+), 11 deletions(-) create mode 100644 openframe-api-lib/src/main/java/com/openframe/api/config/DeviceFilterExecutorConfig.java create mode 100644 openframe-api-lib/src/main/java/com/openframe/api/dto/device/DeviceFilterFacet.java create mode 100644 openframe-api-lib/src/test/java/com/openframe/api/service/DeviceFilterServiceTest.java create mode 100644 openframe-api-service-core/src/test/java/com/openframe/api/datafetcher/DeviceFiltersSelectionSetTest.java diff --git a/openframe-api-lib/src/main/java/com/openframe/api/config/DeviceFilterExecutorConfig.java b/openframe-api-lib/src/main/java/com/openframe/api/config/DeviceFilterExecutorConfig.java new file mode 100644 index 000000000..6401b14b6 --- /dev/null +++ b/openframe-api-lib/src/main/java/com/openframe/api/config/DeviceFilterExecutorConfig.java @@ -0,0 +1,53 @@ +package com.openframe.api.config; + +import org.springframework.beans.factory.annotation.Value; +import org.springframework.context.annotation.Bean; +import org.springframework.context.annotation.Configuration; +import org.springframework.scheduling.concurrent.ThreadPoolTaskExecutor; + +import java.util.concurrent.Executor; +import java.util.concurrent.ThreadPoolExecutor; + +/** + * Thread pool for the {@code deviceFilters} facet fan-out. + * + * The fan-out in {@code DeviceFilterService} used {@code CompletableFuture.supplyAsync} with no + * executor, i.e. {@link java.util.concurrent.ForkJoinPool#commonPool()}, whose parallelism is + * {@code availableProcessors() - 1}. Tenant pods run on a 700m CPU limit, so the JVM reports ONE + * processor ({@code system.cpu.count = 1}) and the common pool has at most one worker — the six + * "parallel" facet queries actually ran one after another on the request thread, and the + * {@code allOf} join was a no-op. Raising the CPU limit does not fix this: at 2 CPUs the common + * pool's parallelism is still 1. It needs its own pool. + * + * The tasks are blocking HTTP calls to the Pinot broker, not CPU work, so the pool is sized for + * concurrent round trips rather than cores — a single-core pod can hold six of these in flight + * for the cost of six idle threads. + * + * Bounded queue + {@link ThreadPoolExecutor.CallerRunsPolicy}: under saturation the work runs on + * the calling request thread, which is exactly today's behaviour, so overload degrades to "slow" + * instead of "rejected". + */ +@Configuration +public class DeviceFilterExecutorConfig { + + public static final String DEVICE_FILTER_FACET_EXECUTOR = "deviceFilterFacetExecutor"; + + @Bean(DEVICE_FILTER_FACET_EXECUTOR) + public Executor deviceFilterFacetExecutor( + @Value("${openframe.api.device-filters.facet-threads:6}") int threads, + @Value("${openframe.api.device-filters.facet-queue-capacity:64}") int queueCapacity) { + + ThreadPoolTaskExecutor executor = new ThreadPoolTaskExecutor(); + executor.setCorePoolSize(threads); + executor.setMaxPoolSize(threads); + executor.setQueueCapacity(queueCapacity); + executor.setThreadNamePrefix("device-facet-"); + executor.setRejectedExecutionHandler(new ThreadPoolExecutor.CallerRunsPolicy()); + // Idle pods keep no threads: the facets are bursty (a dashboard or filter-UI load), and + // between bursts there is no reason to hold six parked threads in a 1Gi container. + executor.setAllowCoreThreadTimeOut(true); + executor.setKeepAliveSeconds(60); + executor.initialize(); + return executor; + } +} diff --git a/openframe-api-lib/src/main/java/com/openframe/api/dto/device/DeviceFilterFacet.java b/openframe-api-lib/src/main/java/com/openframe/api/dto/device/DeviceFilterFacet.java new file mode 100644 index 000000000..4633b2f4e --- /dev/null +++ b/openframe-api-lib/src/main/java/com/openframe/api/dto/device/DeviceFilterFacet.java @@ -0,0 +1,38 @@ +package com.openframe.api.dto.device; + +import java.util.EnumSet; +import java.util.Set; + +/** + * One independently-computed piece of {@link DeviceFilters}. + * + * Every facet costs its own Pinot round trip, so a caller that needs one number should not + * pay for six. {@code DeviceFilterService} takes a set of these and skips the queries for + * everything outside it; the GraphQL data fetcher derives the set from the selection set, so + * {@code deviceFilters { filteredCount }} runs ONE query instead of six. + * + * {@link #graphQlField()} is the field name in the {@code DeviceFilters} GraphQL type and must + * stay in sync with {@code device.graphqls} — it is what the selection-set lookup matches on. + */ +public enum DeviceFilterFacet { + + STATUSES("statuses"), + DEVICE_TYPES("deviceTypes"), + OS_TYPES("osTypes"), + ORGANIZATION_IDS("organizationIds"), + TAG_KEYS("tagKeys"), + FILTERED_COUNT("filteredCount"); + + /** Every facet — the behaviour non-GraphQL callers (the external REST API) still want. */ + public static final Set ALL = EnumSet.allOf(DeviceFilterFacet.class); + + private final String graphQlField; + + DeviceFilterFacet(String graphQlField) { + this.graphQlField = graphQlField; + } + + public String graphQlField() { + return graphQlField; + } +} diff --git a/openframe-api-lib/src/main/java/com/openframe/api/service/DeviceFilterService.java b/openframe-api-lib/src/main/java/com/openframe/api/service/DeviceFilterService.java index 30ecfc198..038d9a23a 100644 --- a/openframe-api-lib/src/main/java/com/openframe/api/service/DeviceFilterService.java +++ b/openframe-api-lib/src/main/java/com/openframe/api/service/DeviceFilterService.java @@ -1,18 +1,32 @@ package com.openframe.api.service; +import com.openframe.api.config.DeviceFilterExecutorConfig; import com.openframe.api.dto.device.DeviceFilterCriteria; +import com.openframe.api.dto.device.DeviceFilterFacet; import com.openframe.api.dto.device.DeviceFilters; import com.openframe.data.pinot.repository.PinotDeviceRepository; import com.openframe.data.service.TenantIdProvider; import lombok.extern.slf4j.Slf4j; +import org.springframework.beans.factory.annotation.Qualifier; import org.springframework.stereotype.Service; import java.util.ArrayList; import java.util.List; import java.util.Map; +import java.util.Set; import java.util.concurrent.CompletableFuture; - +import java.util.concurrent.Executor; +import java.util.function.Supplier; + +import static com.openframe.api.dto.device.DeviceFilterFacet.DEVICE_TYPES; +import static com.openframe.api.dto.device.DeviceFilterFacet.FILTERED_COUNT; +import static com.openframe.api.dto.device.DeviceFilterFacet.ORGANIZATION_IDS; +import static com.openframe.api.dto.device.DeviceFilterFacet.OS_TYPES; +import static com.openframe.api.dto.device.DeviceFilterFacet.STATUSES; +import static com.openframe.api.dto.device.DeviceFilterFacet.TAG_KEYS; import static java.util.Collections.emptyList; +import static java.util.concurrent.CompletableFuture.completedFuture; +import static java.util.concurrent.CompletableFuture.supplyAsync; @Service @Slf4j @@ -21,16 +35,41 @@ public class DeviceFilterService { private final PinotDeviceRepository pinotDeviceRepository; private final DeviceFilterOptionMapper optionMapper; private final TenantIdProvider tenantIdProvider; + private final Executor facetExecutor; public DeviceFilterService(PinotDeviceRepository pinotDeviceRepository, DeviceFilterOptionMapper optionMapper, - TenantIdProvider tenantIdProvider) { + TenantIdProvider tenantIdProvider, + @Qualifier(DeviceFilterExecutorConfig.DEVICE_FILTER_FACET_EXECUTOR) + Executor facetExecutor) { this.pinotDeviceRepository = pinotDeviceRepository; this.optionMapper = optionMapper; this.tenantIdProvider = tenantIdProvider; + this.facetExecutor = facetExecutor; } + /** + * Every facet. For callers with no selection set of their own — the external REST API, whose + * response DTO always carries all of them. + */ public CompletableFuture getDeviceFilters(DeviceFilterCriteria filters) { + return getDeviceFilters(filters, DeviceFilterFacet.ALL); + } + + /** + * Only the requested facets; each one skipped is one Pinot round trip not made. + * + * Unrequested facets come back as empty lists (the mappers map {@code null} to + * {@code List.of()}), never {@code null}, so the non-null GraphQL list types hold even if a + * field is somehow read without having been selected. {@code filteredCount} is the exception — + * it is a scalar and stays {@code null} when unrequested, which is only observable if a caller + * asks for it without listing it in {@code requestedFacets}. + * + * @param requestedFacets facets to compute; {@code null} means all, EMPTY means none. + */ + public CompletableFuture getDeviceFilters(DeviceFilterCriteria filters, + Set requestedFacets) { + Set facets = requestedFacets != null ? requestedFacets : DeviceFilterFacet.ALL; List statuses = filters != null && filters.getStatuses() != null ? filters.getStatuses().stream().map(Enum::name).toList() : emptyList(); List deviceTypes = filters != null && filters.getDeviceTypes() != null ? @@ -42,17 +81,17 @@ public CompletableFuture getDeviceFilters(DeviceFilterCriteria fi String tenantId = tenantIdProvider.getTenantId(); - CompletableFuture> statusesFuture = CompletableFuture.supplyAsync(() -> + CompletableFuture> statusesFuture = facetQuery(facets, STATUSES, () -> pinotDeviceRepository.getStatusFilterOptions(tenantId, statuses, deviceTypes, osTypes, organizationIds, tagKeys, tagKeyValues)); - CompletableFuture> deviceTypesFuture = CompletableFuture.supplyAsync(() -> + CompletableFuture> deviceTypesFuture = facetQuery(facets, DEVICE_TYPES, () -> pinotDeviceRepository.getDeviceTypeFilterOptions(tenantId, statuses, deviceTypes, osTypes, organizationIds, tagKeys, tagKeyValues)); - CompletableFuture> osTypesFuture = CompletableFuture.supplyAsync(() -> + CompletableFuture> osTypesFuture = facetQuery(facets, OS_TYPES, () -> pinotDeviceRepository.getOsTypeFilterOptions(tenantId, statuses, deviceTypes, osTypes, organizationIds, tagKeys, tagKeyValues)); - CompletableFuture> organizationsFuture = CompletableFuture.supplyAsync(() -> + CompletableFuture> organizationsFuture = facetQuery(facets, ORGANIZATION_IDS, () -> pinotDeviceRepository.getOrganizationFilterOptions(tenantId, statuses, deviceTypes, osTypes, organizationIds, tagKeys, tagKeyValues)); - CompletableFuture> tagKeysFuture = CompletableFuture.supplyAsync(() -> + CompletableFuture> tagKeysFuture = facetQuery(facets, TAG_KEYS, () -> pinotDeviceRepository.getTagKeyFilterOptions(tenantId, statuses, deviceTypes, osTypes, organizationIds, tagKeys, tagKeyValues)); - CompletableFuture filteredCountFuture = CompletableFuture.supplyAsync(() -> + CompletableFuture filteredCountFuture = facetQuery(facets, FILTERED_COUNT, () -> pinotDeviceRepository.getFilteredDeviceCount(tenantId, statuses, deviceTypes, osTypes, organizationIds, tagKeys, tagKeyValues)); return CompletableFuture.allOf( @@ -69,6 +108,21 @@ public CompletableFuture getDeviceFilters(DeviceFilterCriteria fi ); } + /** + * Submits one facet query, or short-circuits to {@code null} when the caller didn't ask for it. + * + * The executor is explicit on purpose — see {@link DeviceFilterExecutorConfig} for why the + * default {@code ForkJoinPool.commonPool()} silently ran these serially on tenant pods. + */ + private CompletableFuture facetQuery(Set facets, + DeviceFilterFacet facet, + Supplier query) { + if (!facets.contains(facet)) { + return completedFuture(null); + } + return supplyAsync(query, facetExecutor); + } + /** * Builds the tagKeyValues filter list for Pinot queries. * Combines tagKeys and tagValues from DeviceFilterCriteria into "key:value" format. diff --git a/openframe-api-lib/src/test/java/com/openframe/api/service/DeviceFilterServiceTest.java b/openframe-api-lib/src/test/java/com/openframe/api/service/DeviceFilterServiceTest.java new file mode 100644 index 000000000..2095ac443 --- /dev/null +++ b/openframe-api-lib/src/test/java/com/openframe/api/service/DeviceFilterServiceTest.java @@ -0,0 +1,125 @@ +package com.openframe.api.service; + +import com.openframe.api.dto.device.DeviceFilterCriteria; +import com.openframe.api.dto.device.DeviceFilterFacet; +import com.openframe.api.dto.device.DeviceFilters; +import com.openframe.data.pinot.repository.PinotDeviceRepository; +import com.openframe.data.service.TenantIdProvider; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.Mock; +import org.mockito.junit.jupiter.MockitoExtension; +import org.mockito.junit.jupiter.MockitoSettings; +import org.mockito.quality.Strictness; + +import java.util.EnumSet; +import java.util.Map; +import java.util.Set; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.verifyNoMoreInteractions; +import static org.mockito.Mockito.when; + +/** + * Each facet is one Pinot round trip, so what matters here is which repository methods are NOT + * called: the selection-set narrowing is the whole point of the second {@code getDeviceFilters} + * overload. + */ +@ExtendWith(MockitoExtension.class) +@MockitoSettings(strictness = Strictness.LENIENT) +class DeviceFilterServiceTest { + + private static final String TENANT = "tenant-1"; + + @Mock private PinotDeviceRepository pinotDeviceRepository; + @Mock private TenantIdProvider tenantIdProvider; + @Mock private com.openframe.data.repository.organization.OrganizationRepository organizationRepository; + + private DeviceFilterService service() { + when(tenantIdProvider.getTenantId()).thenReturn(TENANT); + // Direct executor: the production pool is about wall-clock, not semantics, and running the + // facet queries inline keeps assertion order deterministic. + return new DeviceFilterService( + pinotDeviceRepository, + new DeviceFilterOptionMapper(organizationRepository), + tenantIdProvider, + Runnable::run); + } + + @Test + void filteredCountOnly_runsOneQueryAndSkipsEveryFacet() { + when(pinotDeviceRepository.getFilteredDeviceCount(anyString(), any(), any(), any(), any(), any(), any())) + .thenReturn(7); + + DeviceFilters result = service() + .getDeviceFilters(DeviceFilterCriteria.builder().build(), Set.of(DeviceFilterFacet.FILTERED_COUNT)) + .join(); + + assertThat(result.getFilteredCount()).isEqualTo(7); + verify(pinotDeviceRepository).getFilteredDeviceCount(anyString(), any(), any(), any(), any(), any(), any()); + verifyNoMoreInteractions(pinotDeviceRepository); + } + + @Test + void unrequestedFacetsAreEmptyListsNotNull() { + DeviceFilters result = service() + .getDeviceFilters(DeviceFilterCriteria.builder().build(), Set.of(DeviceFilterFacet.FILTERED_COUNT)) + .join(); + + // The GraphQL type declares these non-null, so an unselected facet must not surface as null + // if it is ever read. + assertThat(result.getStatuses()).isEmpty(); + assertThat(result.getDeviceTypes()).isEmpty(); + assertThat(result.getOsTypes()).isEmpty(); + assertThat(result.getOrganizationIds()).isEmpty(); + assertThat(result.getTagKeys()).isEmpty(); + } + + @Test + void dashboardCounterSelection_runsOnlyItsTwoQueries() { + when(pinotDeviceRepository.getStatusFilterOptions(anyString(), any(), any(), any(), any(), any(), any())) + .thenReturn(Map.of("ONLINE", 3)); + when(pinotDeviceRepository.getFilteredDeviceCount(anyString(), any(), any(), any(), any(), any(), any())) + .thenReturn(3); + + DeviceFilters result = service() + .getDeviceFilters(DeviceFilterCriteria.builder().build(), + EnumSet.of(DeviceFilterFacet.STATUSES, DeviceFilterFacet.FILTERED_COUNT)) + .join(); + + assertThat(result.getStatuses()).hasSize(1); + assertThat(result.getFilteredCount()).isEqualTo(3); + verify(pinotDeviceRepository, never()) + .getDeviceTypeFilterOptions(anyString(), any(), any(), any(), any(), any(), any()); + verify(pinotDeviceRepository, never()) + .getOsTypeFilterOptions(anyString(), any(), any(), any(), any(), any(), any()); + verify(pinotDeviceRepository, never()) + .getOrganizationFilterOptions(anyString(), any(), any(), any(), any(), any(), any()); + verify(pinotDeviceRepository, never()) + .getTagKeyFilterOptions(anyString(), any(), any(), any(), any(), any(), any()); + } + + @Test + void nullFacetSet_keepsTheOldAllFacetsBehaviour() { + service().getDeviceFilters(DeviceFilterCriteria.builder().build(), null).join(); + + verify(pinotDeviceRepository).getStatusFilterOptions(anyString(), any(), any(), any(), any(), any(), any()); + verify(pinotDeviceRepository).getDeviceTypeFilterOptions(anyString(), any(), any(), any(), any(), any(), any()); + verify(pinotDeviceRepository).getOsTypeFilterOptions(anyString(), any(), any(), any(), any(), any(), any()); + verify(pinotDeviceRepository).getOrganizationFilterOptions(anyString(), any(), any(), any(), any(), any(), any()); + verify(pinotDeviceRepository).getTagKeyFilterOptions(anyString(), any(), any(), any(), any(), any(), any()); + verify(pinotDeviceRepository).getFilteredDeviceCount(anyString(), any(), any(), any(), any(), any(), any()); + } + + @Test + void singleArgOverload_stillRequestsEveryFacet() { + service().getDeviceFilters(DeviceFilterCriteria.builder().build()).join(); + + verify(pinotDeviceRepository).getStatusFilterOptions(anyString(), any(), any(), any(), any(), any(), any()); + verify(pinotDeviceRepository).getFilteredDeviceCount(anyString(), any(), any(), any(), any(), any(), any()); + } +} diff --git a/openframe-api-service-core/src/main/java/com/openframe/api/datafetcher/DeviceDataFetcher.java b/openframe-api-service-core/src/main/java/com/openframe/api/datafetcher/DeviceDataFetcher.java index 684129b4b..db3fbbe42 100644 --- a/openframe-api-service-core/src/main/java/com/openframe/api/datafetcher/DeviceDataFetcher.java +++ b/openframe-api-service-core/src/main/java/com/openframe/api/datafetcher/DeviceDataFetcher.java @@ -11,6 +11,8 @@ import com.openframe.api.dto.GenericEdge; import com.openframe.api.dto.device.DeviceFilterCriteria; import com.openframe.api.dto.device.DeviceFilterInput; +import com.openframe.api.dto.device.DeviceFilterCriteria; +import com.openframe.api.dto.device.DeviceFilterFacet; import com.openframe.api.dto.device.DeviceFilters; import com.openframe.api.dto.shared.ConnectionArgs; import com.openframe.api.dto.shared.CursorPaginationCriteria; @@ -26,6 +28,7 @@ import com.openframe.data.document.tag.Tag; import com.openframe.data.document.tool.ToolConnection; import graphql.relay.Relay; +import graphql.schema.DataFetchingFieldSelectionSet; import jakarta.validation.Valid; import jakarta.validation.constraints.NotBlank; import lombok.RequiredArgsConstructor; @@ -33,7 +36,9 @@ import org.dataloader.DataLoader; import org.springframework.validation.annotation.Validated; +import java.util.EnumSet; import java.util.List; +import java.util.Set; import java.util.concurrent.CompletableFuture; @DgsComponent @@ -50,11 +55,38 @@ public class DeviceDataFetcher { private final GraphQLDeviceMapper mapper; @DgsQuery - public CompletableFuture deviceFilters(@InputArgument @Valid DeviceFilterInput filter) { - log.debug("Fetching device filters with filter: {}", filter); + public CompletableFuture deviceFilters(@InputArgument @Valid DeviceFilterInput filter, + DgsDataFetchingEnvironment dfe) { DeviceFilterCriteria filterOptions = mapper.toDeviceFilterCriteria(filter); + Set facets = requestedFacets(dfe); + log.debug("Fetching device filters with filter: {}, facets: {}", filter, facets); + + return deviceFilterService.getDeviceFilters(filterOptions, facets); + } - return deviceFilterService.getDeviceFilters(filterOptions); + /** + * The {@code DeviceFilters} fields this query actually selected. + * + * Each facet is its own Pinot round trip, and the callers are lopsided: the filter UI asks for + * all six, while the onboarding auto-detect asks for {@code filteredCount} alone and the + * dashboard counters for two. Resolving the whole object regardless of the selection set meant + * a one-integer query still paid for six round trips. + * + * Falls back to every facet when there is no selection set to read, so a caller that somehow + * arrives without one keeps the old, complete result rather than an empty one. + */ + private static Set requestedFacets(DgsDataFetchingEnvironment dfe) { + DataFetchingFieldSelectionSet selectionSet = dfe != null ? dfe.getSelectionSet() : null; + if (selectionSet == null) { + return DeviceFilterFacet.ALL; + } + EnumSet facets = EnumSet.noneOf(DeviceFilterFacet.class); + for (DeviceFilterFacet facet : DeviceFilterFacet.values()) { + if (selectionSet.contains(facet.graphQlField())) { + facets.add(facet); + } + } + return facets; } @DgsQuery diff --git a/openframe-api-service-core/src/test/java/com/openframe/api/datafetcher/DeviceFiltersSelectionSetTest.java b/openframe-api-service-core/src/test/java/com/openframe/api/datafetcher/DeviceFiltersSelectionSetTest.java new file mode 100644 index 000000000..2ffd7e2d1 --- /dev/null +++ b/openframe-api-service-core/src/test/java/com/openframe/api/datafetcher/DeviceFiltersSelectionSetTest.java @@ -0,0 +1,170 @@ +package com.openframe.api.datafetcher; + +import com.netflix.graphql.dgs.DgsDataFetchingEnvironment; +import com.openframe.api.dto.device.DeviceFilterCriteria; +import com.openframe.api.dto.device.DeviceFilterFacet; +import com.openframe.api.dto.device.DeviceFilters; +import com.openframe.api.mapper.GraphQLDeviceMapper; +import com.openframe.api.service.DeviceFilterService; +import com.openframe.api.service.DeviceService; +import com.openframe.api.service.TagService; +import graphql.ExecutionResult; +import graphql.GraphQL; +import graphql.schema.GraphQLSchema; +import graphql.schema.idl.RuntimeWiring; +import graphql.schema.idl.SchemaGenerator; +import graphql.schema.idl.SchemaParser; +import graphql.schema.idl.TypeDefinitionRegistry; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.ArgumentCaptor; +import org.mockito.Captor; +import org.mockito.Mock; +import org.mockito.junit.jupiter.MockitoExtension; + +import java.io.InputStream; +import java.nio.charset.StandardCharsets; +import java.util.Set; +import java.util.concurrent.CompletableFuture; + +import static com.openframe.api.dto.device.DeviceFilterFacet.DEVICE_TYPES; +import static com.openframe.api.dto.device.DeviceFilterFacet.FILTERED_COUNT; +import static com.openframe.api.dto.device.DeviceFilterFacet.ORGANIZATION_IDS; +import static com.openframe.api.dto.device.DeviceFilterFacet.OS_TYPES; +import static com.openframe.api.dto.device.DeviceFilterFacet.STATUSES; +import static com.openframe.api.dto.device.DeviceFilterFacet.TAG_KEYS; +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.when; + +/** + * The narrowing in {@link DeviceDataFetcher#deviceFilters} rests entirely on + * {@code DataFetchingFieldSelectionSet.contains(field)} answering correctly. If it under-reports, + * a facet is silently skipped and the UI shows zeroes with no error anywhere — so this drives a + * REAL graphql-java execution rather than a hand-built selection set, and covers the two shapes a + * unit test would miss: fragments (which is how every Relay document is compiled) and aliases. + */ +@ExtendWith(MockitoExtension.class) +class DeviceFiltersSelectionSetTest { + + /** + * Mirrors the {@code DeviceFilters} block of {@code schema/device.graphqls}, with the fields + * made nullable so the stub resolver doesn't have to populate them. + * {@link #everyFacetFieldIsRealInTheProductionSchema()} guards against drift. + */ + private static final String SDL = """ + type Query { deviceFilters: DeviceFilters } + type DeviceFilters { + statuses: [DeviceFilterOption] + deviceTypes: [DeviceFilterOption] + osTypes: [DeviceFilterOption] + organizationIds: [DeviceFilterOption] + tagKeys: [TagFilterOption] + filteredCount: Int + } + type DeviceFilterOption { value: String label: String count: Int } + type TagFilterOption { key: String value: String count: Int } + """; + + @Mock private DeviceService deviceService; + @Mock private DeviceFilterService deviceFilterService; + @Mock private TagService tagService; + @Mock private GraphQLDeviceMapper mapper; + + @Captor private ArgumentCaptor> facetsCaptor; + + private GraphQL graphQL; + + @BeforeEach + void setUp() { + DeviceDataFetcher dataFetcher = new DeviceDataFetcher(deviceService, deviceFilterService, tagService, mapper); + + TypeDefinitionRegistry registry = new SchemaParser().parse(SDL); + RuntimeWiring wiring = RuntimeWiring.newRuntimeWiring() + .type("Query", builder -> builder.dataFetcher("deviceFilters", + env -> dataFetcher.deviceFilters(null, new DgsDataFetchingEnvironment(env)))) + .build(); + GraphQLSchema schema = new SchemaGenerator().makeExecutableSchema(registry, wiring); + graphQL = GraphQL.newGraphQL(schema).build(); + } + + /** + * Stubbed here rather than in {@code setUp} so the schema-drift test, which never executes a + * query, doesn't trip Mockito's strict unnecessary-stubbing check. + */ + private Set facetsFor(String query) { + when(deviceFilterService.getDeviceFilters(any(DeviceFilterCriteria.class), facetsCaptor.capture())) + .thenReturn(CompletableFuture.completedFuture(DeviceFilters.builder().build())); + when(mapper.toDeviceFilterCriteria(any())).thenReturn(DeviceFilterCriteria.builder().build()); + + ExecutionResult result = graphQL.execute(query); + assertThat(result.getErrors()).isEmpty(); + return facetsCaptor.getValue(); + } + + @Test + void filteredCountOnly_narrowsToOneFacet() { + // The onboarding auto-detect query. Used to cost six Pinot round trips for one integer. + assertThat(facetsFor("{ deviceFilters { filteredCount } }")).containsExactly(FILTERED_COUNT); + } + + @Test + void dashboardCounterSelection_narrowsToTwoFacets() { + assertThat(facetsFor("{ deviceFilters { statuses { value count } filteredCount } }")) + .containsExactlyInAnyOrder(STATUSES, FILTERED_COUNT); + } + + @Test + void customersOverviewSelection_narrowsToOneFacet() { + assertThat(facetsFor("{ deviceFilters { organizationIds { value count } } }")) + .containsExactly(ORGANIZATION_IDS); + } + + @Test + void filterUiSelection_stillGetsEveryFacet() { + assertThat(facetsFor(""" + { deviceFilters { + statuses { value count } + deviceTypes { value count } + osTypes { value count } + organizationIds { value count } + tagKeys { key value count } + filteredCount + } }""")) + .containsExactlyInAnyOrder(STATUSES, DEVICE_TYPES, OS_TYPES, ORGANIZATION_IDS, TAG_KEYS, FILTERED_COUNT); + } + + @Test + void fieldsInsideFragments_areDetected() { + // Relay compiles its documents into fragment spreads, so a selection-set check that only + // saw inline fields would silently drop every facet the device filter UI asks for. + assertThat(facetsFor(""" + { deviceFilters { ...counts ... on DeviceFilters { organizationIds { value } } } } + fragment counts on DeviceFilters { statuses { value } filteredCount }""")) + .containsExactlyInAnyOrder(STATUSES, FILTERED_COUNT, ORGANIZATION_IDS); + } + + @Test + void aliasedFields_areDetectedByTheirRealName() { + assertThat(facetsFor("{ deviceFilters { byStatus: statuses { value } total: filteredCount } }")) + .containsExactlyInAnyOrder(STATUSES, FILTERED_COUNT); + } + + @Test + void everyFacetFieldIsRealInTheProductionSchema() throws Exception { + String schema; + try (InputStream in = getClass().getResourceAsStream("/schema/device.graphqls")) { + assertThat(in).as("schema/device.graphqls on the test classpath").isNotNull(); + schema = new String(in.readAllBytes(), StandardCharsets.UTF_8); + } + String deviceFiltersType = schema.substring(schema.indexOf("type DeviceFilters {")); + deviceFiltersType = deviceFiltersType.substring(0, deviceFiltersType.indexOf('}')); + + for (DeviceFilterFacet facet : DeviceFilterFacet.values()) { + assertThat(deviceFiltersType) + .as("DeviceFilterFacet.%s maps to a field that must exist on type DeviceFilters", facet) + .contains(facet.graphQlField() + ":"); + } + } +} From 048f304b16a5d3e21faece34da766a786f0d0534 Mon Sep 17 00:00:00 2001 From: Oleksandr Didukh Date: Fri, 7 Aug 2026 18:14:50 +0200 Subject: [PATCH 2/2] chore(api-lib): Doc updates; make DeviceFilterFacet unmodifiable --- .../config/DeviceFilterExecutorConfig.java | 24 ++++++------------- .../api/dto/device/DeviceFilterFacet.java | 4 +++- .../api/service/DeviceFilterService.java | 2 +- 3 files changed, 11 insertions(+), 19 deletions(-) diff --git a/openframe-api-lib/src/main/java/com/openframe/api/config/DeviceFilterExecutorConfig.java b/openframe-api-lib/src/main/java/com/openframe/api/config/DeviceFilterExecutorConfig.java index 6401b14b6..6170add43 100644 --- a/openframe-api-lib/src/main/java/com/openframe/api/config/DeviceFilterExecutorConfig.java +++ b/openframe-api-lib/src/main/java/com/openframe/api/config/DeviceFilterExecutorConfig.java @@ -9,23 +9,13 @@ import java.util.concurrent.ThreadPoolExecutor; /** - * Thread pool for the {@code deviceFilters} facet fan-out. - * - * The fan-out in {@code DeviceFilterService} used {@code CompletableFuture.supplyAsync} with no - * executor, i.e. {@link java.util.concurrent.ForkJoinPool#commonPool()}, whose parallelism is - * {@code availableProcessors() - 1}. Tenant pods run on a 700m CPU limit, so the JVM reports ONE - * processor ({@code system.cpu.count = 1}) and the common pool has at most one worker — the six - * "parallel" facet queries actually ran one after another on the request thread, and the - * {@code allOf} join was a no-op. Raising the CPU limit does not fix this: at 2 CPUs the common - * pool's parallelism is still 1. It needs its own pool. - * - * The tasks are blocking HTTP calls to the Pinot broker, not CPU work, so the pool is sized for - * concurrent round trips rather than cores — a single-core pod can hold six of these in flight - * for the cost of six idle threads. - * - * Bounded queue + {@link ThreadPoolExecutor.CallerRunsPolicy}: under saturation the work runs on - * the calling request thread, which is exactly today's behaviour, so overload degrades to "slow" - * instead of "rejected". + * Thread pool for the {@code deviceFilters} facet fan-out. The facet queries are blocking HTTP + * calls to the Pinot broker, so {@code supplyAsync} without an executor is the wrong default: + * on single-core tenant pods it spawns a new unpooled thread per task, and on larger hosts it + * would block the shared {@code ForkJoinPool.commonPool()}. A dedicated pool sized for + * concurrent round trips (not cores) gives reused, bounded threads; under saturation + * {@link ThreadPoolExecutor.CallerRunsPolicy} degrades to running on the request thread + * instead of rejecting. */ @Configuration public class DeviceFilterExecutorConfig { diff --git a/openframe-api-lib/src/main/java/com/openframe/api/dto/device/DeviceFilterFacet.java b/openframe-api-lib/src/main/java/com/openframe/api/dto/device/DeviceFilterFacet.java index 4633b2f4e..b0d9ba6f4 100644 --- a/openframe-api-lib/src/main/java/com/openframe/api/dto/device/DeviceFilterFacet.java +++ b/openframe-api-lib/src/main/java/com/openframe/api/dto/device/DeviceFilterFacet.java @@ -1,5 +1,6 @@ package com.openframe.api.dto.device; +import java.util.Collections; import java.util.EnumSet; import java.util.Set; @@ -24,7 +25,8 @@ public enum DeviceFilterFacet { FILTERED_COUNT("filteredCount"); /** Every facet — the behaviour non-GraphQL callers (the external REST API) still want. */ - public static final Set ALL = EnumSet.allOf(DeviceFilterFacet.class); + public static final Set ALL = + Collections.unmodifiableSet(EnumSet.allOf(DeviceFilterFacet.class)); private final String graphQlField; diff --git a/openframe-api-lib/src/main/java/com/openframe/api/service/DeviceFilterService.java b/openframe-api-lib/src/main/java/com/openframe/api/service/DeviceFilterService.java index 038d9a23a..d1f4aa097 100644 --- a/openframe-api-lib/src/main/java/com/openframe/api/service/DeviceFilterService.java +++ b/openframe-api-lib/src/main/java/com/openframe/api/service/DeviceFilterService.java @@ -112,7 +112,7 @@ public CompletableFuture getDeviceFilters(DeviceFilterCriteria fi * Submits one facet query, or short-circuits to {@code null} when the caller didn't ask for it. * * The executor is explicit on purpose — see {@link DeviceFilterExecutorConfig} for why the - * default {@code ForkJoinPool.commonPool()} silently ran these serially on tenant pods. + * {@code supplyAsync} default is wrong for these blocking Pinot calls. */ private CompletableFuture facetQuery(Set facets, DeviceFilterFacet facet,