This is an automated email from the ASF dual-hosted git repository. sergehuber pushed a commit to branch UNOMI-964-graphql-condition-factory-flake in repository https://gitbox.apache.org/repos/asf/unomi.git
commit f4705e11185a9930edc573433c8d0db8bedd3887 Author: Serge Huber <[email protected]> AuthorDate: Wed Jul 15 10:26:38 2026 +0200 UNOMI-964: Fix flaky GraphQLListIT via live condition-type resolution The GraphQL condition factories (Profile/Event/ProfileAlias/Topic) were lazy static singletons whose base ConditionFactory snapshotted getAllConditionTypes() once. Under PaxExam @PerSuite that snapshot was frozen for the whole container, captured at first get(); if taken before the condition-type cache was warm, getConditionType("profilePropertyCondition") returned null, producing a match-none active-member query for the rest of the run. This is why prior timing-based fixes never worked. - Remove the static singletons; return a fresh factory instance per get(). - Drop the conditionTypesMap snapshot; resolve types live via DefinitionsService.getConditionType(id). Also removes the retained first-request DataFetchingEnvironment/ServiceManager. - Harden AddProfileToListCommand: use (send(event) & PROFILE_UPDATED) == PROFILE_UPDATED instead of exact equality (latent bitmask bug). - Revert the misleading timing workarounds/comment in GraphQLListIT.testCRUD (deferred refresh + tripled retries). - Add unit tests for live condition-type resolution, fresh-per-get factories, and the AddProfileToListCommand bitmask handling. --- .../commands/list/AddProfileToListCommand.java | 2 +- .../condition/factories/ConditionFactory.java | 7 +- .../condition/factories/EventConditionFactory.java | 9 +- .../factories/ProfileAliasConditionFactory.java | 10 +- .../factories/ProfileConditionFactory.java | 9 +- .../condition/factories/TopicConditionFactory.java | 10 +- .../commands/list/AddProfileToListCommandTest.java | 125 +++++++++++++++++++++ .../condition/factories/ConditionFactoryTest.java | 103 +++++++++++++++++ .../apache/unomi/itests/graphql/GraphQLListIT.java | 18 ++- 9 files changed, 245 insertions(+), 48 deletions(-) diff --git a/graphql/cxs-impl/src/main/java/org/apache/unomi/graphql/commands/list/AddProfileToListCommand.java b/graphql/cxs-impl/src/main/java/org/apache/unomi/graphql/commands/list/AddProfileToListCommand.java index 4c7e69e77..f254d1638 100644 --- a/graphql/cxs-impl/src/main/java/org/apache/unomi/graphql/commands/list/AddProfileToListCommand.java +++ b/graphql/cxs-impl/src/main/java/org/apache/unomi/graphql/commands/list/AddProfileToListCommand.java @@ -66,7 +66,7 @@ public class AddProfileToListCommand extends BaseCommand<CDPList> { .setPersistent(true) .build(); - if (serviceManager.getService(EventService.class).send(event) == EventService.PROFILE_UPDATED) { + if ((serviceManager.getService(EventService.class).send(event) & EventService.PROFILE_UPDATED) == EventService.PROFILE_UPDATED) { profileService.save(event.getProfile()); } diff --git a/graphql/cxs-impl/src/main/java/org/apache/unomi/graphql/condition/factories/ConditionFactory.java b/graphql/cxs-impl/src/main/java/org/apache/unomi/graphql/condition/factories/ConditionFactory.java index 87a6384b6..13e4097e8 100644 --- a/graphql/cxs-impl/src/main/java/org/apache/unomi/graphql/condition/factories/ConditionFactory.java +++ b/graphql/cxs-impl/src/main/java/org/apache/unomi/graphql/condition/factories/ConditionFactory.java @@ -46,17 +46,12 @@ public class ConditionFactory { protected String conditionTypeId; - private Map<String, ConditionType> conditionTypesMap; - public ConditionFactory(final String conditionTypeId, final DataFetchingEnvironment environment) { this.environment = environment; this.conditionTypeId = conditionTypeId; final ServiceManager context = environment.getContext(); this.definitionsService = context.getService(DefinitionsService.class); - - this.conditionTypesMap = definitionsService.getAllConditionTypes().stream() - .collect(Collectors.toMap(ConditionType::getItemId, Function.identity())); } public Condition matchAllCondition() { @@ -127,7 +122,7 @@ public class ConditionFactory { } public ConditionType getConditionType(final String typeId) { - return this.conditionTypesMap.get(typeId); + return definitionsService.getConditionType(typeId); } public <INPUT> Condition filtersToCondition( diff --git a/graphql/cxs-impl/src/main/java/org/apache/unomi/graphql/condition/factories/EventConditionFactory.java b/graphql/cxs-impl/src/main/java/org/apache/unomi/graphql/condition/factories/EventConditionFactory.java index d9bc0c8a0..734f236f5 100644 --- a/graphql/cxs-impl/src/main/java/org/apache/unomi/graphql/condition/factories/EventConditionFactory.java +++ b/graphql/cxs-impl/src/main/java/org/apache/unomi/graphql/condition/factories/EventConditionFactory.java @@ -38,13 +38,8 @@ import java.util.stream.Collectors; public class EventConditionFactory extends ConditionFactory { - private static EventConditionFactory instance; - - public static synchronized EventConditionFactory get(final DataFetchingEnvironment environment) { - if (instance == null) { - instance = new EventConditionFactory(environment); - } - return instance; + public static EventConditionFactory get(final DataFetchingEnvironment environment) { + return new EventConditionFactory(environment); } private EventConditionFactory(final DataFetchingEnvironment environment) { diff --git a/graphql/cxs-impl/src/main/java/org/apache/unomi/graphql/condition/factories/ProfileAliasConditionFactory.java b/graphql/cxs-impl/src/main/java/org/apache/unomi/graphql/condition/factories/ProfileAliasConditionFactory.java index 28c6340aa..3fdcad1bf 100644 --- a/graphql/cxs-impl/src/main/java/org/apache/unomi/graphql/condition/factories/ProfileAliasConditionFactory.java +++ b/graphql/cxs-impl/src/main/java/org/apache/unomi/graphql/condition/factories/ProfileAliasConditionFactory.java @@ -26,14 +26,8 @@ import java.util.Map; public class ProfileAliasConditionFactory extends ConditionFactory { - private static ProfileAliasConditionFactory instance; - - public static synchronized ProfileAliasConditionFactory get(final DataFetchingEnvironment environment) { - if (instance == null) { - instance = new ProfileAliasConditionFactory(environment); - } - - return instance; + public static ProfileAliasConditionFactory get(final DataFetchingEnvironment environment) { + return new ProfileAliasConditionFactory(environment); } private ProfileAliasConditionFactory(final DataFetchingEnvironment environment) { diff --git a/graphql/cxs-impl/src/main/java/org/apache/unomi/graphql/condition/factories/ProfileConditionFactory.java b/graphql/cxs-impl/src/main/java/org/apache/unomi/graphql/condition/factories/ProfileConditionFactory.java index 7fd7666ae..727d5ceec 100644 --- a/graphql/cxs-impl/src/main/java/org/apache/unomi/graphql/condition/factories/ProfileConditionFactory.java +++ b/graphql/cxs-impl/src/main/java/org/apache/unomi/graphql/condition/factories/ProfileConditionFactory.java @@ -39,13 +39,8 @@ import java.util.stream.Collectors; public class ProfileConditionFactory extends ConditionFactory { - private static ProfileConditionFactory instance; - - public static synchronized ProfileConditionFactory get(final DataFetchingEnvironment environment) { - if (instance == null) { - instance = new ProfileConditionFactory(environment); - } - return instance; + public static ProfileConditionFactory get(final DataFetchingEnvironment environment) { + return new ProfileConditionFactory(environment); } private ProfileConditionFactory(final DataFetchingEnvironment environment) { diff --git a/graphql/cxs-impl/src/main/java/org/apache/unomi/graphql/condition/factories/TopicConditionFactory.java b/graphql/cxs-impl/src/main/java/org/apache/unomi/graphql/condition/factories/TopicConditionFactory.java index 9c683619d..d75131dd3 100644 --- a/graphql/cxs-impl/src/main/java/org/apache/unomi/graphql/condition/factories/TopicConditionFactory.java +++ b/graphql/cxs-impl/src/main/java/org/apache/unomi/graphql/condition/factories/TopicConditionFactory.java @@ -26,14 +26,8 @@ import java.util.Map; public class TopicConditionFactory extends ConditionFactory { - private static TopicConditionFactory instance; - - public static synchronized TopicConditionFactory get(final DataFetchingEnvironment environment) { - if (instance == null) { - instance = new TopicConditionFactory(environment); - } - - return instance; + public static TopicConditionFactory get(final DataFetchingEnvironment environment) { + return new TopicConditionFactory(environment); } private TopicConditionFactory(final DataFetchingEnvironment environment) { diff --git a/graphql/cxs-impl/src/test/java/org/apache/unomi/graphql/commands/list/AddProfileToListCommandTest.java b/graphql/cxs-impl/src/test/java/org/apache/unomi/graphql/commands/list/AddProfileToListCommandTest.java new file mode 100644 index 000000000..c57c9438d --- /dev/null +++ b/graphql/cxs-impl/src/test/java/org/apache/unomi/graphql/commands/list/AddProfileToListCommandTest.java @@ -0,0 +1,125 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.unomi.graphql.commands.list; + +import graphql.schema.DataFetchingEnvironment; +import org.apache.unomi.api.Event; +import org.apache.unomi.api.Profile; +import org.apache.unomi.api.services.EventService; +import org.apache.unomi.api.services.ProfileService; +import org.apache.unomi.graphql.services.ServiceManager; +import org.apache.unomi.graphql.types.input.CDPProfileIDInput; +import org.apache.unomi.lists.UserList; +import org.apache.unomi.services.UserListService; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.Mock; +import org.mockito.junit.jupiter.MockitoExtension; + +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.doReturn; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +/** + * Guards the UNOMI-964 bitmask fix in {@link AddProfileToListCommand}. {@link EventService#send} + * returns a bitmask; the profile must be persisted whenever the PROFILE_UPDATED bit is set, + * even when it is OR-ed with other change flags. The previous {@code == PROFILE_UPDATED} check + * silently dropped the save whenever another flag was also set. + */ +@ExtendWith(MockitoExtension.class) +class AddProfileToListCommandTest { + + private static final String LIST_ID = "testListId"; + private static final String PROFILE_ID = "test_profile_id"; + + @Mock + private DataFetchingEnvironment environment; + @Mock + private ServiceManager serviceManager; + @Mock + private UserListService userListService; + @Mock + private ProfileService profileService; + @Mock + private EventService eventService; + + private Profile profile; + + @BeforeEach + void setUp() { + doReturn(serviceManager).when(environment).getContext(); + when(serviceManager.getService(UserListService.class)).thenReturn(userListService); + when(serviceManager.getService(ProfileService.class)).thenReturn(profileService); + when(serviceManager.getService(EventService.class)).thenReturn(eventService); + + final UserList userList = new UserList(); + userList.setItemId(LIST_ID); + when(userListService.load(LIST_ID)).thenReturn(userList); + + profile = new Profile(PROFILE_ID); + when(profileService.load(PROFILE_ID)).thenReturn(profile); + } + + private AddProfileToListCommand command() { + return AddProfileToListCommand.create() + .listId(LIST_ID) + .profileIDInput(new CDPProfileIDInput(PROFILE_ID, null)) + .setEnvironment(environment) + .build(); + } + + @Test + void savesProfile_whenEventReturnsProfileUpdatedExactly() { + when(eventService.send(any(Event.class))).thenReturn(EventService.PROFILE_UPDATED); + + command().execute(); + + verify(profileService).save(profile); + } + + @Test + void savesProfile_whenProfileUpdatedBitIsCombinedWithOtherFlags() { + when(eventService.send(any(Event.class))) + .thenReturn(EventService.PROFILE_UPDATED | EventService.SESSION_UPDATED); + + command().execute(); + + verify(profileService).save(profile); + } + + @Test + void doesNotSaveProfile_whenEventReportsNoChange() { + when(eventService.send(any(Event.class))).thenReturn(EventService.NO_CHANGE); + + command().execute(); + + verify(profileService, never()).save(any(Profile.class)); + } + + @Test + void doesNotSaveProfile_whenProfileUpdatedBitIsNotSet() { + when(eventService.send(any(Event.class))).thenReturn(EventService.SESSION_UPDATED); + + command().execute(); + + verify(profileService, never()).save(any(Profile.class)); + } +} diff --git a/graphql/cxs-impl/src/test/java/org/apache/unomi/graphql/condition/factories/ConditionFactoryTest.java b/graphql/cxs-impl/src/test/java/org/apache/unomi/graphql/condition/factories/ConditionFactoryTest.java new file mode 100644 index 000000000..cef87edb8 --- /dev/null +++ b/graphql/cxs-impl/src/test/java/org/apache/unomi/graphql/condition/factories/ConditionFactoryTest.java @@ -0,0 +1,103 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.unomi.graphql.condition.factories; + +import graphql.schema.DataFetchingEnvironment; +import org.apache.unomi.api.conditions.ConditionType; +import org.apache.unomi.api.services.DefinitionsService; +import org.apache.unomi.graphql.services.ServiceManager; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.Mock; +import org.mockito.junit.jupiter.MockitoExtension; + +import static org.junit.jupiter.api.Assertions.assertNotSame; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertSame; +import static org.mockito.Mockito.doReturn; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +/** + * Guards the UNOMI-964 fix: the GraphQL condition factories must resolve condition types + * live from {@link DefinitionsService} (not from a frozen snapshot) and must not be cached + * as static singletons. A stale snapshot captured before the condition-type cache was warm + * produced null condition types, which the ES dispatcher turned into match-none queries. + */ +@ExtendWith(MockitoExtension.class) +class ConditionFactoryTest { + + @Mock + private DataFetchingEnvironment environment; + @Mock + private ServiceManager serviceManager; + @Mock + private DefinitionsService definitionsService; + + @BeforeEach + void setUp() { + doReturn(serviceManager).when(environment).getContext(); + when(serviceManager.getService(DefinitionsService.class)).thenReturn(definitionsService); + } + + @Test + void getConditionType_resolvesLiveFromDefinitionsService() { + final ConditionFactory factory = new ConditionFactory("profilePropertyCondition", environment); + + final ConditionType profilePropertyCondition = mock(ConditionType.class); + // First lookup happens before the definition is registered, second after: a live + // resolver must reflect the change without the factory being recreated. + when(definitionsService.getConditionType("profilePropertyCondition")) + .thenReturn(null, profilePropertyCondition); + + assertNull(factory.getConditionType("profilePropertyCondition"), + "Type should be absent while the definition cache is still cold"); + assertSame(profilePropertyCondition, factory.getConditionType("profilePropertyCondition"), + "Type should be visible once the definition is registered (live resolution)"); + } + + @Test + void constructor_doesNotSnapshotAllConditionTypes() { + new ConditionFactory("profilePropertyCondition", environment); + + verify(definitionsService, never()).getAllConditionTypes(); + } + + @Test + void profileConditionFactory_returnsFreshInstancePerGet() { + assertNotSame(ProfileConditionFactory.get(environment), ProfileConditionFactory.get(environment)); + } + + @Test + void eventConditionFactory_returnsFreshInstancePerGet() { + assertNotSame(EventConditionFactory.get(environment), EventConditionFactory.get(environment)); + } + + @Test + void profileAliasConditionFactory_returnsFreshInstancePerGet() { + assertNotSame(ProfileAliasConditionFactory.get(environment), ProfileAliasConditionFactory.get(environment)); + } + + @Test + void topicConditionFactory_returnsFreshInstancePerGet() { + assertNotSame(TopicConditionFactory.get(environment), TopicConditionFactory.get(environment)); + } +} diff --git a/itests/src/test/java/org/apache/unomi/itests/graphql/GraphQLListIT.java b/itests/src/test/java/org/apache/unomi/itests/graphql/GraphQLListIT.java index b75a83ecb..7b378948e 100644 --- a/itests/src/test/java/org/apache/unomi/itests/graphql/GraphQLListIT.java +++ b/itests/src/test/java/org/apache/unomi/itests/graphql/GraphQLListIT.java @@ -75,18 +75,15 @@ public class GraphQLListIT extends BaseGraphQLIT { Assert.assertEquals("testListId", context.getValue("data.cdp.addProfileToList.id")); } - refreshPersistence(UserList.class); + refreshPersistence(UserList.class, Profile.class); - // Profile.class refresh is deferred into the retry loop: addProfileToList processes - // list membership via an async rule-engine event, so the profile write may not be - // visible yet when we first reach this point. + // The list membership is written to the profile synchronously by addProfileToList, + // then made searchable via the index refresh above; keepTrying only absorbs normal + // Elasticsearch refresh latency. final ResponseContext findListsContext = keepTrying("Failed waiting for profile in list query", () -> { - try { - refreshPersistence(Profile.class); - try (CloseableHttpResponse response = post("graphql/list/find-lists.json")) { - return ResponseContext.parse(response.getEntity()); - } + try (CloseableHttpResponse response = post("graphql/list/find-lists.json")) { + return ResponseContext.parse(response.getEntity()); } catch (Exception e) { return null; } @@ -102,8 +99,7 @@ public class GraphQLListIT extends BaseGraphQLIT { Object profileId = context.getValue("data.cdp.findLists.edges[0].node.active.edges[0].node.cdp_profileIDs[0].id"); return profile.getItemId().equals(profileId); }, - // async rule-engine processing needs more retries on loaded CI - DEFAULT_TRYING_TIMEOUT, DEFAULT_TRYING_TRIES * 3); + DEFAULT_TRYING_TIMEOUT, DEFAULT_TRYING_TRIES); Assert.assertEquals("testListId", findListsContext.getValue("data.cdp.findLists.edges[0].node.id"));
