From c72baaceff8b8eb50ed51afc5ffca2f31d7b1871 Mon Sep 17 00:00:00 2001 From: James Rosewell Date: Mon, 9 Nov 2026 20:59:31 +0000 Subject: [PATCH 01/16] Test that the Cloud Request Engine discovers when it is built, and retries only after a failure that can pass CloudRequestEngineDiscoveryTests uses a stand-in for the HTTP client that counts the requests to each endpoint. A build fetches the accessible properties and the evidence keys once each and processing fetches neither again. A 4xx answer to either fails the build with a CloudRequestException carrying the status, once both requests are made, also when the other request failed in a way that can pass. A 5xx answer, a body that cannot be read, a refused connection and a timeout each complete the build with one warning. A list that arrived is kept, processing fetches only what is missing and sends no data request until both are known, the Pipeline's Evidence filter sees the keys a retry fetched, and the configured timeout applies to the discovery requests. The existing tests that expected getPublicProperties and getEvidenceKeyFilter to fetch and throw when the server was unavailable, and a 200 answer holding errors to throw, now expect the build to complete with one warning. The cloud aspect engines are tested to build without logging when the accessible products are not known yet, and to read their metadata once they are. --- .../flowelements/ProductMissingTests.java | 31 ++ .../flowelements/ProductMissingTests.java | 31 ++ .../GeoLocationCloudEngineTests.java | 32 ++ .../CloudAspectEngineBaseTests.java | 36 +- .../CloudRequestEngineDiscoveryTests.java | 402 ++++++++++++++++++ .../flowelements/CloudRequestEngineTests.java | 98 +++-- 6 files changed, 568 insertions(+), 62 deletions(-) create mode 100644 pipeline/pipeline.cloudrequestengine/src/test/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudRequestEngineDiscoveryTests.java diff --git a/device-detection/device-detection.cloud/src/test/java/fiftyone/devicedetection/cloud/flowelements/ProductMissingTests.java b/device-detection/device-detection.cloud/src/test/java/fiftyone/devicedetection/cloud/flowelements/ProductMissingTests.java index 37c1ec6d..32eb7356 100644 --- a/device-detection/device-detection.cloud/src/test/java/fiftyone/devicedetection/cloud/flowelements/ProductMissingTests.java +++ b/device-detection/device-detection.cloud/src/test/java/fiftyone/devicedetection/cloud/flowelements/ProductMissingTests.java @@ -114,6 +114,37 @@ public void ProductMissing_PropertyIsMissing() throws Exception { } } + /** + * After a failure at build that can pass, the Cloud Request Engine does + * not know the accessible products yet. The engine builds without + * logging anything, and reads its metadata once the products are known + * rather than keeping none. + */ + @Test + public void DiscoveryNotComplete_LoadsTheMetadataLater() throws Exception { + ProductMetaData product = new ProductMetaData(); + product.dataTier = "test"; + product.properties = Arrays.asList( + new PropertyMetaData("hardwarevendor", "String", "cat", null)); + Map products = new HashMap<>(); + products.put("device", product); + CloudRequestEngine requestEngine = mock(CloudRequestEngine.class); + when(requestEngine.getPublicProperties()).thenReturn(null, products); + Pipeline pipeline = mock(Pipeline.class); + when(pipeline.getElement(CloudRequestEngine.class)) + .thenReturn(requestEngine); + + try (DeviceDetectionCloudEngine engine = + new DeviceDetectionCloudEngineBuilder(loggerFactory).build()) { + engine.addPipeline(pipeline); + List logged = warningsAndErrors(); + assertEquals(logged.toString(), 0, logged.size()); + + assertEquals(1, engine.getProperties().size()); + assertEquals("hardwarevendor", engine.getProperties().get(0).getName()); + } + } + /** * An engine whose Cloud Request Engine reports the accessible products * of a resource key that includes IP Intelligence alone. diff --git a/ip-intelligence/ip-intelligence.cloud/src/test/java/fiftyone/ipintelligence/cloud/flowelements/ProductMissingTests.java b/ip-intelligence/ip-intelligence.cloud/src/test/java/fiftyone/ipintelligence/cloud/flowelements/ProductMissingTests.java index df07df77..3249c2ab 100644 --- a/ip-intelligence/ip-intelligence.cloud/src/test/java/fiftyone/ipintelligence/cloud/flowelements/ProductMissingTests.java +++ b/ip-intelligence/ip-intelligence.cloud/src/test/java/fiftyone/ipintelligence/cloud/flowelements/ProductMissingTests.java @@ -114,6 +114,37 @@ public void ProductMissing_PropertyIsMissing() throws Exception { } } + /** + * After a failure at build that can pass, the Cloud Request Engine does + * not know the accessible products yet. The engine builds without + * logging anything, and reads its metadata once the products are known + * rather than keeping none. + */ + @Test + public void DiscoveryNotComplete_LoadsTheMetadataLater() throws Exception { + ProductMetaData product = new ProductMetaData(); + product.dataTier = "test"; + product.properties = Arrays.asList( + new PropertyMetaData("registeredname", "String", "cat", null)); + Map products = new HashMap<>(); + products.put("ip", product); + CloudRequestEngine requestEngine = mock(CloudRequestEngine.class); + when(requestEngine.getPublicProperties()).thenReturn(null, products); + Pipeline pipeline = mock(Pipeline.class); + when(pipeline.getElement(CloudRequestEngine.class)) + .thenReturn(requestEngine); + + try (IPIntelligenceCloudEngine engine = + new IPIntelligenceCloudEngineBuilder(loggerFactory).build()) { + engine.addPipeline(pipeline); + List logged = warningsAndErrors(); + assertEquals(logged.toString(), 0, logged.size()); + + assertEquals(1, engine.getProperties().size()); + assertEquals("registeredname", engine.getProperties().get(0).getName()); + } + } + /** * An engine whose Cloud Request Engine reports the accessible products * of a resource key that includes Device Detection alone. diff --git a/location/geo-location.cloud/src/test/java/fiftyone/geolocation/flowelements/GeoLocationCloudEngineTests.java b/location/geo-location.cloud/src/test/java/fiftyone/geolocation/flowelements/GeoLocationCloudEngineTests.java index 66f40b96..1520a46e 100644 --- a/location/geo-location.cloud/src/test/java/fiftyone/geolocation/flowelements/GeoLocationCloudEngineTests.java +++ b/location/geo-location.cloud/src/test/java/fiftyone/geolocation/flowelements/GeoLocationCloudEngineTests.java @@ -116,6 +116,38 @@ public void ProductMissing_PropertyIsMissing() throws Exception { } } + /** + * After a failure at build that can pass, the Cloud Request Engine does + * not know the accessible products yet. The engine builds without + * logging anything, and reads its metadata once the products are known + * rather than keeping none. + */ + @Test + public void DiscoveryNotComplete_LoadsTheMetadataLater() throws Exception { + ProductMetaData product = new ProductMetaData(); + product.dataTier = "test"; + product.properties = Arrays.asList( + new PropertyMetaData("town", "String", "cat", null)); + Map products = new HashMap<>(); + products.put("location", product); + CloudRequestEngine requestEngine = mock(CloudRequestEngine.class); + when(requestEngine.getPublicProperties()).thenReturn(null, products); + Pipeline pipeline = mock(Pipeline.class); + when(pipeline.getElement(CloudRequestEngine.class)) + .thenReturn(requestEngine); + + try (GeoLocationCloudEngine engine = + new GeoLocationCloudEngineBuilder(loggerFactory) + .build(Enums.GeoLocationProvider.FiftyOneDegrees)) { + engine.addPipeline(pipeline); + List logged = warningsAndErrors(); + assertEquals(logged.toString(), 0, logged.size()); + + assertEquals(1, engine.getProperties().size()); + assertEquals("town", engine.getProperties().get(0).getName()); + } + } + /** * An engine whose Cloud Request Engine reports the accessible products * of a resource key that includes IP Intelligence alone. diff --git a/pipeline/pipeline.cloudrequestengine/src/test/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudAspectEngineBaseTests.java b/pipeline/pipeline.cloudrequestengine/src/test/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudAspectEngineBaseTests.java index f0f4ac5b..3ec05bb4 100644 --- a/pipeline/pipeline.cloudrequestengine/src/test/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudAspectEngineBaseTests.java +++ b/pipeline/pipeline.cloudrequestengine/src/test/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudAspectEngineBaseTests.java @@ -22,7 +22,6 @@ package fiftyone.pipeline.cloudrequestengine.flowelements; -import static fiftyone.pipeline.cloudrequestengine.Constants.Messages.ExceptionFailedToLoadProperties; import static org.junit.jupiter.api.Assertions.*; import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.doAnswer; @@ -319,27 +318,30 @@ public void loadProperties_wrongProperties() throws Exception { } /** - * Accessible properties that have not been loaded are not a missing - * product, so reading the property metadata still fails rather than - * keeping an empty result. + * Accessible properties the Cloud Request Engine has not fetched yet, + * after a failure at build that can pass, are not a missing product. + * The Pipeline builds, the metadata is empty and nothing is logged, and + * the metadata is read again once the properties are known rather than + * kept empty. * @throws Exception */ @Test public void loadProperties_notLoaded() throws Exception { propertiesReturnedByRequestEngine = null; - try { - createPipeline(); - fail("RuntimeException should be thrown"); - } catch (RuntimeException ex){ - assertEquals( - String.format( - ExceptionFailedToLoadProperties, - "test", - "test"), - ex.getMessage() - ); - } - maxErrors = 1; + createPipeline(); + + assertEquals(0, engine.getProperties().size()); + + List properties = new ArrayList<>(); + properties.add(new AccessiblePropertyMetaData.PropertyMetaData("ismobile", "Boolean", null, null)); + AccessiblePropertyMetaData.ProductMetaData product = new AccessiblePropertyMetaData.ProductMetaData(); + product.properties = properties; + Map products = new HashMap<>(); + products.put("test", product); + requestEngine.setPublicProperties(products); + + assertEquals(1, engine.getProperties().size()); + assertTrue(propertiesContainName(engine.getProperties(), "ismobile")); } /** diff --git a/pipeline/pipeline.cloudrequestengine/src/test/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudRequestEngineDiscoveryTests.java b/pipeline/pipeline.cloudrequestengine/src/test/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudRequestEngineDiscoveryTests.java new file mode 100644 index 00000000..1ae9bb3e --- /dev/null +++ b/pipeline/pipeline.cloudrequestengine/src/test/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudRequestEngineDiscoveryTests.java @@ -0,0 +1,402 @@ +/* ********************************************************************* + * This Original Work is copyright of 51 Degrees Mobile Experts Limited. + * Copyright 2026 51 Degrees Mobile Experts Limited, Davidson House, + * Forbury Square, Reading, Berkshire, United Kingdom RG1 3EU. + * + * This Original Work is licensed under the European Union Public Licence + * (EUPL) v.1.2 and is subject to its terms as set out below. + * + * If a copy of the EUPL was not distributed with this file, You can obtain + * one at https://opensource.org/licenses/EUPL-1.2. + * + * The 'Compatible Licences' set out in the Appendix to the EUPL (as may be + * amended by the European Commission) shall be deemed incompatible for + * the purposes of the Work and the provisions of the compatibility + * clause in Article 5 of the EUPL shall not apply. + * + * If using the Work as, or as part of, a network application, by + * including the attribution notice(s) required under Article 5 of the EUPL + * in the end user terms of the application under an appropriate heading, + * such notice(s) shall fulfill the requirements of that article. + * ********************************************************************* */ + +package fiftyone.pipeline.cloudrequestengine.flowelements; + +import fiftyone.common.testhelpers.TestLogger; +import fiftyone.common.testhelpers.TestLoggerFactory; +import fiftyone.pipeline.cloudrequestengine.CloudRequestException; +import fiftyone.pipeline.core.data.FlowData; +import fiftyone.pipeline.core.flowelements.Pipeline; +import fiftyone.pipeline.core.flowelements.PipelineBuilder; +import fiftyone.pipeline.engines.services.HttpClient; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; +import org.slf4j.ILoggerFactory; +import org.slf4j.Logger; + +import java.io.IOException; +import java.net.ConnectException; +import java.net.HttpURLConnection; +import java.net.SocketTimeoutException; +import java.net.URL; +import java.util.ArrayList; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import java.util.stream.Stream; + +import static org.junit.jupiter.api.Assertions.*; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyMap; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +/** + * The Cloud Request Engine fetches its accessible properties and accepted + * Evidence keys when it is built. A 4xx answer to either fails the build. + * Any other failure is logged as one warning, the build completes, and + * processing fetches what is missing before it sends a data request. + */ +public class CloudRequestEngineDiscoveryTests { + + private static final String PROPERTIES = "accessibleproperties"; + private static final String EVIDENCE_KEYS = "evidencekeys"; + private static final String DATA = "json"; + + private static final String PROPERTIES_BODY = + "{\"Products\": {\"device\": {\"DataTier\": \"tier\", " + + "\"Properties\": [{\"Name\": \"IsMobile\", \"Type\": \"Boolean\"," + + " \"Category\": \"Device\"}]}}}"; + private static final String EVIDENCE_KEYS_BODY = + "[\"query.User-Agent\", \"header.User-Agent\"]"; + private static final String DATA_BODY = "{\"device\": {\"ismobile\": true}}"; + + /** + * A failure that is not an HTTP status, such as a refused connection. + */ + private static final int UNREACHABLE = -1; + + /** + * A failure where the request hangs until the timeout. + */ + private static final int TIMES_OUT = -2; + + /** + * A 200 answer whose body cannot be read. + */ + private static final int UNREADABLE = -3; + + private static final int TIMEOUT_SECONDS = 3; + + private TestLoggerFactory loggerFactory; + private HttpClient httpClient; + private final Map requests = new HashMap<>(); + private final Map answers = new HashMap<>(); + private final List connections = new ArrayList<>(); + + @BeforeEach + public void init() throws IOException { + ILoggerFactory internal = mock(ILoggerFactory.class); + when(internal.getLogger(anyString())).thenReturn(mock(Logger.class)); + loggerFactory = new TestLoggerFactory(internal); + answers.put(PROPERTIES, 200); + answers.put(EVIDENCE_KEYS, 200); + answers.put(DATA, 200); + + httpClient = mock(HttpClient.class); + when(httpClient.connect(any(URL.class))).thenAnswer(invocation -> { + URL url = invocation.getArgument(0); + String name = endpoint(url); + requests.merge(name, 1, Integer::sum); + int answer = answers.get(name); + if (answer == UNREACHABLE) { + throw new ConnectException("Connection refused"); + } + HttpURLConnection connection = mock(HttpURLConnection.class); + when(connection.getURL()).thenReturn(url); + when(connection.getResponseCode()) + .thenReturn(answer < 0 ? 200 : answer); + when(connection.getHeaderFields()) + .thenReturn(new HashMap>()); + connections.add(connection); + return connection; + }); + when(httpClient.getResponseString( + any(HttpURLConnection.class), anyMap())) + .thenAnswer(invocation -> body(invocation.getArgument(0))); + when(httpClient.getResponseString(any(HttpURLConnection.class))) + .thenAnswer(invocation -> body(invocation.getArgument(0))); + when(httpClient.postData( + any(HttpURLConnection.class), anyMap(), any(byte[].class))) + .thenAnswer(invocation -> body(invocation.getArgument(0))); + } + + /** + * In the normal case the built engine is ready. Both lists are fetched + * once, at build, and processing sends only the data request. + */ + @Test + public void Build_FetchesBothLists() throws Exception { + CloudRequestEngine engine = build(); + + assertEquals(1, requests(PROPERTIES), "accessibleproperties"); + assertEquals(1, requests(EVIDENCE_KEYS), "evidencekeys"); + assertEquals(0, requests(DATA), "json"); + assertNotNull(engine.getPublicProperties()); + assertTrue(engine.getPublicProperties().containsKey("device")); + assertTrue(engine.getEvidenceKeyFilter().include("header.user-agent")); + assertEquals(0, warnings().size(), warnings().toString()); + + process(engine, false); + + assertEquals(1, requests(PROPERTIES), "accessibleproperties"); + assertEquals(1, requests(EVIDENCE_KEYS), "evidencekeys"); + assertEquals(1, requests(DATA), "json"); + } + + private static Stream definitiveFailures() { + List arguments = new ArrayList<>(); + for (int status : new int[] { 400, 401, 403, 404, 429 }) { + arguments.add(Arguments.of(PROPERTIES, status)); + arguments.add(Arguments.of(EVIDENCE_KEYS, status)); + } + return arguments.stream(); + } + + /** + * A 4xx answer to either request is a definitive configuration error, + * which fails the build once both requests have been made. + */ + @ParameterizedTest(name = "{1} from {0}") + @MethodSource("definitiveFailures") + public void Build_4xx_FailsTheBuild(String endpoint, int status) { + answers.put(endpoint, status); + + CloudRequestException error = + assertThrows(CloudRequestException.class, this::build); + + assertEquals(status, error.getHttpStatusCode()); + assertEquals(1, requests(PROPERTIES), "accessibleproperties"); + assertEquals(1, requests(EVIDENCE_KEYS), "evidencekeys"); + } + + /** + * A 4xx answer fails the build even when the other request failed in a + * way that can pass. + */ + @Test + public void Build_4xxAndTransient_FailsTheBuild() { + answers.put(PROPERTIES, 503); + answers.put(EVIDENCE_KEYS, 403); + + CloudRequestException error = + assertThrows(CloudRequestException.class, this::build); + + assertEquals(403, error.getHttpStatusCode()); + } + + private static Stream transientFailures() { + return Stream.of( + Arguments.of("500 from accessibleproperties", 500, 200), + Arguments.of("503 from evidencekeys", 200, 503), + Arguments.of("502 from both", 502, 502), + Arguments.of("unreadable accessibleproperties", UNREADABLE, 200), + Arguments.of("unreadable evidencekeys", 200, UNREADABLE), + Arguments.of("connection refused to both", UNREACHABLE, UNREACHABLE), + Arguments.of("timeout on evidencekeys", 200, TIMES_OUT)); + } + + /** + * Any other failure, the service unreachable, a timeout, a 5xx answer or + * a body that cannot be read, is logged as one warning and the build + * completes. + */ + @ParameterizedTest(name = "{0}") + @MethodSource("transientFailures") + public void Build_TransientFailure_CompletesWithOneWarning( + String name, + int propertiesAnswer, + int evidenceKeysAnswer) throws Exception { + answers.put(PROPERTIES, propertiesAnswer); + answers.put(EVIDENCE_KEYS, evidenceKeysAnswer); + + CloudRequestEngine engine = build(); + + assertNotNull(engine); + assertEquals(1, requests(PROPERTIES), "accessibleproperties"); + assertEquals(1, requests(EVIDENCE_KEYS), "evidencekeys"); + assertEquals(1, warnings().size(), warnings().toString()); + } + + /** + * A list that arrived at build is kept, and the other is empty until it + * is fetched. Reading them makes no request. + */ + @Test + public void Build_PartialFailure_KeepsWhatArrived() throws Exception { + answers.put(EVIDENCE_KEYS, 503); + + CloudRequestEngine engine = build(); + + assertNotNull(engine.getPublicProperties()); + assertTrue(engine.getPublicProperties().containsKey("device")); + assertFalse(engine.getEvidenceKeyFilter().include("header.user-agent")); + assertEquals(1, requests(PROPERTIES), "accessibleproperties"); + assertEquals(1, requests(EVIDENCE_KEYS), "evidencekeys"); + } + + /** + * Processing fetches only what the build could not, then sends the data + * request. The Pipeline's Evidence filter, built before the retry, sees + * the keys the retry fetched. + */ + @Test + public void Process_FetchesOnlyWhatIsMissing() throws Exception { + answers.put(EVIDENCE_KEYS, 503); + CloudRequestEngine engine = build(); + answers.put(EVIDENCE_KEYS, 200); + + try (Pipeline pipeline = new PipelineBuilder(loggerFactory) + .addFlowElement(engine) + .build()) { + assertFalse(pipeline.getEvidenceKeyFilter() + .include("header.user-agent")); + try (FlowData data = pipeline.createFlowData()) { + data.addEvidence("header.user-agent", "iPhone"); + data.process(); + } + assertTrue(pipeline.getEvidenceKeyFilter() + .include("header.user-agent")); + } + + assertEquals(1, requests(PROPERTIES), "accessibleproperties"); + assertEquals(2, requests(EVIDENCE_KEYS), "evidencekeys"); + assertEquals(1, requests(DATA), "json"); + assertTrue(engine.getEvidenceKeyFilter().include("header.user-agent")); + } + + /** + * While discovery still fails no data request is sent, and the failure + * is recorded as SuppressProcessExceptions says. + */ + @Test + public void Process_DiscoveryStillFailing_Suppressed() throws Exception { + answers.put(PROPERTIES, 503); + CloudRequestEngine engine = build(); + + try (Pipeline pipeline = new PipelineBuilder(loggerFactory) + .setSuppressProcessException(true) + .addFlowElement(engine) + .build(); + FlowData data = pipeline.createFlowData()) { + data.addEvidence("header.user-agent", "iPhone"); + data.process(); + + assertFalse(data.getErrors().isEmpty()); + } + + assertEquals(2, requests(PROPERTIES), "accessibleproperties"); + assertEquals(0, requests(DATA), "json"); + } + + @Test + public void Process_DiscoveryStillFailing_Throws() throws Exception { + answers.put(PROPERTIES, UNREACHABLE); + answers.put(EVIDENCE_KEYS, UNREACHABLE); + CloudRequestEngine engine = build(); + + try (Pipeline pipeline = new PipelineBuilder(loggerFactory) + .addFlowElement(engine) + .build(); + FlowData data = pipeline.createFlowData()) { + data.addEvidence("header.user-agent", "iPhone"); + assertThrows(RuntimeException.class, data::process); + } + + assertEquals(0, requests(DATA), "json"); + } + + /** + * The configured timeout applies to the discovery requests, so a + * service that does not answer delays the build by the timeout alone. + */ + @Test + public void Build_AppliesTheTimeoutToDiscovery() throws Exception { + build(); + + assertEquals(2, connections.size()); + for (HttpURLConnection connection : connections) { + verify(connection).setConnectTimeout(TIMEOUT_SECONDS * 1000); + verify(connection).setReadTimeout(TIMEOUT_SECONDS * 1000); + } + } + + private CloudRequestEngine build() throws Exception { + return new CloudRequestEngineBuilder(loggerFactory, httpClient) + .setResourceKey("resource_key") + .setTimeOutSeconds(TIMEOUT_SECONDS) + .build(); + } + + private void process(CloudRequestEngine engine, boolean suppress) + throws Exception { + try (Pipeline pipeline = new PipelineBuilder(loggerFactory) + .setSuppressProcessException(suppress) + .addFlowElement(engine) + .build(); + FlowData data = pipeline.createFlowData()) { + data.addEvidence("header.user-agent", "iPhone"); + data.process(); + } + } + + private int requests(String endpoint) { + return requests.getOrDefault(endpoint, 0); + } + + private List warnings() { + List warnings = new ArrayList<>(); + for (TestLogger logger : loggerFactory.loggers) { + warnings.addAll(logger.warningsLogged); + } + return warnings; + } + + private static String endpoint(URL url) { + String path = url.getPath().toLowerCase(); + if (path.endsWith(PROPERTIES)) { + return PROPERTIES; + } + if (path.endsWith(EVIDENCE_KEYS)) { + return EVIDENCE_KEYS; + } + return DATA; + } + + private String body(HttpURLConnection connection) throws IOException { + String name = endpoint(connection.getURL()); + int answer = answers.get(name); + if (answer == TIMES_OUT) { + throw new SocketTimeoutException("Read timed out"); + } + if (answer == UNREADABLE) { + return "proxy error"; + } + if (answer != 200) { + return "{\"errors\": [\"" + name + " answered " + answer + "\"]}"; + } + switch (name) { + case PROPERTIES: + return PROPERTIES_BODY; + case EVIDENCE_KEYS: + return EVIDENCE_KEYS_BODY; + default: + return DATA_BODY; + } + } +} diff --git a/pipeline/pipeline.cloudrequestengine/src/test/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudRequestEngineTests.java b/pipeline/pipeline.cloudrequestengine/src/test/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudRequestEngineTests.java index 80ebde30..100de4cf 100644 --- a/pipeline/pipeline.cloudrequestengine/src/test/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudRequestEngineTests.java +++ b/pipeline/pipeline.cloudrequestengine/src/test/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudRequestEngineTests.java @@ -263,14 +263,11 @@ private static Stream getValidateErrorArgs() { } /** - * Test cloud request engine handles errors from the cloud service - * as expected. - * An AggregateException should be thrown by the cloud request engine - * containing the errors from the cloud service - * and the pipeline is configured to throw any exceptions up - * the stack in an AggregateException. - * We also check that the exception message includes the content - * from the JSON response. + * Errors from the cloud service when the engine is built. A 4xx answer + * is a definitive configuration error, which fails the build with a + * CloudRequestException that carries the cloud's message. Any other + * answer that holds errors is logged as one warning, the build + * completes, and the accessible properties are unknown until a retry. */ @ParameterizedTest(name = "response code {1} - {0}") @MethodSource("getValidateErrorArgs") @@ -284,24 +281,33 @@ public void validateErrorHandling(String name, int responseCode, boolean include configureMockedClient(); - Exception exception = null; - - try { - new CloudRequestEngineBuilder(loggerFactory, httpClient) - .setResourceKey(resourceKey) - .build() - .getPublicProperties(); - } catch (Exception ex) { - exception = ex; + if (responseCode >= 400 && responseCode < 500) { + CloudRequestException cloudEx = assertThrows( + CloudRequestException.class, + () -> new CloudRequestEngineBuilder(loggerFactory, httpClient) + .setResourceKey(resourceKey) + .build()); + assertEquals(responseCode, cloudEx.getHttpStatusCode()); + if (includeMessage) { + assertTrue(cloudEx.getMessage().contains(errorMessage), + "Exception message did not contain the expected text."); + } + } else { + CloudRequestEngine engine = + new CloudRequestEngineBuilder(loggerFactory, httpClient) + .setResourceKey(resourceKey) + .build(); + assertNull(engine.getPublicProperties()); + assertEquals(1, warningCount()); } + } - assertNotNull(exception, "Expected exception to occur"); - assertTrue(exception instanceof CloudRequestException); - CloudRequestException cloudEx = (CloudRequestException) exception; - if (includeMessage) { - assertTrue(cloudEx.getMessage().contains(errorMessage), - "Exception message did not contain the expected text."); + private int warningCount() { + int count = 0; + for (TestLogger logger : loggerFactory.loggers) { + count += logger.warningsLogged.size(); } + return count; } private static Stream getPrecidenceArgs() { @@ -656,22 +662,22 @@ public void getPublicProperties() throws Exception { } /** - * Verify that PropertyNotLoadedException is thrown on getPublicProperties if remote server is unavailable + * An unreachable server when the engine is built is a failure that can + * pass. The build completes with one warning, and the accessible + * properties are unknown until processing fetches them. */ @Test - public void getPublicProperties_Throw_Exception_When_Server_Unavailable() throws IOException { + public void getPublicProperties_Server_Unavailable_At_Build() throws Exception { final String resourceKey = "resource_key"; configureFailingMockClient(); - try { - new CloudRequestEngineBuilder(loggerFactory, httpClient) - .setResourceKey(resourceKey) - .build() - .getPublicProperties(); - fail("Expected exception was not thrown"); - } catch (Exception e) { - assertInstanceOf(PropertyNotLoadedException.class, e); - } + CloudRequestEngine engine = + new CloudRequestEngineBuilder(loggerFactory, httpClient) + .setResourceKey(resourceKey) + .build(); + + assertNull(engine.getPublicProperties()); + assertEquals(1, warningCount()); } /** @@ -689,21 +695,23 @@ public void getEvidenceKeyFilter() throws Exception { } /** - * Verify that PropertyNotLoadedException is thrown on getEvidenceKeyFilter if remote server is unavailable + * An unreachable server when the engine is built leaves the accepted + * Evidence empty, rather than failing whatever reads it, until + * processing fetches it. */ @Test - public void getEvidenceKeyFilter_Throw_Exception_When_Server_Unavailable() throws IOException { + public void getEvidenceKeyFilter_Server_Unavailable_At_Build() throws Exception { final String resourceKey = "resource_key"; configureFailingMockClient(); - try { - new CloudRequestEngineBuilder(loggerFactory, httpClient) - .setResourceKey(resourceKey) - .build() - .getEvidenceKeyFilter(); - fail("Expected exception was not thrown"); - } catch (Exception e) { - assertInstanceOf(PropertyNotLoadedException.class, e); - } + EvidenceKeyFilter filter = + new CloudRequestEngineBuilder(loggerFactory, httpClient) + .setResourceKey(resourceKey) + .build() + .getEvidenceKeyFilter(); + + assertNotNull(filter); + assertFalse(filter.include("query.User-Agent")); + assertEquals(1, warningCount()); } } From a4b6fd5e24e6812a702244536af8c8a721eded5d Mon Sep 17 00:00:00 2001 From: James Rosewell Date: Mon, 9 Nov 2026 21:05:43 +0000 Subject: [PATCH 02/16] Discover when the Cloud Request Engine is built, and retry on first use only after a failure that can pass The engine fetched its accessible properties and accepted evidence keys only when something first asked for them, and a failure surfaced wherever that was. The builder now calls discover, which makes both requests. A 4xx answer to either is a definitive configuration error, and its CloudRequestException fails the build once both requests are made. Any other failure, the service unreachable, a timeout, a 5xx answer or a body that cannot be read, is logged as one warning, the build completes and the list that arrived is kept. The configured timeout now applies to both requests. getPublicProperties and getEvidenceKeyFilter make no request. The products are null until they are known. The evidence filter is one CloudEvidenceKeyFilter that includes nothing until the keys are known and reads them on each call, and it derives from EvidenceKeyFilterWhitelist so the Pipeline's filter keeps it rather than copying its keys once. Processing fetches what is missing, evidence keys first, before it sends the data request, and a request that could not be made is reported as its IOException, as for the data request. The Device Detection, IP Intelligence and Location cloud engines, and CloudAspectEngineBase, read their metadata when the products are known rather than logging errors at build and keeping none. A missing product still builds and logs once. The web integration reads a filter that derives from EvidenceKeyFilterWhitelist for the Vary header, as it reads the exact class. --- .../DeviceDetectionCloudEngine.java | 36 +++- .../IPIntelligenceCloudEngine.java | 36 +++- .../flowelements/GeoLocationCloudEngine.java | 36 +++- .../cloudrequestengine/Constants.java | 7 + .../flowelements/CloudAspectEngineBase.java | 22 +- .../flowelements/CloudEvidenceKeyFilter.java | 94 +++++++++ .../flowelements/CloudRequestEngine.java | 3 +- .../CloudRequestEngineBuilder.java | 7 +- .../CloudRequestEngineDefault.java | 188 +++++++++++++++--- .../ClientsidePropertyServiceCore.java | 10 +- 10 files changed, 370 insertions(+), 69 deletions(-) create mode 100644 pipeline/pipeline.cloudrequestengine/src/main/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudEvidenceKeyFilter.java diff --git a/device-detection/device-detection.cloud/src/main/java/fiftyone/devicedetection/cloud/flowelements/DeviceDetectionCloudEngine.java b/device-detection/device-detection.cloud/src/main/java/fiftyone/devicedetection/cloud/flowelements/DeviceDetectionCloudEngine.java index 2e6e2f97..e43fc4b2 100644 --- a/device-detection/device-detection.cloud/src/main/java/fiftyone/devicedetection/cloud/flowelements/DeviceDetectionCloudEngine.java +++ b/device-detection/device-detection.cloud/src/main/java/fiftyone/devicedetection/cloud/flowelements/DeviceDetectionCloudEngine.java @@ -55,9 +55,10 @@ */ public class DeviceDetectionCloudEngine extends CloudAspectEngineBase { - List aspectProperties; + volatile List aspectProperties; private String dataSourceTier; CloudRequestEngine cloudRequestEngine; + private final Object metadataLock = new Object(); /** * Construct a new instance of the {@link DeviceDetectionCloudEngine}. @@ -72,9 +73,26 @@ public DeviceDetectionCloudEngine( this.cloudRequestEngine = null; } + /** + * The property metadata, from the products the Cloud Request Engine + * reports. Until they are known, after a failure at build that can + * pass, this is empty and is read again on the next call. + * @return the property metadata + */ @Override public List getProperties() { - return aspectProperties; + List loaded = aspectProperties; + if (loaded == null && cloudRequestEngine != null) { + synchronized (metadataLock) { + if (aspectProperties == null) { + loadAspectProperties(cloudRequestEngine); + } + loaded = aspectProperties; + } + } + return loaded == null ? + new ArrayList() : + loaded; } @Override @@ -188,9 +206,10 @@ public void addPipeline(Pipeline pipeline) { if (cloudRequestEngine == null) { cloudRequestEngine = pipeline.getElement(CloudRequestEngine.class); if (cloudRequestEngine != null) { - if (loadAspectProperties(cloudRequestEngine) == false) { - logger.error("Failed to load aspect properties"); - } + // The products are not known after a failure at build that + // can pass, which the Cloud Request Engine has logged. + // getProperties reads them once they are. + loadAspectProperties(cloudRequestEngine); } } @@ -209,7 +228,7 @@ private boolean loadAspectProperties(CloudRequestEngine engine) { findProduct(engine.getPublicProperties()); if (product != null) { - aspectProperties = new ArrayList<>(); + List properties = new ArrayList<>(); dataSourceTier = product.dataTier; for (AccessiblePropertyMetaData.PropertyMetaData item : @@ -221,13 +240,12 @@ private boolean loadAspectProperties(CloudRequestEngine engine) { item.getPropertyType(), new ArrayList(), true); - aspectProperties.add(property); + properties.add(property); } + aspectProperties = properties; return true; } else { - logger.error("Aspect properties could not be loaded for" + - " the Device Detection cloud engine", this); return false; } } diff --git a/ip-intelligence/ip-intelligence.cloud/src/main/java/fiftyone/ipintelligence/cloud/flowelements/IPIntelligenceCloudEngine.java b/ip-intelligence/ip-intelligence.cloud/src/main/java/fiftyone/ipintelligence/cloud/flowelements/IPIntelligenceCloudEngine.java index da503923..bec4510d 100644 --- a/ip-intelligence/ip-intelligence.cloud/src/main/java/fiftyone/ipintelligence/cloud/flowelements/IPIntelligenceCloudEngine.java +++ b/ip-intelligence/ip-intelligence.cloud/src/main/java/fiftyone/ipintelligence/cloud/flowelements/IPIntelligenceCloudEngine.java @@ -63,8 +63,9 @@ */ public class IPIntelligenceCloudEngine extends CloudAspectEngineBase { - List aspectProperties; + volatile List aspectProperties; private String dataSourceTier; + private final Object metadataLock = new Object(); // Lower-cased names of properties the cloud returns as weighted lists // (cloud type "Weighted..."), so processEngine can read their value/weight // pairs rather than treating them as a plain list. @@ -84,9 +85,26 @@ public IPIntelligenceCloudEngine( this.cloudRequestEngine = null; } + /** + * The property metadata, from the products the Cloud Request Engine + * reports. Until they are known, after a failure at build that can + * pass, this is empty and is read again on the next call. + * @return the property metadata + */ @Override public List getProperties() { - return aspectProperties; + List loaded = aspectProperties; + if (loaded == null && cloudRequestEngine != null) { + synchronized (metadataLock) { + if (aspectProperties == null) { + loadAspectProperties(cloudRequestEngine); + } + loaded = aspectProperties; + } + } + return loaded == null ? + new ArrayList() : + loaded; } @Override @@ -222,9 +240,10 @@ public void addPipeline(Pipeline pipeline) { if (cloudRequestEngine == null) { cloudRequestEngine = pipeline.getElement(CloudRequestEngine.class); if (cloudRequestEngine != null) { - if (loadAspectProperties(cloudRequestEngine) == false) { - logger.error("Failed to load aspect properties"); - } + // The products are not known after a failure at build that + // can pass, which the Cloud Request Engine has logged. + // getProperties reads them once they are. + loadAspectProperties(cloudRequestEngine); } } @@ -243,7 +262,7 @@ private boolean loadAspectProperties(CloudRequestEngine engine) { findProduct(engine.getPublicProperties()); if (product != null) { - aspectProperties = new ArrayList<>(); + List properties = new ArrayList<>(); weightedPropertyNames.clear(); dataSourceTier = product.dataTier; @@ -259,13 +278,12 @@ private boolean loadAspectProperties(CloudRequestEngine engine) { resolvePropertyType(item), new ArrayList(), true); - aspectProperties.add(property); + properties.add(property); } + aspectProperties = properties; return true; } else { - logger.error("Aspect properties could not be loaded for" + - " the IP Intelligence cloud engine", this); return false; } } diff --git a/location/geo-location.cloud/src/main/java/fiftyone/geolocation/flowelements/GeoLocationCloudEngine.java b/location/geo-location.cloud/src/main/java/fiftyone/geolocation/flowelements/GeoLocationCloudEngine.java index 7fb48e1c..243abac9 100644 --- a/location/geo-location.cloud/src/main/java/fiftyone/geolocation/flowelements/GeoLocationCloudEngine.java +++ b/location/geo-location.cloud/src/main/java/fiftyone/geolocation/flowelements/GeoLocationCloudEngine.java @@ -47,15 +47,33 @@ public class GeoLocationCloudEngine extends CloudAspectEngineBase { private CloudRequestEngine cloudRequestEngine = null; - private List properties; + private volatile List properties; + private final Object metadataLock = new Object(); private String dataSourceTier; private String elementDataKey; private EvidenceKeyFilter evidenceKeyFilter = new EvidenceKeyFilterWhitelist(Collections.emptyList()); private String dataProviderPrefix; + /** + * The property metadata, from the products the Cloud Request Engine + * reports. Until they are known, after a failure at build that can + * pass, this is empty and is read again on the next call. + * @return the property metadata + */ @Override public List getProperties() { - return properties; + List loaded = properties; + if (loaded == null && cloudRequestEngine != null) { + synchronized (metadataLock) { + if (properties == null) { + loadAspectProperties(cloudRequestEngine); + } + loaded = properties; + } + } + return loaded == null ? + new ArrayList() : + loaded; } @Override @@ -167,9 +185,10 @@ public void addPipeline(Pipeline pipeline) { if (cloudRequestEngine == null) { cloudRequestEngine = pipeline.getElement(CloudRequestEngine.class); if (cloudRequestEngine != null) { - if (loadAspectProperties(cloudRequestEngine) == false) { - logger.error("Failed to load aspect properties"); - } + // The products are not known after a failure at build that + // can pass, which the Cloud Request Engine has logged. + // getProperties reads them once they are. + loadAspectProperties(cloudRequestEngine); } } @@ -181,7 +200,7 @@ private boolean loadAspectProperties(CloudRequestEngine engine) { findProduct(engine.getPublicProperties()); if (product != null) { - properties = new ArrayList<>(); + List loaded = new ArrayList<>(); dataSourceTier = product.dataTier; for (AccessiblePropertyMetaData.PropertyMetaData item : product.properties) @@ -197,13 +216,12 @@ private boolean loadAspectProperties(CloudRequestEngine engine) { item.delayExecution, item.evidenceProperties); - properties.add(property); + loaded.add(property); } + properties = loaded; return true; } else { - logger.error("Aspect properties could not be loaded for" + - " the Location cloud engine", this); return false; } } diff --git a/pipeline/pipeline.cloudrequestengine/src/main/java/fiftyone/pipeline/cloudrequestengine/Constants.java b/pipeline/pipeline.cloudrequestengine/src/main/java/fiftyone/pipeline/cloudrequestengine/Constants.java index 613dcff5..4ac6fbbb 100644 --- a/pipeline/pipeline.cloudrequestengine/src/main/java/fiftyone/pipeline/cloudrequestengine/Constants.java +++ b/pipeline/pipeline.cloudrequestengine/src/main/java/fiftyone/pipeline/cloudrequestengine/Constants.java @@ -48,6 +48,13 @@ public class Messages { + "see our explainer: " + "https://51degrees.com/documentation/_services__cloud__resource_keys.html?utm_source=code&utm_medium=comment&utm_campaign=51degrees-java&utm_content=pipeline.cloudrequestengine-src-main-java-fiftyone-pipeline-cloudrequestengine-constants.java&utm_term=exceptionfailedtoloadproperties"; + public static final String MessageDiscoveryDeferred = + "The cloud request engine could not fetch its accessible " + + "properties or accepted evidence keys when it was built, " + + "because of a failure that can pass. It will fetch them " + + "again when it is first used, and sends no data request " + + "until it has both."; + public static final String MessageProductNotInResourceKey = "The resource key does not include the '%s' product, so " + "%s has no properties and its results hold no values. " diff --git a/pipeline/pipeline.cloudrequestengine/src/main/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudAspectEngineBase.java b/pipeline/pipeline.cloudrequestengine/src/main/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudAspectEngineBase.java index a828a948..ee605233 100644 --- a/pipeline/pipeline.cloudrequestengine/src/main/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudAspectEngineBase.java +++ b/pipeline/pipeline.cloudrequestengine/src/main/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudAspectEngineBase.java @@ -22,7 +22,6 @@ package fiftyone.pipeline.cloudrequestengine.flowelements; -import static fiftyone.pipeline.cloudrequestengine.Constants.Messages.ExceptionFailedToLoadProperties; import static fiftyone.pipeline.cloudrequestengine.Constants.Messages.MessageProductNotInResourceKey; import static fiftyone.pipeline.cloudrequestengine.Constants.Messages.ProcessCloudEngineNotImplemented; @@ -146,6 +145,13 @@ protected void setRequestEngine(RequestEngineAccessor requestEngine) { this.requestEngine = requestEngine; } + /** + * The property metadata of this engine, from the products the Cloud + * Request Engine reports. After a failure at build that can pass the + * products are not known yet, and an empty list is returned without + * being kept, so the metadata is read again on the next call. + * @return the property metadata + */ @Override public List getProperties() { List localRef = aspectProperties; @@ -154,17 +160,13 @@ public List getProperties() { localRef = aspectProperties; if (localRef == null) { if(loadAspectProperties() == false) { - throw new RuntimeException( - String.format( - ExceptionFailedToLoadProperties, - this.getElementDataKey(), - this.getElementDataKey()) - ); + return new ArrayList<>(); } + localRef = aspectProperties; } } } - return aspectProperties; + return localRef; } @Override @@ -221,8 +223,8 @@ private boolean loadAspectProperties() { aspectProperties = properties; return true; } else { - logger.error("Aspect properties could not be loaded for " + - this.getClass().getName(), this); + // The Cloud Request Engine has logged why the products are not + // known yet. return false; } } diff --git a/pipeline/pipeline.cloudrequestengine/src/main/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudEvidenceKeyFilter.java b/pipeline/pipeline.cloudrequestengine/src/main/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudEvidenceKeyFilter.java new file mode 100644 index 00000000..bfcac399 --- /dev/null +++ b/pipeline/pipeline.cloudrequestengine/src/main/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudEvidenceKeyFilter.java @@ -0,0 +1,94 @@ +/* ********************************************************************* + * This Original Work is copyright of 51 Degrees Mobile Experts Limited. + * Copyright 2026 51 Degrees Mobile Experts Limited, Davidson House, + * Forbury Square, Reading, Berkshire, United Kingdom RG1 3EU. + * + * This Original Work is licensed under the European Union Public Licence + * (EUPL) v.1.2 and is subject to its terms as set out below. + * + * If a copy of the EUPL was not distributed with this file, You can obtain + * one at https://opensource.org/licenses/EUPL-1.2. + * + * The 'Compatible Licences' set out in the Appendix to the EUPL (as may be + * amended by the European Commission) shall be deemed incompatible for + * the purposes of the Work and the provisions of the compatibility + * clause in Article 5 of the EUPL shall not apply. + * + * If using the Work as, or as part of, a network application, by + * including the attribution notice(s) required under Article 5 of the EUPL + * in the end user terms of the application under an appropriate heading, + * such notice(s) shall fulfill the requirements of that article. + * ********************************************************************* */ + +package fiftyone.pipeline.cloudrequestengine.flowelements; + +import fiftyone.pipeline.core.data.EvidenceKeyFilterWhitelist; + +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; +import java.util.Map; + +/** + * The Evidence keys the cloud service accepts, from its evidencekeys + * request. It includes nothing until the keys are known, and reads them on + * each call, so a filter that holds this one sees the keys as soon as they + * arrive. It derives from {@link EvidenceKeyFilterWhitelist} so that the + * Pipeline's filter keeps it rather than copying its keys once, and so + * that the web integration lists its headers in the Vary header. + */ +class CloudEvidenceKeyFilter extends EvidenceKeyFilterWhitelist { + + private volatile EvidenceKeyFilterWhitelist keys = null; + + CloudEvidenceKeyFilter() { + super(new ArrayList(), String.CASE_INSENSITIVE_ORDER); + } + + /** + * @return true once the keys are known + */ + boolean isKnown() { + return keys != null; + } + + /** + * Set the keys the cloud service accepts. + * @param evidenceKeys the keys + */ + void setKeys(List evidenceKeys) { + keys = new EvidenceKeyFilterWhitelist( + evidenceKeys, + String.CASE_INSENSITIVE_ORDER); + } + + /** + * @return the keys, or null if they are not known + */ + List getKeys() { + EvidenceKeyFilterWhitelist current = keys; + return current == null ? + null : + new ArrayList<>(current.getWhitelist().keySet()); + } + + @Override + public boolean include(String key) { + EvidenceKeyFilterWhitelist current = keys; + return current != null && current.include(key); + } + + @Override + public Integer order(String key) { + EvidenceKeyFilterWhitelist current = keys; + return current == null ? null : current.order(key); + } + + @Override + public Map getWhitelist() { + EvidenceKeyFilterWhitelist current = keys; + return current == null ? + Collections.emptyMap() : + current.getWhitelist(); + } +} diff --git a/pipeline/pipeline.cloudrequestengine/src/main/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudRequestEngine.java b/pipeline/pipeline.cloudrequestengine/src/main/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudRequestEngine.java index 1025b579..d3027f2d 100644 --- a/pipeline/pipeline.cloudrequestengine/src/main/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudRequestEngine.java +++ b/pipeline/pipeline.cloudrequestengine/src/main/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudRequestEngine.java @@ -41,7 +41,8 @@ public interface CloudRequestEngine /** * A collection of the properties that the cloud service can populate in the * JSON response. Keyed on property name. - * @return public properties + * @return public properties, or null if they are not known yet, after a + * failure when the engine was built that can pass */ Map getPublicProperties(); } diff --git a/pipeline/pipeline.cloudrequestengine/src/main/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudRequestEngineBuilder.java b/pipeline/pipeline.cloudrequestengine/src/main/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudRequestEngineBuilder.java index bc72731b..f2da2735 100644 --- a/pipeline/pipeline.cloudrequestengine/src/main/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudRequestEngineBuilder.java +++ b/pipeline/pipeline.cloudrequestengine/src/main/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudRequestEngineBuilder.java @@ -98,7 +98,7 @@ protected CloudRequestEngine newEngine(List properties) throws Exception } } - return new CloudRequestEngineDefault( + CloudRequestEngineDefault engine = new CloudRequestEngineDefault( loggerFactory.getLogger(CloudRequestEngine.class.getName()), new CloudRequestDataFactory(loggerFactory), httpClient, @@ -109,6 +109,11 @@ protected CloudRequestEngine newEngine(List properties) throws Exception evidenceKeysEndpoint, timeoutMillis, cloudRequestOrigin); + // Fetch the accessible properties and the accepted evidence keys + // now, so that the built engine is ready. A 4xx answer fails the + // build, and any other failure is retried on first use. + engine.discover(); + return engine; } private String getEnvironmentVariable(String name) { diff --git a/pipeline/pipeline.cloudrequestengine/src/main/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudRequestEngineDefault.java b/pipeline/pipeline.cloudrequestengine/src/main/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudRequestEngineDefault.java index 12ccc17f..65181de0 100644 --- a/pipeline/pipeline.cloudrequestengine/src/main/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudRequestEngineDefault.java +++ b/pipeline/pipeline.cloudrequestengine/src/main/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudRequestEngineDefault.java @@ -82,12 +82,38 @@ public class CloudRequestEngineDefault private String cloudRequestOrigin; private Integer timeoutMillis; + /** + * The lowest HTTP status of a client error. A discovery request answered + * with a status from this up to {@link #HTTP_SERVER_ERROR_MIN} is a + * definitive configuration error. + */ + private static final int HTTP_CLIENT_ERROR_MIN = 400; + + /** + * The lowest HTTP status of a server error. + */ + private static final int HTTP_SERVER_ERROR_MIN = 500; + private List propertyMetaData; + + /** + * The products the resource key gives access to, from the + * accessibleproperties request, or null until that request succeeds. + */ private volatile Map publicProperties; - private final Object publicPropertiesLock = new Object(); - private volatile EvidenceKeyFilter evidenceKeyFilter; - private final Object evidenceKeyFilterLock = new Object(); + /** + * The accepted Evidence, from the evidencekeys request. It is the same + * instance before and after the request succeeds. + */ + private final CloudEvidenceKeyFilter evidenceKeyFilter = + new CloudEvidenceKeyFilter(); + + /** + * Held while the discovery requests are made, so that concurrent first + * uses make them once. + */ + private final Object discoveryLock = new Object(); public CloudRequestEngineDefault( Logger logger, @@ -193,40 +219,141 @@ public String getElementDataKey() { return "cloud-response"; } + /** + * The accepted Evidence keys from the evidencekeys request. Until that + * request succeeds the filter includes nothing, and it makes no request + * itself. A filter that holds this one, such as the Pipeline's, sees + * the keys as soon as a retry fetches them. + * @return the accepted Evidence + */ @Override - public EvidenceKeyFilter getEvidenceKeyFilter() throws CloudRequestException, AggregateException, PropertyNotLoadedException { - if (evidenceKeyFilter == null) { - synchronized (evidenceKeyFilterLock) { - if (evidenceKeyFilter == null) { - getCloudEvidenceKeys(); - } - } - } + public EvidenceKeyFilter getEvidenceKeyFilter() { return evidenceKeyFilter; } + /** + * The products the resource key gives access to, from the + * accessibleproperties request, which the builder makes when it builds + * the engine. After a failure at build that can pass this is null until + * processing fetches them. It makes no request itself. + * @return the products, or null if they are not known yet + */ @Override public Map - getPublicProperties() throws CloudRequestException, AggregateException, PropertyNotLoadedException { - if (publicProperties == null) { - synchronized (publicPropertiesLock) { + getPublicProperties() { + return publicProperties; + } + + /** + * Fetch the accepted Evidence keys and the accessible properties that + * are not known yet. The builder calls this when it builds the engine, + * so in the normal case a built engine is ready to process. Both + * requests are made. A 4xx answer to either is a definitive + * configuration error, and its {@link CloudRequestException} is thrown, + * because retrying with the same configuration can never succeed. Any + * other failure, such as the service being unreachable, a timeout, a + * 5xx answer or a body that cannot be read, is logged as one warning + * and the list that did arrive is kept. Processing fetches what is + * still missing. + * @throws CloudRequestException for a 4xx answer to either request + */ + public void discover() { + List failures = new ArrayList<>(); + synchronized (discoveryLock) { + if (evidenceKeyFilter.isKnown() == false) { + try { + fetchEvidenceKeys(); + } catch (RuntimeException e) { + failures.add(e); + } + } + if (publicProperties == null) { + try { + fetchPublicProperties(); + } catch (RuntimeException e) { + failures.add(e); + } + } + } + if (failures.isEmpty()) { + return; + } + for (RuntimeException failure : failures) { + CloudRequestException definitive = definitiveError(failure); + if (definitive != null) { + throw definitive; + } + } + RuntimeException first = failures.get(0); + for (int i = 1; i < failures.size(); i++) { + first.addSuppressed(failures.get(i)); + } + logger.warn(MessageDiscoveryDeferred, first); + } + + /** + * The failure of a discovery request that the cloud service received + * and rejected with a 4xx status, so that retrying with the same + * configuration can never succeed. + * @param failure the failure of a discovery request + * @return the exception with the 4xx status, or null if there is none + */ + private static CloudRequestException definitiveError(Throwable failure) { + if (failure instanceof CloudRequestException) { + int status = ((CloudRequestException) failure).getHttpStatusCode(); + return status >= HTTP_CLIENT_ERROR_MIN && + status < HTTP_SERVER_ERROR_MIN ? + (CloudRequestException) failure : + null; + } + if (failure instanceof AggregateException) { + for (Throwable inner : failure.getSuppressed()) { + CloudRequestException definitive = definitiveError(inner); + if (definitive != null) { + return definitive; + } + } + } + return null; + } + + /** + * Fetch what discovery could not when the engine was built, evidence + * keys first, stopping at the first failure, which is a process-time + * failure. No data request is sent until both are known. + * @throws IOException if a discovery request could not be made + */ + private void completeDiscovery() throws IOException { + if (evidenceKeyFilter.isKnown() && publicProperties != null) { + return; + } + synchronized (discoveryLock) { + try { + if (evidenceKeyFilter.isKnown() == false) { + fetchEvidenceKeys(); + } if (publicProperties == null) { - getCloudProperties(); + fetchPublicProperties(); + } + } catch (PropertyNotLoadedException e) { + // A request that could not be made is reported as the + // IOException it was, as for the data request. + if (e.getCause() instanceof IOException) { + throw (IOException) e.getCause(); } + throw e; } } - return publicProperties; } @Override protected void processEngine(FlowData data, CloudRequestData aspectData) throws IOException { + ((CloudRequestDataInternal)aspectData).setProcessStarted(true); + completeDiscovery(); + byte[] content = getContent(data); HttpURLConnection connection = httpClient.connect(toUrl(endPoint.trim())); - if (timeoutMillis != null) { - connection.setConnectTimeout(timeoutMillis); - connection.setReadTimeout(timeoutMillis); - } - ((CloudRequestDataInternal)aspectData).setProcessStarted(true); + setTimeouts(connection); Map headers = new HashMap<>(); setCommonHeaders(headers); @@ -425,7 +552,17 @@ private static URL toUrl(String spec) throws MalformedURLException { } } - private void getCloudProperties() throws CloudRequestException, AggregateException, PropertyNotLoadedException { + /** + * Apply the configured timeout to a request. + */ + private void setTimeouts(HttpURLConnection connection) { + if (timeoutMillis != null) { + connection.setConnectTimeout(timeoutMillis); + connection.setReadTimeout(timeoutMillis); + } + } + + private void fetchPublicProperties() throws CloudRequestException, AggregateException, PropertyNotLoadedException { try { String jsonResult; @@ -433,6 +570,7 @@ private void getCloudProperties() throws CloudRequestException, AggregateExcepti setCommonHeaders(headers); HttpURLConnection connection = httpClient.connect(toUrl(propertiesEndpoint.trim() + (resourceKey != null ? "?Resource=" + resourceKey : ""))); + setTimeouts(connection); jsonResult = httpClient.getResponseString(connection, headers); validateResponse(jsonResult, connection); @@ -455,7 +593,7 @@ private void getCloudProperties() throws CloudRequestException, AggregateExcepti } } - private void getCloudEvidenceKeys() throws CloudRequestException, AggregateException, PropertyNotLoadedException { + private void fetchEvidenceKeys() throws CloudRequestException, AggregateException, PropertyNotLoadedException { try { String jsonResult; @@ -463,6 +601,7 @@ private void getCloudEvidenceKeys() throws CloudRequestException, AggregateExcep setCommonHeaders(headers); HttpURLConnection connection = httpClient.connect(toUrl(evidenceKeysEndpoint.trim())); + setTimeouts(connection); jsonResult = httpClient.getResponseString(connection, headers); validateResponse(jsonResult, connection, false); @@ -472,8 +611,7 @@ private void getCloudEvidenceKeys() throws CloudRequestException, AggregateExcep for (int i = 0; i < jsonArray.length(); i++) { keys.add(jsonArray.get(i).toString()); } - evidenceKeyFilter = new EvidenceKeyFilterWhitelist(keys, - String.CASE_INSENSITIVE_ORDER); + evidenceKeyFilter.setKeys(keys); } } catch (IOException e) { throw new PropertyNotLoadedException(e); diff --git a/pipeline/pipeline.web.packages/pipeline.web/src/main/java/fiftyone/pipeline/web/services/ClientsidePropertyServiceCore.java b/pipeline/pipeline.web.packages/pipeline.web/src/main/java/fiftyone/pipeline/web/services/ClientsidePropertyServiceCore.java index 67439d7c..630ac847 100644 --- a/pipeline/pipeline.web.packages/pipeline.web/src/main/java/fiftyone/pipeline/web/services/ClientsidePropertyServiceCore.java +++ b/pipeline/pipeline.web.packages/pipeline.web/src/main/java/fiftyone/pipeline/web/services/ClientsidePropertyServiceCore.java @@ -150,11 +150,11 @@ protected final void init() { } for (EvidenceKeyFilter filter : filters) { - // If the filter is a white list or derived type then - // get all HTTP header evidence keys from white list - // and add them to the headers that could affect the - // generated JavaScript. - if (filter.getClass().equals(EvidenceKeyFilterWhitelist.class)) { + // If the filter is a white list or derived type, such as + // the filter of a parallel group, then get all HTTP header + // evidence keys from white list and add them to the + // headers that could affect the generated JavaScript. + if (filter instanceof EvidenceKeyFilterWhitelist) { EvidenceKeyFilterWhitelist whitelist = (EvidenceKeyFilterWhitelist) filter; for (String key : whitelist.getWhitelist().keySet()) { if (key.startsWith(EVIDENCE_HTTPHEADER_PREFIX + EVIDENCE_SEPERATOR) && From 4defa2ab87dd6bde5808e9c2a6f0e2898dc69ac6 Mon Sep 17 00:00:00 2001 From: James Rosewell Date: Mon, 9 Nov 2026 21:07:42 +0000 Subject: [PATCH 03/16] Let a Cloud Request Engine be built from a kept copy of what an earlier one discovered The specification's Resilience section says an engine SHOULD be buildable from a kept copy of the discovery results, the accepted evidence keys and the accessible properties, which depend only on the resource key, so that a Pipeline can be built offline and a host that starts often need not call the cloud service each time. CloudDiscoveryState holds the evidence keys and the accessibleproperties answer, and writes and reads them as one JSON document. CloudRequestEngineBuilder.getDiscoveryState returns the copy after a build that knows both lists, and setDiscoveryState makes a later build use it with no discovery request. The tests are in this commit because they use the new class and methods. --- .../CloudDiscoveryState.java | 115 ++++++++++++++++++ .../CloudRequestEngineBuilder.java | 43 ++++++- .../CloudRequestEngineDefault.java | 56 +++++++-- .../CloudRequestEngineDiscoveryTests.java | 52 ++++++++ 4 files changed, 253 insertions(+), 13 deletions(-) create mode 100644 pipeline/pipeline.cloudrequestengine/src/main/java/fiftyone/pipeline/cloudrequestengine/CloudDiscoveryState.java diff --git a/pipeline/pipeline.cloudrequestengine/src/main/java/fiftyone/pipeline/cloudrequestengine/CloudDiscoveryState.java b/pipeline/pipeline.cloudrequestengine/src/main/java/fiftyone/pipeline/cloudrequestengine/CloudDiscoveryState.java new file mode 100644 index 00000000..0731e46d --- /dev/null +++ b/pipeline/pipeline.cloudrequestengine/src/main/java/fiftyone/pipeline/cloudrequestengine/CloudDiscoveryState.java @@ -0,0 +1,115 @@ +/* ********************************************************************* + * This Original Work is copyright of 51 Degrees Mobile Experts Limited. + * Copyright 2026 51 Degrees Mobile Experts Limited, Davidson House, + * Forbury Square, Reading, Berkshire, United Kingdom RG1 3EU. + * + * This Original Work is licensed under the European Union Public Licence + * (EUPL) v.1.2 and is subject to its terms as set out below. + * + * If a copy of the EUPL was not distributed with this file, You can obtain + * one at https://opensource.org/licenses/EUPL-1.2. + * + * The 'Compatible Licences' set out in the Appendix to the EUPL (as may be + * amended by the European Commission) shall be deemed incompatible for + * the purposes of the Work and the provisions of the compatibility + * clause in Article 5 of the EUPL shall not apply. + * + * If using the Work as, or as part of, a network application, by + * including the attribution notice(s) required under Article 5 of the EUPL + * in the end user terms of the application under an appropriate heading, + * such notice(s) shall fulfill the requirements of that article. + * ********************************************************************* */ + +package fiftyone.pipeline.cloudrequestengine; + +import org.json.JSONArray; +import org.json.JSONObject; + +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; +import java.util.Objects; + +/** + * A copy of what the Cloud Request Engine discovered for a resource key, + * being the accepted Evidence keys and the accessible properties, which + * depend only on the resource key. Read it from the builder after a + * successful build, keep it, for example as {@link #toJson()}, and give it + * to a later builder, which then makes no discovery request. A Pipeline can + * so be built offline, and a host that starts often need not call the + * cloud service each time. + * @see Specification + */ +public final class CloudDiscoveryState { + + private static final String EVIDENCE_KEYS = "evidenceKeys"; + + private static final String ACCESSIBLE_PROPERTIES = "accessibleProperties"; + + private final List evidenceKeys; + + private final String accessibleProperties; + + /** + * Construct a new instance. + * @param evidenceKeys the evidence keys the cloud service accepts + * @param accessibleProperties the answer to the accessibleproperties + * request, as JSON + * @throws org.json.JSONException if the accessible properties are not + * a JSON object with a Products member + */ + public CloudDiscoveryState( + List evidenceKeys, + String accessibleProperties) { + Objects.requireNonNull(evidenceKeys, "evidenceKeys"); + Objects.requireNonNull(accessibleProperties, "accessibleProperties"); + new JSONObject(accessibleProperties).getJSONObject("Products"); + this.evidenceKeys = Collections.unmodifiableList( + new ArrayList<>(evidenceKeys)); + this.accessibleProperties = accessibleProperties; + } + + /** + * @return the evidence keys the cloud service accepts + */ + public List getEvidenceKeys() { + return evidenceKeys; + } + + /** + * @return the answer to the accessibleproperties request, as JSON + */ + public String getAccessibleProperties() { + return accessibleProperties; + } + + /** + * @return this copy as one JSON document, which {@link #fromJson} + * reads back + */ + public String toJson() { + JSONObject json = new JSONObject(); + json.put(EVIDENCE_KEYS, new JSONArray(evidenceKeys)); + json.put(ACCESSIBLE_PROPERTIES, new JSONObject(accessibleProperties)); + return json.toString(); + } + + /** + * Read a copy written by {@link #toJson()}. + * @param json the JSON document + * @return the copy + * @throws org.json.JSONException if the document is not one + * {@link #toJson()} writes + */ + public static CloudDiscoveryState fromJson(String json) { + JSONObject document = new JSONObject(json); + JSONArray keys = document.getJSONArray(EVIDENCE_KEYS); + List evidenceKeys = new ArrayList<>(); + for (int i = 0; i < keys.length(); i++) { + evidenceKeys.add(keys.getString(i)); + } + return new CloudDiscoveryState( + evidenceKeys, + document.getJSONObject(ACCESSIBLE_PROPERTIES).toString()); + } +} diff --git a/pipeline/pipeline.cloudrequestengine/src/main/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudRequestEngineBuilder.java b/pipeline/pipeline.cloudrequestengine/src/main/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudRequestEngineBuilder.java index f2da2735..5e23b4ea 100644 --- a/pipeline/pipeline.cloudrequestengine/src/main/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudRequestEngineBuilder.java +++ b/pipeline/pipeline.cloudrequestengine/src/main/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudRequestEngineBuilder.java @@ -25,6 +25,7 @@ import fiftyone.pipeline.annotations.DefaultValue; import fiftyone.pipeline.annotations.ElementBuilder; +import fiftyone.pipeline.cloudrequestengine.CloudDiscoveryState; import fiftyone.pipeline.cloudrequestengine.data.CloudRequestData; import fiftyone.pipeline.core.data.FlowData; import fiftyone.pipeline.core.data.factories.ElementDataFactory; @@ -56,6 +57,8 @@ public class CloudRequestEngineBuilder extends private String licenseKey = null; private String cloudRequestOrigin = null; private int timeoutMillis = Constants.DEFAULT_TIMEOUT_MILLIS; + private CloudDiscoveryState discoveryState = null; + private CloudRequestEngineDefault built = null; // Function to get environment variable value. This enable testable code. private Function getEnvVar = (name) -> { @@ -109,12 +112,44 @@ protected CloudRequestEngine newEngine(List properties) throws Exception evidenceKeysEndpoint, timeoutMillis, cloudRequestOrigin); - // Fetch the accessible properties and the accepted evidence keys - // now, so that the built engine is ready. A 4xx answer fails the - // build, and any other failure is retried on first use. - engine.discover(); + if (discoveryState != null) { + engine.useDiscoveryState(discoveryState); + } + else { + // Fetch the accessible properties and the accepted evidence + // keys now, so that the built engine is ready. A 4xx answer + // fails the build, and any other failure is retried on first + // use. + engine.discover(); + } + built = engine; return engine; } + + /** + * Build the engine from a copy of what an earlier engine discovered for + * the same resource key, so that the build makes no discovery request. + * The copy comes from {@link #getDiscoveryState()} after a successful + * build, and can be kept with {@link CloudDiscoveryState#toJson()}. + * @param state the copy, or null to discover when building + * @return this builder + */ + public CloudRequestEngineBuilder setDiscoveryState( + CloudDiscoveryState state) { + this.discoveryState = state; + return this; + } + + /** + * A copy of what the engine this builder last built discovered, to give + * to a later build with {@link #setDiscoveryState(CloudDiscoveryState)}. + * @return the copy, or null if no engine has been built or it does not + * yet know both the accepted evidence keys and the accessible properties + */ + public CloudDiscoveryState getDiscoveryState() { + CloudRequestEngineDefault engine = built; + return engine == null ? null : engine.getDiscoveryState(); + } private String getEnvironmentVariable(String name) { return System.getenv(name); diff --git a/pipeline/pipeline.cloudrequestengine/src/main/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudRequestEngineDefault.java b/pipeline/pipeline.cloudrequestengine/src/main/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudRequestEngineDefault.java index 65181de0..3f1be59b 100644 --- a/pipeline/pipeline.cloudrequestengine/src/main/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudRequestEngineDefault.java +++ b/pipeline/pipeline.cloudrequestengine/src/main/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudRequestEngineDefault.java @@ -22,6 +22,7 @@ package fiftyone.pipeline.cloudrequestengine.flowelements; +import fiftyone.pipeline.cloudrequestengine.CloudDiscoveryState; import fiftyone.pipeline.cloudrequestengine.CloudRequestException; import fiftyone.pipeline.cloudrequestengine.data.CloudRequestData; import fiftyone.pipeline.core.Constants; @@ -102,6 +103,12 @@ public class CloudRequestEngineDefault */ private volatile Map publicProperties; + /** + * The answer to the accessibleproperties request that gave + * {@link #publicProperties}, kept for {@link #getDiscoveryState()}. + */ + private volatile String accessibleProperties; + /** * The accepted Evidence, from the evidencekeys request. It is the same * instance before and after the request succeeds. @@ -291,6 +298,33 @@ public void discover() { logger.warn(MessageDiscoveryDeferred, first); } + /** + * A copy of what discovery fetched, which a later builder can use in + * place of the discovery requests. + * @return the copy, or null unless both the accepted evidence keys and + * the accessible properties are known + */ + public CloudDiscoveryState getDiscoveryState() { + synchronized (discoveryLock) { + List keys = evidenceKeyFilter.getKeys(); + return keys == null || accessibleProperties == null ? + null : + new CloudDiscoveryState(keys, accessibleProperties); + } + } + + /** + * Use a copy of what an earlier discovery fetched, in place of the + * discovery requests. + * @param state the copy + */ + public void useDiscoveryState(CloudDiscoveryState state) { + synchronized (discoveryLock) { + evidenceKeyFilter.setKeys(state.getEvidenceKeys()); + setPublicProperties(state.getAccessibleProperties()); + } + } + /** * The failure of a discovery request that the cloud service received * and rejected with a 4xx status, so that retrying with the same @@ -574,16 +608,8 @@ private void fetchPublicProperties() throws CloudRequestException, AggregateExce jsonResult = httpClient.getResponseString(connection, headers); validateResponse(jsonResult, connection); - JSONObject jsonObj = null; if (jsonResult.isEmpty() == false) { - jsonObj = new JSONObject(jsonResult); - } - - if (jsonObj != null) { - AccessiblePropertyMetaData.LicencedProducts accessiblePropertyData = - new AccessiblePropertyMetaData.LicencedProducts(jsonObj.getJSONObject("Products")); - - publicProperties = accessiblePropertyData.products; + setPublicProperties(jsonResult); } else { throw new RuntimeException("Failed to retrieve available properties " + "from cloud service at " + propertiesEndpoint + "."); @@ -593,6 +619,18 @@ private void fetchPublicProperties() throws CloudRequestException, AggregateExce } } + /** + * Read the answer to the accessibleproperties request. + * @param json the answer + */ + private void setPublicProperties(String json) { + AccessiblePropertyMetaData.LicencedProducts accessiblePropertyData = + new AccessiblePropertyMetaData.LicencedProducts( + new JSONObject(json).getJSONObject("Products")); + accessibleProperties = json; + publicProperties = accessiblePropertyData.products; + } + private void fetchEvidenceKeys() throws CloudRequestException, AggregateException, PropertyNotLoadedException { try { String jsonResult; diff --git a/pipeline/pipeline.cloudrequestengine/src/test/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudRequestEngineDiscoveryTests.java b/pipeline/pipeline.cloudrequestengine/src/test/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudRequestEngineDiscoveryTests.java index 1ae9bb3e..742adaff 100644 --- a/pipeline/pipeline.cloudrequestengine/src/test/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudRequestEngineDiscoveryTests.java +++ b/pipeline/pipeline.cloudrequestengine/src/test/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudRequestEngineDiscoveryTests.java @@ -24,6 +24,7 @@ import fiftyone.common.testhelpers.TestLogger; import fiftyone.common.testhelpers.TestLoggerFactory; +import fiftyone.pipeline.cloudrequestengine.CloudDiscoveryState; import fiftyone.pipeline.cloudrequestengine.CloudRequestException; import fiftyone.pipeline.core.data.FlowData; import fiftyone.pipeline.core.flowelements.Pipeline; @@ -336,6 +337,57 @@ public void Build_AppliesTheTimeoutToDiscovery() throws Exception { } } + /** + * A copy of what discovery fetched is read from the builder after a + * build, kept as JSON, and given to a later build, which makes no + * discovery request and is ready to process. + */ + @Test + public void DiscoveryState_KeptAndUsedWithNoRequest() throws Exception { + CloudRequestEngineBuilder first = builder(); + first.build(); + CloudDiscoveryState state = first.getDiscoveryState(); + assertNotNull(state); + String kept = state.toJson(); + + answers.put(PROPERTIES, UNREACHABLE); + answers.put(EVIDENCE_KEYS, UNREACHABLE); + requests.clear(); + CloudRequestEngine engine = builder() + .setDiscoveryState(CloudDiscoveryState.fromJson(kept)) + .build(); + + assertEquals(0, requests(PROPERTIES), "accessibleproperties"); + assertEquals(0, requests(EVIDENCE_KEYS), "evidencekeys"); + assertTrue(engine.getPublicProperties().containsKey("device")); + assertTrue(engine.getEvidenceKeyFilter().include("header.user-agent")); + assertEquals(0, warnings().size(), warnings().toString()); + + process(engine, false); + + assertEquals(0, requests(PROPERTIES), "accessibleproperties"); + assertEquals(0, requests(EVIDENCE_KEYS), "evidencekeys"); + assertEquals(1, requests(DATA), "json"); + } + + /** + * A build that does not know both lists gives no copy. + */ + @Test + public void DiscoveryState_NoneWithoutBothLists() throws Exception { + answers.put(EVIDENCE_KEYS, 503); + CloudRequestEngineBuilder builder = builder(); + builder.build(); + + assertNull(builder.getDiscoveryState()); + } + + private CloudRequestEngineBuilder builder() { + return new CloudRequestEngineBuilder(loggerFactory, httpClient) + .setResourceKey("resource_key") + .setTimeOutSeconds(TIMEOUT_SECONDS); + } + private CloudRequestEngine build() throws Exception { return new CloudRequestEngineBuilder(loggerFactory, httpClient) .setResourceKey("resource_key") From 4f62134b6d12ed6044fb630abe7aecbde07d5132 Mon Sep 17 00:00:00 2001 From: James Rosewell Date: Mon, 9 Nov 2026 21:21:48 +0000 Subject: [PATCH 04/16] Test that discovery makes both requests at once, times out after 2 seconds, and that the Pipeline reads metadata again once it is known Each discovery request in the new test waits for the other to start, so they only succeed if they are made at once. A build with no timeout set applies 2 seconds to both, as the page's Configuration options table and .NET give. The Pipeline's map of available properties, built before the Cloud Request Engine knows the accessible properties, holds the cloud aspect engine's properties once they are known. The shared test client now gives each request a connection of its own, as the requests can be made at the same time. --- .../CloudAspectEngineBaseTests.java | 27 ++++++++ .../CloudRequestEngineDiscoveryTests.java | 67 ++++++++++++++++++- .../CloudRequestEngineTestsBase.java | 30 +++++---- 3 files changed, 108 insertions(+), 16 deletions(-) diff --git a/pipeline/pipeline.cloudrequestengine/src/test/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudAspectEngineBaseTests.java b/pipeline/pipeline.cloudrequestengine/src/test/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudAspectEngineBaseTests.java index 3ec05bb4..06f0b850 100644 --- a/pipeline/pipeline.cloudrequestengine/src/test/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudAspectEngineBaseTests.java +++ b/pipeline/pipeline.cloudrequestengine/src/test/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudAspectEngineBaseTests.java @@ -344,6 +344,33 @@ public void loadProperties_notLoaded() throws Exception { assertTrue(propertiesContainName(engine.getProperties(), "ismobile")); } + /** + * The Pipeline's map of available properties, built before the Cloud + * Request Engine knows the accessible properties, holds the engine's + * properties once they are known rather than keeping what it first saw. + * @throws Exception + */ + @Test + public void elementAvailableProperties_ReadAgainOnceKnown() throws Exception { + propertiesReturnedByRequestEngine = null; + createPipeline(); + + assertTrue( + pipeline.getElementAvailableProperties().get("test") == null || + pipeline.getElementAvailableProperties().get("test").isEmpty()); + + List properties = new ArrayList<>(); + properties.add(new AccessiblePropertyMetaData.PropertyMetaData("ismobile", "Boolean", null, null)); + AccessiblePropertyMetaData.ProductMetaData product = new AccessiblePropertyMetaData.ProductMetaData(); + product.properties = properties; + Map products = new HashMap<>(); + products.put("test", product); + requestEngine.setPublicProperties(products); + + assertTrue(pipeline.getElementAvailableProperties().get("test") + .containsKey("ismobile")); + } + /** * A property keyed engine, such as the hardware profile engine, whose * product the resource key does not include builds, gives no profiles diff --git a/pipeline/pipeline.cloudrequestengine/src/test/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudRequestEngineDiscoveryTests.java b/pipeline/pipeline.cloudrequestengine/src/test/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudRequestEngineDiscoveryTests.java index 742adaff..a5bbc81f 100644 --- a/pipeline/pipeline.cloudrequestengine/src/test/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudRequestEngineDiscoveryTests.java +++ b/pipeline/pipeline.cloudrequestengine/src/test/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudRequestEngineDiscoveryTests.java @@ -44,9 +44,13 @@ import java.net.SocketTimeoutException; import java.net.URL; import java.util.ArrayList; +import java.util.Collections; import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; import java.util.stream.Stream; import static org.junit.jupiter.api.Assertions.*; @@ -94,11 +98,28 @@ public class CloudRequestEngineDiscoveryTests { private static final int TIMEOUT_SECONDS = 3; + /** + * How long a discovery request waits for the other one to start. + */ + private static final int WAIT_SECONDS = 5; + + /** + * The timeout the page's Configuration options table and .NET give. + */ + private static final int DEFAULT_TIMEOUT_SECONDS = 2; + private TestLoggerFactory loggerFactory; private HttpClient httpClient; - private final Map requests = new HashMap<>(); + private final Map requests = new ConcurrentHashMap<>(); private final Map answers = new HashMap<>(); - private final List connections = new ArrayList<>(); + private final List connections = + Collections.synchronizedList(new ArrayList()); + + /** + * When set, each discovery request waits until the other has started, + * so the two only complete if they are made at once. + */ + private volatile CountDownLatch bothDiscoveryRequests = null; @BeforeEach public void init() throws IOException { @@ -114,6 +135,14 @@ public void init() throws IOException { URL url = invocation.getArgument(0); String name = endpoint(url); requests.merge(name, 1, Integer::sum); + CountDownLatch latch = bothDiscoveryRequests; + if (latch != null && name.equals(DATA) == false) { + latch.countDown(); + if (latch.await(WAIT_SECONDS, TimeUnit.SECONDS) == false) { + throw new SocketTimeoutException( + "The other discovery request was not made"); + } + } int answer = answers.get(name); if (answer == UNREACHABLE) { throw new ConnectException("Connection refused"); @@ -388,6 +417,40 @@ private CloudRequestEngineBuilder builder() { .setTimeOutSeconds(TIMEOUT_SECONDS); } + /** + * The two discovery requests are made at once, as the page allows, so + * a service that does not answer delays the build by one timeout. Each + * request here waits for the other to start, so made one after the + * other they would both fail. + */ + @Test + public void Build_MakesBothRequestsAtOnce() throws Exception { + bothDiscoveryRequests = new CountDownLatch(2); + + CloudRequestEngine engine = build(); + + assertEquals(0, warnings().size(), warnings().toString()); + assertNotNull(engine.getPublicProperties()); + assertTrue(engine.getEvidenceKeyFilter().include("header.user-agent")); + } + + /** + * With no timeout set, the requests time out after 2 seconds. + */ + @Test + public void Build_DefaultTimeoutIsTwoSeconds() throws Exception { + new CloudRequestEngineBuilder(loggerFactory, httpClient) + .setResourceKey("resource_key") + .build(); + + assertEquals(2, connections.size()); + for (HttpURLConnection connection : connections) { + verify(connection).setConnectTimeout( + DEFAULT_TIMEOUT_SECONDS * 1000); + verify(connection).setReadTimeout(DEFAULT_TIMEOUT_SECONDS * 1000); + } + } + private CloudRequestEngine build() throws Exception { return new CloudRequestEngineBuilder(loggerFactory, httpClient) .setResourceKey("resource_key") diff --git a/pipeline/pipeline.cloudrequestengine/src/test/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudRequestEngineTestsBase.java b/pipeline/pipeline.cloudrequestengine/src/test/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudRequestEngineTestsBase.java index 0320d94e..98d85411 100644 --- a/pipeline/pipeline.cloudrequestengine/src/test/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudRequestEngineTestsBase.java +++ b/pipeline/pipeline.cloudrequestengine/src/test/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudRequestEngineTestsBase.java @@ -69,24 +69,26 @@ public Object answer(InvocationOnMock invocationOnMock) throws Throwable { invocationOnMock.getMethod().getName() + "' should not have been called."); } }); - final HttpURLConnection connection = mock(HttpURLConnection.class, new Answer() { - @Override - public Object answer(InvocationOnMock invocationOnMock) throws Throwable { - throw new Exception("The method '" + - invocationOnMock.getMethod().getName() + "' should not have been called."); - } - }); - doNothing().when(connection).setConnectTimeout(anyInt()); - doNothing().when(connection).setReadTimeout(anyInt()); - Map> responseHeaders = new HashMap>(); - doReturn(responseHeaders).when(connection).getHeaderFields(); + // Each request gets a connection of its own, because the engine + // makes its two discovery requests at once. doAnswer(new Answer() { @Override public Object answer(InvocationOnMock invocationOnMock) throws Throwable { - URL url = (URL)invocationOnMock.getArgument(0); - doReturn(url).when(connection).getURL(); + URL url = (URL)invocationOnMock.getArgument(0); + HttpURLConnection connection = mock(HttpURLConnection.class, new Answer() { + @Override + public Object answer(InvocationOnMock invocation) throws Throwable { + throw new Exception("The method '" + + invocation.getMethod().getName() + "' should not have been called."); + } + }); + doNothing().when(connection).setConnectTimeout(anyInt()); + doNothing().when(connection).setReadTimeout(anyInt()); + Map> responseHeaders = new HashMap>(); + doReturn(responseHeaders).when(connection).getHeaderFields(); + doReturn(url).when(connection).getURL(); if (url.getPath().endsWith("properties")) { - propertiesEndPoint = url.getQuery(); + propertiesEndPoint = url.getQuery(); doReturn(accessiblePropertiesResponseCode).when(connection).getResponseCode(); } else { From bef79b0b0a66ee0d80e268230ac78483b8240231 Mon Sep 17 00:00:00 2001 From: James Rosewell Date: Mon, 9 Nov 2026 21:26:00 +0000 Subject: [PATCH 05/16] Make the discovery requests at once, time out after 2 seconds by default, and read metadata again until it is loaded The two discovery requests are now made at once, as the specification allows, so a service that does not answer delays the build by one timeout rather than two. The evidence keys request runs on another thread while the accessible properties request runs on the building thread, and the build waits for both. The default timeout for each request to the cloud service is now 2 seconds, as the specification's Configuration options table gives and as .NET's CLOUD_REQUEST_TIMEOUT_DEFAULT_SECONDS is. It was 100 seconds. FlowElement gains hasLoadedProperties, true by default, as .NET's aspect engines have HasLoadedProperties. The Cloud Request Engine returns true once both discovery lists are known, the cloud aspect engines once their metadata is loaded, and a parallel group once all its elements have. PipelineDefault keeps its map of available properties only once every element has loaded its properties, and builds it again on each call until then, as .NET's Pipeline.ElementAvailableProperties does after a PropertiesNotYetLoadedException. The web integration works out the Vary headers again on each response until every element has loaded, as .NET's ClientsidePropertyService.HeadersAffectingJavaScript does after an exception from an element's EvidenceKeyFilter. --- .../DeviceDetectionCloudEngine.java | 11 +++++ .../IPIntelligenceCloudEngine.java | 11 +++++ .../flowelements/GeoLocationCloudEngine.java | 11 +++++ .../flowelements/CloudAspectEngineBase.java | 11 +++++ .../CloudRequestEngineBuilder.java | 5 ++- .../CloudRequestEngineDefault.java | 33 +++++++++++--- .../flowelements/Constants.java | 4 +- .../core/flowelements/FlowElement.java | 13 ++++++ .../core/flowelements/ParallelElements.java | 13 ++++++ .../core/flowelements/PipelineDefault.java | 44 +++++++++++++++++-- .../ClientsidePropertyServiceCore.java | 40 +++++++++++++++-- .../web/ClientsidePropertyServiceTests.java | 22 ++++++++++ 12 files changed, 200 insertions(+), 18 deletions(-) diff --git a/device-detection/device-detection.cloud/src/main/java/fiftyone/devicedetection/cloud/flowelements/DeviceDetectionCloudEngine.java b/device-detection/device-detection.cloud/src/main/java/fiftyone/devicedetection/cloud/flowelements/DeviceDetectionCloudEngine.java index e43fc4b2..400ca1da 100644 --- a/device-detection/device-detection.cloud/src/main/java/fiftyone/devicedetection/cloud/flowelements/DeviceDetectionCloudEngine.java +++ b/device-detection/device-detection.cloud/src/main/java/fiftyone/devicedetection/cloud/flowelements/DeviceDetectionCloudEngine.java @@ -95,6 +95,17 @@ public List getProperties() { loaded; } + /** + * Whether the property metadata is loaded, which reads it if the Cloud + * Request Engine now knows the accessible properties. + * @return true once the property metadata is loaded + */ + @Override + public boolean hasLoadedProperties() { + getProperties(); + return aspectProperties != null; + } + @Override public String getDataSourceTier() { return dataSourceTier; diff --git a/ip-intelligence/ip-intelligence.cloud/src/main/java/fiftyone/ipintelligence/cloud/flowelements/IPIntelligenceCloudEngine.java b/ip-intelligence/ip-intelligence.cloud/src/main/java/fiftyone/ipintelligence/cloud/flowelements/IPIntelligenceCloudEngine.java index bec4510d..99b84cbb 100644 --- a/ip-intelligence/ip-intelligence.cloud/src/main/java/fiftyone/ipintelligence/cloud/flowelements/IPIntelligenceCloudEngine.java +++ b/ip-intelligence/ip-intelligence.cloud/src/main/java/fiftyone/ipintelligence/cloud/flowelements/IPIntelligenceCloudEngine.java @@ -107,6 +107,17 @@ public List getProperties() { loaded; } + /** + * Whether the property metadata is loaded, which reads it if the Cloud + * Request Engine now knows the accessible properties. + * @return true once the property metadata is loaded + */ + @Override + public boolean hasLoadedProperties() { + getProperties(); + return aspectProperties != null; + } + @Override public String getDataSourceTier() { return dataSourceTier; diff --git a/location/geo-location.cloud/src/main/java/fiftyone/geolocation/flowelements/GeoLocationCloudEngine.java b/location/geo-location.cloud/src/main/java/fiftyone/geolocation/flowelements/GeoLocationCloudEngine.java index 243abac9..cde6a493 100644 --- a/location/geo-location.cloud/src/main/java/fiftyone/geolocation/flowelements/GeoLocationCloudEngine.java +++ b/location/geo-location.cloud/src/main/java/fiftyone/geolocation/flowelements/GeoLocationCloudEngine.java @@ -76,6 +76,17 @@ public List getProperties() { loaded; } + /** + * Whether the property metadata is loaded, which reads it if the Cloud + * Request Engine now knows the accessible properties. + * @return true once the property metadata is loaded + */ + @Override + public boolean hasLoadedProperties() { + getProperties(); + return properties != null; + } + @Override public String getDataSourceTier() { return dataSourceTier; diff --git a/pipeline/pipeline.cloudrequestengine/src/main/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudAspectEngineBase.java b/pipeline/pipeline.cloudrequestengine/src/main/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudAspectEngineBase.java index ee605233..0d83d2b1 100644 --- a/pipeline/pipeline.cloudrequestengine/src/main/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudAspectEngineBase.java +++ b/pipeline/pipeline.cloudrequestengine/src/main/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudAspectEngineBase.java @@ -169,6 +169,17 @@ public List getProperties() { return localRef; } + /** + * Whether the property metadata is loaded, which reads it if the Cloud + * Request Engine now knows the accessible properties. + * @return true once the property metadata is loaded + */ + @Override + public boolean hasLoadedProperties() { + getProperties(); + return aspectProperties != null; + } + @Override public TypedKey getTypedDataKey() { if (typedKey == null) { diff --git a/pipeline/pipeline.cloudrequestengine/src/main/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudRequestEngineBuilder.java b/pipeline/pipeline.cloudrequestengine/src/main/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudRequestEngineBuilder.java index 5e23b4ea..b0d420a0 100644 --- a/pipeline/pipeline.cloudrequestengine/src/main/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudRequestEngineBuilder.java +++ b/pipeline/pipeline.cloudrequestengine/src/main/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudRequestEngineBuilder.java @@ -247,9 +247,10 @@ public CloudRequestEngineBuilder setLicenseKey(String licenseKey) { } /** - * Timeout in seconds for the request to the endpoint. + * Timeout in seconds for each request to the cloud service, the two + * discovery requests when the engine is built and each data request. *

- * Default value is 100 seconds + * Default value is 2 seconds * @param timeout in seconds * @return this builder */ diff --git a/pipeline/pipeline.cloudrequestengine/src/main/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudRequestEngineDefault.java b/pipeline/pipeline.cloudrequestengine/src/main/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudRequestEngineDefault.java index 3f1be59b..0bf8338f 100644 --- a/pipeline/pipeline.cloudrequestengine/src/main/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudRequestEngineDefault.java +++ b/pipeline/pipeline.cloudrequestengine/src/main/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudRequestEngineDefault.java @@ -51,6 +51,8 @@ import java.net.URLEncoder; import java.nio.charset.StandardCharsets; import java.util.*; +import java.util.concurrent.CompletableFuture; +import java.util.concurrent.CompletionException; import java.util.regex.Pattern; import java.util.stream.Collectors; @@ -267,13 +269,12 @@ public EvidenceKeyFilter getEvidenceKeyFilter() { public void discover() { List failures = new ArrayList<>(); synchronized (discoveryLock) { - if (evidenceKeyFilter.isKnown() == false) { - try { - fetchEvidenceKeys(); - } catch (RuntimeException e) { - failures.add(e); - } - } + // The two requests are made at once, so a service that does not + // answer delays the build by one timeout rather than two. + CompletableFuture evidenceKeys = + evidenceKeyFilter.isKnown() ? + CompletableFuture.completedFuture(null) : + CompletableFuture.runAsync(this::fetchEvidenceKeys); if (publicProperties == null) { try { fetchPublicProperties(); @@ -281,6 +282,13 @@ public void discover() { failures.add(e); } } + try { + evidenceKeys.join(); + } catch (CompletionException e) { + failures.add(e.getCause() instanceof RuntimeException ? + (RuntimeException) e.getCause() : + e); + } } if (failures.isEmpty()) { return; @@ -298,6 +306,17 @@ public void discover() { logger.warn(MessageDiscoveryDeferred, first); } + /** + * Whether discovery has fetched both the accepted evidence keys and the + * accessible properties, so that what reads them, such as the + * Pipeline's map of available properties, can keep what it reads. + * @return true once both are known + */ + @Override + public boolean hasLoadedProperties() { + return evidenceKeyFilter.isKnown() && publicProperties != null; + } + /** * A copy of what discovery fetched, which a later builder can use in * place of the discovery requests. diff --git a/pipeline/pipeline.cloudrequestengine/src/main/java/fiftyone/pipeline/cloudrequestengine/flowelements/Constants.java b/pipeline/pipeline.cloudrequestengine/src/main/java/fiftyone/pipeline/cloudrequestengine/flowelements/Constants.java index f6300acf..b9b26c6a 100644 --- a/pipeline/pipeline.cloudrequestengine/src/main/java/fiftyone/pipeline/cloudrequestengine/flowelements/Constants.java +++ b/pipeline/pipeline.cloudrequestengine/src/main/java/fiftyone/pipeline/cloudrequestengine/flowelements/Constants.java @@ -28,6 +28,8 @@ public class Constants { "https://cloud.51degrees.com/api/v4"; // Environment variable name to set end point public static final String FOD_CLOUD_API_URL = "FOD_CLOUD_API_URL"; - public static final int DEFAULT_TIMEOUT_MILLIS = 100000; + // Default timeout for each request to the cloud service, as the + // specification's Configuration options table gives. + public static final int DEFAULT_TIMEOUT_MILLIS = 2000; } diff --git a/pipeline/pipeline.core/src/main/java/fiftyone/pipeline/core/flowelements/FlowElement.java b/pipeline/pipeline.core/src/main/java/fiftyone/pipeline/core/flowelements/FlowElement.java index c27716d2..b03878a8 100644 --- a/pipeline/pipeline.core/src/main/java/fiftyone/pipeline/core/flowelements/FlowElement.java +++ b/pipeline/pipeline.core/src/main/java/fiftyone/pipeline/core/flowelements/FlowElement.java @@ -103,6 +103,19 @@ public interface FlowElement< */ boolean isConcurrent(); + /** + * Whether the element knows all the property metadata and accepted + * Evidence it will expose. An element that gets them from a remote + * service returns false until it has them, and anything that keeps what + * it reads from the element, such as the Pipeline's map of available + * properties, reads it again until then. + * @return true if the property metadata and accepted Evidence are + * complete + */ + default boolean hasLoadedProperties() { + return true; + } + /** * Indicates whether the element has been closed using the {@link #close()} * method, either explicitly or as a result of a 'try with resource'. diff --git a/pipeline/pipeline.core/src/main/java/fiftyone/pipeline/core/flowelements/ParallelElements.java b/pipeline/pipeline.core/src/main/java/fiftyone/pipeline/core/flowelements/ParallelElements.java index 79b8e414..b1a479b7 100644 --- a/pipeline/pipeline.core/src/main/java/fiftyone/pipeline/core/flowelements/ParallelElements.java +++ b/pipeline/pipeline.core/src/main/java/fiftyone/pipeline/core/flowelements/ParallelElements.java @@ -117,6 +117,19 @@ public boolean isConcurrent() { return true; } + /** + * @return true if every child element has loaded its properties + */ + @Override + public boolean hasLoadedProperties() { + for (FlowElement element : flowElements) { + if (element.hasLoadedProperties() == false) { + return false; + } + } + return true; + } + @Override protected void managedResourcesCleanup() { for (FlowElement element : flowElements) { diff --git a/pipeline/pipeline.core/src/main/java/fiftyone/pipeline/core/flowelements/PipelineDefault.java b/pipeline/pipeline.core/src/main/java/fiftyone/pipeline/core/flowelements/PipelineDefault.java index f53cda4a..302b51d9 100644 --- a/pipeline/pipeline.core/src/main/java/fiftyone/pipeline/core/flowelements/PipelineDefault.java +++ b/pipeline/pipeline.core/src/main/java/fiftyone/pipeline/core/flowelements/PipelineDefault.java @@ -71,9 +71,15 @@ class PipelineDefault implements PipelineInternal { * Map of {@link ElementPropertyMetaData}s keyed first on the data key of * the element which they belong to, then by the property name. */ - private final Map> + private volatile Map> elementAvailableProperties; + /** + * Held while the map of available properties is built after an element + * had not loaded its properties when the Pipeline was built. + */ + private final Object availablePropertiesLock = new Object(); + /** * Lock used for synchronised blocks. */ @@ -155,8 +161,7 @@ class PipelineDefault implements PipelineInternal { for (FlowElement element : this.flowElements) { isConcurrent = isConcurrent || element.isConcurrent(); } - this.elementAvailableProperties = - getElementAvailableProperties(flowElements); + getElementAvailableProperties(); logger.debug("Pipeline '" + hashCode() + "' created."); @@ -360,10 +365,41 @@ public List getFlowElements() { return Collections.unmodifiableList(flowElements); } + /** + * The available properties of each element. The map is kept once every + * element has loaded its properties. Until then, as after a cloud + * element could not reach its service when it was built, it is built + * again on each call, so it gains the properties once they are known. + * @return the available properties, keyed on element data key then + * property name + */ @Override public Map> getElementAvailableProperties() { - return Collections.unmodifiableMap(elementAvailableProperties); + Map> properties = + elementAvailableProperties; + if (properties == null) { + synchronized (availablePropertiesLock) { + properties = elementAvailableProperties; + if (properties == null) { + boolean loaded = allPropertiesLoaded(); + properties = getElementAvailableProperties(flowElements); + if (loaded) { + elementAvailableProperties = properties; + } + } + } + } + return Collections.unmodifiableMap(properties); + } + + private boolean allPropertiesLoaded() { + for (FlowElement element : flowElements) { + if (element.hasLoadedProperties() == false) { + return false; + } + } + return true; } public void close(boolean closing) throws Exception { diff --git a/pipeline/pipeline.web.packages/pipeline.web/src/main/java/fiftyone/pipeline/web/services/ClientsidePropertyServiceCore.java b/pipeline/pipeline.web.packages/pipeline.web/src/main/java/fiftyone/pipeline/web/services/ClientsidePropertyServiceCore.java index 630ac847..d51d2cfe 100644 --- a/pipeline/pipeline.web.packages/pipeline.web/src/main/java/fiftyone/pipeline/web/services/ClientsidePropertyServiceCore.java +++ b/pipeline/pipeline.web.packages/pipeline.web/src/main/java/fiftyone/pipeline/web/services/ClientsidePropertyServiceCore.java @@ -108,6 +108,17 @@ private enum ContentTypes { */ protected Pipeline pipeline; + /** + * True once the Vary headers were worked out while every element + * had loaded its properties, so they can be kept. + */ + private volatile boolean headersComplete = false; + + /** + * Held while the Vary headers are worked out again. + */ + private final Object headersLock = new Object(); + /** * A list of all the HTTP headers that are requested evidence for * elements that populate JavaScript properties. @@ -142,10 +153,12 @@ public Default( */ @SuppressWarnings("rawtypes") protected final void init() { - headersAffectingJavaScript = new ArrayList<>(); + List headers = new ArrayList<>(); + boolean complete = true; // Get evidence filters for all elements. List filters = new ArrayList<>(); for (FlowElement element : pipeline.getFlowElements()) { + complete = complete && element.hasLoadedProperties(); filters.add(element.getEvidenceKeyFilter()); } @@ -160,13 +173,32 @@ protected final void init() { if (key.startsWith(EVIDENCE_HTTPHEADER_PREFIX + EVIDENCE_SEPERATOR) && hasControlChar(key) == false) { String header = key.substring(key.indexOf(EVIDENCE_SEPERATOR) + 1); - if(headersAffectingJavaScript.contains(header) == false){ - headersAffectingJavaScript.add(header); + if(headers.contains(header) == false){ + headers.add(header); } } } } } + headersAffectingJavaScript = headers; + headersComplete = complete; + } + + /** + * The headers for the Vary header. Until every element has loaded its + * properties, as after a cloud element could not reach its service + * when it was built, they are worked out again on each call, so they + * gain the cloud's headers once those are known. + */ + private List getHeadersAffectingJavaScript() { + if (headersComplete == false && pipeline != null) { + synchronized (headersLock) { + if (headersComplete == false) { + init(); + } + } + } + return headersAffectingJavaScript; } private boolean hasControlChar(String str) { @@ -269,7 +301,7 @@ private void setHeaders( response.setContentLength(contentLength); response.setStatus(200); response.setHeader("Cache-Control", stringJoin(cacheControl, ",")); - response.setHeader("Vary", stringJoin(headersAffectingJavaScript, ",")); + response.setHeader("Vary", stringJoin(getHeadersAffectingJavaScript(), ",")); response.setHeader("ETag", Integer.toString(hash)); response.setHeader("Access-Control-Allow-Origin", "*"); } diff --git a/pipeline/pipeline.web.packages/pipeline.web/src/test/java/fiftyone/pipeline/web/ClientsidePropertyServiceTests.java b/pipeline/pipeline.web.packages/pipeline.web/src/test/java/fiftyone/pipeline/web/ClientsidePropertyServiceTests.java index f819bd99..31c8f9d4 100644 --- a/pipeline/pipeline.web.packages/pipeline.web/src/test/java/fiftyone/pipeline/web/ClientsidePropertyServiceTests.java +++ b/pipeline/pipeline.web.packages/pipeline.web/src/test/java/fiftyone/pipeline/web/ClientsidePropertyServiceTests.java @@ -115,6 +115,28 @@ public void JavaScript_VaryHeader_UA() throws IOException { JavaScript_VaryHeader(elements, "User-Agent"); } + /** + * An element that has not loaded its properties, as a cloud element + * after a failure to reach its service when it was built, gains its + * headers in the Vary header once it has them. + */ + @Test + @SuppressWarnings("rawtypes") + public void JavaScript_VaryHeader_ReadAgainUntilLoaded() throws IOException { + FlowElement element = mock(FlowElement.class); + when(element.getEvidenceKeyFilter()).thenReturn( + new EvidenceKeyFilterWhitelist(new ArrayList()), + new EvidenceKeyFilterWhitelist(Arrays.asList("header.User-Agent"))); + when(element.hasLoadedProperties()).thenReturn(false, true); + List elements = new ArrayList<>(); + elements.add(element); + Configure(elements); + + service.serveJavascript(request, response); + + verify(response).setHeader(eq("Vary"), eq("User-Agent")); + } + @Test public void JavaScript_VaryHeader_UA_AND_SEC() throws IOException { List> elements = new ArrayList>(); From be26bcf32d6c5f7429e36a8d2b7791ef066fe542 Mon Sep 17 00:00:00 2001 From: James Rosewell Date: Mon, 9 Nov 2026 21:32:22 +0000 Subject: [PATCH 06/16] Test that the SetHeaders element keeps its configuration once every element has loaded its properties With every element loaded, the configuration built from the Pipeline's available properties is built once and kept across requests. While an element has not loaded its properties, it is built again on each request, and kept once every element has loaded. Both fail on the code before the fix, where the configuration was built again on every request, because getOrDefault never stored it. --- .../flowelements/SetHeadersElementTests.java | 76 +++++++++++++++++++ 1 file changed, 76 insertions(+) diff --git a/pipeline/pipeline.engines.fiftyone/src/test/java/fiftyone/pipeline/engines/fiftyone/flowelements/SetHeadersElementTests.java b/pipeline/pipeline.engines.fiftyone/src/test/java/fiftyone/pipeline/engines/fiftyone/flowelements/SetHeadersElementTests.java index cd25d63a..231fe150 100644 --- a/pipeline/pipeline.engines.fiftyone/src/test/java/fiftyone/pipeline/engines/fiftyone/flowelements/SetHeadersElementTests.java +++ b/pipeline/pipeline.engines.fiftyone/src/test/java/fiftyone/pipeline/engines/fiftyone/flowelements/SetHeadersElementTests.java @@ -49,8 +49,11 @@ import java.util.stream.Stream; import static org.junit.jupiter.api.Assertions.*; +import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; /** @@ -234,6 +237,79 @@ public void setHeadersElement_MultipleProperties_SameHeader() throws Exception { } } + /** + * Once every element of the Pipeline has loaded its properties, the + * configuration built from the Pipeline's available properties is kept, + * so later requests do not build it again. + */ + @Test + public void setHeadersElement_ConfigKeptOnceLoaded() throws Exception { + FlowElement source = mock(FlowElement.class); + when(source.hasLoadedProperties()).thenReturn(true); + Pipeline mockPipeline = mockPipeline(source); + createElement(); + + for (int i = 0; i < 3; i++) { + element.processInternal(mockFlowData(mockPipeline)); + } + + verify(mockPipeline, times(1)).getElementAvailableProperties(); + } + + /** + * While an element has not loaded its properties, as a cloud element + * after a failure to reach its service when the Pipeline was built, the + * configuration is built again on each request, and kept once every + * element has loaded. + */ + @Test + public void setHeadersElement_ConfigBuiltAgainUntilLoaded() throws Exception { + FlowElement source = mock(FlowElement.class); + when(source.hasLoadedProperties()).thenReturn(false, true); + Pipeline mockPipeline = mockPipeline(source); + createElement(); + + for (int i = 0; i < 4; i++) { + element.processInternal(mockFlowData(mockPipeline)); + } + + verify(mockPipeline, times(2)).getElementAvailableProperties(); + } + + @SuppressWarnings({"rawtypes", "unchecked"}) + private static Pipeline mockPipeline(FlowElement source) { + Pipeline mockPipeline = mock(Pipeline.class); + List elements = new ArrayList<>(); + elements.add(source); + when(mockPipeline.getFlowElements()).thenReturn(elements); + when(mockPipeline.getElementAvailableProperties()).thenReturn( + new HashMap>()); + return mockPipeline; + } + + private static FlowData mockFlowData(Pipeline mockPipeline) { + FlowData data = mock(FlowData.class); + when(data.getPipeline()).thenReturn(mockPipeline); + when(data.getOrAdd(anyString(), any())).thenReturn(mock(SetHeadersData.class)); + return data; + } + + private void createElement() { + element = new SetHeadersElement( + loggerFactory.getLogger(SetHeadersElement.class.getName()), + new ElementDataFactory() { + @Override + public SetHeadersData create( + FlowData flowData, + FlowElement flowElement) { + return new SetHeadersData( + loggerFactory.getLogger( + SetHeadersData.class.getName()), + flowData); + } + }); + } + private void createPipeline( Map propertyNameValues) throws Exception { sourceElement = new ActivePropertySourceElement( From 9f35ac909f4210aaa4ee9f0030e581ffbf7451fa Mon Sep 17 00:00:00 2001 From: James Rosewell Date: Mon, 9 Nov 2026 21:32:51 +0000 Subject: [PATCH 07/16] Keep the SetHeaders element's configuration once every element has loaded its properties The SetHeaders element built its configuration from the Pipeline's available properties on every request, because getOrDefault never stored it. It now keeps it once every element of the Pipeline has loaded its properties, and builds it again on each request until then, as after a cloud element could not reach its service when the Pipeline was built. So the configuration gains the cloud's SetHeader properties once a retry fetches them, and is then built once. This is the same rule the Pipeline's map of available properties and the web integration's Vary header follow. --- .../flowelements/SetHeadersElement.java | 30 ++++++++++++++++++- 1 file changed, 29 insertions(+), 1 deletion(-) diff --git a/pipeline/pipeline.engines.fiftyone/src/main/java/fiftyone/pipeline/engines/fiftyone/flowelements/SetHeadersElement.java b/pipeline/pipeline.engines.fiftyone/src/main/java/fiftyone/pipeline/engines/fiftyone/flowelements/SetHeadersElement.java index 78d4c29b..9bb05c6e 100644 --- a/pipeline/pipeline.engines.fiftyone/src/main/java/fiftyone/pipeline/engines/fiftyone/flowelements/SetHeadersElement.java +++ b/pipeline/pipeline.engines.fiftyone/src/main/java/fiftyone/pipeline/engines/fiftyone/flowelements/SetHeadersElement.java @@ -38,6 +38,7 @@ import fiftyone.pipeline.core.data.EvidenceKeyFilterWhitelist; import fiftyone.pipeline.core.data.FlowData; import fiftyone.pipeline.core.data.factories.ElementDataFactory; +import fiftyone.pipeline.core.flowelements.FlowElement; import fiftyone.pipeline.core.flowelements.FlowElementBase; import fiftyone.pipeline.core.flowelements.Pipeline; import fiftyone.pipeline.engines.data.AspectPropertyValue; @@ -101,8 +102,20 @@ protected void processInternal(FlowData data) throws Exception { PipelineConfig config; Pipeline pipeline = data.getPipeline(); if ((config = pipelineConfigs.get(pipeline)) == null) { + // The configuration is kept once every element has loaded its + // properties. Until then, as after a cloud element could not + // reach its service when the Pipeline was built, it is built + // again on each request, so it gains the SetHeader properties + // once they are known. + boolean loaded = allPropertiesLoaded(pipeline); config = populateConfig(pipeline); - config = pipelineConfigs.getOrDefault(pipeline, config); + if (loaded) { + PipelineConfig kept = + pipelineConfigs.putIfAbsent(pipeline, config); + if (kept != null) { + config = kept; + } + } } SetHeadersData elementData = data.getOrAdd( @@ -139,6 +152,21 @@ protected void unmanagedResourcesCleanup() { // Nothing to clean up here } + /** + * @param pipeline the Pipeline + * @return true if every element of the Pipeline has loaded its + * properties + */ + @SuppressWarnings("rawtypes") + private static boolean allPropertiesLoaded(Pipeline pipeline) { + for (FlowElement element : pipeline.getFlowElements()) { + if (element.hasLoadedProperties() == false) { + return false; + } + } + return true; + } + /** * Build the response headers from the SetHeader properties in the FlowData. * @param data flowData object From ea5fc04454c85cb11ae384b121123167e523c486 Mon Sep 17 00:00:00 2001 From: James Rosewell Date: Mon, 9 Nov 2026 21:45:31 +0000 Subject: [PATCH 08/16] Give the SetHeaders test mocks their type parameters, as the build treats raw types as errors --- .../engines/fiftyone/flowelements/SetHeadersElementTests.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pipeline/pipeline.engines.fiftyone/src/test/java/fiftyone/pipeline/engines/fiftyone/flowelements/SetHeadersElementTests.java b/pipeline/pipeline.engines.fiftyone/src/test/java/fiftyone/pipeline/engines/fiftyone/flowelements/SetHeadersElementTests.java index 231fe150..93029030 100644 --- a/pipeline/pipeline.engines.fiftyone/src/test/java/fiftyone/pipeline/engines/fiftyone/flowelements/SetHeadersElementTests.java +++ b/pipeline/pipeline.engines.fiftyone/src/test/java/fiftyone/pipeline/engines/fiftyone/flowelements/SetHeadersElementTests.java @@ -244,7 +244,7 @@ public void setHeadersElement_MultipleProperties_SameHeader() throws Exception { */ @Test public void setHeadersElement_ConfigKeptOnceLoaded() throws Exception { - FlowElement source = mock(FlowElement.class); + FlowElement source = mock(FlowElement.class); when(source.hasLoadedProperties()).thenReturn(true); Pipeline mockPipeline = mockPipeline(source); createElement(); @@ -264,7 +264,7 @@ public void setHeadersElement_ConfigKeptOnceLoaded() throws Exception { */ @Test public void setHeadersElement_ConfigBuiltAgainUntilLoaded() throws Exception { - FlowElement source = mock(FlowElement.class); + FlowElement source = mock(FlowElement.class); when(source.hasLoadedProperties()).thenReturn(false, true); Pipeline mockPipeline = mockPipeline(source); createElement(); From 8f709b4bccdfa707c6a78ddf90a0637177c7561b Mon Sep 17 00:00:00 2001 From: James Rosewell Date: Mon, 9 Nov 2026 22:24:30 +0000 Subject: [PATCH 09/16] Stub the product map once per call, so no varargs array is created thenReturn(null, products) uses the varargs overload, and a generic Map array is an unchecked warning that the build treats as an error, so each stub now chains two single-value thenReturn calls. --- .../cloud/flowelements/ProductMissingTests.java | 4 +++- .../cloud/flowelements/ProductMissingTests.java | 4 +++- .../geolocation/flowelements/GeoLocationCloudEngineTests.java | 4 +++- 3 files changed, 9 insertions(+), 3 deletions(-) diff --git a/device-detection/device-detection.cloud/src/test/java/fiftyone/devicedetection/cloud/flowelements/ProductMissingTests.java b/device-detection/device-detection.cloud/src/test/java/fiftyone/devicedetection/cloud/flowelements/ProductMissingTests.java index 93133bd5..8528c636 100644 --- a/device-detection/device-detection.cloud/src/test/java/fiftyone/devicedetection/cloud/flowelements/ProductMissingTests.java +++ b/device-detection/device-detection.cloud/src/test/java/fiftyone/devicedetection/cloud/flowelements/ProductMissingTests.java @@ -129,7 +129,9 @@ public void DiscoveryNotComplete_LoadsTheMetadataLater() throws Exception { Map products = new HashMap<>(); products.put("device", product); CloudRequestEngine requestEngine = mock(CloudRequestEngine.class); - when(requestEngine.getPublicProperties()).thenReturn(null, products); + when(requestEngine.getPublicProperties()) + .thenReturn(null) + .thenReturn(products); Pipeline pipeline = mock(Pipeline.class); when(pipeline.getElement(CloudRequestEngine.class)) .thenReturn(requestEngine); diff --git a/ip-intelligence/ip-intelligence.cloud/src/test/java/fiftyone/ipintelligence/cloud/flowelements/ProductMissingTests.java b/ip-intelligence/ip-intelligence.cloud/src/test/java/fiftyone/ipintelligence/cloud/flowelements/ProductMissingTests.java index eed28895..53be6a7b 100644 --- a/ip-intelligence/ip-intelligence.cloud/src/test/java/fiftyone/ipintelligence/cloud/flowelements/ProductMissingTests.java +++ b/ip-intelligence/ip-intelligence.cloud/src/test/java/fiftyone/ipintelligence/cloud/flowelements/ProductMissingTests.java @@ -129,7 +129,9 @@ public void DiscoveryNotComplete_LoadsTheMetadataLater() throws Exception { Map products = new HashMap<>(); products.put("ip", product); CloudRequestEngine requestEngine = mock(CloudRequestEngine.class); - when(requestEngine.getPublicProperties()).thenReturn(null, products); + when(requestEngine.getPublicProperties()) + .thenReturn(null) + .thenReturn(products); Pipeline pipeline = mock(Pipeline.class); when(pipeline.getElement(CloudRequestEngine.class)) .thenReturn(requestEngine); diff --git a/location/geo-location.cloud/src/test/java/fiftyone/geolocation/flowelements/GeoLocationCloudEngineTests.java b/location/geo-location.cloud/src/test/java/fiftyone/geolocation/flowelements/GeoLocationCloudEngineTests.java index 3bd56083..8c36cc1d 100644 --- a/location/geo-location.cloud/src/test/java/fiftyone/geolocation/flowelements/GeoLocationCloudEngineTests.java +++ b/location/geo-location.cloud/src/test/java/fiftyone/geolocation/flowelements/GeoLocationCloudEngineTests.java @@ -131,7 +131,9 @@ public void DiscoveryNotComplete_LoadsTheMetadataLater() throws Exception { Map products = new HashMap<>(); products.put("location", product); CloudRequestEngine requestEngine = mock(CloudRequestEngine.class); - when(requestEngine.getPublicProperties()).thenReturn(null, products); + when(requestEngine.getPublicProperties()) + .thenReturn(null) + .thenReturn(products); Pipeline pipeline = mock(Pipeline.class); when(pipeline.getElement(CloudRequestEngine.class)) .thenReturn(requestEngine); From 40452b7d5d2a5a4de52ab482ccfa56dbd73f70a3 Mon Sep 17 00:00:00 2001 From: James Rosewell Date: Mon, 9 Nov 2026 22:41:20 +0000 Subject: [PATCH 10/16] Test that a 408 or 429 at discovery is transient and does not fail the build The specification now treats a 408 or a 429 answer to a discovery request as a transient failure, because the same request can succeed later, so 429 moves from the cases that fail the build to those that do not, and 408 cases join it. --- .../CloudRequestEngineDiscoveryTests.java | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/pipeline/pipeline.cloudrequestengine/src/test/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudRequestEngineDiscoveryTests.java b/pipeline/pipeline.cloudrequestengine/src/test/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudRequestEngineDiscoveryTests.java index a5bbc81f..654273c6 100644 --- a/pipeline/pipeline.cloudrequestengine/src/test/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudRequestEngineDiscoveryTests.java +++ b/pipeline/pipeline.cloudrequestengine/src/test/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudRequestEngineDiscoveryTests.java @@ -191,7 +191,7 @@ public void Build_FetchesBothLists() throws Exception { private static Stream definitiveFailures() { List arguments = new ArrayList<>(); - for (int status : new int[] { 400, 401, 403, 404, 429 }) { + for (int status : new int[] { 400, 401, 403, 404 }) { arguments.add(Arguments.of(PROPERTIES, status)); arguments.add(Arguments.of(EVIDENCE_KEYS, status)); } @@ -199,8 +199,9 @@ private static Stream definitiveFailures() { } /** - * A 4xx answer to either request is a definitive configuration error, - * which fails the build once both requests have been made. + * A 4xx answer to either request, other than 408 or 429, is a + * definitive configuration error, which fails the build once both + * requests have been made. */ @ParameterizedTest(name = "{1} from {0}") @MethodSource("definitiveFailures") @@ -235,6 +236,10 @@ private static Stream transientFailures() { Arguments.of("500 from accessibleproperties", 500, 200), Arguments.of("503 from evidencekeys", 200, 503), Arguments.of("502 from both", 502, 502), + Arguments.of("408 from accessibleproperties", 408, 200), + Arguments.of("408 from evidencekeys", 200, 408), + Arguments.of("429 from accessibleproperties", 429, 200), + Arguments.of("429 from evidencekeys", 200, 429), Arguments.of("unreadable accessibleproperties", UNREADABLE, 200), Arguments.of("unreadable evidencekeys", 200, UNREADABLE), Arguments.of("connection refused to both", UNREACHABLE, UNREACHABLE), @@ -242,9 +247,9 @@ private static Stream transientFailures() { } /** - * Any other failure, the service unreachable, a timeout, a 5xx answer or - * a body that cannot be read, is logged as one warning and the build - * completes. + * Any other failure, the service unreachable, a timeout, a 5xx, 408 or + * 429 answer or a body that cannot be read, is logged as one warning and + * the build completes. */ @ParameterizedTest(name = "{0}") @MethodSource("transientFailures") From 7bc481c8fc509ccbea0eb3dbc9f6138bbe3daede Mon Sep 17 00:00:00 2001 From: James Rosewell Date: Mon, 9 Nov 2026 22:42:02 +0000 Subject: [PATCH 11/16] Treat a 408 or 429 at discovery as transient, so it does not fail the build A build fails on a 4xx answer because no retry with the same configuration can succeed, which is not so for a 408 or a 429, as RFC 9110 section 15.5.9 and RFC 6585 section 4 say, so those two are logged as a warning and retried on first use like a 5xx. --- .../CloudRequestEngineBuilder.java | 4 +- .../CloudRequestEngineDefault.java | 44 +++++++++++++------ 2 files changed, 32 insertions(+), 16 deletions(-) diff --git a/pipeline/pipeline.cloudrequestengine/src/main/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudRequestEngineBuilder.java b/pipeline/pipeline.cloudrequestengine/src/main/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudRequestEngineBuilder.java index b0d420a0..5530411d 100644 --- a/pipeline/pipeline.cloudrequestengine/src/main/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudRequestEngineBuilder.java +++ b/pipeline/pipeline.cloudrequestengine/src/main/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudRequestEngineBuilder.java @@ -118,8 +118,8 @@ protected CloudRequestEngine newEngine(List properties) throws Exception else { // Fetch the accessible properties and the accepted evidence // keys now, so that the built engine is ready. A 4xx answer - // fails the build, and any other failure is retried on first - // use. + // other than 408 or 429 fails the build, and any other failure + // is retried on first use. engine.discover(); } built = engine; diff --git a/pipeline/pipeline.cloudrequestengine/src/main/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudRequestEngineDefault.java b/pipeline/pipeline.cloudrequestengine/src/main/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudRequestEngineDefault.java index 0bf8338f..5c95377f 100644 --- a/pipeline/pipeline.cloudrequestengine/src/main/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudRequestEngineDefault.java +++ b/pipeline/pipeline.cloudrequestengine/src/main/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudRequestEngineDefault.java @@ -87,11 +87,24 @@ public class CloudRequestEngineDefault /** * The lowest HTTP status of a client error. A discovery request answered - * with a status from this up to {@link #HTTP_SERVER_ERROR_MIN} is a - * definitive configuration error. + * with a status from this up to {@link #HTTP_SERVER_ERROR_MIN}, other + * than {@link #HTTP_REQUEST_TIMEOUT} or {@link #HTTP_TOO_MANY_REQUESTS}, + * is a definitive configuration error. */ private static final int HTTP_CLIENT_ERROR_MIN = 400; + /** + * Request Timeout, a client error that a later retry can pass, as RFC + * 9110 section 15.5.9 says. + */ + private static final int HTTP_REQUEST_TIMEOUT = 408; + + /** + * Too Many Requests, a client error that a later retry can pass, as RFC + * 6585 section 4 says. + */ + private static final int HTTP_TOO_MANY_REQUESTS = 429; + /** * The lowest HTTP status of a server error. */ @@ -257,14 +270,15 @@ public EvidenceKeyFilter getEvidenceKeyFilter() { * Fetch the accepted Evidence keys and the accessible properties that * are not known yet. The builder calls this when it builds the engine, * so in the normal case a built engine is ready to process. Both - * requests are made. A 4xx answer to either is a definitive - * configuration error, and its {@link CloudRequestException} is thrown, - * because retrying with the same configuration can never succeed. Any - * other failure, such as the service being unreachable, a timeout, a - * 5xx answer or a body that cannot be read, is logged as one warning - * and the list that did arrive is kept. Processing fetches what is - * still missing. - * @throws CloudRequestException for a 4xx answer to either request + * requests are made. A 4xx answer to either, other than 408 or 429, is + * a definitive configuration error, and its + * {@link CloudRequestException} is thrown, because retrying with the + * same configuration can never succeed. Any other failure, such as the + * service being unreachable, a timeout, a 5xx, 408 or 429 answer or a + * body that cannot be read, is logged as one warning and the list that + * did arrive is kept. Processing fetches what is still missing. + * @throws CloudRequestException for a 4xx answer to either request, + * other than 408 or 429 */ public void discover() { List failures = new ArrayList<>(); @@ -346,16 +360,18 @@ public void useDiscoveryState(CloudDiscoveryState state) { /** * The failure of a discovery request that the cloud service received - * and rejected with a 4xx status, so that retrying with the same - * configuration can never succeed. + * and rejected with a 4xx status other than 408 or 429, so that + * retrying with the same configuration can never succeed. * @param failure the failure of a discovery request - * @return the exception with the 4xx status, or null if there is none + * @return the exception with that status, or null if there is none */ private static CloudRequestException definitiveError(Throwable failure) { if (failure instanceof CloudRequestException) { int status = ((CloudRequestException) failure).getHttpStatusCode(); return status >= HTTP_CLIENT_ERROR_MIN && - status < HTTP_SERVER_ERROR_MIN ? + status < HTTP_SERVER_ERROR_MIN && + status != HTTP_REQUEST_TIMEOUT && + status != HTTP_TOO_MANY_REQUESTS ? (CloudRequestException) failure : null; } From 726204bf7b2b3873584a6c604ba82828ad5d8b5e Mon Sep 17 00:00:00 2001 From: James Rosewell Date: Mon, 9 Nov 2026 23:38:03 +0000 Subject: [PATCH 12/16] Revert "Treat a 408 or 429 at discovery as transient, so it does not fail the build" This reverts commit 7bc481c8fc509ccbea0eb3dbc9f6138bbe3daede. --- .../CloudRequestEngineBuilder.java | 4 +- .../CloudRequestEngineDefault.java | 44 ++++++------------- 2 files changed, 16 insertions(+), 32 deletions(-) diff --git a/pipeline/pipeline.cloudrequestengine/src/main/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudRequestEngineBuilder.java b/pipeline/pipeline.cloudrequestengine/src/main/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudRequestEngineBuilder.java index 5530411d..b0d420a0 100644 --- a/pipeline/pipeline.cloudrequestengine/src/main/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudRequestEngineBuilder.java +++ b/pipeline/pipeline.cloudrequestengine/src/main/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudRequestEngineBuilder.java @@ -118,8 +118,8 @@ protected CloudRequestEngine newEngine(List properties) throws Exception else { // Fetch the accessible properties and the accepted evidence // keys now, so that the built engine is ready. A 4xx answer - // other than 408 or 429 fails the build, and any other failure - // is retried on first use. + // fails the build, and any other failure is retried on first + // use. engine.discover(); } built = engine; diff --git a/pipeline/pipeline.cloudrequestengine/src/main/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudRequestEngineDefault.java b/pipeline/pipeline.cloudrequestengine/src/main/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudRequestEngineDefault.java index 5c95377f..0bf8338f 100644 --- a/pipeline/pipeline.cloudrequestengine/src/main/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudRequestEngineDefault.java +++ b/pipeline/pipeline.cloudrequestengine/src/main/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudRequestEngineDefault.java @@ -87,24 +87,11 @@ public class CloudRequestEngineDefault /** * The lowest HTTP status of a client error. A discovery request answered - * with a status from this up to {@link #HTTP_SERVER_ERROR_MIN}, other - * than {@link #HTTP_REQUEST_TIMEOUT} or {@link #HTTP_TOO_MANY_REQUESTS}, - * is a definitive configuration error. + * with a status from this up to {@link #HTTP_SERVER_ERROR_MIN} is a + * definitive configuration error. */ private static final int HTTP_CLIENT_ERROR_MIN = 400; - /** - * Request Timeout, a client error that a later retry can pass, as RFC - * 9110 section 15.5.9 says. - */ - private static final int HTTP_REQUEST_TIMEOUT = 408; - - /** - * Too Many Requests, a client error that a later retry can pass, as RFC - * 6585 section 4 says. - */ - private static final int HTTP_TOO_MANY_REQUESTS = 429; - /** * The lowest HTTP status of a server error. */ @@ -270,15 +257,14 @@ public EvidenceKeyFilter getEvidenceKeyFilter() { * Fetch the accepted Evidence keys and the accessible properties that * are not known yet. The builder calls this when it builds the engine, * so in the normal case a built engine is ready to process. Both - * requests are made. A 4xx answer to either, other than 408 or 429, is - * a definitive configuration error, and its - * {@link CloudRequestException} is thrown, because retrying with the - * same configuration can never succeed. Any other failure, such as the - * service being unreachable, a timeout, a 5xx, 408 or 429 answer or a - * body that cannot be read, is logged as one warning and the list that - * did arrive is kept. Processing fetches what is still missing. - * @throws CloudRequestException for a 4xx answer to either request, - * other than 408 or 429 + * requests are made. A 4xx answer to either is a definitive + * configuration error, and its {@link CloudRequestException} is thrown, + * because retrying with the same configuration can never succeed. Any + * other failure, such as the service being unreachable, a timeout, a + * 5xx answer or a body that cannot be read, is logged as one warning + * and the list that did arrive is kept. Processing fetches what is + * still missing. + * @throws CloudRequestException for a 4xx answer to either request */ public void discover() { List failures = new ArrayList<>(); @@ -360,18 +346,16 @@ public void useDiscoveryState(CloudDiscoveryState state) { /** * The failure of a discovery request that the cloud service received - * and rejected with a 4xx status other than 408 or 429, so that - * retrying with the same configuration can never succeed. + * and rejected with a 4xx status, so that retrying with the same + * configuration can never succeed. * @param failure the failure of a discovery request - * @return the exception with that status, or null if there is none + * @return the exception with the 4xx status, or null if there is none */ private static CloudRequestException definitiveError(Throwable failure) { if (failure instanceof CloudRequestException) { int status = ((CloudRequestException) failure).getHttpStatusCode(); return status >= HTTP_CLIENT_ERROR_MIN && - status < HTTP_SERVER_ERROR_MIN && - status != HTTP_REQUEST_TIMEOUT && - status != HTTP_TOO_MANY_REQUESTS ? + status < HTTP_SERVER_ERROR_MIN ? (CloudRequestException) failure : null; } From 19cc0faf40f8ce0eccab0d88ddf2c999ccbb7e31 Mon Sep 17 00:00:00 2001 From: James Rosewell Date: Mon, 9 Nov 2026 23:38:04 +0000 Subject: [PATCH 13/16] Revert "Test that a 408 or 429 at discovery is transient and does not fail the build" This reverts commit 40452b7d5d2a5a4de52ab482ccfa56dbd73f70a3. --- .../CloudRequestEngineDiscoveryTests.java | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/pipeline/pipeline.cloudrequestengine/src/test/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudRequestEngineDiscoveryTests.java b/pipeline/pipeline.cloudrequestengine/src/test/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudRequestEngineDiscoveryTests.java index 654273c6..a5bbc81f 100644 --- a/pipeline/pipeline.cloudrequestengine/src/test/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudRequestEngineDiscoveryTests.java +++ b/pipeline/pipeline.cloudrequestengine/src/test/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudRequestEngineDiscoveryTests.java @@ -191,7 +191,7 @@ public void Build_FetchesBothLists() throws Exception { private static Stream definitiveFailures() { List arguments = new ArrayList<>(); - for (int status : new int[] { 400, 401, 403, 404 }) { + for (int status : new int[] { 400, 401, 403, 404, 429 }) { arguments.add(Arguments.of(PROPERTIES, status)); arguments.add(Arguments.of(EVIDENCE_KEYS, status)); } @@ -199,9 +199,8 @@ private static Stream definitiveFailures() { } /** - * A 4xx answer to either request, other than 408 or 429, is a - * definitive configuration error, which fails the build once both - * requests have been made. + * A 4xx answer to either request is a definitive configuration error, + * which fails the build once both requests have been made. */ @ParameterizedTest(name = "{1} from {0}") @MethodSource("definitiveFailures") @@ -236,10 +235,6 @@ private static Stream transientFailures() { Arguments.of("500 from accessibleproperties", 500, 200), Arguments.of("503 from evidencekeys", 200, 503), Arguments.of("502 from both", 502, 502), - Arguments.of("408 from accessibleproperties", 408, 200), - Arguments.of("408 from evidencekeys", 200, 408), - Arguments.of("429 from accessibleproperties", 429, 200), - Arguments.of("429 from evidencekeys", 200, 429), Arguments.of("unreadable accessibleproperties", UNREADABLE, 200), Arguments.of("unreadable evidencekeys", 200, UNREADABLE), Arguments.of("connection refused to both", UNREACHABLE, UNREACHABLE), @@ -247,9 +242,9 @@ private static Stream transientFailures() { } /** - * Any other failure, the service unreachable, a timeout, a 5xx, 408 or - * 429 answer or a body that cannot be read, is logged as one warning and - * the build completes. + * Any other failure, the service unreachable, a timeout, a 5xx answer or + * a body that cannot be read, is logged as one warning and the build + * completes. */ @ParameterizedTest(name = "{0}") @MethodSource("transientFailures") From 1e1dae8a52ff37cb8dd3f7368814c1220dfd17bc Mon Sep 17 00:00:00 2001 From: James Rosewell Date: Tue, 10 Nov 2026 00:25:06 +0000 Subject: [PATCH 14/16] Test that a 408 at discovery fails the build, as every 4xx does The specification on rc makes every 4xx answer to a discovery request a definitive configuration error, so the cases that fail the build now include 408 beside 429. --- .../flowelements/CloudRequestEngineDiscoveryTests.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pipeline/pipeline.cloudrequestengine/src/test/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudRequestEngineDiscoveryTests.java b/pipeline/pipeline.cloudrequestengine/src/test/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudRequestEngineDiscoveryTests.java index a5bbc81f..07189b21 100644 --- a/pipeline/pipeline.cloudrequestengine/src/test/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudRequestEngineDiscoveryTests.java +++ b/pipeline/pipeline.cloudrequestengine/src/test/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudRequestEngineDiscoveryTests.java @@ -191,7 +191,7 @@ public void Build_FetchesBothLists() throws Exception { private static Stream definitiveFailures() { List arguments = new ArrayList<>(); - for (int status : new int[] { 400, 401, 403, 404, 429 }) { + for (int status : new int[] { 400, 401, 403, 404, 408, 429 }) { arguments.add(Arguments.of(PROPERTIES, status)); arguments.add(Arguments.of(EVIDENCE_KEYS, status)); } From 127b173e15c9c97744650b41ec3571bcee03d344 Mon Sep 17 00:00:00 2001 From: James Rosewell Date: Tue, 10 Nov 2026 00:25:18 +0000 Subject: [PATCH 15/16] Test that a 4xx at discovery fails the build when its body is not JSON A 4xx answer is a definitive configuration error whatever its body holds, such as an error page from a proxy. The accessibleproperties answer is read as JSON before its status is used, so a 403 with an HTML body there fails this test, as the build completes with a warning. --- .../CloudRequestEngineDiscoveryTests.java | 28 +++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/pipeline/pipeline.cloudrequestengine/src/test/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudRequestEngineDiscoveryTests.java b/pipeline/pipeline.cloudrequestengine/src/test/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudRequestEngineDiscoveryTests.java index 07189b21..aeadfb1a 100644 --- a/pipeline/pipeline.cloudrequestengine/src/test/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudRequestEngineDiscoveryTests.java +++ b/pipeline/pipeline.cloudrequestengine/src/test/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudRequestEngineDiscoveryTests.java @@ -35,6 +35,7 @@ import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.Arguments; import org.junit.jupiter.params.provider.MethodSource; +import org.junit.jupiter.params.provider.ValueSource; import org.slf4j.ILoggerFactory; import org.slf4j.Logger; @@ -112,6 +113,11 @@ public class CloudRequestEngineDiscoveryTests { private HttpClient httpClient; private final Map requests = new ConcurrentHashMap<>(); private final Map answers = new HashMap<>(); + + /** + * A body to answer with in place of the usual one, by endpoint. + */ + private final Map bodies = new HashMap<>(); private final List connections = Collections.synchronizedList(new ArrayList()); @@ -215,6 +221,25 @@ public void Build_4xx_FailsTheBuild(String endpoint, int status) { assertEquals(1, requests(EVIDENCE_KEYS), "evidencekeys"); } + /** + * A 4xx answer fails the build even when its body is not JSON, such as + * an error page from a proxy, because the status alone makes it + * definitive. + */ + @ParameterizedTest(name = "{0}") + @ValueSource(strings = { PROPERTIES, EVIDENCE_KEYS }) + public void Build_4xxWithBodyNotJson_FailsTheBuild(String endpoint) { + answers.put(endpoint, 403); + bodies.put(endpoint, "Forbidden"); + + CloudRequestException error = + assertThrows(CloudRequestException.class, this::build); + + assertEquals(403, error.getHttpStatusCode()); + assertEquals(1, requests(PROPERTIES), "accessibleproperties"); + assertEquals(1, requests(EVIDENCE_KEYS), "evidencekeys"); + } + /** * A 4xx answer fails the build even when the other request failed in a * way that can pass. @@ -502,6 +527,9 @@ private String body(HttpURLConnection connection) throws IOException { if (answer == UNREADABLE) { return "proxy error"; } + if (bodies.containsKey(name)) { + return bodies.get(name); + } if (answer != 200) { return "{\"errors\": [\"" + name + " answered " + answer + "\"]}"; } From 7d4cebdd234afed486cf492a81acb88ddaf1496f Mon Sep 17 00:00:00 2001 From: James Rosewell Date: Tue, 10 Nov 2026 00:25:24 +0000 Subject: [PATCH 16/16] Keep the status of a cloud answer whose body is not JSON An answer that cannot be read as JSON now raises a CloudRequestException carrying its HTTP status, as .NET does, so a 4xx answer to a discovery request fails the build whatever its body holds, as the specification says. --- .../pipeline/cloudrequestengine/Constants.java | 4 ++++ .../flowelements/CloudRequestEngineDefault.java | 15 ++++++++++++++- 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/pipeline/pipeline.cloudrequestengine/src/main/java/fiftyone/pipeline/cloudrequestengine/Constants.java b/pipeline/pipeline.cloudrequestengine/src/main/java/fiftyone/pipeline/cloudrequestengine/Constants.java index 4ac6fbbb..e957273e 100644 --- a/pipeline/pipeline.cloudrequestengine/src/main/java/fiftyone/pipeline/cloudrequestengine/Constants.java +++ b/pipeline/pipeline.cloudrequestengine/src/main/java/fiftyone/pipeline/cloudrequestengine/Constants.java @@ -40,6 +40,10 @@ public class Messages { public static final String ExceptionCloudError = "Error returned from 51Degrees cloud service: %s"; + + public static final String ExceptionResponseNotJson = + "Cloud service returned status code '%d' with content that is " + + "not JSON"; public static final String ExceptionFailedToLoadProperties = "Failed to load aspect properties for element '%s'. This is " diff --git a/pipeline/pipeline.cloudrequestengine/src/main/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudRequestEngineDefault.java b/pipeline/pipeline.cloudrequestengine/src/main/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudRequestEngineDefault.java index 0bf8338f..d0ecc75e 100644 --- a/pipeline/pipeline.cloudrequestengine/src/main/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudRequestEngineDefault.java +++ b/pipeline/pipeline.cloudrequestengine/src/main/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudRequestEngineDefault.java @@ -38,6 +38,7 @@ import fiftyone.pipeline.engines.services.HttpClient; import fiftyone.pipeline.exceptions.AggregateException; import org.json.JSONArray; +import org.json.JSONException; import org.json.JSONObject; import org.slf4j.Logger; @@ -702,7 +703,19 @@ private void validateResponse(String jsonResult, List messages = new ArrayList<>(); if (hasData && checkForErrorMessages) { - JSONObject jObj = new JSONObject(jsonResult); + JSONObject jObj; + try { + jObj = new JSONObject(jsonResult); + } catch (JSONException e) { + // The status says whether the failure is definitive, so it + // is kept when the body cannot be read. + CloudRequestException error = new CloudRequestException( + String.format(ExceptionResponseNotJson, code), + code, + connection.getHeaderFields()); + error.initCause(e); + throw error; + } boolean hasErrors = jObj.keySet().contains("errors"); hasData = hasErrors ? jObj.keySet().size() > 1 :