Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -332,6 +332,19 @@ public static boolean assignTeamsFromClaim(User user, List<String> teamNames) {
*/
public static User addOrUpdateBotUser(User user) {
User originalUser = retrieveWithAuthMechanism(user);
// Short-circuit when the incoming bot user has no real change vs. what's already in the
// database. Without this guard every OM boot calls into addOrUpdateUser ->
// userRepository.createOrUpdate, and UserUpdater.entitySpecificUpdate then runs
// updateTeams/updateRoles/etc. with the incoming `user.getTeams() == null`, which strips
// the bot's stored team relationships, bumps the version, and triggers an Elasticsearch
// reindex of the bot user (and any team membership change ripples into the team_search
// index too). With many bots this is a reindex storm on every restart.
if (originalUser != null
&& Objects.equals(listOrEmpty(originalUser.getRoles()), listOrEmpty(user.getRoles()))
&& Objects.equals(originalUser.getDescription(), user.getDescription())
&& Objects.equals(originalUser.getDisplayName(), user.getDisplayName())) {
return originalUser;
Comment on lines +342 to +346
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Edge Case: Short-circuit doesn't compare email field changes

The guard at line 342-346 compares roles, description, and displayName but not email. The UserUtil.user(...) method (which builds the in-memory bot user) may set an email based on domain. If an admin changes the domain configuration between restarts, the email update would be silently skipped by the short-circuit. Consider whether email should be included in the comparison, or document why it's excluded.

Was this helpful? React with 👍 / 👎

}
AuthenticationMechanism authMechanism =
originalUser != null ? originalUser.getAuthenticationMechanism() : null;
// the user did not have an auth mechanism and auth config is present
Expand Down Expand Up @@ -362,8 +375,12 @@ private static User retrieveWithAuthMechanism(User user) {
EntityRepository<User> userRepository =
(UserRepository) Entity.getEntityRepository(Entity.USER);
try {
// Include "roles" so the no-op short-circuit in addOrUpdateBotUser can compare it
// against the incoming user. description and displayName are scalar fields stored in
// the entity JSON column, so they're already populated by the base read without an
// extra field fetch.
return userRepository.getByName(
null, user.getName(), new Fields(Set.of("authenticationMechanism")));
null, user.getName(), new Fields(Set.of("authenticationMechanism", "roles")));
} catch (EntityNotFoundException e) {
LOG.debug("Bot entity: {} does not exists.", user);
return null;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,146 @@
/*
* Copyright 2026 Collate
* Licensed 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.openmetadata.service.util;

import static org.junit.jupiter.api.Assertions.assertNotEquals;
import static org.junit.jupiter.api.Assertions.assertSame;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.eq;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.mockStatic;
import static org.mockito.Mockito.never;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;

import java.util.ArrayList;
import java.util.List;
import java.util.UUID;
import org.junit.jupiter.api.Test;
import org.mockito.MockedStatic;
import org.openmetadata.schema.entity.teams.User;
import org.openmetadata.schema.type.EntityReference;
import org.openmetadata.service.Entity;
import org.openmetadata.service.jdbi3.UserRepository;

/**
* Unit coverage for the boot-time bot-team-strip loop.
*
* <p>{@code BotResource.initialize()} calls {@link UserUtil#addOrUpdateBotUser(User)} for every
* bot on every OM boot. The in-memory User built by {@code UserUtil.user(...)} does not have
* the {@code teams} field populated, so without the short-circuit guard the call falls through
* to {@code userRepository.createOrUpdate}, which runs {@code UserUpdater.updateTeams} with
* {@code updated.teams == null} and wipes the bot's stored team relationships, bumps the
* version, and triggers an Elasticsearch reindex on every boot.
*/
class UserUtilBotTest {

@Test
void addOrUpdateBotUserShortCircuitsWhenNothingChanged() {
UserRepository userRepository = mock(UserRepository.class);

User stored =
new User()
.withId(UUID.randomUUID())
.withName("ingestion-bot")
.withFullyQualifiedName("ingestion-bot")
.withDisplayName("ingestion-bot")
.withDescription(null)
.withIsBot(true)
.withRoles(new ArrayList<>())
// Pre-populate authMechanism so that if the short-circuit guard regresses, the
// fall-through path doesn't immediately blow up in JWTTokenGenerator (which
// isn't initialized in a pure unit test). With the bug present, the test still
// proceeds into addOrUpdateUser and the `never()` verify below fires the
// regression signal.
.withAuthenticationMechanism(
new org.openmetadata.schema.entity.teams.AuthenticationMechanism()
.withAuthType(org.openmetadata.schema.entity.teams.AuthenticationMechanism.AuthType.JWT));
Comment on lines +66 to +67
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Quality: Fully qualified class names used instead of imports

The test uses fully qualified names org.openmetadata.schema.entity.teams.AuthenticationMechanism inline (lines 66-67 and 108-109) instead of importing the class. Per project conventions, wildcard imports and fully qualified names should be avoided — add a proper import statement instead.

Import the class and use short names:

// Add to imports section:
import org.openmetadata.schema.entity.teams.AuthenticationMechanism;
import org.openmetadata.schema.entity.teams.AuthenticationMechanism.AuthType;

// Then replace inline usages with:
.withAuthenticationMechanism(
    new AuthenticationMechanism()
        .withAuthType(AuthType.JWT));
  • Apply fix

Check the box to apply the fix or reply for a change | Was this helpful? React with 👍 / 👎

User incoming =
new User()
.withId(UUID.randomUUID())
.withName("ingestion-bot")
.withFullyQualifiedName("ingestion-bot")
.withDisplayName("ingestion-bot")
.withDescription(null)
.withIsBot(true)
.withRoles(null);

when(userRepository.getByName(any(), eq("ingestion-bot"), any())).thenReturn(stored);

try (MockedStatic<Entity> entityStatic = mockStatic(Entity.class)) {
entityStatic.when(() -> Entity.getEntityRepository(Entity.USER)).thenReturn(userRepository);
User result = UserUtil.addOrUpdateBotUser(incoming);
assertSame(
stored,
result,
"Short-circuit must return the original user without going through the PUT path");
}

// The whole point of the fix: never hit createOrUpdate when nothing changed.
verify(userRepository, never()).createOrUpdate(any(), any(), any());
}

@Test
void addOrUpdateBotUserGoesThroughUpsertWhenDisplayNameChanged() {
UserRepository userRepository = mock(UserRepository.class);

User stored =
new User()
.withId(UUID.randomUUID())
.withName("ingestion-bot")
.withFullyQualifiedName("ingestion-bot")
.withDisplayName("Old Display Name")
.withIsBot(true)
// Provide an authMechanism on the persisted row so the upsert path doesn't try
// to generate a fresh JWT via JWTTokenGenerator (which isn't initialized in a
// pure unit test).
.withAuthenticationMechanism(
new org.openmetadata.schema.entity.teams.AuthenticationMechanism()
.withAuthType(org.openmetadata.schema.entity.teams.AuthenticationMechanism.AuthType.JWT));
User incoming =
new User()
.withId(UUID.randomUUID())
.withName("ingestion-bot")
.withFullyQualifiedName("ingestion-bot")
.withDisplayName("New Display Name")
.withIsBot(true);

when(userRepository.getByName(any(), eq("ingestion-bot"), any())).thenReturn(stored);
// Returning null from createOrUpdate path is fine: addOrUpdateUser wraps exceptions but
// we only care that the method was invoked (i.e. the short-circuit did NOT fire).
when(userRepository.findByNameOrNull(any(), any())).thenReturn(null);

try (MockedStatic<Entity> entityStatic = mockStatic(Entity.class)) {
entityStatic.when(() -> Entity.getEntityRepository(Entity.USER)).thenReturn(userRepository);
try {
User result = UserUtil.addOrUpdateBotUser(incoming);
assertNotEquals(
stored,
result,
"When fields differ the upsert path must run and produce a different User");
} catch (RuntimeException ignored) {
// The downstream createOrUpdate call may throw against the mock; the assertion we
// care about is that the short-circuit guard did NOT fire, which we verify below.
}
}
Comment on lines +125 to +135
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Quality: Swallowed RuntimeException uses flow-control exception pattern

The test at line 131 catches RuntimeException ignored to handle the case where the mock doesn't fully stub the downstream path. This is a flow-control exception anti-pattern and makes the test fragile — if addOrUpdateBotUser throws for an unexpected reason, the test silently passes. Consider stubbing createOrUpdate to return a value (or use doNothing()/doReturn(...)) so the method completes normally, and assert on the result instead.

Stub the mock properly instead of catching RuntimeException:

// Stub createOrUpdate to return the incoming user so the method completes normally:
when(userRepository.createOrUpdate(any(), any(User.class), any())).thenReturn(incoming);

try (MockedStatic<Entity> entityStatic = mockStatic(Entity.class)) {
  entityStatic.when(() -> Entity.getEntityRepository(Entity.USER)).thenReturn(userRepository);
  User result = UserUtil.addOrUpdateBotUser(incoming);
  assertNotEquals(
      stored,
      result,
      "When fields differ the upsert path must run and produce a different User");
}
  • Apply fix

Check the box to apply the fix or reply for a change | Was this helpful? React with 👍 / 👎


verify(userRepository).createOrUpdate(any(), any(User.class), any());
}

@SuppressWarnings("unused")
private static List<EntityReference> roleRef(String name) {
List<EntityReference> refs = new ArrayList<>();
refs.add(new EntityReference().withId(UUID.randomUUID()).withName(name).withType("role"));
return refs;
}
Comment on lines +140 to +145
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Quality: Unused private helper method roleRef should be removed

The private method roleRef at lines 140-145 is annotated with @SuppressWarnings("unused") and is never called. Commented-out or dead code should be removed per project conventions. If it's intended for future tests, add it when those tests are written.

Was this helpful? React with 👍 / 👎

}
Loading