From a3d661af39c03a3b084ab00f94c2df9b3371cdf1 Mon Sep 17 00:00:00 2001 From: brunobat Date: Mon, 10 Jul 2023 10:49:07 +0200 Subject: [PATCH] Don't load remote build analytics config if user has not accepted --- .../tools/analytics-common/pom.xml | 6 ++ .../io/quarkus/analytics/ConfigService.java | 80 ++++++++++++++----- .../io/quarkus/analytics/rest/RestClient.java | 8 +- .../analytics/AnalyticsServiceTest.java | 6 ++ .../quarkus/analytics/ConfigServiceTest.java | 61 +++++++++++--- .../analytics/rest/RestClientFailTest.java | 21 ++++- .../analytics/rest/RestClientTest.java | 5 ++ independent-projects/tools/pom.xml | 1 + 8 files changed, 150 insertions(+), 38 deletions(-) diff --git a/independent-projects/tools/analytics-common/pom.xml b/independent-projects/tools/analytics-common/pom.xml index 578c18425be74..dd1463013a9f5 100644 --- a/independent-projects/tools/analytics-common/pom.xml +++ b/independent-projects/tools/analytics-common/pom.xml @@ -67,6 +67,12 @@ ${system-stubs-jupiter.version} test + + org.awaitility + awaitility + ${awaitility.version} + test + \ No newline at end of file diff --git a/independent-projects/tools/analytics-common/src/main/java/io/quarkus/analytics/ConfigService.java b/independent-projects/tools/analytics-common/src/main/java/io/quarkus/analytics/ConfigService.java index 3b408770c0a72..4f05e502a6838 100644 --- a/independent-projects/tools/analytics-common/src/main/java/io/quarkus/analytics/ConfigService.java +++ b/independent-projects/tools/analytics-common/src/main/java/io/quarkus/analytics/ConfigService.java @@ -67,8 +67,6 @@ public ConfigService(final ConfigClient client, final AnonymousUserId userId, fi this.lastRefreshTime = initLastRefreshTime(fileLocations.getRemoteConfigFile()); this.remoteConfigFile = fileLocations.getRemoteConfigFile(); this.localConfigFile = fileLocations.getLocalConfigFile(); - loadConfig(RemoteConfig.class, remoteConfigFile) - .ifPresentOrElse(c -> this.config = c, this::loadConfigFromInternet); } public void userAcceptance(Function analyticsEnabledSupplier) { @@ -118,6 +116,33 @@ public void userAcceptance(Function analyticsEnabledSupplier) { * @return true if active */ public boolean isActive() { + if (!isLocalConfigActive()) { + if (log.isDebugEnabled()) { + log.debug("[Quarkus build analytics] Local config is not active. Skipping analytics."); + } + return false; + } + AnalyticsRemoteConfig analyticsRemoteConfig = getRemoteConfig(); + if (!analyticsRemoteConfig.isActive()) { + if (log.isDebugEnabled()) { + log.debug("[Quarkus build analytics] Remote config is not active. Skipping analytics."); + } + return false; + } + if (!isUserEnabled(analyticsRemoteConfig, userId.getUuid())) { + if (log.isDebugEnabled()) { + log.debug("[Quarkus build analytics] Remote config is not active for anonymous user. " + + "Skipping analytics."); + } + return false; + } + return true; + } + + boolean isLocalConfigActive() { + if (getProperty(QUARKUS_ANALYTICS_DISABLED_LOCAL_PROP, false)) { + return false; // disabled by local property + } if (!Files.exists(localConfigFile)) { return false; // disabled because user has not decided yet } else if (!loadConfig(LocalConfig.class, localConfigFile) @@ -125,12 +150,7 @@ public boolean isActive() { .orElse(true)) { return false; // disabled by the user and recorded on the local config } - - if (getProperty(QUARKUS_ANALYTICS_DISABLED_LOCAL_PROP, false)) { - return false; // disabled by local property - } - AnalyticsRemoteConfig analyticsRemoteConfig = getRemoteConfig(); - return analyticsRemoteConfig.isActive() && isUserEnabled(analyticsRemoteConfig, userId.getUuid()); + return true; } /** @@ -152,6 +172,28 @@ boolean isUserEnabled(final AnalyticsRemoteConfig analyticsRemoteConfig, final S .noneMatch(uId -> uId.equals(user)); } + AnalyticsRemoteConfig getRemoteConfig() { + try { + if (!isLocalConfigActive()) { + return checkAgainConfig(); // disabled. Will check again in a few hours. + } + + if (this.config == null || shouldRefreshRemoteConfig(this.config)) { + this.config = loadConfig(RemoteConfig.class, remoteConfigFile) + .filter(remoteConfig -> !shouldRefreshRemoteConfig(remoteConfig)) + .orElseGet(() -> (RemoteConfig) loadConfigFromInternet()); + } + return this.config; + } catch (Exception e) { + if (log.isDebugEnabled()) { + log.debug("[Quarkus build analytics] Failed to load remote config. Will check again latter. " + + "Exception: " + e.getMessage()); + } + this.config = checkAgainConfig(); + return this.config; + } + } + private boolean validInput(String input) { String[] allowedValues = { "n", "nn", "no", "y", "yy", "yes" }; for (String allowedValue : allowedValues) { @@ -162,18 +204,12 @@ private boolean validInput(String input) { return false; } - private AnalyticsRemoteConfig getRemoteConfig() { - if (shouldRefresh()) { - loadConfigFromInternet(); - } - return this.config; - } - - private boolean shouldRefresh() { - return lastRefreshTime == null || Duration.between( - lastRefreshTime, - Instant.now()).compareTo( - this.config.getRefreshInterval()) > 0; + private boolean shouldRefreshRemoteConfig(final AnalyticsRemoteConfig remoteConfig) { + return lastRefreshTime == null || + Duration.between( + lastRefreshTime, + Instant.now()).compareTo( + remoteConfig.getRefreshInterval()) > 0; } private Optional loadConfig(Class clazz, Path file) { @@ -188,10 +224,10 @@ private Optional loadConfig(Class clazz, Path file) { } } - private void loadConfigFromInternet() { + private AnalyticsRemoteConfig loadConfigFromInternet() { AnalyticsRemoteConfig analyticsRemoteConfig = this.client.getConfig().orElse(checkAgainConfig()); this.lastRefreshTime = Instant.now(); - this.config = storeRemoteConfigOnDisk(analyticsRemoteConfig); + return storeRemoteConfigOnDisk(analyticsRemoteConfig); } private AnalyticsRemoteConfig storeRemoteConfigOnDisk(AnalyticsRemoteConfig config) { diff --git a/independent-projects/tools/analytics-common/src/main/java/io/quarkus/analytics/rest/RestClient.java b/independent-projects/tools/analytics-common/src/main/java/io/quarkus/analytics/rest/RestClient.java index 4a1d5779a882a..e1065d20a64da 100644 --- a/independent-projects/tools/analytics-common/src/main/java/io/quarkus/analytics/rest/RestClient.java +++ b/independent-projects/tools/analytics-common/src/main/java/io/quarkus/analytics/rest/RestClient.java @@ -113,9 +113,11 @@ Optional getConfig(final URI uri) { } return Optional.empty(); } catch (IOException | InterruptedException | ExecutionException | TimeoutException e) { - log.warn("[Quarkus build analytics] Analytics remote config not received. " + - e.getClass().getName() + ": " + - (e.getMessage() == null ? "(no message)" : e.getMessage())); + if (log.isDebugEnabled()) { + log.debug("[Quarkus build analytics] Analytics remote config not received. " + + e.getClass().getName() + ": " + + (e.getMessage() == null ? "(no message)" : e.getMessage())); + } } return Optional.empty(); } diff --git a/independent-projects/tools/analytics-common/src/test/java/io/quarkus/analytics/AnalyticsServiceTest.java b/independent-projects/tools/analytics-common/src/test/java/io/quarkus/analytics/AnalyticsServiceTest.java index 2e821c9b92188..ffbe4f8de592f 100644 --- a/independent-projects/tools/analytics-common/src/test/java/io/quarkus/analytics/AnalyticsServiceTest.java +++ b/independent-projects/tools/analytics-common/src/test/java/io/quarkus/analytics/AnalyticsServiceTest.java @@ -27,6 +27,7 @@ import static io.quarkus.analytics.dto.segment.ContextBuilder.CommonSystemProperties.GRAALVM_VERSION_VERSION; import static io.quarkus.analytics.dto.segment.ContextBuilder.CommonSystemProperties.GRADLE_VERSION; import static io.quarkus.analytics.dto.segment.ContextBuilder.CommonSystemProperties.MAVEN_VERSION; +import static org.awaitility.Awaitility.await; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNotNull; @@ -37,6 +38,7 @@ import java.util.List; import java.util.Map; import java.util.Set; +import java.util.concurrent.TimeUnit; import java.util.stream.Collectors; import org.junit.jupiter.api.AfterAll; @@ -153,6 +155,10 @@ void sendAnalyticsTest() throws IOException { Map.of(), new File(FILE_LOCATIONS.getFolder().toUri())); service.close(); + + await().atMost(5, TimeUnit.SECONDS) + .untilAsserted(() -> wireMockServer.verify(1, postRequestedFor(urlEqualTo("/v1/track")))); + wireMockServer.verify(postRequestedFor(urlEqualTo("/v1/track")) .withRequestBody(notMatching("null"))); assertTrue(new File(FILE_LOCATIONS.getFolder().toString() + "/" + FILE_LOCATIONS.lastTrackFileName()).exists()); diff --git a/independent-projects/tools/analytics-common/src/test/java/io/quarkus/analytics/ConfigServiceTest.java b/independent-projects/tools/analytics-common/src/test/java/io/quarkus/analytics/ConfigServiceTest.java index 5eadff2a19057..f9710f19e33f6 100644 --- a/independent-projects/tools/analytics-common/src/test/java/io/quarkus/analytics/ConfigServiceTest.java +++ b/independent-projects/tools/analytics-common/src/test/java/io/quarkus/analytics/ConfigServiceTest.java @@ -41,12 +41,18 @@ void tearDown() throws IOException { @Test void activeWithRemoteConfig() throws IOException { - ConfigService configService = createConfigService(); - + RemoteConfig remoteConfig = RemoteConfig.builder() + .active(true) + .denyQuarkusVersions(Collections.emptyList()) + .denyUserIds(Collections.emptyList()) + .refreshInterval(Duration.ofHours(12)).build(); + FileUtils.write(remoteConfig, fileLocations.getRemoteConfigFile()); long lastModified = fileLocations.getRemoteConfigFile().toFile().lastModified(); + ConfigService configService = createConfigService(remoteConfig); + assertNotNull(configService); - assertTrue(configService.isActive()); // if remote config not found, it will be downloaded (it shouldn't) + assertTrue(configService.isActive(), "Remote config is there and active"); assertEquals(lastModified, fileLocations.getRemoteConfigFile().toFile().lastModified(), "File must not change"); } @@ -180,8 +186,10 @@ void userAcceptance_yes() throws IOException { assertEquals(ACCEPTANCE_PROMPT, s); return "y"; }); - assertTrue(Files.exists(fileLocations.getLocalConfigFile()), "Local config file must be present"); + assertTrue(configService.isActive()); + assertTrue(Files.exists(fileLocations.getLocalConfigFile()), "Local config file must be present"); + assertTrue(Files.exists(fileLocations.getRemoteConfigFile()), "remote config file was not created"); } @Test @@ -194,21 +202,55 @@ void userAcceptance_no() throws IOException { assertEquals(ACCEPTANCE_PROMPT, s); return "n"; }); - assertTrue(Files.exists(fileLocations.getLocalConfigFile()), "Local config file must be present"); assertFalse(configService.isActive()); + assertTrue(Files.exists(fileLocations.getLocalConfigFile()), "Local config file must be present"); + assertFalse(Files.exists(fileLocations.getRemoteConfigFile()), "remote config file cannot be present"); } @Test - void userAcceptance_fail() throws IOException { + void userAcceptance_invalidInput() throws IOException { deleteLocalConfigFile(); ConfigService configService = createConfigService(); configService.userAcceptance(s -> { - throw new RuntimeException("User input failed"); + assertEquals(ACCEPTANCE_PROMPT, s); + return "not valid input"; }); + assertFalse(configService.isActive()); + assertFalse(Files.exists(fileLocations.getLocalConfigFile()), "Local config file cannot be present"); + assertFalse(Files.exists(fileLocations.getRemoteConfigFile()), "remote config file cannot be present"); + } + + @Test + void noRemoteConfigOnInit() throws IOException { + deleteLocalConfigFile(); assertFalse(Files.exists(fileLocations.getLocalConfigFile()), "Local config file cannot be present"); + assertFalse(Files.exists(fileLocations.getRemoteConfigFile()), "remote config file cannot be present"); + ConfigService configService = new ConfigService(new TestRestClient(NoopRemoteConfig.INSTANCE), + AnonymousUserId.getInstance(fileLocations, MessageWriter.info()), + fileLocations, + MessageWriter.info()); assertFalse(configService.isActive()); + assertFalse(Files.exists(fileLocations.getLocalConfigFile()), "Local config file cannot be present on 1st init"); + assertFalse(Files.exists(fileLocations.getRemoteConfigFile()), "remote config file cannot be present on 1st init"); + } + + @Test + void noRemoteConfigUserDisabledByProp() throws IOException { + deleteLocalConfigFile(); + System.setProperty("quarkus.analytics.disabled", "true"); + ConfigService configService = createConfigService(); + + configService.userAcceptance(s -> { + fail("User should not be asked"); + return null; + }); + + assertFalse(configService.isActive()); + assertFalse(Files.exists(fileLocations.getLocalConfigFile()), "Local config is present"); + assertFalse(Files.exists(fileLocations.getRemoteConfigFile()), "remote config file cannot be present on 1st init"); + System.clearProperty("quarkus.analytics.disabled"); } private void deleteLocalConfigFile() { @@ -221,9 +263,10 @@ private ConfigService createConfigService() throws IOException { .denyQuarkusVersions(Collections.emptyList()) .denyUserIds(Collections.emptyList()) .refreshInterval(Duration.ofHours(12)).build(); + return createConfigService(remoteConfig); + } - FileUtils.write(remoteConfig, fileLocations.getRemoteConfigFile()); - + private ConfigService createConfigService(final RemoteConfig remoteConfig) throws IOException { ConfigService configService = new ConfigService(new TestRestClient(remoteConfig), AnonymousUserId.getInstance(fileLocations, MessageWriter.info()), fileLocations, diff --git a/independent-projects/tools/analytics-common/src/test/java/io/quarkus/analytics/rest/RestClientFailTest.java b/independent-projects/tools/analytics-common/src/test/java/io/quarkus/analytics/rest/RestClientFailTest.java index 425d1371137e7..3770f2ee8bc77 100644 --- a/independent-projects/tools/analytics-common/src/test/java/io/quarkus/analytics/rest/RestClientFailTest.java +++ b/independent-projects/tools/analytics-common/src/test/java/io/quarkus/analytics/rest/RestClientFailTest.java @@ -9,9 +9,10 @@ import static io.quarkus.analytics.common.ContextTestData.createContext; import static io.quarkus.analytics.rest.RestClient.IDENTITY_ENDPOINT; import static io.quarkus.analytics.util.StringUtils.getObjectMapper; +import static org.awaitility.Awaitility.await; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertNotNull; -import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.fail; import java.net.URI; import java.net.URISyntaxException; @@ -19,6 +20,7 @@ import java.time.Instant; import java.util.Optional; import java.util.concurrent.CompletableFuture; +import java.util.concurrent.ExecutionException; import java.util.concurrent.TimeUnit; import java.util.concurrent.TimeoutException; @@ -65,14 +67,25 @@ static void stop() { } @Test - void postIdentityServerTTLExceeded() - throws URISyntaxException, JsonProcessingException { + void postIdentityServerTTLExceeded() throws URISyntaxException, JsonProcessingException { RestClient restClient = new RestClient(); Identity identity = createIdentity(); CompletableFuture> post = restClient.post( identity, new URI("http://localhost:" + MOCK_SERVER_PORT + "/" + IDENTITY_ENDPOINT)); - assertThrows(TimeoutException.class, () -> post.get(100, TimeUnit.MILLISECONDS).statusCode()); + + try { + post.get(100, TimeUnit.MILLISECONDS).statusCode(); + fail("Should have thrown an TimeoutException or ExecutionException"); + } catch (ExecutionException | TimeoutException e) { + // ok + } catch (Exception e) { + fail("Should have thrown an TimeoutException or ExecutionException"); + } + + await().atMost(5, TimeUnit.SECONDS) + .untilAsserted(() -> wireMockServer.verify(1, postRequestedFor(urlEqualTo("/" + IDENTITY_ENDPOINT)))); + wireMockServer.verify(postRequestedFor(urlEqualTo("/" + IDENTITY_ENDPOINT)) .withRequestBody(equalToJson(getObjectMapper().writeValueAsString(identity)))); } diff --git a/independent-projects/tools/analytics-common/src/test/java/io/quarkus/analytics/rest/RestClientTest.java b/independent-projects/tools/analytics-common/src/test/java/io/quarkus/analytics/rest/RestClientTest.java index 80a36b23df4d4..9f8ce3e6333b4 100644 --- a/independent-projects/tools/analytics-common/src/test/java/io/quarkus/analytics/rest/RestClientTest.java +++ b/independent-projects/tools/analytics-common/src/test/java/io/quarkus/analytics/rest/RestClientTest.java @@ -10,6 +10,7 @@ import static io.quarkus.analytics.rest.RestClient.IDENTITY_ENDPOINT; import static io.quarkus.analytics.rest.RestClient.TRACK_ENDPOINT; import static io.quarkus.analytics.util.StringUtils.getObjectMapper; +import static org.awaitility.Awaitility.await; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertNotNull; @@ -99,6 +100,10 @@ void postIdentity() CompletableFuture> post = restClient.post(identity, new URI("http://localhost:" + MOCK_SERVER_PORT + "/" + IDENTITY_ENDPOINT)); assertEquals(201, post.get(1, TimeUnit.SECONDS).statusCode()); + + await().atMost(5, TimeUnit.SECONDS) + .untilAsserted(() -> wireMockServer.verify(1, postRequestedFor(urlEqualTo("/" + IDENTITY_ENDPOINT)))); + wireMockServer.verify(postRequestedFor(urlEqualTo("/" + IDENTITY_ENDPOINT)) .withRequestBody(equalToJson(getObjectMapper().writeValueAsString(identity)))); } diff --git a/independent-projects/tools/pom.xml b/independent-projects/tools/pom.xml index 9b3eb1bada869..a59202baa9c89 100644 --- a/independent-projects/tools/pom.xml +++ b/independent-projects/tools/pom.xml @@ -66,6 +66,7 @@ 25 3.1.2 2.0.2 + 4.2.0 registry-client