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..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 @@ -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,37 @@ 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; + } + + /** + * 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 @@ -111,7 +140,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); @@ -188,9 +217,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); } } @@ -205,17 +235,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())) { - aspectProperties = new ArrayList<>(); - dataSourceTier = map.get(getElementDataKey()).dataTier; + if (product != null) { + List properties = new ArrayList<>(); + dataSourceTier = product.dataTier; for (AccessiblePropertyMetaData.PropertyMetaData item : - map.get(getElementDataKey()).properties) { + product.properties) { AspectPropertyMetaData property = new AspectPropertyMetaDataDefault( item.name, this, @@ -223,13 +251,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/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..2cd708eb6 --- /dev/null +++ b/device-detection/device-detection.cloud/src/test/java/fiftyone/devicedetection/cloud/flowelements/ProductMissingTests.java @@ -0,0 +1,255 @@ +/* ********************************************************************* + * 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.assertFalse; +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).startsWith("warn ")); + 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()); + } + } + + /** + * 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. + */ + @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()); + } + } + } + + /** + * 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) + .thenReturn(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 one product, holding + * one string property. + */ + // thenReturn has a varargs overload, so a generic Map is an unchecked array. + @SuppressWarnings("unchecked") + 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(property, "String", "cat", null)); + Map publicProperties = new HashMap<>(); + publicProperties.put(productKey, 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 the given cloud response. + */ + private DeviceDataCloud process( + DeviceDetectionCloudEngine engine, + String response) { + CloudRequestData cloudData = mock(CloudRequestData.class); + when(cloudData.getJsonResponse()).thenReturn(response); + + 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; + } + + /** + * 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. + */ + 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/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..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 @@ -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,37 @@ 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; + } + + /** + * 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 @@ -123,7 +152,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); @@ -222,9 +251,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); } } @@ -239,18 +269,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())) { - aspectProperties = new ArrayList<>(); + if (product != null) { + List properties = 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()); } @@ -261,13 +289,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/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..a597f4662 --- /dev/null +++ b/ip-intelligence/ip-intelligence.cloud/src/test/java/fiftyone/ipintelligence/cloud/flowelements/ProductMissingTests.java @@ -0,0 +1,256 @@ +/* ********************************************************************* + * 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.assertFalse; +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).startsWith("warn ")); + 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()); + } + } + + /** + * 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. + */ + @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()); + } + } + } + + /** + * 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) + .thenReturn(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 one product, holding + * one string property. + */ + // thenReturn has a varargs overload, so a generic Map is an unchecked array. + @SuppressWarnings("unchecked") + 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(property, "String", "cat", null)); + Map publicProperties = new HashMap<>(); + publicProperties.put(productKey, 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 the given cloud response. + */ + private IPIntelligenceDataCloud process( + IPIntelligenceCloudEngine engine, + String response) { + CloudRequestData cloudData = mock(CloudRequestData.class); + when(cloudData.getJsonResponse()).thenReturn(response); + + 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; + } + + /** + * 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. + */ + 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/main/java/fiftyone/geolocation/flowelements/GeoLocationCloudEngine.java b/location/geo-location.cloud/src/main/java/fiftyone/geolocation/flowelements/GeoLocationCloudEngine.java index 098faac53..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 @@ -47,15 +47,44 @@ 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; + } + + /** + * 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 @@ -107,7 +136,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); @@ -167,9 +196,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); } } @@ -177,16 +207,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())) { - properties = new ArrayList<>(); - dataSourceTier = map.get(getElementDataKey()).dataTier; + if (product != null) { + List loaded = new ArrayList<>(); + dataSourceTier = product.dataTier; - for (AccessiblePropertyMetaData.PropertyMetaData item : map.get(getElementDataKey()).properties) + for (AccessiblePropertyMetaData.PropertyMetaData item : product.properties) { AspectPropertyMetaData property = new AspectPropertyMetaDataDefault( item.name, @@ -199,13 +227,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 Device Detection cloud engine", this); return false; } } 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..3336d8ddc --- /dev/null +++ b/location/geo-location.cloud/src/test/java/fiftyone/geolocation/flowelements/GeoLocationCloudEngineTests.java @@ -0,0 +1,259 @@ +/* ********************************************************************* + * 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.assertFalse; +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).startsWith("warn ")); + 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()); + } + } + + /** + * 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. + */ + @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()); + } + } + } + + /** + * 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) + .thenReturn(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 one product, holding + * one string property. + */ + // thenReturn has a varargs overload, so a generic Map is an unchecked array. + @SuppressWarnings("unchecked") + private GeoLocationCloudEngine buildEngine( + String productKey, + String property) throws Exception { + GeoLocationCloudEngine engine = + new GeoLocationCloudEngineBuilder(loggerFactory) + .build(Enums.GeoLocationProvider.FiftyOneDegrees); + + ProductMetaData product = new ProductMetaData(); + product.dataTier = "test"; + product.properties = Arrays.asList( + new PropertyMetaData(property, "String", "cat", null)); + Map publicProperties = new HashMap<>(); + publicProperties.put(productKey, 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 the given cloud response. + */ + private CloudGeoData process( + GeoLocationCloudEngine engine, + String response) { + CloudRequestData cloudData = mock(CloudRequestData.class); + when(cloudData.getJsonResponse()).thenReturn(response); + + 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; + } + + /** + * 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. + */ + 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/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/Constants.java b/pipeline/pipeline.cloudrequestengine/src/main/java/fiftyone/pipeline/cloudrequestengine/Constants.java index 593c618d6..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 " @@ -48,6 +52,18 @@ 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. " + + "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..80b39e9a6 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,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 +43,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 +111,7 @@ public CloudRequestEngine getInstance() throws PipelineConfigurationException { private volatile List aspectProperties; private String dataSourceTier; private RequestEngineAccessor requestEngine; + private final AtomicBoolean productMissingLogged = new AtomicBoolean(false); /** * Construct a new instance of the {@link CloudAspectEngineBase}. @@ -141,6 +144,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; @@ -149,17 +159,24 @@ 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; + } + + /** + * 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 @@ -192,17 +209,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, @@ -218,12 +233,60 @@ 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; } } + /** + * 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. + * @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) { + return product; + } + if (productMissingLogged.compareAndSet(false, true)) { + logger.warn(String.format( + MessageProductNotInResourceKey, + getElementDataKey(), + getClass().getSimpleName())); + } + AccessiblePropertyMetaData.ProductMetaData empty = + new AccessiblePropertyMetaData.ProductMetaData(); + empty.properties = new ArrayList<>(); + return empty; + } + + /** + * 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 + */ + protected JSONObject getElementJson(JSONObject response) { + JSONObject element = response.optJSONObject(getElementDataKey()); + return element == null ? new JSONObject() : element; + } + private List loadElementProperties( List itemProperties) { List result = null; 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..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 @@ -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) -> { @@ -98,7 +101,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 +112,43 @@ protected CloudRequestEngine newEngine(List properties) throws Exception evidenceKeysEndpoint, timeoutMillis, cloudRequestOrigin); + 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) { @@ -207,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 985b5429c..1ca5d11c3 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; @@ -38,6 +39,7 @@ import fiftyone.pipeline.exceptions.AggregateException; import fiftyone.pipeline.util.LicenseKeyMask; import org.json.JSONArray; +import org.json.JSONException; import org.json.JSONObject; import org.slf4j.Logger; @@ -51,6 +53,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; @@ -73,6 +77,17 @@ 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, 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", "licence", "values"))); + private HttpClient httpClient; private String endPoint; @@ -83,12 +98,44 @@ 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 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. + */ + 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, @@ -194,40 +241,185 @@ 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() { + 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) { + // 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(); + } catch (RuntimeException e) { + failures.add(e); } } + try { + evidenceKeys.join(); + } catch (CompletionException e) { + failures.add(e.getCause() instanceof RuntimeException ? + (RuntimeException) e.getCause() : + e); + } } - return evidenceKeyFilter; + 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); } + /** + * 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 Map - getPublicProperties() throws CloudRequestException, AggregateException, PropertyNotLoadedException { - if (publicProperties == null) { - synchronized (publicPropertiesLock) { + 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. + * @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 + * 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); @@ -275,10 +467,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. @@ -293,10 +496,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 */ @@ -358,7 +580,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 */ @@ -426,6 +648,16 @@ private static URL toUrl(String spec) throws MalformedURLException { } } + /** + * Apply the configured timeout to a request. + */ + private void setTimeouts(HttpURLConnection connection) { + if (timeoutMillis != null) { + connection.setConnectTimeout(timeoutMillis); + connection.setReadTimeout(timeoutMillis); + } + } + /** * The license key sent with each request, and each key in it when it * holds several separated by '|', to mask in messages. @@ -439,7 +671,7 @@ private List getLicenseKeys() { return keys; } - private void getCloudProperties() throws CloudRequestException, AggregateException, PropertyNotLoadedException { + private void fetchPublicProperties() throws CloudRequestException, AggregateException, PropertyNotLoadedException { try { String jsonResult; @@ -447,19 +679,12 @@ 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); - 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 + "."); @@ -469,7 +694,19 @@ private void getCloudProperties() throws CloudRequestException, AggregateExcepti } } - private void getCloudEvidenceKeys() throws CloudRequestException, AggregateException, PropertyNotLoadedException { + /** + * 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; @@ -477,6 +714,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); @@ -486,8 +724,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); @@ -521,7 +758,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 : 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.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..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 @@ -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; @@ -72,9 +73,14 @@ protected void processCloudEngine(FlowData data, TData aspectData, String json) // Extract data from json to the aspectData instance. JSONObject map = new JSONObject(json); // 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. 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; + } + for (Object entry : profiles) { // Iterate through the devices, parsing each one and // adding it to the result. 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..4f073192e 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; @@ -30,10 +29,12 @@ 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; +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() - ); - } - maxErrors = 1; + createPipeline(); + + assertEquals(0, engine.getProperties().size()); + assertProductMissingLoggedOnce(); + maxWarnings = 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,20 +309,154 @@ public void loadProperties_wrongProperties() throws Exception { devicePropertyData.properties = properties; propertiesReturnedByRequestEngine.put("test2", devicePropertyData); - try { - createPipeline(); - fail("RuntimeException should be thrown"); - } catch (RuntimeException ex){ - assertEquals( - String.format( - ExceptionFailedToLoadProperties, - "test", - "test"), - ex.getMessage() - ); + createPipeline(); + + for (int i = 0; i < 3; i++) { + assertEquals(0, engine.getProperties().size()); } + assertProductMissingLoggedOnce(); + maxWarnings = 1; + } - maxErrors = 1; + /** + * 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; + 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")); + } + + /** + * 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 + * 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(); + 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() { + List errors = new ArrayList<>(); + List warnings = new ArrayList<>(); + for (TestLogger logger : loggerFactory.loggers) { + errors.addAll(logger.errorsLogged); + warnings.addAll(logger.warningsLogged); + } + assertEquals(1, warnings.size(), warnings.toString()); + assertTrue(warnings.get(0).contains("'test'"), warnings.get(0)); + assertEquals(0, errors.size(), errors.toString()); } /** 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..aeadfb1ae --- /dev/null +++ b/pipeline/pipeline.cloudrequestengine/src/test/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudRequestEngineDiscoveryTests.java @@ -0,0 +1,545 @@ +/* ********************************************************************* + * 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.CloudDiscoveryState; +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.junit.jupiter.params.provider.ValueSource; +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.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.*; +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; + + /** + * 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 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()); + + /** + * 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 { + 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); + 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"); + } + 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, 408, 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 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. + */ + @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); + } + } + + /** + * 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); + } + + /** + * 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") + .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 (bodies.containsKey(name)) { + return bodies.get(name); + } + 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 1259fd677..9e9f3a006 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; import org.slf4j.ILoggerFactory; @@ -48,7 +51,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; @@ -325,14 +330,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") @@ -346,24 +348,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() { @@ -388,6 +399,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) @@ -508,6 +520,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") @@ -529,10 +542,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") @@ -565,6 +580,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) @@ -592,6 +608,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) @@ -613,6 +630,142 @@ 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); + } + + /** + * 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 + */ + 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. @@ -718,22 +871,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()); } /** @@ -751,21 +904,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()); } } 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 { 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.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 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..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 @@ -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( 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..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,31 +153,52 @@ 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()); } 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) && 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>();