From 65bbf25704058ba673a7c5906b4cd0ced604cc45 Mon Sep 17 00:00:00 2001 From: James Rosewell Date: Mon, 9 Nov 2026 20:23:27 +0000 Subject: [PATCH 01/28] Test that a cloud engine whose product the resource key lacks builds and logs once A resource key can be valid and include no product that holds a cloud engine's properties. The Device Detection, IP Intelligence and Location cloud engines are tested to build with empty property metadata, to log the missing product once, to log and throw nothing when processing responses that hold nothing for them, and to report a property read as missing with ProductNotAccessibleWithResourceKey. The base class tests now expect a missing product to build with empty metadata and log once, keep the failure when the accessible properties are not loaded, and check that a property keyed engine gives no profiles. The Location cloud module gets Mockito as a test dependency for its first tests. --- .../flowelements/ProductMissingTests.java | 181 +++++++++++++++++ .../flowelements/ProductMissingTests.java | 182 +++++++++++++++++ location/geo-location.cloud/pom.xml | 4 + .../GeoLocationCloudEngineTests.java | 184 ++++++++++++++++++ .../CloudAspectEngineBaseTests.java | 151 ++++++++++++-- 5 files changed, 683 insertions(+), 19 deletions(-) create mode 100644 device-detection/device-detection.cloud/src/test/java/fiftyone/devicedetection/cloud/flowelements/ProductMissingTests.java create mode 100644 ip-intelligence/ip-intelligence.cloud/src/test/java/fiftyone/ipintelligence/cloud/flowelements/ProductMissingTests.java create mode 100644 location/geo-location.cloud/src/test/java/fiftyone/geolocation/flowelements/GeoLocationCloudEngineTests.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 new file mode 100644 index 000000000..37c1ec6d5 --- /dev/null +++ b/device-detection/device-detection.cloud/src/test/java/fiftyone/devicedetection/cloud/flowelements/ProductMissingTests.java @@ -0,0 +1,181 @@ +/* ********************************************************************* + * 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.devicedetection.cloud.flowelements; + +import fiftyone.devicedetection.cloud.data.DeviceDataCloud; +import fiftyone.pipeline.cloudrequestengine.data.CloudRequestData; +import fiftyone.pipeline.cloudrequestengine.flowelements.CloudRequestEngine; +import fiftyone.pipeline.core.data.AccessiblePropertyMetaData.ProductMetaData; +import fiftyone.pipeline.core.data.AccessiblePropertyMetaData.PropertyMetaData; +import fiftyone.pipeline.core.data.FlowData; +import fiftyone.pipeline.core.flowelements.Pipeline; +import fiftyone.pipeline.engines.exceptions.PropertyMissingException; +import fiftyone.pipeline.engines.services.MissingPropertyReason; +import fiftyone.pipeline.engines.services.MissingPropertyServiceDefault; +import org.junit.Before; +import org.junit.Test; +import org.mockito.Mockito; +import org.mockito.invocation.Invocation; +import org.slf4j.ILoggerFactory; +import org.slf4j.Logger; + +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collections; +import java.util.HashMap; +import java.util.List; +import java.util.Map; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +/** + * A resource key can be valid and include no product that holds the + * Device Detection properties. The engine then builds with empty property + * metadata, logs the missing product once, and logs or throws nothing on + * each request. + */ +public class ProductMissingTests { + + private Logger logger; + private ILoggerFactory loggerFactory; + + @Before + public void init() { + logger = mock(Logger.class); + loggerFactory = mock(ILoggerFactory.class); + when(loggerFactory.getLogger(anyString())).thenReturn(logger); + } + + @Test + public void ProductMissing_BuildsWithEmptyMetadata() throws Exception { + try (DeviceDetectionCloudEngine engine = buildEngine()) { + assertNotNull(engine.getProperties()); + assertEquals(0, engine.getProperties().size()); + List logged = warningsAndErrors(); + assertEquals(logged.toString(), 1, logged.size()); + assertTrue(logged.get(0), logged.get(0).contains("'device'")); + } + } + + @Test + public void ProductMissing_ProcessLogsAndThrowsNothing() throws Exception { + try (DeviceDetectionCloudEngine engine = buildEngine()) { + for (int i = 0; i < 3; i++) { + process(engine); + } + List logged = warningsAndErrors(); + assertEquals(logged.toString(), 1, logged.size()); + } + } + + /** + * Reading a property behaves as for any property the response does not + * carry. + */ + @Test + public void ProductMissing_PropertyIsMissing() throws Exception { + try (DeviceDetectionCloudEngine engine = buildEngine()) { + DeviceDataCloud device = process(engine); + try { + device.getHardwareVendor(); + fail("A PropertyMissingException should have been thrown"); + } catch (PropertyMissingException e) { + assertEquals( + MissingPropertyReason.ProductNotAccessibleWithResourceKey, + e.getReason()); + } + } + } + + /** + * An engine whose Cloud Request Engine reports the accessible products + * of a resource key that includes IP Intelligence alone. + */ + private DeviceDetectionCloudEngine buildEngine() throws Exception { + DeviceDetectionCloudEngine engine = + new DeviceDetectionCloudEngineBuilder(loggerFactory).build(); + + ProductMetaData product = new ProductMetaData(); + product.dataTier = "test"; + product.properties = Arrays.asList( + new PropertyMetaData("registeredname", "String", "cat", null)); + Map publicProperties = new HashMap<>(); + publicProperties.put("ip", product); + + CloudRequestEngine requestEngine = mock(CloudRequestEngine.class); + when(requestEngine.getPublicProperties()).thenReturn(publicProperties); + + Pipeline pipeline = mock(Pipeline.class); + when(pipeline.getElement(CloudRequestEngine.class)) + .thenReturn(requestEngine); + + engine.addPipeline(pipeline); + return engine; + } + + /** + * Process a cloud response that holds nothing for the Device Detection + * product. + */ + private DeviceDataCloud process(DeviceDetectionCloudEngine engine) { + CloudRequestData cloudData = mock(CloudRequestData.class); + when(cloudData.getJsonResponse()).thenReturn( + "{ 'ip': { 'registeredname': 'Example' }, " + + "'javascriptProperties': [] }"); + + FlowData flowData = mock(FlowData.class); + when(flowData.getFromElement(any(CloudRequestEngine.class))) + .thenReturn(cloudData); + + DeviceDataCloud device = new DeviceDataCloudInternal( + loggerFactory.getLogger(DeviceDataCloud.class.getSimpleName()), + flowData, + engine, + MissingPropertyServiceDefault.getInstance()); + engine.processEngine(flowData, device); + return device; + } + + /** + * Every warning and error logged, at any overload. + */ + private List warningsAndErrors() { + List logged = new ArrayList<>(); + for (Invocation invocation : + Mockito.mockingDetails(logger).getInvocations()) { + String level = invocation.getMethod().getName(); + if (level.equals("warn") || level.equals("error")) { + logged.add(level + " " + + Arrays.toString(invocation.getArguments())); + } + } + return Collections.unmodifiableList(logged); + } +} 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 new file mode 100644 index 000000000..df07df779 --- /dev/null +++ b/ip-intelligence/ip-intelligence.cloud/src/test/java/fiftyone/ipintelligence/cloud/flowelements/ProductMissingTests.java @@ -0,0 +1,182 @@ +/* ********************************************************************* + * 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.ipintelligence.cloud.flowelements; + +import fiftyone.ipintelligence.cloud.data.IPIntelligenceDataCloud; +import fiftyone.pipeline.cloudrequestengine.data.CloudRequestData; +import fiftyone.pipeline.cloudrequestengine.flowelements.CloudRequestEngine; +import fiftyone.pipeline.core.data.AccessiblePropertyMetaData.ProductMetaData; +import fiftyone.pipeline.core.data.AccessiblePropertyMetaData.PropertyMetaData; +import fiftyone.pipeline.core.data.FlowData; +import fiftyone.pipeline.core.flowelements.Pipeline; +import fiftyone.pipeline.engines.exceptions.PropertyMissingException; +import fiftyone.pipeline.engines.services.MissingPropertyReason; +import fiftyone.pipeline.engines.services.MissingPropertyServiceDefault; +import org.junit.Before; +import org.junit.Test; +import org.mockito.Mockito; +import org.mockito.invocation.Invocation; +import org.slf4j.ILoggerFactory; +import org.slf4j.Logger; + +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collections; +import java.util.HashMap; +import java.util.List; +import java.util.Map; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +/** + * A resource key can be valid and include no product that holds the + * IP Intelligence properties. The engine then builds with empty property + * metadata, logs the missing product once, and logs or throws nothing on + * each request. + */ +public class ProductMissingTests { + + private Logger logger; + private ILoggerFactory loggerFactory; + + @Before + public void init() { + logger = mock(Logger.class); + loggerFactory = mock(ILoggerFactory.class); + when(loggerFactory.getLogger(anyString())).thenReturn(logger); + } + + @Test + public void ProductMissing_BuildsWithEmptyMetadata() throws Exception { + try (IPIntelligenceCloudEngine engine = buildEngine()) { + assertNotNull(engine.getProperties()); + assertEquals(0, engine.getProperties().size()); + List logged = warningsAndErrors(); + assertEquals(logged.toString(), 1, logged.size()); + assertTrue(logged.get(0), logged.get(0).contains("'ip'")); + } + } + + @Test + public void ProductMissing_ProcessLogsAndThrowsNothing() throws Exception { + try (IPIntelligenceCloudEngine engine = buildEngine()) { + for (int i = 0; i < 3; i++) { + process(engine); + } + List logged = warningsAndErrors(); + assertEquals(logged.toString(), 1, logged.size()); + } + } + + /** + * Reading a property behaves as for any property the response does not + * carry. + */ + @Test + public void ProductMissing_PropertyIsMissing() throws Exception { + try (IPIntelligenceCloudEngine engine = buildEngine()) { + IPIntelligenceDataCloud data = process(engine); + try { + data.getRegisteredName(); + fail("A PropertyMissingException should have been thrown"); + } catch (PropertyMissingException e) { + assertEquals( + MissingPropertyReason.ProductNotAccessibleWithResourceKey, + e.getReason()); + } + } + } + + /** + * An engine whose Cloud Request Engine reports the accessible products + * of a resource key that includes Device Detection alone. + */ + private IPIntelligenceCloudEngine buildEngine() throws Exception { + IPIntelligenceCloudEngine engine = + new IPIntelligenceCloudEngineBuilder(loggerFactory).build(); + + ProductMetaData product = new ProductMetaData(); + product.dataTier = "test"; + product.properties = Arrays.asList( + new PropertyMetaData("hardwarevendor", "String", "cat", null)); + Map publicProperties = new HashMap<>(); + publicProperties.put("device", product); + + CloudRequestEngine requestEngine = mock(CloudRequestEngine.class); + when(requestEngine.getPublicProperties()).thenReturn(publicProperties); + + Pipeline pipeline = mock(Pipeline.class); + when(pipeline.getElement(CloudRequestEngine.class)) + .thenReturn(requestEngine); + + engine.addPipeline(pipeline); + return engine; + } + + /** + * Process a cloud response that holds nothing for the IP Intelligence + * product. + */ + private IPIntelligenceDataCloud process(IPIntelligenceCloudEngine engine) { + CloudRequestData cloudData = mock(CloudRequestData.class); + when(cloudData.getJsonResponse()).thenReturn( + "{ 'device': { 'hardwarevendor': 'Example' }, " + + "'javascriptProperties': [] }"); + + FlowData flowData = mock(FlowData.class); + when(flowData.getFromElement(any(CloudRequestEngine.class))) + .thenReturn(cloudData); + + IPIntelligenceDataCloud data = new IPIntelligenceDataCloudInternal( + loggerFactory.getLogger( + IPIntelligenceDataCloud.class.getSimpleName()), + flowData, + engine, + MissingPropertyServiceDefault.getInstance()); + engine.processEngine(flowData, data); + return data; + } + + /** + * Every warning and error logged, at any overload. + */ + private List warningsAndErrors() { + List logged = new ArrayList<>(); + for (Invocation invocation : + Mockito.mockingDetails(logger).getInvocations()) { + String level = invocation.getMethod().getName(); + if (level.equals("warn") || level.equals("error")) { + logged.add(level + " " + + Arrays.toString(invocation.getArguments())); + } + } + return Collections.unmodifiableList(logged); + } +} diff --git a/location/geo-location.cloud/pom.xml b/location/geo-location.cloud/pom.xml index 4195dafd5..17679707e 100644 --- a/location/geo-location.cloud/pom.xml +++ b/location/geo-location.cloud/pom.xml @@ -55,5 +55,9 @@ junit junit + + org.mockito + mockito-core + 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 new file mode 100644 index 000000000..66f40b96e --- /dev/null +++ b/location/geo-location.cloud/src/test/java/fiftyone/geolocation/flowelements/GeoLocationCloudEngineTests.java @@ -0,0 +1,184 @@ +/* ********************************************************************* + * 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.geolocation.flowelements; + +import fiftyone.geolocation.core.Enums; +import fiftyone.geolocation.data.CloudGeoData; +import fiftyone.geolocation.data.CloudGeoDataDefault; +import fiftyone.pipeline.cloudrequestengine.data.CloudRequestData; +import fiftyone.pipeline.cloudrequestengine.flowelements.CloudRequestEngine; +import fiftyone.pipeline.core.data.AccessiblePropertyMetaData.ProductMetaData; +import fiftyone.pipeline.core.data.AccessiblePropertyMetaData.PropertyMetaData; +import fiftyone.pipeline.core.data.FlowData; +import fiftyone.pipeline.core.flowelements.Pipeline; +import fiftyone.pipeline.engines.exceptions.PropertyMissingException; +import fiftyone.pipeline.engines.services.MissingPropertyReason; +import fiftyone.pipeline.engines.services.MissingPropertyServiceDefault; +import org.junit.Before; +import org.junit.Test; +import org.mockito.Mockito; +import org.mockito.invocation.Invocation; +import org.slf4j.ILoggerFactory; +import org.slf4j.Logger; + +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collections; +import java.util.HashMap; +import java.util.List; +import java.util.Map; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +/** + * A resource key can be valid and include no product that holds the + * Location properties. The engine then builds with empty property + * metadata, logs the missing product once, and logs or throws nothing on + * each request. + */ +public class GeoLocationCloudEngineTests { + + private Logger logger; + private ILoggerFactory loggerFactory; + + @Before + public void init() { + logger = mock(Logger.class); + loggerFactory = mock(ILoggerFactory.class); + when(loggerFactory.getLogger(anyString())).thenReturn(logger); + } + + @Test + public void ProductMissing_BuildsWithEmptyMetadata() throws Exception { + try (GeoLocationCloudEngine engine = buildEngine()) { + assertNotNull(engine.getProperties()); + assertEquals(0, engine.getProperties().size()); + List logged = warningsAndErrors(); + assertEquals(logged.toString(), 1, logged.size()); + assertTrue(logged.get(0), logged.get(0).contains("'location'")); + } + } + + @Test + public void ProductMissing_ProcessLogsAndThrowsNothing() throws Exception { + try (GeoLocationCloudEngine engine = buildEngine()) { + for (int i = 0; i < 3; i++) { + process(engine); + } + List logged = warningsAndErrors(); + assertEquals(logged.toString(), 1, logged.size()); + } + } + + /** + * Reading a property behaves as for any property the response does not + * carry. + */ + @Test + public void ProductMissing_PropertyIsMissing() throws Exception { + try (GeoLocationCloudEngine engine = buildEngine()) { + CloudGeoData data = process(engine); + try { + data.getTown(); + fail("A PropertyMissingException should have been thrown"); + } catch (PropertyMissingException e) { + assertEquals( + MissingPropertyReason.ProductNotAccessibleWithResourceKey, + e.getReason()); + } + } + } + + /** + * An engine whose Cloud Request Engine reports the accessible products + * of a resource key that includes IP Intelligence alone. + */ + private GeoLocationCloudEngine buildEngine() throws Exception { + GeoLocationCloudEngine engine = + new GeoLocationCloudEngineBuilder(loggerFactory) + .build(Enums.GeoLocationProvider.FiftyOneDegrees); + + ProductMetaData product = new ProductMetaData(); + product.dataTier = "test"; + product.properties = Arrays.asList( + new PropertyMetaData("registeredname", "String", "cat", null)); + Map publicProperties = new HashMap<>(); + publicProperties.put("ip", product); + + CloudRequestEngine requestEngine = mock(CloudRequestEngine.class); + when(requestEngine.getPublicProperties()).thenReturn(publicProperties); + + Pipeline pipeline = mock(Pipeline.class); + when(pipeline.getElement(CloudRequestEngine.class)) + .thenReturn(requestEngine); + + engine.addPipeline(pipeline); + return engine; + } + + /** + * Process a cloud response that holds nothing for the Location + * product. + */ + private CloudGeoData process(GeoLocationCloudEngine engine) { + CloudRequestData cloudData = mock(CloudRequestData.class); + when(cloudData.getJsonResponse()).thenReturn( + "{ 'ip': { 'registeredname': 'Example' }, " + + "'javascriptProperties': [] }"); + + FlowData flowData = mock(FlowData.class); + when(flowData.getFromElement(any(CloudRequestEngine.class))) + .thenReturn(cloudData); + + CloudGeoData data = new CloudGeoDataDefault( + loggerFactory.getLogger(CloudGeoData.class.getSimpleName()), + flowData, + engine, + MissingPropertyServiceDefault.getInstance()); + engine.processEngine(flowData, data); + return data; + } + + /** + * Every warning and error logged, at any overload. + */ + private List warningsAndErrors() { + List logged = new ArrayList<>(); + for (Invocation invocation : + Mockito.mockingDetails(logger).getInvocations()) { + String level = invocation.getMethod().getName(); + if (level.equals("warn") || level.equals("error")) { + logged.add(level + " " + + Arrays.toString(invocation.getArguments())); + } + } + return Collections.unmodifiableList(logged); + } +} 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 514a9369e..f0f4ac5bb 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 @@ -34,6 +34,7 @@ import java.util.List; import java.util.Map; +import fiftyone.common.testhelpers.TestLogger; import fiftyone.common.testhelpers.TestLoggerFactory; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; @@ -57,6 +58,7 @@ import fiftyone.pipeline.engines.data.AspectData; import fiftyone.pipeline.engines.data.AspectDataBase; import fiftyone.pipeline.engines.data.AspectPropertyMetaData; +import fiftyone.pipeline.engines.data.MultiProfileData; import fiftyone.pipeline.engines.flowelements.AspectEngine; import fiftyone.pipeline.engines.flowelements.AspectEngineBase; @@ -107,6 +109,55 @@ protected void processCloudEngine(FlowData data, TestData aspectData, String jso } } + public class TestProfiles extends AspectDataBase + implements MultiProfileData { + private final List profiles = new ArrayList<>(); + + public TestProfiles( + Logger logger, + FlowData flowData, + AspectEngine engine) { + super(logger, flowData, engine); + } + + @Override + public List getProfiles() { + return profiles; + } + + @Override + public void addProfile(TestData profile) { + profiles.add(profile); + } + } + + private class TestKeyedInstance + extends PropertyKeyedCloudEngineBase { + public TestKeyedInstance() { + super(loggerFactory.getLogger("TestKeyedInstance"), new ElementDataFactory() { + @Override + public TestProfiles create(FlowData flowData, FlowElement flowElement) { + return new TestProfiles(null, flowData, (AspectEngine) flowElement); + } + }); + } + + @Override + public String getElementDataKey() { + return "test"; + } + + @Override + protected TestData createProfileData(FlowData flowData) { + return new TestData(null, flowData, this); + } + + @Override + protected void unmanagedResourcesCleanup() { + + } + } + private class TestRequestEngine extends AspectEngineBase implements CloudRequestEngine { @@ -227,31 +278,26 @@ public void loadProperties() throws Exception { } /** - * Test that an exception is thrown by the Properties auto property if - * the cloud request engine returns no properties. - * @throws Exception + * A resource key that includes no products at all includes none for this + * engine, so the Pipeline builds, the engine has empty property metadata + * and the missing product is logged once. + * @throws Exception */ @Test public void loadProperties_noProperties() throws Exception { - try { - createPipeline(); - fail("RuntimeException should be thrown"); - } catch (RuntimeException ex){ - assertEquals( - String.format( - ExceptionFailedToLoadProperties, - "test", - "test"), - ex.getMessage() - ); - } + createPipeline(); + + assertEquals(0, engine.getProperties().size()); + assertProductMissingLoggedOnce(); maxErrors = 1; } - + /** - * Test that an exception is thrown by the Properties auto property if - * the cloud engine only returns properties for other engines. - * @throws Exception + * A resource key whose products hold only other engines' properties + * does not include this engine's product. The Pipeline builds, the engine + * has empty property metadata, and the missing product is logged once + * however often the metadata is read. + * @throws Exception */ @Test public void loadProperties_wrongProperties() throws Exception { @@ -263,6 +309,24 @@ public void loadProperties_wrongProperties() throws Exception { devicePropertyData.properties = properties; propertiesReturnedByRequestEngine.put("test2", devicePropertyData); + createPipeline(); + + for (int i = 0; i < 3; i++) { + assertEquals(0, engine.getProperties().size()); + } + assertProductMissingLoggedOnce(); + maxErrors = 1; + } + + /** + * 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. + * @throws Exception + */ + @Test + public void loadProperties_notLoaded() throws Exception { + propertiesReturnedByRequestEngine = null; try { createPipeline(); fail("RuntimeException should be thrown"); @@ -275,10 +339,59 @@ public void loadProperties_wrongProperties() throws Exception { ex.getMessage() ); } + maxErrors = 1; + } + /** + * A property keyed engine, such as the hardware profile engine, whose + * product the resource key does not include builds, gives no profiles + * for a response that holds nothing for it, and logs nothing on each + * request. + * @throws Exception + */ + @Test + public void propertyKeyed_productMissing_noProfiles() throws Exception { + List properties = new ArrayList<>(); + properties.add(new AccessiblePropertyMetaData.PropertyMetaData("ismobile", "Boolean", null, null)); + AccessiblePropertyMetaData.ProductMetaData otherProduct = new AccessiblePropertyMetaData.ProductMetaData(); + otherProduct.properties = properties; + propertiesReturnedByRequestEngine.put("test2", otherProduct); + TestKeyedInstance keyed = new TestKeyedInstance(); + requestEngine = new TestRequestEngine(); + requestEngine.setPublicProperties(propertiesReturnedByRequestEngine); + pipeline = new PipelineBuilder(loggerFactory) + .addFlowElement(requestEngine) + .addFlowElement(keyed) + .build(); + + for (int i = 0; i < 3; i++) { + TestProfiles profiles = new TestProfiles( + null, + Mockito.mock(FlowData.class), + keyed); + keyed.processCloudEngine( + Mockito.mock(FlowData.class), + profiles, + "{ \"test2\": { \"ismobile\": true } }"); + assertEquals(0, profiles.getProfiles().size()); + } + assertEquals(0, keyed.getProperties().size()); + assertProductMissingLoggedOnce(); maxErrors = 1; } + private void assertProductMissingLoggedOnce() { + List errors = new ArrayList<>(); + List warnings = new ArrayList<>(); + for (TestLogger logger : loggerFactory.loggers) { + errors.addAll(logger.errorsLogged); + warnings.addAll(logger.warningsLogged); + } + assertEquals(1, errors.size(), errors.toString()); + assertTrue(errors.get(0).contains("'test'"), errors.get(0)); + assertEquals(0, warnings.size(), warnings.toString()); + } + /** * Test loading sub-property meta data where a cloud aspect engine * has nested properties. E.g. the cloud property keyed engine. From 50c4c42562d2516e3b95a206cd8d28fd8e2aa05e Mon Sep 17 00:00:00 2001 From: James Rosewell Date: Mon, 9 Nov 2026 20:25:51 +0000 Subject: [PATCH 02/28] Build a cloud engine whose product the resource key lacks, log that once, and process quietly A resource key can be valid and include no product that holds a cloud engine's properties. The Device Detection, IP Intelligence and Location cloud engines then left their property metadata null, logged two errors, and threw on every request when getJSONObject found no part of the response for them. The engines built on CloudAspectEngineBase, such as the hardware profile engines, threw when their metadata was read. CloudAspectEngineBase now has findProduct, which returns the engine's product from the accessible products, or an empty product when the resource key does not include it, logging that once for each engine instance. getElementJson returns the engine's part of the response, or an empty object when the product is missing. The three engines, the base class's own metadata loading and the property keyed base use them, so each builds with empty property metadata, logs one error, and processes a response with nothing for it without logging or throwing. Reading a property then throws PropertyMissingException with ProductNotAccessibleWithResourceKey, as for any property the response does not carry. Accessible products that were never loaded still fail as before. The Location engine's failure message named the Device Detection engine, and now names the Location engine. --- .../DeviceDetectionCloudEngine.java | 14 ++-- .../IPIntelligenceCloudEngine.java | 14 ++-- .../flowelements/GeoLocationCloudEngine.java | 16 ++-- .../cloudrequestengine/Constants.java | 5 ++ .../flowelements/CloudAspectEngineBase.java | 79 +++++++++++++++++-- .../PropertyKeyedCloudEngineBase.java | 17 +++- 6 files changed, 110 insertions(+), 35 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 b6ea1aa59..2e6e2f979 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 @@ -111,7 +111,7 @@ protected void processEngine(FlowData data, DeviceDataCloud aspectData) { // Extract data from json to the aspectData instance. JSONObject jsonObj = new JSONObject(json); - JSONObject deviceObj = jsonObj.getJSONObject("device"); + JSONObject deviceObj = getElementJson(jsonObj); Map deviceMap = new TreeMap<>(String.CASE_INSENSITIVE_ORDER); @@ -205,17 +205,15 @@ public void addPipeline(Pipeline pipeline) { * {@link #aspectProperties} */ private boolean loadAspectProperties(CloudRequestEngine engine) { - Map map = - engine.getPublicProperties(); + AccessiblePropertyMetaData.ProductMetaData product = + findProduct(engine.getPublicProperties()); - if (map != null && - map.size() > 0 && - map.containsKey(getElementDataKey())) { + if (product != null) { aspectProperties = new ArrayList<>(); - dataSourceTier = map.get(getElementDataKey()).dataTier; + dataSourceTier = product.dataTier; for (AccessiblePropertyMetaData.PropertyMetaData item : - map.get(getElementDataKey()).properties) { + product.properties) { AspectPropertyMetaData property = new AspectPropertyMetaDataDefault( item.name, this, 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 ce84df4d8..da5039234 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 @@ -123,7 +123,7 @@ protected void processEngine(FlowData data, IPIntelligenceDataCloud aspectData) // Extract data from json to the aspectData instance. JSONObject jsonObj = new JSONObject(json); - JSONObject ipiObj = jsonObj.getJSONObject("ip"); + JSONObject ipiObj = getElementJson(jsonObj); Map propertyMap = new TreeMap<>(String.CASE_INSENSITIVE_ORDER); @@ -239,18 +239,16 @@ public void addPipeline(Pipeline pipeline) { * {@link #aspectProperties} */ private boolean loadAspectProperties(CloudRequestEngine engine) { - Map map = - engine.getPublicProperties(); + AccessiblePropertyMetaData.ProductMetaData product = + findProduct(engine.getPublicProperties()); - if (map != null && - map.size() > 0 && - map.containsKey(getElementDataKey())) { + if (product != null) { aspectProperties = new ArrayList<>(); weightedPropertyNames.clear(); - dataSourceTier = map.get(getElementDataKey()).dataTier; + dataSourceTier = product.dataTier; for (AccessiblePropertyMetaData.PropertyMetaData item : - map.get(getElementDataKey()).properties) { + product.properties) { if (item.type != null && item.type.startsWith("Weighted")) { weightedPropertyNames.add(item.name.toLowerCase()); } 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 098faac53..7fb48e1cd 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 @@ -107,7 +107,7 @@ protected void processEngine(FlowData data, CloudGeoData aspectData) { String json = requestData.getJsonResponse(); JSONObject jsonObj = new JSONObject(json); - JSONObject geoObj = jsonObj.getJSONObject(getElementDataKey()); + JSONObject geoObj = getElementJson(jsonObj); Map methodData = new TreeMap<>(String.CASE_INSENSITIVE_ORDER); @@ -177,16 +177,14 @@ public void addPipeline(Pipeline pipeline) { } private boolean loadAspectProperties(CloudRequestEngine engine) { - Map map = - engine.getPublicProperties(); + AccessiblePropertyMetaData.ProductMetaData product = + findProduct(engine.getPublicProperties()); - if (map != null && - map.size() > 0 && - map.containsKey(getElementDataKey())) { + if (product != null) { properties = new ArrayList<>(); - dataSourceTier = map.get(getElementDataKey()).dataTier; + dataSourceTier = product.dataTier; - for (AccessiblePropertyMetaData.PropertyMetaData item : map.get(getElementDataKey()).properties) + for (AccessiblePropertyMetaData.PropertyMetaData item : product.properties) { AspectPropertyMetaData property = new AspectPropertyMetaDataDefault( item.name, @@ -205,7 +203,7 @@ private boolean loadAspectProperties(CloudRequestEngine engine) { } else { logger.error("Aspect properties could not be loaded for" + - " the Device Detection cloud engine", this); + " 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 593c618d6..613dcff5a 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,11 @@ 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 MessageProductNotInResourceKey = + "The resource key does not include the '%s' product, so " + + "%s has no properties and its results hold no values. " + + "Use a resource key that includes the product to get them."; + public static final String ProcessCloudEngineNotImplemented = "This method should be overridden in the class derived from " + "CloudAspectEngineBase class. The implementation should use " 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 70f10ea2a..a828a9485 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 @@ -23,6 +23,7 @@ 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; import fiftyone.pipeline.cloudrequestengine.NotImplementedException; @@ -43,12 +44,14 @@ import fiftyone.pipeline.engines.flowelements.CloudAspectEngine; import fiftyone.pipeline.util.Types; +import org.json.JSONObject; import org.slf4j.Logger; import java.util.ArrayList; import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.concurrent.atomic.AtomicBoolean; /** * Base class for 51Degrees Cloud Aspect Engines @@ -109,6 +112,8 @@ public CloudRequestEngine getInstance() throws PipelineConfigurationException { private volatile List aspectProperties; private String dataSourceTier; private RequestEngineAccessor requestEngine; + private volatile boolean productMissing = false; + private final AtomicBoolean productMissingLogged = new AtomicBoolean(false); /** * Construct a new instance of the {@link CloudAspectEngineBase}. @@ -192,17 +197,15 @@ public TypedKey getTypedDataKey() { */ private boolean loadAspectProperties() { CloudRequestEngine requestEngine = getRequestEngine().getInstance(); - Map map = - requestEngine.getPublicProperties(); + AccessiblePropertyMetaData.ProductMetaData product = + findProduct(requestEngine.getPublicProperties()); - if(map != null && - map.size() > 0 && - map.containsKey(this.getElementDataKey())) { + if(product != null) { List properties = new ArrayList<>(); - dataSourceTier = map.get(this.getElementDataKey()).dataTier; + dataSourceTier = product.dataTier; for (AccessiblePropertyMetaData.PropertyMetaData item : - map.get(this.getElementDataKey()).properties) { + product.properties) { AspectPropertyMetaData property = new AspectPropertyMetaDataDefault( item.name, this, @@ -224,6 +227,68 @@ private boolean loadAspectProperties() { } } + /** + * Find this engine's product among the products the Cloud Request Engine + * reports for the resource key. A valid resource key can include no + * product that holds this engine's properties. That is logged once for + * this engine, and an empty product is returned, so the engine builds + * with empty property metadata and logs nothing on each request. + * @param products the products from + * {@link CloudRequestEngine#getPublicProperties()}, or null if they have + * not been loaded + * @return the product, an empty product if the resource key does not + * include it, or null if the products have not been loaded + */ + protected AccessiblePropertyMetaData.ProductMetaData findProduct( + Map products) { + if (products == null) { + return null; + } + AccessiblePropertyMetaData.ProductMetaData product = + products.get(getElementDataKey()); + if (product != null) { + productMissing = false; + return product; + } + productMissing = true; + if (productMissingLogged.compareAndSet(false, true)) { + logger.error(String.format( + MessageProductNotInResourceKey, + getElementDataKey(), + getClass().getSimpleName())); + } + AccessiblePropertyMetaData.ProductMetaData empty = + new AccessiblePropertyMetaData.ProductMetaData(); + empty.properties = new ArrayList<>(); + return empty; + } + + /** + * True when the resource key does not include this engine's product, so + * the cloud response holds nothing for this engine. + * @return true if the product is missing + */ + protected boolean isProductMissing() { + return productMissing; + } + + /** + * This engine's part of the cloud response. When the resource key does + * not include this engine's product the response has no such part, and + * an empty object is returned, so the element data holds no values. + * @param response the whole cloud response + * @return this engine's part of the response + * @throws org.json.JSONException if the product is included and the + * response has no part for this engine + */ + protected JSONObject getElementJson(JSONObject response) { + if (productMissing) { + JSONObject element = response.optJSONObject(getElementDataKey()); + return element == null ? new JSONObject() : element; + } + return response.getJSONObject(getElementDataKey()); + } + private List loadElementProperties( List itemProperties) { List result = null; diff --git a/pipeline/pipeline.cloudrequestengine/src/main/java/fiftyone/pipeline/cloudrequestengine/flowelements/PropertyKeyedCloudEngineBase.java b/pipeline/pipeline.cloudrequestengine/src/main/java/fiftyone/pipeline/cloudrequestengine/flowelements/PropertyKeyedCloudEngineBase.java index 414743370..2f8dbfe08 100644 --- a/pipeline/pipeline.cloudrequestengine/src/main/java/fiftyone/pipeline/cloudrequestengine/flowelements/PropertyKeyedCloudEngineBase.java +++ b/pipeline/pipeline.cloudrequestengine/src/main/java/fiftyone/pipeline/cloudrequestengine/flowelements/PropertyKeyedCloudEngineBase.java @@ -33,6 +33,7 @@ import fiftyone.pipeline.engines.data.AspectPropertyMetaData; import fiftyone.pipeline.engines.data.MultiProfileData; import fiftyone.pipeline.util.Types; +import org.json.JSONArray; import org.json.JSONObject; import org.slf4j.Logger; @@ -71,10 +72,20 @@ protected void processCloudEngine(FlowData data, TData aspectData, String json) // Extract data from json to the aspectData instance. JSONObject map = new JSONObject(json); + // Load the property meta-data, which finds whether the resource key + // includes this engine's product. + getProperties(); // Access the data relating to this engine. - JSONObject propertyKeyed = map.getJSONObject(getElementDataKey()); - // Access the 'Profiles' property - for (Object entry : propertyKeyed.getJSONArray("profiles")) { + JSONObject propertyKeyed = getElementJson(map); + // Access the 'Profiles' property. The response holds none for a + // product the resource key does not include. + JSONArray profiles = isProductMissing() ? + propertyKeyed.optJSONArray("profiles") : + propertyKeyed.getJSONArray("profiles"); + if (profiles == null) { + return; + } + for (Object entry : profiles) { // Iterate through the devices, parsing each one and // adding it to the result. From c72baaceff8b8eb50ed51afc5ffca2f31d7b1871 Mon Sep 17 00:00:00 2001 From: James Rosewell Date: Mon, 9 Nov 2026 20:59:31 +0000 Subject: [PATCH 03/28] 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 37c1ec6d5..32eb7356a 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 df07df779..3249c2ab1 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 66f40b96e..1520a46e9 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 f0f4ac5bb..3ec05bb44 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 000000000..1ae9bb3e6 --- /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 80ebde301..100de4cff 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 04/28] 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 2e6e2f979..e43fc4b24 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 da5039234..bec4510d2 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 7fb48e1cd..243abac97 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 613dcff5a..4ac6fbbbb 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 a828a9485..ee605233e 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 000000000..bfcac3997 --- /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 1025b5799..d3027f2d0 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 bc72731bf..f2da2735e 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 12ccc17f2..65181de0c 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 67439d7cc..630ac847a 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 05/28] 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 000000000..0731e46d8 --- /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 f2da2735e..5e23b4ea4 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 65181de0c..3f1be59b5 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 1ae9bb3e6..742adaffc 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 06/28] 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 3ec05bb44..06f0b8505 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 742adaffc..a5bbc81fa 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 0320d94e5..98d854117 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 07/28] 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 e43fc4b24..400ca1dad 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 bec4510d2..99b84cbb3 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 243abac97..cde6a4932 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 ee605233e..0d83d2b1a 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 5e23b4ea4..b0d420a0b 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 3f1be59b5..0bf8338f9 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 f6300acfb..b9b26c6ae 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 c27716d21..b03878a83 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 79b8e4148..b1a479b75 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 f53cda4ad..302b51d94 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 630ac847a..d51d2cfe9 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 f819bd994..31c8f9d46 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 08/28] 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 cd25d63ac..231fe1502 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 09/28] 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 78d4c29ba..9bb05c6ea 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 10/28] 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 231fe1502..93029030b 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 2f6a4deab6fdc25221bc6f99f2b7e685f16d3269 Mon Sep 17 00:00:00 2001 From: James Rosewell Date: Mon, 9 Nov 2026 21:45:13 +0000 Subject: [PATCH 11/28] Test that the Cloud Request Engine sends only the Evidence the cloud accepts The Processing section of the cloud request engine specification says the engine sends only the Evidence keys the cloud service needs, and never Evidence whose key strips to resource, license or values. Two new tests check the posted request. process_SendsOnlyAcceptedEvidence sends Evidence the service does not list and expects only the listed keys, compared without regard to case. process_LeavesOutEvidenceNamedResourceLicenseOrValues sends Evidence named resource, license and values in several cases, with and without a configured license key, and expects the configured keys once each and nothing else. Existing tests that check the request now answer the evidencekeys request with the keys they send, as the cloud service would. --- .../flowelements/CloudRequestEngineTests.java | 118 +++++++++++++++++- 1 file changed, 117 insertions(+), 1 deletion(-) 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 100de4cff..9f7d8e6d8 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 @@ -33,11 +33,14 @@ import fiftyone.pipeline.core.flowelements.Pipeline; import fiftyone.pipeline.core.flowelements.PipelineBuilder; import fiftyone.pipeline.engines.services.HttpClientDefault; +import org.json.JSONArray; import org.json.JSONObject; 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.junit.jupiter.params.provider.NullSource; +import org.junit.jupiter.params.provider.ValueSource; import org.mockito.ArgumentCaptor; import org.mockito.ArgumentMatchers; @@ -46,7 +49,9 @@ import java.nio.charset.StandardCharsets; import java.util.ArrayList; import java.util.Arrays; +import java.util.Collection; import java.util.HashMap; +import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.stream.Stream; @@ -332,6 +337,7 @@ public void evidencePrecidence(String name, boolean shouldWarn, String evidence1 loggerFactory.loggers.clear(); String[] evidence1Parts = evidence1.split("="); String[] evidence2Parts = evidence2.split("="); + acceptEvidence(Arrays.asList(evidence1Parts[0], evidence2Parts[0])); configureMockedClient(); CloudRequestEngine engine = new CloudRequestEngineBuilder(loggerFactory, httpClient) @@ -452,6 +458,7 @@ private static Stream getFormDataArgs() { @ParameterizedTest(name = "{0}") @MethodSource("getFormDataArgs") public void getFormData(String name, Map evidence, String expectedValue) throws Exception { + acceptEvidence(evidence.keySet()); configureMockedClient(); CloudRequestEngineDefault engine = (CloudRequestEngineDefault) new CloudRequestEngineBuilder(loggerFactory, httpClient) .setResourceKey("resourcekey") @@ -473,10 +480,12 @@ public void getFormData(String name, Map evidence, String expect /** * The web integration adds the whole web request as server.http-request * for usage sharing to read. It holds every header and cookie, so it is - * never sent to the cloud service. + * never sent to the cloud service, even where the service lists its key. */ @Test public void getFormData_LeavesOutTheWebRequest() throws Exception { + acceptEvidence(Arrays.asList( + "header.User-Agent", "server.client-ip", "server.http-request")); configureMockedClient(); CloudRequestEngineDefault engine = (CloudRequestEngineDefault) new CloudRequestEngineBuilder(loggerFactory, httpClient) .setResourceKey("resourcekey") @@ -509,6 +518,7 @@ public void getFormData_LeavesOutTheWebRequest() throws Exception { */ @Test public void getFormData_KeepsDotsInEvidenceName() throws Exception { + acceptEvidence(Arrays.asList("query.id.usage", "header.id.email")); configureMockedClient(); CloudRequestEngineDefault engine = (CloudRequestEngineDefault) new CloudRequestEngineBuilder(loggerFactory, httpClient) @@ -536,6 +546,7 @@ public void getFormData_KeepsDotsInEvidenceName() throws Exception { */ @Test public void process_PostsWholeEvidenceName() throws Exception { + acceptEvidence(Arrays.asList("query.id.usage")); configureMockedClient(); CloudRequestEngine engine = new CloudRequestEngineBuilder(loggerFactory, httpClient) @@ -557,6 +568,111 @@ public void process_PostsWholeEvidenceName() throws Exception { "The posted body does not carry id.usage: " + items); } + /** + * Only the Evidence whose keys the cloud service accepts is sent, with + * the keys compared without regard to case. + * See the Processing section of the cloud request engine specification. + */ + @Test + public void process_SendsOnlyAcceptedEvidence() throws Exception { + acceptEvidence(Arrays.asList( + "query.User-Agent", "header.Accept-Language")); + Map evidence = new HashMap<>(); + evidence.put("query.User-Agent", "iPhone"); + evidence.put("header.accept-language", "en-GB"); + evidence.put("header.X-Other", "other"); + evidence.put("cookie.session", "abc"); + evidence.put("server.client-ip", "1.2.3.4"); + + List items = postedItems(null, evidence); + + assertEquals( + new HashSet<>(Arrays.asList( + "resource=resourcekey", + "User-Agent=iPhone", + "accept-language=en-GB")), + new HashSet<>(items), + "The request carried Evidence the cloud service does not " + + "accept: " + items); + } + + /** + * Evidence whose key strips to resource, license or values is never + * sent, whatever its case and even where the cloud service lists the + * key, so the request carries the configured resource key, and the + * license key where one is configured, once each. + * See the Processing section of the cloud request engine specification. + */ + @ParameterizedTest(name = "license key {0}") + @NullSource + @ValueSource(strings = {"ABCDEFG"}) + public void process_LeavesOutEvidenceNamedResourceLicenseOrValues( + String licenseKey) throws Exception { + acceptEvidence(Arrays.asList( + "query.User-Agent", + "query.resource", + "header.Resource", + "query.license", + "cookie.LICENSE", + "query.values")); + Map evidence = new HashMap<>(); + evidence.put("query.User-Agent", "iPhone"); + evidence.put("query.resource", "evidence-resource"); + evidence.put("header.Resource", "header-resource"); + evidence.put("query.license", "evidence-license"); + evidence.put("cookie.LICENSE", "cookie-license"); + evidence.put("query.values", "ismobile"); + + List items = postedItems(licenseKey, evidence); + + List expected = new ArrayList<>(); + expected.add("resource=resourcekey"); + if (licenseKey != null) { + expected.add("license=" + licenseKey); + } + expected.add("User-Agent=iPhone"); + assertEquals(expected, items); + } + + /** + * Answer the evidencekeys request with the given keys. + * @param keys the Evidence keys the cloud service accepts + */ + private void acceptEvidence(Collection keys) { + evidenceKeysResponse = new JSONArray(keys).toString(); + } + + /** + * Process the Evidence with an engine on the resource key + * {@code resourcekey} and return the fields of the one request it + * posted, in order. + * @param licenseKey the license key, or null for none + * @param evidence the Evidence to process + * @return the posted fields + */ + private List postedItems( + String licenseKey, + Map evidence) throws Exception { + configureMockedClient(); + CloudRequestEngine engine = + new CloudRequestEngineBuilder(loggerFactory, httpClient) + .setResourceKey("resourcekey") + .setLicenseKey(licenseKey) + .build(); + + try (Pipeline pipeline = new PipelineBuilder(loggerFactory) + .addFlowElement(engine).build(); + FlowData data = pipeline.createFlowData()) { + data.addEvidence(evidence); + data.process(); + } + + ArgumentCaptor body = ArgumentCaptor.forClass(byte[].class); + verify(httpClient, times(1)).postData(any(), any(), body.capture()); + return Arrays.asList( + new String(body.getValue(), StandardCharsets.UTF_8).split("&")); + } + /** * Verify that the request to the cloud service will contain * the configured origin header value. From 0155c335bd169b509767e6cd8a71b35204ffec08 Mon Sep 17 00:00:00 2001 From: James Rosewell Date: Mon, 9 Nov 2026 21:46:06 +0000 Subject: [PATCH 12/28] Send only the Evidence the cloud accepts from the Cloud Request Engine The Cloud Request Engine sends only the Evidence whose keys the cloud service accepts, from its evidencekeys request, and never Evidence whose key strips to resource, license or values, compared without regard to case, as the Processing section of the cloud request engine specification says. The request carries the configured resource key, and license key where one is configured, once each. Before, the engine sent all Evidence except the web request. Conflicts between prefixes are now reported only among the Evidence being sent. --- .../CloudRequestEngineDefault.java | 49 +++++++++++++++++-- 1 file changed, 44 insertions(+), 5 deletions(-) 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 0bf8338f9..00d7a00b0 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 @@ -75,6 +75,15 @@ public class CloudRequestEngineDefault */ private static final String HTTP_REQUEST_KEY = "server.http-request"; + /** + * Form fields the request reserves for the credentials and the list of + * Properties. Evidence whose key strips to one of them is never sent, + * as the Processing section of the specification requires. + */ + private static final Set ENGINE_FIELDS = + Collections.unmodifiableSet(new HashSet<>( + Arrays.asList("resource", "license", "values"))); + private HttpClient httpClient; private String endPoint; @@ -454,10 +463,21 @@ private byte[] getContent(FlowData data) throws UnsupportedEncodingException { return string.getBytes(StandardCharsets.UTF_8); } + /** + * The Evidence to send, keyed by the name the cloud service expects. + * Only Evidence that {@link #isSent(String)} allows is included, and + * where two prefixes give the same name the precedence order decides. + * @param flowData the FlowData + * @return the form fields to send + */ Map getFormData(FlowData flowData) { - Map evidence = new HashMap<>( - flowData.getEvidence().asKeyMap()); - evidence.keySet().removeIf(HTTP_REQUEST_KEY::equalsIgnoreCase); + Map evidence = new HashMap<>(); + for (Map.Entry entry : + flowData.getEvidence().asKeyMap().entrySet()) { + if (isSent(entry.getKey())) { + evidence.put(entry.getKey(), entry.getValue()); + } + } Map formData = new HashMap<>(); // Add evidence in reverse alphabetical order, excluding special keys. @@ -472,10 +492,29 @@ Map getFormData(FlowData flowData) { return formData; } + /** + * Whether Evidence is sent to the cloud service. Only keys the service + * accepts are sent, never one that strips to a name in + * {@link #ENGINE_FIELDS}, compared without regard to case, and never + * the web request. + * @param key the Evidence key + * @return true if the Evidence is sent + */ + private boolean isSent(String key) { + if (HTTP_REQUEST_KEY.equalsIgnoreCase(key) || + evidenceKeyFilter.include(key) == false) { + return false; + } + String[] parts = key.split( + Pattern.quote(Constants.EVIDENCE_SEPERATOR), 2); + return parts.length == 2 && + ENGINE_FIELDS.contains(parts[1].toLowerCase(Locale.ROOT)) == false; + } + /** * Add form data to the evidence. * @param formData the destination map to add the data to - * @param allEvidence all evidence in the FlowData. This is used to + * @param allEvidence all the Evidence being sent. This is used to * report which evidence keys are conflicting * @param evidence evidence to add to the form data */ @@ -537,7 +576,7 @@ private void addFormData( /** * Get evidence with specified prefix. - * @param evidence all evidence in the FlowData + * @param evidence the Evidence being sent * @param type required evidence key prefix * @return evidence with the required key prefix */ From f600e1df641176b69088ca3990c678dfe9ddf04f Mon Sep 17 00:00:00 2001 From: James Rosewell Date: Mon, 9 Nov 2026 22:00:40 +0000 Subject: [PATCH 13/28] Mark the mocked product map unchecked, as the build treats the varargs warning as an error --- .../devicedetection/cloud/flowelements/ProductMissingTests.java | 2 ++ 1 file changed, 2 insertions(+) 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 37c1ec6d5..3c9ef7c1f 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 @@ -118,6 +118,8 @@ public void ProductMissing_PropertyIsMissing() throws Exception { * An engine whose Cloud Request Engine reports the accessible products * of a resource key that includes IP Intelligence alone. */ + // thenReturn has a varargs overload, so a generic Map is an unchecked array. + @SuppressWarnings("unchecked") private DeviceDetectionCloudEngine buildEngine() throws Exception { DeviceDetectionCloudEngine engine = new DeviceDetectionCloudEngineBuilder(loggerFactory).build(); From efbeed089a843905b5e4c9d318cedae67878d7bd Mon Sep 17 00:00:00 2001 From: James Rosewell Date: Mon, 9 Nov 2026 22:00:43 +0000 Subject: [PATCH 14/28] Mark the mocked product map unchecked, as the build treats the varargs warning as an error --- .../ipintelligence/cloud/flowelements/ProductMissingTests.java | 2 ++ 1 file changed, 2 insertions(+) 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 df07df779..3ee13582f 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 @@ -118,6 +118,8 @@ public void ProductMissing_PropertyIsMissing() throws Exception { * An engine whose Cloud Request Engine reports the accessible products * of a resource key that includes Device Detection alone. */ + // thenReturn has a varargs overload, so a generic Map is an unchecked array. + @SuppressWarnings("unchecked") private IPIntelligenceCloudEngine buildEngine() throws Exception { IPIntelligenceCloudEngine engine = new IPIntelligenceCloudEngineBuilder(loggerFactory).build(); From 33a13fe06418f5154b436713be7bda43e0acf2f9 Mon Sep 17 00:00:00 2001 From: James Rosewell Date: Mon, 9 Nov 2026 22:00:46 +0000 Subject: [PATCH 15/28] Mark the mocked product map unchecked, as the build treats the varargs warning as an error --- .../geolocation/flowelements/GeoLocationCloudEngineTests.java | 2 ++ 1 file changed, 2 insertions(+) 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 66f40b96e..80dadab55 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 @@ -120,6 +120,8 @@ public void ProductMissing_PropertyIsMissing() throws Exception { * An engine whose Cloud Request Engine reports the accessible products * of a resource key that includes IP Intelligence alone. */ + // thenReturn has a varargs overload, so a generic Map is an unchecked array. + @SuppressWarnings("unchecked") private GeoLocationCloudEngine buildEngine() throws Exception { GeoLocationCloudEngine engine = new GeoLocationCloudEngineBuilder(loggerFactory) From 8f709b4bccdfa707c6a78ddf90a0637177c7561b Mon Sep 17 00:00:00 2001 From: James Rosewell Date: Mon, 9 Nov 2026 22:24:30 +0000 Subject: [PATCH 16/28] 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 93133bd52..8528c6367 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 eed28895f..53be6a7bf 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 3bd56083d..8c36cc1d3 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 17/28] 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 a5bbc81fa..654273c61 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 18/28] 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 b0d420a0b..5530411db 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 0bf8338f9..5c95377f6 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 c2a5e9c9baf8bd90638e5f6c5ffa3b1c674bfc33 Mon Sep 17 00:00:00 2001 From: James Rosewell Date: Mon, 9 Nov 2026 22:54:49 +0000 Subject: [PATCH 19/28] Test that the Cloud Request Engine never sends Evidence named licence The Processing section of the cloud request engine specification now also leaves out Evidence whose key strips to licence, because the cloud service reads the license key from licence where license is absent. process_LeavesOutEvidenceNamedLicence lists query.licence and cookie.LICENCE as accepted, adds Evidence under each, and expects only the configured keys and User-Agent, with and without a license key. It is written to fail on the previous commit, which sends both as licence and LICENCE. --- .../flowelements/CloudRequestEngineTests.java | 31 +++++++++++++++++++ 1 file changed, 31 insertions(+) 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 9f7d8e6d8..ed99e5150 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 @@ -634,6 +634,37 @@ public void process_LeavesOutEvidenceNamedResourceLicenseOrValues( assertEquals(expected, items); } + /** + * Evidence whose key strips to licence, whatever its case, is never + * sent, because the cloud service reads the license key from licence + * where license is absent. + * See the Processing section of the cloud request engine specification. + */ + @ParameterizedTest(name = "license key {0}") + @NullSource + @ValueSource(strings = {"ABCDEFG"}) + public void process_LeavesOutEvidenceNamedLicence( + String licenseKey) throws Exception { + acceptEvidence(Arrays.asList( + "query.User-Agent", + "query.licence", + "cookie.LICENCE")); + Map evidence = new HashMap<>(); + evidence.put("query.User-Agent", "iPhone"); + evidence.put("query.licence", "evidence-licence"); + evidence.put("cookie.LICENCE", "cookie-licence"); + + List items = postedItems(licenseKey, evidence); + + List expected = new ArrayList<>(); + expected.add("resource=resourcekey"); + if (licenseKey != null) { + expected.add("license=" + licenseKey); + } + expected.add("User-Agent=iPhone"); + assertEquals(expected, items); + } + /** * Answer the evidencekeys request with the given keys. * @param keys the Evidence keys the cloud service accepts From 97a73df54c7edc4b66a83a47efce91214436050e Mon Sep 17 00:00:00 2001 From: James Rosewell Date: Mon, 9 Nov 2026 22:55:01 +0000 Subject: [PATCH 20/28] Never send Evidence named licence from the Cloud Request Engine Evidence whose key strips to licence, compared without regard to case, is now left out of the cloud request along with resource, license and values, as the Processing section of the cloud request engine specification says, because the cloud service reads the license key from licence where license is absent. --- .../flowelements/CloudRequestEngineDefault.java | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) 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 349d8d9a3..8843afb63 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 @@ -77,12 +77,14 @@ public class CloudRequestEngineDefault /** * Form fields the request reserves for the credentials and the list of - * Properties. Evidence whose key strips to one of them is never sent, - * as the Processing section of the specification requires. + * Properties, with licence, which the cloud service reads as the + * license key where license is absent. Evidence whose key strips to one + * of them is never sent, as the Processing section of the + * specification requires. */ private static final Set ENGINE_FIELDS = Collections.unmodifiableSet(new HashSet<>( - Arrays.asList("resource", "license", "values"))); + Arrays.asList("resource", "license", "licence", "values"))); private HttpClient httpClient; From 726204bf7b2b3873584a6c604ba82828ad5d8b5e Mon Sep 17 00:00:00 2001 From: James Rosewell Date: Mon, 9 Nov 2026 23:38:03 +0000 Subject: [PATCH 21/28] 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 5530411db..b0d420a0b 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 5c95377f6..0bf8338f9 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 22/28] 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 654273c61..a5bbc81fa 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 dba6b9a038897f26dec6eb1394a67c0f8de824e6 Mon Sep 17 00:00:00 2001 From: James Rosewell Date: Tue, 10 Nov 2026 00:06:24 +0000 Subject: [PATCH 23/28] Test that a missing product is a warning and a missing response part no error The Start-up activity section of the cloud aspect engine specification on rc says the engine "MUST log the missing product once per Cloud Aspect Engine instance, at warning level, naming its Data Key". The Processing section says "Where the response JSON has no portion for the Engine's Data Key, the Cloud Aspect Engine leaves the Element Data without values and raises no error." The missing product tests now expect one warning and no error. ProductPresent_ResponseWithoutPart_NoValues, for each of the Device Detection, IP Intelligence and Location cloud engines, includes the engine's product and processes a response with no part for it, and expects no exception and a property without a value. propertyKeyed_productPresent_noProfiles does the same for a property keyed engine, with a response that has no part for it and one whose part has no profiles. All are written to fail on the previous commit, which logs an error and throws on a missing part when the product is present. --- .../flowelements/ProductMissingTests.java | 61 +++++++++++++++---- .../flowelements/ProductMissingTests.java | 61 +++++++++++++++---- .../GeoLocationCloudEngineTests.java | 61 +++++++++++++++---- .../CloudAspectEngineBaseTests.java | 50 +++++++++++++-- 4 files changed, 194 insertions(+), 39 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 3c9ef7c1f..a9562e0fb 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 @@ -47,6 +47,7 @@ import java.util.Map; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; @@ -80,6 +81,7 @@ public void ProductMissing_BuildsWithEmptyMetadata() throws Exception { assertEquals(0, engine.getProperties().size()); List logged = warningsAndErrors(); assertEquals(logged.toString(), 1, logged.size()); + assertTrue(logged.get(0), logged.get(0).startsWith("warn ")); assertTrue(logged.get(0), logged.get(0).contains("'device'")); } } @@ -95,6 +97,24 @@ public void ProductMissing_ProcessLogsAndThrowsNothing() throws Exception { } } + /** + * With the Device Detection product included, a response that holds no + * part for it leaves the element data without values and throws + * nothing, as the Processing section of the cloud aspect engine + * specification says. + */ + @Test + public void ProductPresent_ResponseWithoutPart_NoValues() + throws Exception { + try (DeviceDetectionCloudEngine engine = buildEngine("device", "hardwarevendor")) { + DeviceDataCloud data = process( + engine, + "{ 'ip': { 'registeredname': 'Example' }, " + + "'javascriptProperties': [] }"); + assertFalse(data.getHardwareVendor().hasValue()); + } + } + /** * Reading a property behaves as for any property the response does not * carry. @@ -115,21 +135,23 @@ public void ProductMissing_PropertyIsMissing() throws Exception { } /** - * An engine whose Cloud Request Engine reports the accessible products - * of a resource key that includes IP Intelligence alone. + * An engine whose Cloud Request Engine reports one product, holding + * one string property. */ // thenReturn has a varargs overload, so a generic Map is an unchecked array. @SuppressWarnings("unchecked") - private DeviceDetectionCloudEngine buildEngine() throws Exception { + private DeviceDetectionCloudEngine buildEngine( + String productKey, + String property) throws Exception { DeviceDetectionCloudEngine engine = new DeviceDetectionCloudEngineBuilder(loggerFactory).build(); ProductMetaData product = new ProductMetaData(); product.dataTier = "test"; product.properties = Arrays.asList( - new PropertyMetaData("registeredname", "String", "cat", null)); + new PropertyMetaData(property, "String", "cat", null)); Map publicProperties = new HashMap<>(); - publicProperties.put("ip", product); + publicProperties.put(productKey, product); CloudRequestEngine requestEngine = mock(CloudRequestEngine.class); when(requestEngine.getPublicProperties()).thenReturn(publicProperties); @@ -143,14 +165,13 @@ private DeviceDetectionCloudEngine buildEngine() throws Exception { } /** - * Process a cloud response that holds nothing for the Device Detection - * product. + * Process the given cloud response. */ - private DeviceDataCloud process(DeviceDetectionCloudEngine engine) { + private DeviceDataCloud process( + DeviceDetectionCloudEngine engine, + String response) { CloudRequestData cloudData = mock(CloudRequestData.class); - when(cloudData.getJsonResponse()).thenReturn( - "{ 'ip': { 'registeredname': 'Example' }, " + - "'javascriptProperties': [] }"); + when(cloudData.getJsonResponse()).thenReturn(response); FlowData flowData = mock(FlowData.class); when(flowData.getFromElement(any(CloudRequestEngine.class))) @@ -165,6 +186,24 @@ private DeviceDataCloud process(DeviceDetectionCloudEngine engine) { return device; } + /** + * An engine on a resource key that includes IP Intelligence alone. + */ + private DeviceDetectionCloudEngine buildEngine() throws Exception { + return buildEngine("ip", "registeredname"); + } + + /** + * Process a cloud response that holds nothing for the Device Detection + * product. + */ + private DeviceDataCloud process(DeviceDetectionCloudEngine engine) { + return process( + engine, + "{ 'ip': { 'registeredname': 'Example' }, " + + "'javascriptProperties': [] }"); + } + /** * Every warning and error logged, at any overload. */ 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 3ee13582f..88aa35083 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 @@ -47,6 +47,7 @@ import java.util.Map; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; @@ -80,6 +81,7 @@ public void ProductMissing_BuildsWithEmptyMetadata() throws Exception { assertEquals(0, engine.getProperties().size()); List logged = warningsAndErrors(); assertEquals(logged.toString(), 1, logged.size()); + assertTrue(logged.get(0), logged.get(0).startsWith("warn ")); assertTrue(logged.get(0), logged.get(0).contains("'ip'")); } } @@ -95,6 +97,24 @@ public void ProductMissing_ProcessLogsAndThrowsNothing() throws Exception { } } + /** + * With the IP Intelligence product included, a response that holds no + * part for it leaves the element data without values and throws + * nothing, as the Processing section of the cloud aspect engine + * specification says. + */ + @Test + public void ProductPresent_ResponseWithoutPart_NoValues() + throws Exception { + try (IPIntelligenceCloudEngine engine = buildEngine("ip", "registeredname")) { + IPIntelligenceDataCloud data = process( + engine, + "{ 'device': { 'hardwarevendor': 'Example' }, " + + "'javascriptProperties': [] }"); + assertFalse(data.getRegisteredName().hasValue()); + } + } + /** * Reading a property behaves as for any property the response does not * carry. @@ -115,21 +135,23 @@ public void ProductMissing_PropertyIsMissing() throws Exception { } /** - * An engine whose Cloud Request Engine reports the accessible products - * of a resource key that includes Device Detection alone. + * An engine whose Cloud Request Engine reports one product, holding + * one string property. */ // thenReturn has a varargs overload, so a generic Map is an unchecked array. @SuppressWarnings("unchecked") - private IPIntelligenceCloudEngine buildEngine() throws Exception { + private IPIntelligenceCloudEngine buildEngine( + String productKey, + String property) throws Exception { IPIntelligenceCloudEngine engine = new IPIntelligenceCloudEngineBuilder(loggerFactory).build(); ProductMetaData product = new ProductMetaData(); product.dataTier = "test"; product.properties = Arrays.asList( - new PropertyMetaData("hardwarevendor", "String", "cat", null)); + new PropertyMetaData(property, "String", "cat", null)); Map publicProperties = new HashMap<>(); - publicProperties.put("device", product); + publicProperties.put(productKey, product); CloudRequestEngine requestEngine = mock(CloudRequestEngine.class); when(requestEngine.getPublicProperties()).thenReturn(publicProperties); @@ -143,14 +165,13 @@ private IPIntelligenceCloudEngine buildEngine() throws Exception { } /** - * Process a cloud response that holds nothing for the IP Intelligence - * product. + * Process the given cloud response. */ - private IPIntelligenceDataCloud process(IPIntelligenceCloudEngine engine) { + private IPIntelligenceDataCloud process( + IPIntelligenceCloudEngine engine, + String response) { CloudRequestData cloudData = mock(CloudRequestData.class); - when(cloudData.getJsonResponse()).thenReturn( - "{ 'device': { 'hardwarevendor': 'Example' }, " + - "'javascriptProperties': [] }"); + when(cloudData.getJsonResponse()).thenReturn(response); FlowData flowData = mock(FlowData.class); when(flowData.getFromElement(any(CloudRequestEngine.class))) @@ -166,6 +187,24 @@ private IPIntelligenceDataCloud process(IPIntelligenceCloudEngine engine) { return data; } + /** + * An engine on a resource key that includes Device Detection alone. + */ + private IPIntelligenceCloudEngine buildEngine() throws Exception { + return buildEngine("device", "hardwarevendor"); + } + + /** + * Process a cloud response that holds nothing for the IP Intelligence + * product. + */ + private IPIntelligenceDataCloud process(IPIntelligenceCloudEngine engine) { + return process( + engine, + "{ 'device': { 'hardwarevendor': 'Example' }, " + + "'javascriptProperties': [] }"); + } + /** * Every warning and error logged, at any overload. */ 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 80dadab55..4bed66b42 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 @@ -49,6 +49,7 @@ import java.util.Map; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; @@ -82,6 +83,7 @@ public void ProductMissing_BuildsWithEmptyMetadata() throws Exception { assertEquals(0, engine.getProperties().size()); List logged = warningsAndErrors(); assertEquals(logged.toString(), 1, logged.size()); + assertTrue(logged.get(0), logged.get(0).startsWith("warn ")); assertTrue(logged.get(0), logged.get(0).contains("'location'")); } } @@ -97,6 +99,24 @@ public void ProductMissing_ProcessLogsAndThrowsNothing() throws Exception { } } + /** + * With the Location product included, a response that holds no + * part for it leaves the element data without values and throws + * nothing, as the Processing section of the cloud aspect engine + * specification says. + */ + @Test + public void ProductPresent_ResponseWithoutPart_NoValues() + throws Exception { + try (GeoLocationCloudEngine engine = buildEngine("location", "town")) { + CloudGeoData data = process( + engine, + "{ 'ip': { 'registeredname': 'Example' }, " + + "'javascriptProperties': [] }"); + assertFalse(data.getTown().hasValue()); + } + } + /** * Reading a property behaves as for any property the response does not * carry. @@ -117,12 +137,14 @@ public void ProductMissing_PropertyIsMissing() throws Exception { } /** - * An engine whose Cloud Request Engine reports the accessible products - * of a resource key that includes IP Intelligence alone. + * An engine whose Cloud Request Engine reports one product, holding + * one string property. */ // thenReturn has a varargs overload, so a generic Map is an unchecked array. @SuppressWarnings("unchecked") - private GeoLocationCloudEngine buildEngine() throws Exception { + private GeoLocationCloudEngine buildEngine( + String productKey, + String property) throws Exception { GeoLocationCloudEngine engine = new GeoLocationCloudEngineBuilder(loggerFactory) .build(Enums.GeoLocationProvider.FiftyOneDegrees); @@ -130,9 +152,9 @@ private GeoLocationCloudEngine buildEngine() throws Exception { ProductMetaData product = new ProductMetaData(); product.dataTier = "test"; product.properties = Arrays.asList( - new PropertyMetaData("registeredname", "String", "cat", null)); + new PropertyMetaData(property, "String", "cat", null)); Map publicProperties = new HashMap<>(); - publicProperties.put("ip", product); + publicProperties.put(productKey, product); CloudRequestEngine requestEngine = mock(CloudRequestEngine.class); when(requestEngine.getPublicProperties()).thenReturn(publicProperties); @@ -146,14 +168,13 @@ private GeoLocationCloudEngine buildEngine() throws Exception { } /** - * Process a cloud response that holds nothing for the Location - * product. + * Process the given cloud response. */ - private CloudGeoData process(GeoLocationCloudEngine engine) { + private CloudGeoData process( + GeoLocationCloudEngine engine, + String response) { CloudRequestData cloudData = mock(CloudRequestData.class); - when(cloudData.getJsonResponse()).thenReturn( - "{ 'ip': { 'registeredname': 'Example' }, " + - "'javascriptProperties': [] }"); + when(cloudData.getJsonResponse()).thenReturn(response); FlowData flowData = mock(FlowData.class); when(flowData.getFromElement(any(CloudRequestEngine.class))) @@ -168,6 +189,24 @@ private CloudGeoData process(GeoLocationCloudEngine engine) { return data; } + /** + * An engine on a resource key that includes IP Intelligence alone. + */ + private GeoLocationCloudEngine buildEngine() throws Exception { + return buildEngine("ip", "registeredname"); + } + + /** + * Process a cloud response that holds nothing for the Location + * product. + */ + private CloudGeoData process(GeoLocationCloudEngine engine) { + return process( + engine, + "{ 'ip': { 'registeredname': 'Example' }, " + + "'javascriptProperties': [] }"); + } + /** * Every warning and error logged, at any overload. */ 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 f0f4ac5bb..6eba9dc3d 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 @@ -30,6 +30,7 @@ import static org.mockito.Mockito.verify; import java.util.ArrayList; +import java.util.Arrays; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -289,7 +290,7 @@ public void loadProperties_noProperties() throws Exception { assertEquals(0, engine.getProperties().size()); assertProductMissingLoggedOnce(); - maxErrors = 1; + maxWarnings = 1; } /** @@ -315,7 +316,7 @@ public void loadProperties_wrongProperties() throws Exception { assertEquals(0, engine.getProperties().size()); } assertProductMissingLoggedOnce(); - maxErrors = 1; + maxWarnings = 1; } /** @@ -377,7 +378,44 @@ public void propertyKeyed_productMissing_noProfiles() throws Exception { } assertEquals(0, keyed.getProperties().size()); assertProductMissingLoggedOnce(); - maxErrors = 1; + maxWarnings = 1; + } + + /** + * A property keyed engine whose product the resource key includes gives + * no profiles, and throws nothing, for a response that holds no part + * for it or a part without profiles, as the Processing section of the + * cloud aspect engine specification says. + * @throws Exception + */ + @Test + public void propertyKeyed_productPresent_noProfiles() throws Exception { + List properties = new ArrayList<>(); + properties.add(new AccessiblePropertyMetaData.PropertyMetaData("ismobile", "Boolean", null, null)); + AccessiblePropertyMetaData.ProductMetaData product = new AccessiblePropertyMetaData.ProductMetaData(); + product.properties = properties; + propertiesReturnedByRequestEngine.put("test", product); + TestKeyedInstance keyed = new TestKeyedInstance(); + requestEngine = new TestRequestEngine(); + requestEngine.setPublicProperties(propertiesReturnedByRequestEngine); + pipeline = new PipelineBuilder(loggerFactory) + .addFlowElement(requestEngine) + .addFlowElement(keyed) + .build(); + + for (String response : Arrays.asList( + "{ \"test2\": { \"ismobile\": true } }", + "{ \"test\": { } }")) { + TestProfiles profiles = new TestProfiles( + null, + Mockito.mock(FlowData.class), + keyed); + keyed.processCloudEngine( + Mockito.mock(FlowData.class), + profiles, + response); + assertEquals(0, profiles.getProfiles().size(), response); + } } private void assertProductMissingLoggedOnce() { @@ -387,9 +425,9 @@ private void assertProductMissingLoggedOnce() { errors.addAll(logger.errorsLogged); warnings.addAll(logger.warningsLogged); } - assertEquals(1, errors.size(), errors.toString()); - assertTrue(errors.get(0).contains("'test'"), errors.get(0)); - assertEquals(0, warnings.size(), warnings.toString()); + assertEquals(1, warnings.size(), warnings.toString()); + assertTrue(warnings.get(0).contains("'test'"), warnings.get(0)); + assertEquals(0, errors.size(), errors.toString()); } /** From 5e2553c7ccd59650010a9af294146c7161ef459a Mon Sep 17 00:00:00 2001 From: James Rosewell Date: Tue, 10 Nov 2026 00:07:11 +0000 Subject: [PATCH 24/28] Log a missing product as a warning, and treat a missing response part as no values The Start-up activity section of the cloud aspect engine specification on rc says the missing product is logged "once per Cloud Aspect Engine instance, at warning level, naming its Data Key". findProduct now logs it with logger.warn. The Processing section says "Where the response JSON has no portion for the Engine's Data Key, the Cloud Aspect Engine leaves the Element Data without values and raises no error." getElementJson now returns an empty object whenever the response has no part for the engine, whether or not the product is included, and PropertyKeyedCloudEngineBase reads profiles with optJSONArray, so a part without profiles gives none. The productMissing flag and isProductMissing, which only chose between those two behaviours, are removed. --- .../flowelements/CloudAspectEngineBase.java | 33 +++++-------------- .../PropertyKeyedCloudEngineBase.java | 11 ++----- 2 files changed, 12 insertions(+), 32 deletions(-) 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 a828a9485..a448b3f10 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 @@ -112,7 +112,6 @@ public CloudRequestEngine getInstance() throws PipelineConfigurationException { private volatile List aspectProperties; private String dataSourceTier; private RequestEngineAccessor requestEngine; - private volatile boolean productMissing = false; private final AtomicBoolean productMissingLogged = new AtomicBoolean(false); /** @@ -231,7 +230,8 @@ private boolean loadAspectProperties() { * Find this engine's product among the products the Cloud Request Engine * reports for the resource key. A valid resource key can include no * product that holds this engine's properties. That is logged once for - * this engine, and an empty product is returned, so the engine builds + * this engine, as a warning naming its Data Key, and an empty product is + * returned, so the engine builds * with empty property metadata and logs nothing on each request. * @param products the products from * {@link CloudRequestEngine#getPublicProperties()}, or null if they have @@ -247,12 +247,10 @@ protected AccessiblePropertyMetaData.ProductMetaData findProduct( AccessiblePropertyMetaData.ProductMetaData product = products.get(getElementDataKey()); if (product != null) { - productMissing = false; return product; } - productMissing = true; if (productMissingLogged.compareAndSet(false, true)) { - logger.error(String.format( + logger.warn(String.format( MessageProductNotInResourceKey, getElementDataKey(), getClass().getSimpleName())); @@ -264,29 +262,16 @@ protected AccessiblePropertyMetaData.ProductMetaData findProduct( } /** - * True when the resource key does not include this engine's product, so - * the cloud response holds nothing for this engine. - * @return true if the product is missing - */ - protected boolean isProductMissing() { - return productMissing; - } - - /** - * This engine's part of the cloud response. When the resource key does - * not include this engine's product the response has no such part, and - * an empty object is returned, so the element data holds no values. + * This engine's part of the cloud response. Where the response has no + * part for this engine, as when the resource key does not include its + * product, an empty object is returned, so the element data holds no + * values and no error is raised. * @param response the whole cloud response * @return this engine's part of the response - * @throws org.json.JSONException if the product is included and the - * response has no part for this engine */ protected JSONObject getElementJson(JSONObject response) { - if (productMissing) { - JSONObject element = response.optJSONObject(getElementDataKey()); - return element == null ? new JSONObject() : element; - } - return response.getJSONObject(getElementDataKey()); + JSONObject element = response.optJSONObject(getElementDataKey()); + return element == null ? new JSONObject() : element; } private List loadElementProperties( diff --git a/pipeline/pipeline.cloudrequestengine/src/main/java/fiftyone/pipeline/cloudrequestengine/flowelements/PropertyKeyedCloudEngineBase.java b/pipeline/pipeline.cloudrequestengine/src/main/java/fiftyone/pipeline/cloudrequestengine/flowelements/PropertyKeyedCloudEngineBase.java index 2f8dbfe08..7d8a19821 100644 --- a/pipeline/pipeline.cloudrequestengine/src/main/java/fiftyone/pipeline/cloudrequestengine/flowelements/PropertyKeyedCloudEngineBase.java +++ b/pipeline/pipeline.cloudrequestengine/src/main/java/fiftyone/pipeline/cloudrequestengine/flowelements/PropertyKeyedCloudEngineBase.java @@ -72,16 +72,11 @@ protected void processCloudEngine(FlowData data, TData aspectData, String json) // Extract data from json to the aspectData instance. JSONObject map = new JSONObject(json); - // Load the property meta-data, which finds whether the resource key - // includes this engine's product. - getProperties(); // Access the data relating to this engine. JSONObject propertyKeyed = getElementJson(map); - // Access the 'Profiles' property. The response holds none for a - // product the resource key does not include. - JSONArray profiles = isProductMissing() ? - propertyKeyed.optJSONArray("profiles") : - propertyKeyed.getJSONArray("profiles"); + // Access the 'Profiles' property. A response without it, as for a + // product the resource key does not include, gives no profiles. + JSONArray profiles = propertyKeyed.optJSONArray("profiles"); if (profiles == null) { return; } From 1e1dae8a52ff37cb8dd3f7368814c1220dfd17bc Mon Sep 17 00:00:00 2001 From: James Rosewell Date: Tue, 10 Nov 2026 00:25:06 +0000 Subject: [PATCH 25/28] 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 a5bbc81fa..07189b21d 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 26/28] 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 07189b21d..aeadfb1ae 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 27/28] 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 4ac6fbbbb..e957273ee 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 0bf8338f9..d0ecc75ea 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 : From 9fced9b3d7633865b81cdc9130d1fb5e3aa0a944 Mon Sep 17 00:00:00 2001 From: James Rosewell Date: Tue, 10 Nov 2026 10:02:08 +0000 Subject: [PATCH 28/28] Rewrap the findProduct Javadoc paragraph A comment-only change. The paragraph that describes findProduct left "returned, so the engine builds" on a short line of its own, so the paragraph is now wrapped at 80 columns. No code changes. --- .../flowelements/CloudAspectEngineBase.java | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) 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 a448b3f10..9b57c0368 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 @@ -228,11 +228,11 @@ private boolean loadAspectProperties() { /** * Find this engine's product among the products the Cloud Request Engine - * reports for the resource key. A valid resource key can include no - * product that holds this engine's properties. That is logged once for - * this engine, as a warning naming its Data Key, and an empty product is - * returned, so the engine builds - * with empty property metadata and logs nothing on each request. + * reports for the resource key. A valid resource key can include no product + * that holds this engine's properties. That is logged once for this engine, + * as a warning naming its Data Key, and an empty product is returned, so + * the engine builds with empty property metadata and logs nothing on each + * request. * @param products the products from * {@link CloudRequestEngine#getPublicProperties()}, or null if they have * not been loaded