Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

APIM 7550 fix get archived applications info #10072

Open
wants to merge 2 commits into
base: 4.2.x
Choose a base branch
from
Open
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 @@ -21,6 +21,7 @@

import io.gravitee.common.http.MediaType;
import io.gravitee.repository.management.api.search.Order;
import io.gravitee.repository.management.model.ApplicationStatus;
import io.gravitee.rest.api.model.ApiKeyMode;
import io.gravitee.rest.api.model.ApplicationEntity;
import io.gravitee.rest.api.model.NewApplicationEntity;
Expand Down Expand Up @@ -242,7 +243,11 @@ protected List<Application> transformPageContent(final ExecutionContext executio
return Collections.emptyList();
}

Set<ApplicationListItem> applicationListItems = applicationService.findByIds(executionContext, pageContent);
Set<ApplicationListItem> applicationListItems = applicationService.findByIdsAndStatus(
executionContext,
pageContent,
ApplicationStatus.ACTIVE
);
Comparator<String> orderingComparator = Comparator.comparingInt(pageContent::indexOf);

return applicationListItems
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@

import io.gravitee.common.http.HttpStatusCode;
import io.gravitee.repository.management.api.search.Order;
import io.gravitee.repository.management.model.ApplicationStatus;
import io.gravitee.rest.api.model.*;
import io.gravitee.rest.api.model.application.ApplicationListItem;
import io.gravitee.rest.api.model.application.ApplicationSettings;
Expand All @@ -37,7 +38,6 @@
import jakarta.ws.rs.core.HttpHeaders;
import jakarta.ws.rs.core.Response;
import java.util.*;
import java.util.stream.Collectors;
import org.junit.Before;
import org.junit.Test;
import org.mockito.ArgumentCaptor;
Expand Down Expand Up @@ -73,14 +73,14 @@ public void init() {
.findIdsByUser(eq(GraviteeContext.getExecutionContext()), any(), any());
doReturn(new HashSet<>(Arrays.asList(applicationA, applicationB)))
.when(applicationService)
.findByIds(eq(GraviteeContext.getExecutionContext()), eq(Arrays.asList("A", "B")));
.findByIdsAndStatus(eq(GraviteeContext.getExecutionContext()), eq(Arrays.asList("A", "B")), eq(ApplicationStatus.ACTIVE));
doReturn(new HashSet<>(Arrays.asList(applicationB, applicationA)))
.when(applicationService)
.findByIds(eq(GraviteeContext.getExecutionContext()), eq(Arrays.asList("B", "A")));
.findByIdsAndStatus(eq(GraviteeContext.getExecutionContext()), eq(Arrays.asList("B", "A")), eq(ApplicationStatus.ACTIVE));

doReturn(new HashSet<>(Arrays.asList(applicationB)))
.when(applicationService)
.findByIds(eq(GraviteeContext.getExecutionContext()), eq(List.of("B")));
.findByIdsAndStatus(eq(GraviteeContext.getExecutionContext()), eq(List.of("B")), eq(ApplicationStatus.ACTIVE));

doReturn(new Application().id("A").name("A"))
.when(applicationMapper)
Expand Down Expand Up @@ -149,7 +149,11 @@ public void shouldGetApplicationsOrderByName() {
.findIdsByUser(eq(GraviteeContext.getExecutionContext()), any(), eq(sort));
doReturn(mockApplications)
.when(applicationService)
.findByIds(eq(GraviteeContext.getExecutionContext()), eq(Arrays.asList("A", "B", "C", "D")));
.findByIdsAndStatus(
eq(GraviteeContext.getExecutionContext()),
eq(Arrays.asList("A", "B", "C", "D")),
eq(ApplicationStatus.ACTIVE)
);

doReturn(new Application().id("A").name("A"))
.when(applicationMapper)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,12 @@ public interface ApplicationService {

Set<ApplicationListItem> findByIds(final ExecutionContext executionContext, Collection<String> applicationIds);

Set<ApplicationListItem> findByIdsAndStatus(
final ExecutionContext executionContext,
Collection<String> applicationIds,
ApplicationStatus applicationStatus
);

default Set<ApplicationListItem> findByUser(final ExecutionContext executionContext, String username) {
return findByUser(executionContext, username, null);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@
import io.gravitee.alert.api.trigger.Trigger;
import io.gravitee.common.event.Event;
import io.gravitee.notifier.api.Notification;
import io.gravitee.repository.management.api.AlertTriggerRepository;
import io.gravitee.repository.management.model.ApplicationStatus;
import io.gravitee.rest.api.model.ApplicationEntity;
import io.gravitee.rest.api.model.MembershipEntity;
import io.gravitee.rest.api.model.MembershipReferenceType;
Expand All @@ -51,7 +51,6 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.springframework.beans.factory.annotation.Value;
import org.springframework.context.annotation.Lazy;
import org.springframework.scheduling.annotation.Async;
import org.springframework.stereotype.Component;
import org.springframework.util.CollectionUtils;
Expand Down Expand Up @@ -320,7 +319,7 @@ private void updateAlertsRecipients(ApplicationAlertMembershipEvent alertMembers

// get recipients for each application
final Map<String, List<String>> recipientsByApplicationId = applicationService
.findByIds(executionContext, new ArrayList<>(applicationIds))
.findByIdsAndStatus(executionContext, new ArrayList<>(applicationIds), ApplicationStatus.ACTIVE)
.stream()
.collect(
Collectors.toMap(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -233,17 +233,28 @@ public ApplicationEntity findById(final ExecutionContext executionContext, Strin
}

@Override
public Set<ApplicationListItem> findByIds(final ExecutionContext executionContext, Collection<String> applicationIds) {
public Set<ApplicationListItem> findByIds(ExecutionContext executionContext, Collection<String> applicationIds) {
Copy link
Member

Choose a reason for hiding this comment

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

Previously, the findByIds method implicitly searched applications by id and ApplicationStatus.ACTIVE by default. By replacing it with your implementation, you've altered the behaviour for all existing resources or services that currently rely on this method.

Could you please verify whether this method is still being called elsewhere? If so, ensure that the default behaviour hasn't been unintentionally modified. If the change impacts existing functionality, consider using your findByIdsAndStatus method explicitly to preserve the intended behaviour.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. I replaced/renamed findByIds to findByIdsAndStatus and updated all the usages/calls by adding additional parameter: ApplicationStatus.Active
  2. Then I added a new method findByIds

return findByIdsAndStatus(executionContext, applicationIds, null);
}

@Override
public Set<ApplicationListItem> findByIdsAndStatus(
final ExecutionContext executionContext,
Collection<String> applicationIds,
ApplicationStatus applicationStatus
) {
try {
LOGGER.debug("Find application by IDs: {}", applicationIds);

if (applicationIds.isEmpty()) {
return Collections.emptySet();
}

ApplicationCriteria.Builder criteriaBuilder = new ApplicationCriteria.Builder()
.ids(new HashSet<>(applicationIds))
.status(ApplicationStatus.ACTIVE);
ApplicationCriteria.Builder criteriaBuilder = new ApplicationCriteria.Builder().ids(new HashSet<>(applicationIds));

if (applicationStatus != null) {
criteriaBuilder.status(applicationStatus);
}

if (executionContext.hasEnvironmentId()) {
criteriaBuilder.environmentIds(executionContext.getEnvironmentId());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import io.gravitee.repository.management.api.SubscriptionRepository;
import io.gravitee.repository.management.api.search.SubscriptionCriteria;
import io.gravitee.repository.management.model.Api;
import io.gravitee.repository.management.model.ApplicationStatus;
import io.gravitee.repository.management.model.Audit;
import io.gravitee.repository.management.model.Subscription;
import io.gravitee.rest.api.model.*;
Expand Down Expand Up @@ -322,7 +323,7 @@ private Set<String> getIndirectMemberIds(ExecutionContext context, List<String>

// get all indirect members
List<String> applicationsGroups = applicationService
.findByIds(context, applicationIds)
.findByIdsAndStatus(context, applicationIds, ApplicationStatus.ACTIVE)
.stream()
.filter(application -> application.getGroups() != null)
.flatMap((ApplicationListItem applicationListItem) -> applicationListItem.getGroups().stream())
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1595,7 +1595,11 @@ public Metadata getMetadata(ExecutionContext executionContext, SubscriptionMetad
.map(withApplications -> {
Set<String> appIds = subscriptions.stream().map(SubscriptionEntity::getApplication).collect(toSet());
return applicationService
.findByIds(new ExecutionContext(query.getOrganization(), query.getEnvironment()), appIds)
.findByIdsAndStatus(
new ExecutionContext(query.getOrganization(), query.getEnvironment()),
appIds,
ApplicationStatus.ACTIVE
)
.stream()
.collect(toMap(ApplicationListItem::getId, Function.identity()));
});
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,219 @@
/*
* Copyright © 2015 The Gravitee team (http://gravitee.io)
*
* 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 io.gravitee.rest.api.service.impl;

import static org.junit.Assert.*;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.Mockito.*;

import com.google.common.collect.Sets;
import io.gravitee.common.data.domain.Page;
import io.gravitee.repository.exceptions.TechnicalException;
import io.gravitee.repository.management.api.ApplicationRepository;
import io.gravitee.repository.management.api.search.ApplicationCriteria;
import io.gravitee.repository.management.model.ApiKeyMode;
import io.gravitee.repository.management.model.Application;
import io.gravitee.repository.management.model.ApplicationStatus;
import io.gravitee.rest.api.model.MembershipEntity;
import io.gravitee.rest.api.model.RoleEntity;
import io.gravitee.rest.api.model.application.ApplicationListItem;
import io.gravitee.rest.api.model.permissions.RoleScope;
import io.gravitee.rest.api.service.MembershipService;
import io.gravitee.rest.api.service.RoleService;
import io.gravitee.rest.api.service.UserService;
import io.gravitee.rest.api.service.common.ExecutionContext;
import io.gravitee.rest.api.service.common.GraviteeContext;
import io.gravitee.rest.api.service.configuration.application.ClientRegistrationService;
import io.gravitee.rest.api.service.exceptions.TechnicalManagementException;
import java.util.*;
import java.util.stream.Collectors;
import org.junit.After;
import org.junit.Before;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.mockito.InjectMocks;
import org.mockito.Mock;
import org.mockito.junit.MockitoJUnitRunner;

/**
* @author Guillaume CUSNIEUX (guillaume.cusnieux at graviteesource.com)
* @author GraviteeSource Team
*/
@RunWith(MockitoJUnitRunner.class)
public class ApplicationService_FindByIdsAndStatusTest {

private static final List<String> APPLICATION_IDS = Arrays.asList("id-app-1", "id-app-2");

@InjectMocks
private ApplicationServiceImpl applicationService = new ApplicationServiceImpl();

@Mock
private ApplicationRepository applicationRepository;

@Mock
private MembershipService membershipService;

@Mock
private UserService userService;

@Mock
private RoleService roleService;

@Mock
private RoleEntity primaryOwnerRole;

@Mock
private Set<MembershipEntity> primaryOwners;

@Mock
private Application app1;

@Mock
private Application app2;

@Mock
private ClientRegistrationService clientRegistrationService;

@Before
public void setUp() {
GraviteeContext.setCurrentOrganization("DEFAULT");
GraviteeContext.setCurrentEnvironment("DEFAULT");
when(app1.getStatus()).thenReturn(ApplicationStatus.ACTIVE);
when(app1.getId()).thenReturn(APPLICATION_IDS.get(0));
when(app1.getApiKeyMode()).thenReturn(ApiKeyMode.UNSPECIFIED);

when(app2.getStatus()).thenReturn(ApplicationStatus.ACTIVE);
when(app2.getId()).thenReturn(APPLICATION_IDS.get(1));
when(app2.getApiKeyMode()).thenReturn(ApiKeyMode.UNSPECIFIED);

doReturn(primaryOwnerRole)
.when(roleService)
.findPrimaryOwnerRoleByOrganization(GraviteeContext.getCurrentOrganization(), RoleScope.APPLICATION);

doReturn("role-id").when(primaryOwnerRole).getId();

when(membershipService.getMembershipsByReferencesAndRole(any(), any(), any())).thenReturn(primaryOwners);
}

@After
public void tearDown() {
GraviteeContext.cleanContext();
}

@Test
public void shouldFindByIdsAndStatus() throws TechnicalException {
ExecutionContext executionContext = GraviteeContext.getExecutionContext();
ApplicationCriteria criteria = new ApplicationCriteria.Builder()
.ids(Sets.newHashSet(APPLICATION_IDS))
.status(ApplicationStatus.ACTIVE)
.environmentIds(executionContext.getEnvironmentId())
.build();
doReturn(new Page(Arrays.asList(app1, app2), 1, 2, 2)).when(applicationRepository).search(criteria, null);
doReturn(2).when(primaryOwners).size();

final Set<ApplicationListItem> applications = applicationService.findByIdsAndStatus(
executionContext,
APPLICATION_IDS,
ApplicationStatus.ACTIVE
);

assertNotNull(applications);
assertEquals(APPLICATION_IDS, applications.stream().map(ApplicationListItem::getId).collect(Collectors.toList()));
}

@Test
public void shouldFindByIdsAndStatusWithDuplicatedIdsAndStatus() throws TechnicalException {
ExecutionContext executionContext = GraviteeContext.getExecutionContext();
ApplicationCriteria criteria = new ApplicationCriteria.Builder()
.ids(Sets.newHashSet(APPLICATION_IDS))
.status(ApplicationStatus.ACTIVE)
.environmentIds(executionContext.getEnvironmentId())
.build();

doReturn(new Page<>(Arrays.asList(app1, app2), 1, 2, 2)).when(applicationRepository).search(criteria, null);
doReturn(2).when(primaryOwners).size();

final Set<ApplicationListItem> applications = applicationService.findByIdsAndStatus(
executionContext,
List.of("id-app-1", "id-app-1", "id-app-2"),
ApplicationStatus.ACTIVE
);

assertNotNull(applications);
assertEquals(APPLICATION_IDS, applications.stream().map(ApplicationListItem::getId).collect(Collectors.toList()));
}

@Test
public void shouldFindByIdsAndStatusWithNoEnvironmentCriteria() throws TechnicalException {
ExecutionContext executionContext = new ExecutionContext("DEFAULT", null);
ApplicationCriteria criteria = new ApplicationCriteria.Builder()
.ids(Sets.newHashSet(APPLICATION_IDS))
.status(ApplicationStatus.ACTIVE)
.build();
doReturn(new Page(Arrays.asList(app1, app2), 1, 2, 2)).when(applicationRepository).search(criteria, null);
doReturn(2).when(primaryOwners).size();

final Set<ApplicationListItem> applications = applicationService.findByIdsAndStatus(
executionContext,
APPLICATION_IDS,
ApplicationStatus.ACTIVE
);

assertNotNull(applications);
assertEquals(APPLICATION_IDS, applications.stream().map(ApplicationListItem::getId).collect(Collectors.toList()));
}

@Test
public void shouldFindByIdsAndStatusWithEmptySet() throws TechnicalException {
ExecutionContext executionContext = GraviteeContext.getExecutionContext();

final Set<ApplicationListItem> applications = applicationService.findByIdsAndStatus(
executionContext,
Collections.emptySet(),
ApplicationStatus.ACTIVE
);

assertNotNull(applications);
assertTrue(applications.isEmpty());
verify(applicationRepository, times(0)).search(any(), any());
}

@Test(expected = TechnicalManagementException.class)
public void shouldThrowsIfNoPrimaryOwner() throws TechnicalException {
ExecutionContext executionContext = GraviteeContext.getExecutionContext();
ApplicationCriteria criteria = new ApplicationCriteria.Builder()
.ids(Sets.newHashSet(APPLICATION_IDS))
.status(ApplicationStatus.ACTIVE)
.environmentIds(executionContext.getEnvironmentId())
.build();
doReturn(new Page(Arrays.asList(app1, app2), 1, 2, 2)).when(applicationRepository).search(criteria, null);

final Set<ApplicationListItem> applications = applicationService.findByIdsAndStatus(
GraviteeContext.getExecutionContext(),
APPLICATION_IDS,
ApplicationStatus.ACTIVE
);

assertNotNull(applications);
assertEquals(APPLICATION_IDS, applications.stream().map(ApplicationListItem::getId).collect(Collectors.toList()));
}

@Test(expected = TechnicalManagementException.class)
public void shouldThrowTechnicalManagementException() throws TechnicalException {
when(applicationRepository.search(any(), any())).thenThrow(new TechnicalException());
applicationService.findByIdsAndStatus(GraviteeContext.getExecutionContext(), APPLICATION_IDS, ApplicationStatus.ACTIVE);
}
}
Loading
Loading