From 088d6f5aa59218f4e81c607f4b47b0f9ce0a34c0 Mon Sep 17 00:00:00 2001 From: Bruno Pacheco Date: Mon, 15 Apr 2024 22:55:22 +0200 Subject: [PATCH] fix: #17 Retrieve Beacon Facets when logged-in Beacon facets must be retrieved if the user is logged in, no matter if the user is using or not beacon facets to filter the results. The class `BeaconDatasetsSearchService` was altered to comply to this requirement: - if the user is not logged-in automatically CKAN query is triggered without enhancement. Check line 75. - If there are beacon queries we trigger a search on Beacon Network. Check lines 79 and 109. - if there are no beacon filters or beacon's resultsets is not empty, we query on CKAN. Check lines 81 and 134. - Beacon facets/filters are always inserted into CKAN output. Check lines 87 and 175. With those rules, we are able to properly enrich CKAN output with beacon response, including facets and resultsets, if there is any. Several different test cases were introduced to cover most of the scenarios. --- .../services/BeaconDatasetsSearchService.java | 66 ++- .../services/BeaconFilteringTermsService.java | 4 +- src/main/resources/application.properties | 1 + .../BeaconDatasetsSearchServiceTest.java | 425 ++++++++++++++++++ 4 files changed, 470 insertions(+), 26 deletions(-) create mode 100644 src/test/java/io/github/genomicdatainfrastructure/discovery/services/BeaconDatasetsSearchServiceTest.java diff --git a/src/main/java/io/github/genomicdatainfrastructure/discovery/services/BeaconDatasetsSearchService.java b/src/main/java/io/github/genomicdatainfrastructure/discovery/services/BeaconDatasetsSearchService.java index 059be03..3de7088 100644 --- a/src/main/java/io/github/genomicdatainfrastructure/discovery/services/BeaconDatasetsSearchService.java +++ b/src/main/java/io/github/genomicdatainfrastructure/discovery/services/BeaconDatasetsSearchService.java @@ -21,7 +21,6 @@ import io.github.genomicdatainfrastructure.discovery.model.FacetGroup; import io.github.genomicdatainfrastructure.discovery.model.SearchedDataset; import io.github.genomicdatainfrastructure.discovery.remote.beacon.api.BeaconQueryApi; -import io.github.genomicdatainfrastructure.discovery.remote.beacon.model.BeaconIndividualsRequest; import io.github.genomicdatainfrastructure.discovery.remote.beacon.model.BeaconIndividualsResponse; import io.github.genomicdatainfrastructure.discovery.remote.beacon.model.BeaconIndividualsResponseContent; import io.github.genomicdatainfrastructure.discovery.remote.beacon.model.BeaconResultSet; @@ -71,25 +70,21 @@ public BeaconDatasetsSearchService( @Override public DatasetsSearchResponse search(DatasetSearchQuery query, String accessToken) { - var beaconQuery = BeaconIndividualsRequestMapper.from(query); - var beaconAuthorization = retrieveBeaconAuthorization(accessToken); - if (beaconAuthorization == null || beaconQuery.getQuery().getFilters().isEmpty()) { + if (beaconAuthorization == null) { return datasetsSearchService.search(query, accessToken); } - var beaconResponse = queryOnBeacon(beaconAuthorization, beaconQuery); + var resultsets = queryOnBeaconIfThereAreBeaconFilters(beaconAuthorization, query); - var datasetsReponse = DatasetsSearchResponse.builder() - .count(0) - .build(); - if (!beaconResponse.isEmpty()) { - var enhancedQuery = enhanceQueryFacets(query, beaconResponse); - datasetsReponse = datasetsSearchService.search(enhancedQuery, accessToken); - } + var datasetsSearchResponse = queryOnCkanIfThereIsNoBeaconFilterOrResultsetsIsNotEmpty( + accessToken, + query, + resultsets + ); - return enhanceDatasetsResponse(beaconAuthorization, datasetsReponse, beaconResponse); + return enhanceDatasetsResponse(beaconAuthorization, datasetsSearchResponse, resultsets); } private String retrieveBeaconAuthorization(String accessToken) { @@ -111,10 +106,14 @@ private String retrieveBeaconAuthorization(String accessToken) { } } - private List queryOnBeacon( + private List queryOnBeaconIfThereAreBeaconFilters( String beaconAuthorization, - BeaconIndividualsRequest beaconQuery + DatasetSearchQuery query ) { + var beaconQuery = BeaconIndividualsRequestMapper.from(query); + if (beaconQuery.getQuery().getFilters().isEmpty()) { + return List.of(); + } var response = beaconQueryApi.listIndividuals(beaconAuthorization, beaconQuery); @@ -128,10 +127,29 @@ private List queryOnBeacon( .filter(Objects::nonNull) .filter(it -> BEACON_DATASET_TYPE.equals(it.getSetType())) .filter(it -> isNotBlank(it.getId())) - .filter(it -> it.getResultsCount() > 0) + .filter(it -> it.getResultsCount() != null && it.getResultsCount() > 0) .toList(); } + private DatasetsSearchResponse queryOnCkanIfThereIsNoBeaconFilterOrResultsetsIsNotEmpty( + String beaconAuthorization, + DatasetSearchQuery query, + List resultSets + ) { + var nonNullFacets = ofNullable(query.getFacets()).orElseGet(List::of); + var thereIsAtLeastOneBeaconFilter = nonNullFacets.stream() + .anyMatch(it -> BEACON_FACET_GROUP.equals(it.getFacetGroup())); + + if (thereIsAtLeastOneBeaconFilter && resultSets.isEmpty()) { + return DatasetsSearchResponse.builder() + .count(0) + .build(); + } + + var enhancedQuery = enhanceQueryFacets(query, resultSets); + return datasetsSearchService.search(enhancedQuery, beaconAuthorization); + } + private DatasetSearchQuery enhanceQueryFacets( DatasetSearchQuery query, List resultSets @@ -156,30 +174,30 @@ private DatasetSearchQuery enhanceQueryFacets( private DatasetsSearchResponse enhanceDatasetsResponse( String beaconAuthorization, - DatasetsSearchResponse datasetsReponse, + DatasetsSearchResponse datasetsSearchReponse, List resultSets ) { var facetGroupCount = new HashMap(); facetGroupCount.put(BEACON_FACET_GROUP, resultSets.size()); - if (isNotEmpty(datasetsReponse.getFacetGroupCount())) { - facetGroupCount.putAll(datasetsReponse.getFacetGroupCount()); + if (isNotEmpty(datasetsSearchReponse.getFacetGroupCount())) { + facetGroupCount.putAll(datasetsSearchReponse.getFacetGroupCount()); } var facetGroups = new ArrayList(); facetGroups.add(beaconFilteringTermsService.listFilteringTerms(beaconAuthorization)); - if (isNotEmpty(datasetsReponse.getFacetGroups())) { - facetGroups.addAll(datasetsReponse.getFacetGroups()); + if (isNotEmpty(datasetsSearchReponse.getFacetGroups())) { + facetGroups.addAll(datasetsSearchReponse.getFacetGroups()); } var results = List.of(); - if (isNotEmpty(datasetsReponse.getResults())) { + if (isNotEmpty(datasetsSearchReponse.getResults())) { var recordCounts = resultSets.stream() .collect(toMap( BeaconResultSet::getId, BeaconResultSet::getResultsCount )); - results = datasetsReponse.getResults() + results = datasetsSearchReponse.getResults() .stream() .map(it -> it.toBuilder() .recordsCount(recordCounts.get(it.getIdentifier())) @@ -187,7 +205,7 @@ private DatasetsSearchResponse enhanceDatasetsResponse( .toList(); } - return datasetsReponse.toBuilder() + return datasetsSearchReponse.toBuilder() .facetGroupCount(facetGroupCount) .facetGroups(facetGroups) .results(results) diff --git a/src/main/java/io/github/genomicdatainfrastructure/discovery/services/BeaconFilteringTermsService.java b/src/main/java/io/github/genomicdatainfrastructure/discovery/services/BeaconFilteringTermsService.java index 83edb82..2e11b71 100644 --- a/src/main/java/io/github/genomicdatainfrastructure/discovery/services/BeaconFilteringTermsService.java +++ b/src/main/java/io/github/genomicdatainfrastructure/discovery/services/BeaconFilteringTermsService.java @@ -113,8 +113,8 @@ private List buildFacets( return termsGroupedByType.entrySet().stream() .filter(entry -> facetIdsMappedByName.containsKey(entry.getKey())) .map(entry -> Facet.builder() - .key(entry.getKey()) - .label(facetIdsMappedByName.get(entry.getKey())) + .key(facetIdsMappedByName.get(entry.getKey())) + .label(entry.getKey()) .values(entry.getValue()) .build()) .toList(); diff --git a/src/main/resources/application.properties b/src/main/resources/application.properties index d346dfb..7f2e4f4 100644 --- a/src/main/resources/application.properties +++ b/src/main/resources/application.properties @@ -31,6 +31,7 @@ quarkus.rest-client.ckan_yaml.url=http://localhost:4000 quarkus.rest-client.keycloak_yaml.url=http://localhost:4000 quarkus.rest-client.keycloak_yaml.beacon_idp_alias=LSAAI quarkus.rest-client.beacon_yaml.url=http://localhost:4000 +quarkus.rest-client.beacon_yaml.read-timeout=60000 %dev.quarkus.oidc.auth-server-url=https://keycloak-test.healthdata.nl/realms/ckan %dev.quarkus.oidc.client-id=ckan %dev.quarkus.rest-client.ckan_yaml.url=https://ckan-test.healthdata.nl/ diff --git a/src/test/java/io/github/genomicdatainfrastructure/discovery/services/BeaconDatasetsSearchServiceTest.java b/src/test/java/io/github/genomicdatainfrastructure/discovery/services/BeaconDatasetsSearchServiceTest.java new file mode 100644 index 0000000..5f04573 --- /dev/null +++ b/src/test/java/io/github/genomicdatainfrastructure/discovery/services/BeaconDatasetsSearchServiceTest.java @@ -0,0 +1,425 @@ +// SPDX-FileCopyrightText: 2024 PNED G.I.E. +// +// SPDX-License-Identifier: Apache-2.0 + +package io.github.genomicdatainfrastructure.discovery.services; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +import java.util.List; +import java.util.Map; +import java.util.stream.Stream; + +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.MethodSource; +import org.junit.jupiter.params.provider.NullSource; +import org.junit.jupiter.params.provider.ValueSource; + +import io.github.genomicdatainfrastructure.discovery.model.DatasetSearchQuery; +import io.github.genomicdatainfrastructure.discovery.model.DatasetSearchQueryFacet; +import io.github.genomicdatainfrastructure.discovery.model.DatasetsSearchResponse; +import io.github.genomicdatainfrastructure.discovery.model.Facet; +import io.github.genomicdatainfrastructure.discovery.model.FacetGroup; +import io.github.genomicdatainfrastructure.discovery.model.SearchedDataset; +import io.github.genomicdatainfrastructure.discovery.remote.beacon.api.BeaconQueryApi; +import io.github.genomicdatainfrastructure.discovery.remote.beacon.model.BeaconIndividualsResponse; +import io.github.genomicdatainfrastructure.discovery.remote.beacon.model.BeaconIndividualsResponseContent; +import io.github.genomicdatainfrastructure.discovery.remote.beacon.model.BeaconResultSet; +import io.github.genomicdatainfrastructure.discovery.remote.keycloak.api.KeycloakQueryApi; +import io.github.genomicdatainfrastructure.discovery.remote.keycloak.model.KeycloakTokenResponse; +import jakarta.ws.rs.WebApplicationException; + +class BeaconDatasetsSearchServiceTest { + + private BeaconDatasetsSearchService underTest; + private BeaconQueryApi beaconQueryApi; + private KeycloakQueryApi keycloakQueryApi; + private CkanDatasetsSearchService ckanDatasetsSearchService; + private BeaconFilteringTermsService beaconFilteringTermsService; + + @BeforeEach + void setUp() { + beaconQueryApi = mock(BeaconQueryApi.class); + keycloakQueryApi = mock(KeycloakQueryApi.class); + ckanDatasetsSearchService = mock(CkanDatasetsSearchService.class); + beaconFilteringTermsService = mock(BeaconFilteringTermsService.class); + + underTest = new BeaconDatasetsSearchService( + beaconQueryApi, + "beaconIdpAlias", + keycloakQueryApi, + ckanDatasetsSearchService, + beaconFilteringTermsService + ); + } + + @Test + void doesnt_call_beacon_if_access_token_is_null() { + when(ckanDatasetsSearchService.search(any(), any())) + .thenReturn(DatasetsSearchResponse.builder() + .count(1) + .facetGroupCount(Map.of("group", 1)) + .results(List.of( + SearchedDataset.builder() + .id("id") + .title("title") + .build() + )) + .build()); + + var query = DatasetSearchQuery.builder() + .build(); + var actual = underTest.search(query, null); + + verify(keycloakQueryApi, never()).retriveIdpTokens(any(), any()); + verify(ckanDatasetsSearchService).search(any(), any()); + verify(beaconFilteringTermsService, never()).listFilteringTerms(any()); + verify(beaconQueryApi, never()).listIndividuals(any(), any()); + + assertThat(actual) + .usingRecursiveComparison() + .isEqualTo(DatasetsSearchResponse.builder() + .count(1) + .facetGroupCount(Map.of("group", 1)) + .results(List.of( + SearchedDataset.builder() + .id("id") + .title("title") + .build() + )) + .build()); + } + + @ParameterizedTest + @ValueSource(ints = {400, 401, 403}) + void doesnt_call_beacon_if_keycloak_throws_expected_4xx_errors(Integer statusCode) { + when(ckanDatasetsSearchService.search(any(), any())) + .thenReturn(DatasetsSearchResponse.builder() + .count(1) + .facetGroupCount(Map.of("group", 1)) + .results(List.of( + SearchedDataset.builder() + .id("id") + .title("title") + .build() + )) + .build()); + + when(keycloakQueryApi.retriveIdpTokens("beaconIdpAlias", "Bearer dummy")) + .thenThrow(new WebApplicationException(statusCode)); + + var query = DatasetSearchQuery.builder() + .build(); + var actual = underTest.search(query, "dummy"); + + verify(keycloakQueryApi).retriveIdpTokens(any(), any()); + verify(ckanDatasetsSearchService).search(any(), any()); + verify(beaconFilteringTermsService, never()).listFilteringTerms(any()); + verify(beaconQueryApi, never()).listIndividuals(any(), any()); + + assertThat(actual) + .usingRecursiveComparison() + .isEqualTo(DatasetsSearchResponse.builder() + .count(1) + .facetGroupCount(Map.of("group", 1)) + .results(List.of( + SearchedDataset.builder() + .id("id") + .title("title") + .build() + )) + .build()); + } + + @Test + void doesnt_call_beacon_if_there_are_no_beacon_filters() { + when(ckanDatasetsSearchService.search(any(), any())) + .thenReturn(DatasetsSearchResponse.builder() + .count(1) + .facetGroupCount(Map.of("group", 1)) + .results(List.of( + SearchedDataset.builder() + .id("id") + .title("title") + .build() + )) + .build()); + + when(beaconFilteringTermsService.listFilteringTerms(any())) + .thenReturn(FacetGroup.builder() + .key("beacon") + .label("label") + .facets(List.of( + Facet.builder() + .key("key") + .label("label") + .build() + )) + .build()); + + when(keycloakQueryApi.retriveIdpTokens("beaconIdpAlias", "Bearer dummy")) + .thenReturn(KeycloakTokenResponse.builder() + .accessToken("beaconAccessToken") + .build()); + + var query = DatasetSearchQuery.builder() + .build(); + var actual = underTest.search(query, "dummy"); + + verify(keycloakQueryApi).retriveIdpTokens(any(), any()); + verify(beaconQueryApi, never()).listIndividuals(any(), any()); + verify(ckanDatasetsSearchService).search(any(), any()); + verify(beaconFilteringTermsService).listFilteringTerms(any()); + + assertThat(actual) + .usingRecursiveComparison() + .isEqualTo(DatasetsSearchResponse.builder() + .count(1) + .facetGroupCount(Map.of( + "group", 1, + "beacon", 0 + )) + .results(List.of( + SearchedDataset.builder() + .id("id") + .title("title") + .build() + )) + .facetGroups(List.of( + FacetGroup.builder() + .key("beacon") + .label("label") + .facets(List.of( + Facet.builder() + .key("key") + .label("label") + .build() + )) + .build() + )) + .build()); + } + + private static Stream emptyBeaconResultsets() { + return Stream.of( + BeaconIndividualsResponse.builder() + .response(null) + .build(), + BeaconIndividualsResponse.builder() + .response(BeaconIndividualsResponseContent.builder() + .resultSets(List.of()) + .build()) + .build(), + BeaconIndividualsResponse.builder() + .response(BeaconIndividualsResponseContent.builder() + .resultSets(List.of( + BeaconResultSet.builder() + .id(null) + .resultsCount(1) + .setType("dataset") + .build() + )) + .build()) + .build(), + BeaconIndividualsResponse.builder() + .response(BeaconIndividualsResponseContent.builder() + .resultSets(List.of( + BeaconResultSet.builder() + .id("id") + .resultsCount(null) + .setType("dataset") + .build() + )) + .build()) + .build(), + BeaconIndividualsResponse.builder() + .response(BeaconIndividualsResponseContent.builder() + .resultSets(List.of( + BeaconResultSet.builder() + .id("id") + .resultsCount(1) + .setType(null) + .build() + )) + .build()) + .build() + ); + } + + @ParameterizedTest + @NullSource + @MethodSource("emptyBeaconResultsets") + void doesnt_call_ckan_if_there_are_no_beacon_resultsets( + BeaconIndividualsResponse beaconResponse + ) { + when(beaconQueryApi.listIndividuals(any(), any())) + .thenReturn(beaconResponse); + + when(beaconFilteringTermsService.listFilteringTerms(any())) + .thenReturn(FacetGroup.builder() + .key("beacon") + .label("label") + .facets(List.of( + Facet.builder() + .key("key") + .label("label") + .build() + )) + .build()); + + when(keycloakQueryApi.retriveIdpTokens("beaconIdpAlias", "Bearer dummy")) + .thenReturn(KeycloakTokenResponse.builder() + .accessToken("beaconAccessToken") + .build()); + + var query = DatasetSearchQuery.builder() + .facets(List.of( + DatasetSearchQueryFacet.builder() + .facetGroup("beacon") + .facet("key") + .value("value") + .build( + ))) + .build(); + var actual = underTest.search(query, "dummy"); + + verify(keycloakQueryApi).retriveIdpTokens(any(), any()); + verify(beaconQueryApi).listIndividuals(any(), any()); + verify(ckanDatasetsSearchService, never()).search(any(), any()); + verify(beaconFilteringTermsService).listFilteringTerms(any()); + + assertThat(actual) + .usingRecursiveComparison() + .isEqualTo(DatasetsSearchResponse.builder() + .count(0) + .facetGroupCount(Map.of( + "beacon", 0 + )) + .results(List.of()) + .facetGroups(List.of( + FacetGroup.builder() + .key("beacon") + .label("label") + .facets(List.of( + Facet.builder() + .key("key") + .label("label") + .build() + )) + .build() + )) + .build()); + } + + @Test + void calls_ckan_and_beacon() { + when(beaconQueryApi.listIndividuals(any(), any())) + .thenReturn(BeaconIndividualsResponse.builder() + .response(BeaconIndividualsResponseContent.builder() + .resultSets(List.of( + BeaconResultSet.builder() + .id("id") + .resultsCount(1) + .setType("dataset") + .build() + )) + .build()) + .build()); + + when(ckanDatasetsSearchService.search(any(), any())) + .thenReturn(DatasetsSearchResponse.builder() + .count(1) + .facetGroupCount(Map.of("group", 1)) + .results(List.of( + SearchedDataset.builder() + .id("id") + .title("title") + .build() + )) + .build()); + + when(beaconFilteringTermsService.listFilteringTerms(any())) + .thenReturn(FacetGroup.builder() + .key("beacon") + .label("label") + .facets(List.of( + Facet.builder() + .key("key") + .label("label") + .build() + )) + .build()); + + when(keycloakQueryApi.retriveIdpTokens("beaconIdpAlias", "Bearer dummy")) + .thenReturn(KeycloakTokenResponse.builder() + .accessToken("beaconAccessToken") + .build()); + when(beaconFilteringTermsService.listFilteringTerms(any())) + .thenReturn(FacetGroup.builder() + .key("beacon") + .label("label") + .facets(List.of( + Facet.builder() + .key("key") + .label("label") + .build() + )) + .build()); + + when(keycloakQueryApi.retriveIdpTokens("beaconIdpAlias", "Bearer dummy")) + .thenReturn(KeycloakTokenResponse.builder() + .accessToken("beaconAccessToken") + .build()); + + var query = DatasetSearchQuery.builder() + .facets(List.of( + DatasetSearchQueryFacet.builder() + .facetGroup("beacon") + .facet("key") + .value("value") + .build( + ))) + .build(); + var actual = underTest.search(query, "dummy"); + + verify(keycloakQueryApi).retriveIdpTokens(any(), any()); + verify(beaconQueryApi).listIndividuals(any(), any()); + verify(ckanDatasetsSearchService).search(any(), any()); + verify(beaconFilteringTermsService).listFilteringTerms(any()); + + assertThat(actual) + .usingRecursiveComparison() + .isEqualTo(DatasetsSearchResponse.builder() + .count(1) + .facetGroupCount(Map.of( + "group", 1, + "beacon", 1 + )) + .results(List.of( + SearchedDataset.builder() + .id("id") + .title("title") + .build() + )) + .facetGroups(List.of( + FacetGroup.builder() + .key("beacon") + .label("label") + .facets(List.of( + Facet.builder() + .key("key") + .label("label") + .build() + )) + .build() + )) + .build()); + } +}