diff --git a/device-detection/device-detection.cloud/src/main/java/fiftyone/devicedetection/cloud/flowelements/DeviceDetectionCloudEngine.java b/device-detection/device-detection.cloud/src/main/java/fiftyone/devicedetection/cloud/flowelements/DeviceDetectionCloudEngine.java index 2e6e2f97..400ca1da 100644 --- a/device-detection/device-detection.cloud/src/main/java/fiftyone/devicedetection/cloud/flowelements/DeviceDetectionCloudEngine.java +++ b/device-detection/device-detection.cloud/src/main/java/fiftyone/devicedetection/cloud/flowelements/DeviceDetectionCloudEngine.java @@ -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 @@ -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); } } @@ -209,7 +239,7 @@ private boolean loadAspectProperties(CloudRequestEngine engine) { findProduct(engine.getPublicProperties()); if (product != null) { - aspectProperties = new ArrayList<>(); + List properties = new ArrayList<>(); dataSourceTier = product.dataTier; for (AccessiblePropertyMetaData.PropertyMetaData item : @@ -221,13 +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 index a9562e0f..2cd708eb 100644 --- a/device-detection/device-detection.cloud/src/test/java/fiftyone/devicedetection/cloud/flowelements/ProductMissingTests.java +++ b/device-detection/device-detection.cloud/src/test/java/fiftyone/devicedetection/cloud/flowelements/ProductMissingTests.java @@ -134,6 +134,39 @@ public void ProductMissing_PropertyIsMissing() throws Exception { } } + /** + * After a failure at build that can pass, the Cloud Request Engine does + * not know the accessible products yet. The engine builds without + * logging anything, and reads its metadata once the products are known + * rather than keeping none. + */ + @Test + public void DiscoveryNotComplete_LoadsTheMetadataLater() throws Exception { + ProductMetaData product = new ProductMetaData(); + product.dataTier = "test"; + product.properties = Arrays.asList( + new PropertyMetaData("hardwarevendor", "String", "cat", null)); + Map products = new HashMap<>(); + products.put("device", product); + CloudRequestEngine requestEngine = mock(CloudRequestEngine.class); + when(requestEngine.getPublicProperties()) + .thenReturn(null) + .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. diff --git a/ip-intelligence/ip-intelligence.cloud/src/main/java/fiftyone/ipintelligence/cloud/flowelements/IPIntelligenceCloudEngine.java b/ip-intelligence/ip-intelligence.cloud/src/main/java/fiftyone/ipintelligence/cloud/flowelements/IPIntelligenceCloudEngine.java index da503923..99b84cbb 100644 --- a/ip-intelligence/ip-intelligence.cloud/src/main/java/fiftyone/ipintelligence/cloud/flowelements/IPIntelligenceCloudEngine.java +++ b/ip-intelligence/ip-intelligence.cloud/src/main/java/fiftyone/ipintelligence/cloud/flowelements/IPIntelligenceCloudEngine.java @@ -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 @@ -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); } } @@ -243,7 +273,7 @@ private boolean loadAspectProperties(CloudRequestEngine engine) { findProduct(engine.getPublicProperties()); if (product != null) { - aspectProperties = new ArrayList<>(); + List properties = new ArrayList<>(); weightedPropertyNames.clear(); dataSourceTier = product.dataTier; @@ -259,13 +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 index 88aa3508..a597f466 100644 --- a/ip-intelligence/ip-intelligence.cloud/src/test/java/fiftyone/ipintelligence/cloud/flowelements/ProductMissingTests.java +++ b/ip-intelligence/ip-intelligence.cloud/src/test/java/fiftyone/ipintelligence/cloud/flowelements/ProductMissingTests.java @@ -134,6 +134,39 @@ public void ProductMissing_PropertyIsMissing() throws Exception { } } + /** + * After a failure at build that can pass, the Cloud Request Engine does + * not know the accessible products yet. The engine builds without + * logging anything, and reads its metadata once the products are known + * rather than keeping none. + */ + @Test + public void DiscoveryNotComplete_LoadsTheMetadataLater() throws Exception { + ProductMetaData product = new ProductMetaData(); + product.dataTier = "test"; + product.properties = Arrays.asList( + new PropertyMetaData("registeredname", "String", "cat", null)); + Map products = new HashMap<>(); + products.put("ip", product); + CloudRequestEngine requestEngine = mock(CloudRequestEngine.class); + when(requestEngine.getPublicProperties()) + .thenReturn(null) + .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. diff --git a/location/geo-location.cloud/src/main/java/fiftyone/geolocation/flowelements/GeoLocationCloudEngine.java b/location/geo-location.cloud/src/main/java/fiftyone/geolocation/flowelements/GeoLocationCloudEngine.java index 7fb48e1c..cde6a493 100644 --- a/location/geo-location.cloud/src/main/java/fiftyone/geolocation/flowelements/GeoLocationCloudEngine.java +++ b/location/geo-location.cloud/src/main/java/fiftyone/geolocation/flowelements/GeoLocationCloudEngine.java @@ -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 @@ -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); } } @@ -181,7 +211,7 @@ private boolean loadAspectProperties(CloudRequestEngine engine) { findProduct(engine.getPublicProperties()); if (product != null) { - properties = new ArrayList<>(); + List loaded = new ArrayList<>(); dataSourceTier = product.dataTier; for (AccessiblePropertyMetaData.PropertyMetaData item : product.properties) @@ -197,13 +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 Location 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 index 4bed66b4..3336d8dd 100644 --- a/location/geo-location.cloud/src/test/java/fiftyone/geolocation/flowelements/GeoLocationCloudEngineTests.java +++ b/location/geo-location.cloud/src/test/java/fiftyone/geolocation/flowelements/GeoLocationCloudEngineTests.java @@ -136,6 +136,40 @@ public void ProductMissing_PropertyIsMissing() throws Exception { } } + /** + * After a failure at build that can pass, the Cloud Request Engine does + * not know the accessible products yet. The engine builds without + * logging anything, and reads its metadata once the products are known + * rather than keeping none. + */ + @Test + public void DiscoveryNotComplete_LoadsTheMetadataLater() throws Exception { + ProductMetaData product = new ProductMetaData(); + product.dataTier = "test"; + product.properties = Arrays.asList( + new PropertyMetaData("town", "String", "cat", null)); + Map products = new HashMap<>(); + products.put("location", product); + CloudRequestEngine requestEngine = mock(CloudRequestEngine.class); + when(requestEngine.getPublicProperties()) + .thenReturn(null) + .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. diff --git a/pipeline/pipeline.cloudrequestengine/src/main/java/fiftyone/pipeline/cloudrequestengine/CloudDiscoveryState.java b/pipeline/pipeline.cloudrequestengine/src/main/java/fiftyone/pipeline/cloudrequestengine/CloudDiscoveryState.java new file mode 100644 index 00000000..0731e46d --- /dev/null +++ b/pipeline/pipeline.cloudrequestengine/src/main/java/fiftyone/pipeline/cloudrequestengine/CloudDiscoveryState.java @@ -0,0 +1,115 @@ +/* ********************************************************************* + * This Original Work is copyright of 51 Degrees Mobile Experts Limited. + * Copyright 2026 51 Degrees Mobile Experts Limited, Davidson House, + * Forbury Square, Reading, Berkshire, United Kingdom RG1 3EU. + * + * This Original Work is licensed under the European Union Public Licence + * (EUPL) v.1.2 and is subject to its terms as set out below. + * + * If a copy of the EUPL was not distributed with this file, You can obtain + * one at https://opensource.org/licenses/EUPL-1.2. + * + * The 'Compatible Licences' set out in the Appendix to the EUPL (as may be + * amended by the European Commission) shall be deemed incompatible for + * the purposes of the Work and the provisions of the compatibility + * clause in Article 5 of the EUPL shall not apply. + * + * If using the Work as, or as part of, a network application, by + * including the attribution notice(s) required under Article 5 of the EUPL + * in the end user terms of the application under an appropriate heading, + * such notice(s) shall fulfill the requirements of that article. + * ********************************************************************* */ + +package fiftyone.pipeline.cloudrequestengine; + +import org.json.JSONArray; +import org.json.JSONObject; + +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; +import java.util.Objects; + +/** + * A copy of what the Cloud Request Engine discovered for a resource key, + * being the accepted Evidence keys and the accessible properties, which + * depend only on the resource key. Read it from the builder after a + * successful build, keep it, for example as {@link #toJson()}, and give it + * to a later builder, which then makes no discovery request. A Pipeline can + * so be built offline, and a host that starts often need not call the + * cloud service each time. + * @see Specification + */ +public final class CloudDiscoveryState { + + private static final String EVIDENCE_KEYS = "evidenceKeys"; + + private static final String ACCESSIBLE_PROPERTIES = "accessibleProperties"; + + private final List evidenceKeys; + + private final String accessibleProperties; + + /** + * Construct a new instance. + * @param evidenceKeys the evidence keys the cloud service accepts + * @param accessibleProperties the answer to the accessibleproperties + * request, as JSON + * @throws org.json.JSONException if the accessible properties are not + * a JSON object with a Products member + */ + public CloudDiscoveryState( + List evidenceKeys, + String accessibleProperties) { + Objects.requireNonNull(evidenceKeys, "evidenceKeys"); + Objects.requireNonNull(accessibleProperties, "accessibleProperties"); + new JSONObject(accessibleProperties).getJSONObject("Products"); + this.evidenceKeys = Collections.unmodifiableList( + new ArrayList<>(evidenceKeys)); + this.accessibleProperties = accessibleProperties; + } + + /** + * @return the evidence keys the cloud service accepts + */ + public List getEvidenceKeys() { + return evidenceKeys; + } + + /** + * @return the answer to the accessibleproperties request, as JSON + */ + public String getAccessibleProperties() { + return accessibleProperties; + } + + /** + * @return this copy as one JSON document, which {@link #fromJson} + * reads back + */ + public String toJson() { + JSONObject json = new JSONObject(); + json.put(EVIDENCE_KEYS, new JSONArray(evidenceKeys)); + json.put(ACCESSIBLE_PROPERTIES, new JSONObject(accessibleProperties)); + return json.toString(); + } + + /** + * Read a copy written by {@link #toJson()}. + * @param json the JSON document + * @return the copy + * @throws org.json.JSONException if the document is not one + * {@link #toJson()} writes + */ + public static CloudDiscoveryState fromJson(String json) { + JSONObject document = new JSONObject(json); + JSONArray keys = document.getJSONArray(EVIDENCE_KEYS); + List evidenceKeys = new ArrayList<>(); + for (int i = 0; i < keys.length(); i++) { + evidenceKeys.add(keys.getString(i)); + } + return new CloudDiscoveryState( + evidenceKeys, + document.getJSONObject(ACCESSIBLE_PROPERTIES).toString()); + } +} diff --git a/pipeline/pipeline.cloudrequestengine/src/main/java/fiftyone/pipeline/cloudrequestengine/Constants.java b/pipeline/pipeline.cloudrequestengine/src/main/java/fiftyone/pipeline/cloudrequestengine/Constants.java index 613dcff5..e957273e 100644 --- a/pipeline/pipeline.cloudrequestengine/src/main/java/fiftyone/pipeline/cloudrequestengine/Constants.java +++ b/pipeline/pipeline.cloudrequestengine/src/main/java/fiftyone/pipeline/cloudrequestengine/Constants.java @@ -40,6 +40,10 @@ public class Messages { public static final String ExceptionCloudError = "Error returned from 51Degrees cloud service: %s"; + + public static final String ExceptionResponseNotJson = + "Cloud service returned status code '%d' with content that is " + + "not JSON"; public static final String ExceptionFailedToLoadProperties = "Failed to load aspect properties for element '%s'. This is " @@ -48,6 +52,13 @@ public class Messages { + "see our explainer: " + "https://51degrees.com/documentation/_services__cloud__resource_keys.html?utm_source=code&utm_medium=comment&utm_campaign=51degrees-java&utm_content=pipeline.cloudrequestengine-src-main-java-fiftyone-pipeline-cloudrequestengine-constants.java&utm_term=exceptionfailedtoloadproperties"; + public static final String MessageDiscoveryDeferred = + "The cloud request engine could not fetch its accessible " + + "properties or accepted evidence keys when it was built, " + + "because of a failure that can pass. It will fetch them " + + "again when it is first used, and sends no data request " + + "until it has both."; + public static final String MessageProductNotInResourceKey = "The resource key does not include the '%s' product, so " + "%s has no properties and its results hold no values. " diff --git a/pipeline/pipeline.cloudrequestengine/src/main/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudAspectEngineBase.java b/pipeline/pipeline.cloudrequestengine/src/main/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudAspectEngineBase.java index 9b57c036..80b39e9a 100644 --- a/pipeline/pipeline.cloudrequestengine/src/main/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudAspectEngineBase.java +++ b/pipeline/pipeline.cloudrequestengine/src/main/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudAspectEngineBase.java @@ -22,7 +22,6 @@ package fiftyone.pipeline.cloudrequestengine.flowelements; -import static fiftyone.pipeline.cloudrequestengine.Constants.Messages.ExceptionFailedToLoadProperties; import static fiftyone.pipeline.cloudrequestengine.Constants.Messages.MessageProductNotInResourceKey; import static fiftyone.pipeline.cloudrequestengine.Constants.Messages.ProcessCloudEngineNotImplemented; @@ -145,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; @@ -153,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 @@ -220,8 +233,8 @@ private boolean loadAspectProperties() { aspectProperties = properties; return true; } else { - logger.error("Aspect properties could not be loaded for " + - this.getClass().getName(), this); + // The Cloud Request Engine has logged why the products are not + // known yet. return false; } } diff --git a/pipeline/pipeline.cloudrequestengine/src/main/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudEvidenceKeyFilter.java b/pipeline/pipeline.cloudrequestengine/src/main/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudEvidenceKeyFilter.java new file mode 100644 index 00000000..bfcac399 --- /dev/null +++ b/pipeline/pipeline.cloudrequestengine/src/main/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudEvidenceKeyFilter.java @@ -0,0 +1,94 @@ +/* ********************************************************************* + * This Original Work is copyright of 51 Degrees Mobile Experts Limited. + * Copyright 2026 51 Degrees Mobile Experts Limited, Davidson House, + * Forbury Square, Reading, Berkshire, United Kingdom RG1 3EU. + * + * This Original Work is licensed under the European Union Public Licence + * (EUPL) v.1.2 and is subject to its terms as set out below. + * + * If a copy of the EUPL was not distributed with this file, You can obtain + * one at https://opensource.org/licenses/EUPL-1.2. + * + * The 'Compatible Licences' set out in the Appendix to the EUPL (as may be + * amended by the European Commission) shall be deemed incompatible for + * the purposes of the Work and the provisions of the compatibility + * clause in Article 5 of the EUPL shall not apply. + * + * If using the Work as, or as part of, a network application, by + * including the attribution notice(s) required under Article 5 of the EUPL + * in the end user terms of the application under an appropriate heading, + * such notice(s) shall fulfill the requirements of that article. + * ********************************************************************* */ + +package fiftyone.pipeline.cloudrequestengine.flowelements; + +import fiftyone.pipeline.core.data.EvidenceKeyFilterWhitelist; + +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; +import java.util.Map; + +/** + * The Evidence keys the cloud service accepts, from its evidencekeys + * request. It includes nothing until the keys are known, and reads them on + * each call, so a filter that holds this one sees the keys as soon as they + * arrive. It derives from {@link EvidenceKeyFilterWhitelist} so that the + * Pipeline's filter keeps it rather than copying its keys once, and so + * that the web integration lists its headers in the Vary header. + */ +class CloudEvidenceKeyFilter extends EvidenceKeyFilterWhitelist { + + private volatile EvidenceKeyFilterWhitelist keys = null; + + CloudEvidenceKeyFilter() { + super(new ArrayList(), String.CASE_INSENSITIVE_ORDER); + } + + /** + * @return true once the keys are known + */ + boolean isKnown() { + return keys != null; + } + + /** + * Set the keys the cloud service accepts. + * @param evidenceKeys the keys + */ + void setKeys(List evidenceKeys) { + keys = new EvidenceKeyFilterWhitelist( + evidenceKeys, + String.CASE_INSENSITIVE_ORDER); + } + + /** + * @return the keys, or null if they are not known + */ + List getKeys() { + EvidenceKeyFilterWhitelist current = keys; + return current == null ? + null : + new ArrayList<>(current.getWhitelist().keySet()); + } + + @Override + public boolean include(String key) { + EvidenceKeyFilterWhitelist current = keys; + return current != null && current.include(key); + } + + @Override + public Integer order(String key) { + EvidenceKeyFilterWhitelist current = keys; + return current == null ? null : current.order(key); + } + + @Override + public Map getWhitelist() { + EvidenceKeyFilterWhitelist current = keys; + return current == null ? + Collections.emptyMap() : + current.getWhitelist(); + } +} diff --git a/pipeline/pipeline.cloudrequestengine/src/main/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudRequestEngine.java b/pipeline/pipeline.cloudrequestengine/src/main/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudRequestEngine.java index 1025b579..d3027f2d 100644 --- a/pipeline/pipeline.cloudrequestengine/src/main/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudRequestEngine.java +++ b/pipeline/pipeline.cloudrequestengine/src/main/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudRequestEngine.java @@ -41,7 +41,8 @@ public interface CloudRequestEngine /** * A collection of the properties that the cloud service can populate in the * JSON response. Keyed on property name. - * @return public properties + * @return public properties, or null if they are not known yet, after a + * failure when the engine was built that can pass */ Map getPublicProperties(); } diff --git a/pipeline/pipeline.cloudrequestengine/src/main/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudRequestEngineBuilder.java b/pipeline/pipeline.cloudrequestengine/src/main/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudRequestEngineBuilder.java index bc72731b..b0d420a0 100644 --- a/pipeline/pipeline.cloudrequestengine/src/main/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudRequestEngineBuilder.java +++ b/pipeline/pipeline.cloudrequestengine/src/main/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudRequestEngineBuilder.java @@ -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 985b5429..17ea52da 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; @@ -83,12 +87,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 +230,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); @@ -426,6 +607,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 +630,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 +638,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 +653,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 +673,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 +683,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 +717,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 f6300acf..b9b26c6a 100644 --- a/pipeline/pipeline.cloudrequestengine/src/main/java/fiftyone/pipeline/cloudrequestengine/flowelements/Constants.java +++ b/pipeline/pipeline.cloudrequestengine/src/main/java/fiftyone/pipeline/cloudrequestengine/flowelements/Constants.java @@ -28,6 +28,8 @@ public class Constants { "https://cloud.51degrees.com/api/v4"; // Environment variable name to set end point public static final String FOD_CLOUD_API_URL = "FOD_CLOUD_API_URL"; - public static final int DEFAULT_TIMEOUT_MILLIS = 100000; + // Default timeout for each request to the cloud service, as the + // specification's Configuration options table gives. + public static final int DEFAULT_TIMEOUT_MILLIS = 2000; } diff --git a/pipeline/pipeline.cloudrequestengine/src/test/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudAspectEngineBaseTests.java b/pipeline/pipeline.cloudrequestengine/src/test/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudAspectEngineBaseTests.java index 6eba9dc3..4f073192 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; @@ -320,27 +319,57 @@ public void loadProperties_wrongProperties() throws Exception { } /** - * Accessible properties that have not been loaded are not a missing - * product, so reading the property metadata still fails rather than - * keeping an empty result. + * Accessible properties the Cloud Request Engine has not fetched yet, + * after a failure at build that can pass, are not a missing product. + * The Pipeline builds, the metadata is empty and nothing is logged, and + * the metadata is read again once the properties are known rather than + * kept empty. * @throws Exception */ @Test public void loadProperties_notLoaded() throws Exception { propertiesReturnedByRequestEngine = null; - try { - createPipeline(); - fail("RuntimeException should be thrown"); - } catch (RuntimeException ex){ - assertEquals( - String.format( - ExceptionFailedToLoadProperties, - "test", - "test"), - ex.getMessage() - ); - } - maxErrors = 1; + createPipeline(); + + assertEquals(0, engine.getProperties().size()); + + List properties = new ArrayList<>(); + properties.add(new AccessiblePropertyMetaData.PropertyMetaData("ismobile", "Boolean", null, null)); + AccessiblePropertyMetaData.ProductMetaData product = new AccessiblePropertyMetaData.ProductMetaData(); + product.properties = properties; + Map products = new HashMap<>(); + products.put("test", product); + requestEngine.setPublicProperties(products); + + assertEquals(1, engine.getProperties().size()); + assertTrue(propertiesContainName(engine.getProperties(), "ismobile")); + } + + /** + * 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")); } /** diff --git a/pipeline/pipeline.cloudrequestengine/src/test/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudRequestEngineDiscoveryTests.java b/pipeline/pipeline.cloudrequestengine/src/test/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudRequestEngineDiscoveryTests.java new file mode 100644 index 00000000..aeadfb1a --- /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 1259fd67..f6ad4c5d 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 @@ -325,14 +325,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 +343,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() { @@ -718,22 +724,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 +757,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 0320d94e..98d85411 100644 --- a/pipeline/pipeline.cloudrequestengine/src/test/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudRequestEngineTestsBase.java +++ b/pipeline/pipeline.cloudrequestengine/src/test/java/fiftyone/pipeline/cloudrequestengine/flowelements/CloudRequestEngineTestsBase.java @@ -69,24 +69,26 @@ public Object answer(InvocationOnMock invocationOnMock) throws Throwable { invocationOnMock.getMethod().getName() + "' should not have been called."); } }); - final HttpURLConnection connection = mock(HttpURLConnection.class, new Answer() { - @Override - public Object answer(InvocationOnMock invocationOnMock) throws Throwable { - throw new Exception("The method '" + - invocationOnMock.getMethod().getName() + "' should not have been called."); - } - }); - doNothing().when(connection).setConnectTimeout(anyInt()); - doNothing().when(connection).setReadTimeout(anyInt()); - Map> responseHeaders = new HashMap>(); - doReturn(responseHeaders).when(connection).getHeaderFields(); + // Each request gets a connection of its own, because the engine + // makes its two discovery requests at once. doAnswer(new Answer() { @Override public Object answer(InvocationOnMock invocationOnMock) throws Throwable { - URL url = (URL)invocationOnMock.getArgument(0); - doReturn(url).when(connection).getURL(); + URL url = (URL)invocationOnMock.getArgument(0); + HttpURLConnection connection = mock(HttpURLConnection.class, new Answer() { + @Override + public Object answer(InvocationOnMock invocation) throws Throwable { + throw new Exception("The method '" + + invocation.getMethod().getName() + "' should not have been called."); + } + }); + doNothing().when(connection).setConnectTimeout(anyInt()); + doNothing().when(connection).setReadTimeout(anyInt()); + Map> responseHeaders = new HashMap>(); + doReturn(responseHeaders).when(connection).getHeaderFields(); + doReturn(url).when(connection).getURL(); if (url.getPath().endsWith("properties")) { - propertiesEndPoint = url.getQuery(); + propertiesEndPoint = url.getQuery(); doReturn(accessiblePropertiesResponseCode).when(connection).getResponseCode(); } else { diff --git a/pipeline/pipeline.core/src/main/java/fiftyone/pipeline/core/flowelements/FlowElement.java b/pipeline/pipeline.core/src/main/java/fiftyone/pipeline/core/flowelements/FlowElement.java index c27716d2..b03878a8 100644 --- a/pipeline/pipeline.core/src/main/java/fiftyone/pipeline/core/flowelements/FlowElement.java +++ b/pipeline/pipeline.core/src/main/java/fiftyone/pipeline/core/flowelements/FlowElement.java @@ -103,6 +103,19 @@ public interface FlowElement< */ boolean isConcurrent(); + /** + * Whether the element knows all the property metadata and accepted + * Evidence it will expose. An element that gets them from a remote + * service returns false until it has them, and anything that keeps what + * it reads from the element, such as the Pipeline's map of available + * properties, reads it again until then. + * @return true if the property metadata and accepted Evidence are + * complete + */ + default boolean hasLoadedProperties() { + return true; + } + /** * Indicates whether the element has been closed using the {@link #close()} * method, either explicitly or as a result of a 'try with resource'. diff --git a/pipeline/pipeline.core/src/main/java/fiftyone/pipeline/core/flowelements/ParallelElements.java b/pipeline/pipeline.core/src/main/java/fiftyone/pipeline/core/flowelements/ParallelElements.java index 79b8e414..b1a479b7 100644 --- a/pipeline/pipeline.core/src/main/java/fiftyone/pipeline/core/flowelements/ParallelElements.java +++ b/pipeline/pipeline.core/src/main/java/fiftyone/pipeline/core/flowelements/ParallelElements.java @@ -117,6 +117,19 @@ public boolean isConcurrent() { return true; } + /** + * @return true if every child element has loaded its properties + */ + @Override + public boolean hasLoadedProperties() { + for (FlowElement element : flowElements) { + if (element.hasLoadedProperties() == false) { + return false; + } + } + return true; + } + @Override protected void managedResourcesCleanup() { for (FlowElement element : flowElements) { diff --git a/pipeline/pipeline.core/src/main/java/fiftyone/pipeline/core/flowelements/PipelineDefault.java b/pipeline/pipeline.core/src/main/java/fiftyone/pipeline/core/flowelements/PipelineDefault.java index f53cda4a..302b51d9 100644 --- a/pipeline/pipeline.core/src/main/java/fiftyone/pipeline/core/flowelements/PipelineDefault.java +++ b/pipeline/pipeline.core/src/main/java/fiftyone/pipeline/core/flowelements/PipelineDefault.java @@ -71,9 +71,15 @@ class PipelineDefault implements PipelineInternal { * Map of {@link ElementPropertyMetaData}s keyed first on the data key of * the element which they belong to, then by the property name. */ - private final Map> + private volatile Map> elementAvailableProperties; + /** + * Held while the map of available properties is built after an element + * had not loaded its properties when the Pipeline was built. + */ + private final Object availablePropertiesLock = new Object(); + /** * Lock used for synchronised blocks. */ @@ -155,8 +161,7 @@ class PipelineDefault implements PipelineInternal { for (FlowElement element : this.flowElements) { isConcurrent = isConcurrent || element.isConcurrent(); } - this.elementAvailableProperties = - getElementAvailableProperties(flowElements); + getElementAvailableProperties(); logger.debug("Pipeline '" + hashCode() + "' created."); @@ -360,10 +365,41 @@ public List getFlowElements() { return Collections.unmodifiableList(flowElements); } + /** + * The available properties of each element. The map is kept once every + * element has loaded its properties. Until then, as after a cloud + * element could not reach its service when it was built, it is built + * again on each call, so it gains the properties once they are known. + * @return the available properties, keyed on element data key then + * property name + */ @Override public Map> getElementAvailableProperties() { - return Collections.unmodifiableMap(elementAvailableProperties); + Map> properties = + elementAvailableProperties; + if (properties == null) { + synchronized (availablePropertiesLock) { + properties = elementAvailableProperties; + if (properties == null) { + boolean loaded = allPropertiesLoaded(); + properties = getElementAvailableProperties(flowElements); + if (loaded) { + elementAvailableProperties = properties; + } + } + } + } + return Collections.unmodifiableMap(properties); + } + + private boolean allPropertiesLoaded() { + for (FlowElement element : flowElements) { + if (element.hasLoadedProperties() == false) { + return false; + } + } + return true; } public void close(boolean closing) throws Exception { diff --git a/pipeline/pipeline.engines.fiftyone/src/main/java/fiftyone/pipeline/engines/fiftyone/flowelements/SetHeadersElement.java b/pipeline/pipeline.engines.fiftyone/src/main/java/fiftyone/pipeline/engines/fiftyone/flowelements/SetHeadersElement.java index 78d4c29b..9bb05c6e 100644 --- a/pipeline/pipeline.engines.fiftyone/src/main/java/fiftyone/pipeline/engines/fiftyone/flowelements/SetHeadersElement.java +++ b/pipeline/pipeline.engines.fiftyone/src/main/java/fiftyone/pipeline/engines/fiftyone/flowelements/SetHeadersElement.java @@ -38,6 +38,7 @@ import fiftyone.pipeline.core.data.EvidenceKeyFilterWhitelist; import fiftyone.pipeline.core.data.FlowData; import fiftyone.pipeline.core.data.factories.ElementDataFactory; +import fiftyone.pipeline.core.flowelements.FlowElement; import fiftyone.pipeline.core.flowelements.FlowElementBase; import fiftyone.pipeline.core.flowelements.Pipeline; import fiftyone.pipeline.engines.data.AspectPropertyValue; @@ -101,8 +102,20 @@ protected void processInternal(FlowData data) throws Exception { PipelineConfig config; Pipeline pipeline = data.getPipeline(); if ((config = pipelineConfigs.get(pipeline)) == null) { + // The configuration is kept once every element has loaded its + // properties. Until then, as after a cloud element could not + // reach its service when the Pipeline was built, it is built + // again on each request, so it gains the SetHeader properties + // once they are known. + boolean loaded = allPropertiesLoaded(pipeline); config = populateConfig(pipeline); - config = pipelineConfigs.getOrDefault(pipeline, config); + if (loaded) { + PipelineConfig kept = + pipelineConfigs.putIfAbsent(pipeline, config); + if (kept != null) { + config = kept; + } + } } SetHeadersData elementData = data.getOrAdd( @@ -139,6 +152,21 @@ protected void unmanagedResourcesCleanup() { // Nothing to clean up here } + /** + * @param pipeline the Pipeline + * @return true if every element of the Pipeline has loaded its + * properties + */ + @SuppressWarnings("rawtypes") + private static boolean allPropertiesLoaded(Pipeline pipeline) { + for (FlowElement element : pipeline.getFlowElements()) { + if (element.hasLoadedProperties() == false) { + return false; + } + } + return true; + } + /** * Build the response headers from the SetHeader properties in the FlowData. * @param data flowData object diff --git a/pipeline/pipeline.engines.fiftyone/src/test/java/fiftyone/pipeline/engines/fiftyone/flowelements/SetHeadersElementTests.java b/pipeline/pipeline.engines.fiftyone/src/test/java/fiftyone/pipeline/engines/fiftyone/flowelements/SetHeadersElementTests.java index cd25d63a..93029030 100644 --- a/pipeline/pipeline.engines.fiftyone/src/test/java/fiftyone/pipeline/engines/fiftyone/flowelements/SetHeadersElementTests.java +++ b/pipeline/pipeline.engines.fiftyone/src/test/java/fiftyone/pipeline/engines/fiftyone/flowelements/SetHeadersElementTests.java @@ -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 67439d7c..d51d2cfe 100644 --- a/pipeline/pipeline.web.packages/pipeline.web/src/main/java/fiftyone/pipeline/web/services/ClientsidePropertyServiceCore.java +++ b/pipeline/pipeline.web.packages/pipeline.web/src/main/java/fiftyone/pipeline/web/services/ClientsidePropertyServiceCore.java @@ -108,6 +108,17 @@ private enum ContentTypes { */ protected Pipeline pipeline; + /** + * True once the Vary headers were worked out while every element + * had loaded its properties, so they can be kept. + */ + private volatile boolean headersComplete = false; + + /** + * Held while the Vary headers are worked out again. + */ + private final Object headersLock = new Object(); + /** * A list of all the HTTP headers that are requested evidence for * elements that populate JavaScript properties. @@ -142,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 f819bd99..31c8f9d4 100644 --- a/pipeline/pipeline.web.packages/pipeline.web/src/test/java/fiftyone/pipeline/web/ClientsidePropertyServiceTests.java +++ b/pipeline/pipeline.web.packages/pipeline.web/src/test/java/fiftyone/pipeline/web/ClientsidePropertyServiceTests.java @@ -115,6 +115,28 @@ public void JavaScript_VaryHeader_UA() throws IOException { JavaScript_VaryHeader(elements, "User-Agent"); } + /** + * An element that has not loaded its properties, as a cloud element + * after a failure to reach its service when it was built, gains its + * headers in the Vary header once it has them. + */ + @Test + @SuppressWarnings("rawtypes") + public void JavaScript_VaryHeader_ReadAgainUntilLoaded() throws IOException { + FlowElement element = mock(FlowElement.class); + when(element.getEvidenceKeyFilter()).thenReturn( + new EvidenceKeyFilterWhitelist(new ArrayList()), + new EvidenceKeyFilterWhitelist(Arrays.asList("header.User-Agent"))); + when(element.hasLoadedProperties()).thenReturn(false, true); + List elements = new ArrayList<>(); + elements.add(element); + Configure(elements); + + service.serveJavascript(request, response); + + verify(response).setHeader(eq("Vary"), eq("User-Agent")); + } + @Test public void JavaScript_VaryHeader_UA_AND_SEC() throws IOException { List> elements = new ArrayList>();