diff --git a/location/geo-location.cloud/src/main/java/fiftyone/geolocation/data/CloudGeoData.java b/location/geo-location.cloud/src/main/java/fiftyone/geolocation/data/CloudGeoData.java index 181e740f4..5ca54ae3b 100644 --- a/location/geo-location.cloud/src/main/java/fiftyone/geolocation/data/CloudGeoData.java +++ b/location/geo-location.cloud/src/main/java/fiftyone/geolocation/data/CloudGeoData.java @@ -26,9 +26,16 @@ import java.util.Map; /** - * - * @author joseph + * The Location Element Data from the cloud. */ public interface CloudGeoData extends GeoData { - public void setNoValueReasons(Map value); + + /** + * Has no effect. + * @param value the messages for properties with no value, by name + * @deprecated The cloud engine sets the message of each property with no + * value on the value itself. + */ + @Deprecated + void setNoValueReasons(Map value); } diff --git a/location/geo-location.cloud/src/main/java/fiftyone/geolocation/data/CloudGeoDataDefault.java b/location/geo-location.cloud/src/main/java/fiftyone/geolocation/data/CloudGeoDataDefault.java index 4a217422d..f6080b94c 100644 --- a/location/geo-location.cloud/src/main/java/fiftyone/geolocation/data/CloudGeoDataDefault.java +++ b/location/geo-location.cloud/src/main/java/fiftyone/geolocation/data/CloudGeoDataDefault.java @@ -24,145 +24,32 @@ import fiftyone.geolocation.core.data.GeoDataDefault; import fiftyone.pipeline.core.data.FlowData; -import fiftyone.pipeline.core.data.TryGetResult; -import fiftyone.pipeline.core.data.types.JavaScript; import fiftyone.pipeline.engines.data.AspectData; import fiftyone.pipeline.engines.data.AspectPropertyMetaData; -import fiftyone.pipeline.engines.data.AspectPropertyValue; -import fiftyone.pipeline.engines.data.AspectPropertyValueDefault; import fiftyone.pipeline.engines.flowelements.AspectEngine; import fiftyone.pipeline.engines.services.MissingPropertyService; import java.util.Map; -import java.util.Objects; -import org.json.JSONObject; import org.slf4j.Logger; +/** + * The Location Element Data from the cloud. The cloud engine stores each + * property the response holds as an AspectPropertyValue, so the getters of + * GeoDataDefault return them as they are. + */ public class CloudGeoDataDefault extends GeoDataDefault implements CloudGeoData { - private Map noValueReasons; - public CloudGeoDataDefault(Logger logger, FlowData flowData, AspectEngine engine, MissingPropertyService missingPropertyService) { super(logger, flowData, engine, missingPropertyService); } - public void setNoValueReasons(Map value){ - noValueReasons = value; - } - - @SuppressWarnings("unchecked") - @Override - public AspectPropertyValue getJavaScript() { - return getAs("javascript", AspectPropertyValue.class, JavaScript.class); - } - - @Override - public AspectPropertyValue getRoad() { - return getValueAsString("road"); - } - - @Override - public AspectPropertyValue getTown() { - return getValueAsString("town"); - } - - @Override - public AspectPropertyValue getSuburb() { - return getValueAsString("suburb"); - } - - @Override - public AspectPropertyValue getCounty() { - return getValueAsString("county"); - } - - @Override - public AspectPropertyValue getRegion() { - return getValueAsString("region"); - } - - @Override - public AspectPropertyValue getState() { - return getValueAsString("state"); - } - - @Override - public AspectPropertyValue getZipCode() { - return getValueAsString("zipcode"); - } - - @Override - public AspectPropertyValue getCountry() { - return getValueAsString("country"); - } - - @Override - public AspectPropertyValue getCountryCode() { - return getValueAsString("countrycode"); - } - - @Override - public AspectPropertyValue getCountryCode3() { - return getValueAsString("countrycode3"); - } - - @Override - public AspectPropertyValue getTimeZoneOffset() { - return getValueAs("timezoneoffset"); - } - + /** + * Has no effect. + * @param value the messages for properties with no value, by name + * @deprecated The cloud engine sets the message of each property with no + * value on the value itself. + */ + @Deprecated @Override - public AspectPropertyValue getAddress() { - return getValueAsString("address"); + public void setNoValueReasons(Map value) { } - - private AspectPropertyValue getValueAsString(String key) { - return getValueAs(key); - } - - @SuppressWarnings("unchecked") - private AspectPropertyValue getValueAs(String key) { - AspectPropertyValue value = getAs(key, AspectPropertyValue.class); - - if(noValueReasons.containsKey(key)) - value.setNoValueMessage(noValueReasons.get(key)); - - return value; - } - - @Override - protected TryGetResult tryGetValue(String key, Class type, Class... parameterisedTypes) { - if (AspectPropertyValue.class.isAssignableFrom(type)) { - TryGetResult result = new TryGetResult<>(); - Map map = asKeyMap(); - if (map.containsKey(key)) { - Object obj = map.get(key); - - try { - Object temp = setValue(key, obj); - T value = type.cast(temp); - result.setValue(value); - } catch (ClassCastException e) { - throw new ClassCastException("Expected property '" + key + - "' to be of type '" + type.getSimpleName() + - "' but it is '" + obj.getClass().getSimpleName() + "'"); - } - } - return result; - } - else { - return super.tryGetValue(key, type, parameterisedTypes); - } - } - - private AspectPropertyValue setValue(String key, Object obj) { - AspectPropertyValue temp = new AspectPropertyValueDefault(); - if (Objects.isNull(obj) || obj == JSONObject.NULL) { - temp.setNoValueMessage(noValueReasons.get(key)); - } - else { - temp.setValue(obj); - } - return temp; - } - } 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 cde6a4932..6c8eae436 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 @@ -37,6 +37,8 @@ import fiftyone.pipeline.core.flowelements.Pipeline; import fiftyone.pipeline.engines.data.AspectPropertyMetaData; import fiftyone.pipeline.engines.data.AspectPropertyMetaDataDefault; +import fiftyone.pipeline.engines.data.AspectPropertyValue; +import fiftyone.pipeline.engines.data.AspectPropertyValueDefault; import org.json.JSONObject; import org.slf4j.Logger; @@ -141,39 +143,34 @@ protected void processEngine(FlowData data, CloudGeoData aspectData) { Map methodData = new TreeMap<>(String.CASE_INSENSITIVE_ORDER); for (AspectPropertyMetaData property : getProperties()) { + String key = property.getName().toLowerCase(); + // A property the response leaves out has no entry, so + // reading it throws a PropertyMissingException. + if (geoObj.has(key) == false) { + continue; + } + Object value; switch(property.getType().getSimpleName()) { case ("List"): - JSONArray jsonArray = geoObj.optJSONArray(property.getName().toLowerCase()); - methodData.put( - property.getName(), - jsonArray == null ? null : jsonArray.toList()); + JSONArray jsonArray = geoObj.optJSONArray(key); + value = jsonArray == null ? null : jsonArray.toList(); break; case ("JavaScript"): - String javascript = geoObj.optString(property.getName().toLowerCase(), null); - methodData.put( - property.getName(), - javascript == null ? null : new JavaScript(javascript)); + String javascript = geoObj.optString(key, null); + value = javascript == null ? + null : + new JavaScript(javascript); break; default: - methodData.put( - property.getName(), - geoObj.opt(property.getName().toLowerCase())); - break; + value = geoObj.opt(key); + break; } + methodData.put( + property.getName(), + toAspectPropertyValue(geoObj, key, value)); } aspectData.populateFromMap(methodData); - - Map noValueReasonsMap = new TreeMap<>(String.CASE_INSENSITIVE_ORDER); - for(Map.Entry entry : methodData.entrySet()){ - if("null".equals(String.valueOf(entry.getValue()))){ - String key = entry.getKey().toLowerCase(); - Object reason = tryToGet(geoObj, key); - noValueReasonsMap.put(entry.getKey().toLowerCase(), (reason == null ? "" : reason.toString())); - } - } - - aspectData.setNoValueReasons(noValueReasonsMap); } else { throw new PipelineConfigurationException( @@ -187,6 +184,41 @@ public static Object tryToGet(JSONObject jsonObj, String key) { return jsonObj.opt(key + "nullreason"); } + /** + * The message for a property with no value when the response gives no + * reason. + */ + private static final String UNKNOWN_NULL_REASON = "Unknown"; + + /** + * Wrap a value read from the response. A JSON null is no value, with the + * reason the response gives under the property's name followed by + * nullreason. + * @param geoObj the response's part for this engine + * @param key the lower case property name + * @param value the value read, null or JSONObject.NULL where the + * response has none + * @return the property value + */ + private static AspectPropertyValue toAspectPropertyValue( + JSONObject geoObj, + String key, + Object value) { + AspectPropertyValue result = + new AspectPropertyValueDefault<>(); + if (value == null || value == JSONObject.NULL) { + Object reason = tryToGet(geoObj, key); + result.setNoValueMessage( + reason == null || reason == JSONObject.NULL ? + UNKNOWN_NULL_REASON : + reason.toString()); + } + else { + result.setValue(value); + } + return result; + } + @Override protected void unmanagedResourcesCleanup() { } diff --git a/location/geo-location.cloud/src/test/java/fiftyone/geolocation/flowelements/GeoLocationCloudEngineNullValueTests.java b/location/geo-location.cloud/src/test/java/fiftyone/geolocation/flowelements/GeoLocationCloudEngineNullValueTests.java new file mode 100644 index 000000000..8bfe09c1b --- /dev/null +++ b/location/geo-location.cloud/src/test/java/fiftyone/geolocation/flowelements/GeoLocationCloudEngineNullValueTests.java @@ -0,0 +1,230 @@ +/* ********************************************************************* + * This Original Work is copyright of 51 Degrees Mobile Experts Limited. + * Copyright 2026 51 Degrees Mobile Experts Limited, Davidson House, + * Forbury Square, Reading, Berkshire, United Kingdom RG1 3EU. + * + * This Original Work is licensed under the European Union Public Licence + * (EUPL) v.1.2 and is subject to its terms as set out below. + * + * If a copy of the EUPL was not distributed with this file, You can obtain + * one at https://opensource.org/licenses/EUPL-1.2. + * + * The 'Compatible Licences' set out in the Appendix to the EUPL (as may be + * amended by the European Commission) shall be deemed incompatible for + * the purposes of the Work and the provisions of the compatibility + * clause in Article 5 of the EUPL shall not apply. + * + * If using the Work as, or as part of, a network application, by + * including the attribution notice(s) required under Article 5 of the EUPL + * in the end user terms of the application under an appropriate heading, + * such notice(s) shall fulfill the requirements of that article. + * ********************************************************************* */ + +package fiftyone.geolocation.flowelements; + +import fiftyone.geolocation.core.Enums; +import fiftyone.geolocation.data.CloudGeoData; +import fiftyone.geolocation.data.CloudGeoDataDefault; +import fiftyone.pipeline.cloudrequestengine.data.CloudRequestData; +import fiftyone.pipeline.cloudrequestengine.flowelements.CloudRequestEngine; +import fiftyone.pipeline.core.data.AccessiblePropertyMetaData.ProductMetaData; +import fiftyone.pipeline.core.data.AccessiblePropertyMetaData.PropertyMetaData; +import fiftyone.pipeline.core.data.FlowData; +import fiftyone.pipeline.core.flowelements.Pipeline; +import fiftyone.pipeline.engines.data.AspectPropertyValue; +import fiftyone.pipeline.engines.exceptions.PropertyMissingException; +import fiftyone.pipeline.engines.services.MissingPropertyServiceDefault; +import org.junit.Test; +import org.slf4j.ILoggerFactory; +import org.slf4j.LoggerFactory; + +import java.util.Arrays; +import java.util.HashMap; +import java.util.Map; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +/** + * A property the cloud response gives as null has no value, and its message + * is the text under the property's name followed by nullreason, as the + * Processing section of the cloud aspect engine specification says. A + * property the response leaves out is missing, so reading it throws, as the + * Missing Properties section of the properties specification says. The tests + * read each property by name, from the map the JSON Builder reads, and from + * the typed getter. + */ +public class GeoLocationCloudEngineNullValueTests { + + private static final String SUBURB_NULL_REASON = + "No suburb was found at this location."; + + private static final String NULL_SUBURB_RESPONSE = + "{ 'location': { " + + "'suburb': null, " + + "'suburbnullreason': '" + SUBURB_NULL_REASON + "', " + + "'town': 'Reading' }, " + + "'javascriptProperties': [] }"; + + private static final String NULL_SUBURB_WITHOUT_REASON_RESPONSE = + "{ 'location': { 'suburb': null, 'town': 'Reading' }, " + + "'javascriptProperties': [] }"; + + private static final String NO_SUBURB_RESPONSE = + "{ 'location': { 'town': 'Reading' }, " + + "'javascriptProperties': [] }"; + + private final ILoggerFactory loggerFactory = + LoggerFactory.getILoggerFactory(); + + @Test + public void NullValue_NoValueWithNullReason() throws Exception { + try (GeoLocationCloudEngine engine = buildEngine()) { + CloudGeoData data = process(engine, NULL_SUBURB_RESPONSE); + + assertNoValue(SUBURB_NULL_REASON, data.get("Suburb")); + assertNoValue(SUBURB_NULL_REASON, data.asKeyMap().get("Suburb")); + assertNoValue(SUBURB_NULL_REASON, data.getSuburb()); + } + } + + @Test + public void Value_HasValue() throws Exception { + try (GeoLocationCloudEngine engine = buildEngine()) { + CloudGeoData data = process(engine, NULL_SUBURB_RESPONSE); + + assertValue("Reading", data.get("Town")); + assertValue("Reading", data.asKeyMap().get("Town")); + assertValue("Reading", data.getTown()); + } + } + + @Test + public void NullValueWithoutReason_NoValueUnknown() throws Exception { + try (GeoLocationCloudEngine engine = buildEngine()) { + CloudGeoData data = process( + engine, + NULL_SUBURB_WITHOUT_REASON_RESPONSE); + + assertNoValue("Unknown", data.get("Suburb")); + assertNoValue("Unknown", data.asKeyMap().get("Suburb")); + assertNoValue("Unknown", data.getSuburb()); + } + } + + @Test + public void NoEntry_PropertyIsMissing() throws Exception { + try (GeoLocationCloudEngine engine = buildEngine()) { + CloudGeoData data = process(engine, NO_SUBURB_RESPONSE); + + assertFalse(data.asKeyMap().containsKey("Suburb")); + assertMissing(() -> data.get("Suburb")); + assertMissing(() -> data.getSuburb()); + } + } + + @Test + @SuppressWarnings("deprecation") + public void SetNoValueReasons_ChangesNothing() throws Exception { + try (GeoLocationCloudEngine engine = buildEngine()) { + CloudGeoData data = process(engine, NULL_SUBURB_RESPONSE); + Map reasons = new HashMap<>(); + reasons.put("suburb", "Another reason"); + data.setNoValueReasons(reasons); + + assertNoValue(SUBURB_NULL_REASON, data.get("Suburb")); + assertNoValue(SUBURB_NULL_REASON, data.getSuburb()); + } + } + + private static void assertMissing(Runnable read) { + try { + read.run(); + fail("Expected a PropertyMissingException"); + } + catch (PropertyMissingException e) { + // The property is not in the result, as expected. + } + } + + private static AspectPropertyValue asAspectPropertyValue(Object value) { + assertTrue( + "Expected an AspectPropertyValue but found " + + (value == null ? "null" : value.getClass().getName()), + value instanceof AspectPropertyValue); + return (AspectPropertyValue) value; + } + + private static void assertNoValue(String reason, Object value) { + AspectPropertyValue propertyValue = asAspectPropertyValue(value); + assertFalse( + "Expected no value but found " + propertyValue, + propertyValue.hasValue()); + assertEquals(reason, propertyValue.getNoValueMessage()); + } + + private static void assertValue(Object expected, Object value) + throws Exception { + AspectPropertyValue propertyValue = asAspectPropertyValue(value); + assertTrue( + "Expected a value but found none, because " + + propertyValue.getNoValueMessage(), + propertyValue.hasValue()); + assertEquals(expected, propertyValue.getValue()); + } + + /** + * An engine whose Cloud Request Engine reports the Location product with + * the Suburb and Town properties. + */ + private GeoLocationCloudEngine buildEngine() throws Exception { + GeoLocationCloudEngine engine = + new GeoLocationCloudEngineBuilder(loggerFactory) + .build(Enums.GeoLocationProvider.FiftyOneDegrees); + + ProductMetaData product = new ProductMetaData(); + product.dataTier = "test"; + product.properties = Arrays.asList( + new PropertyMetaData("Suburb", "String", "Location", null), + new PropertyMetaData("Town", "String", "Location", null)); + Map publicProperties = new HashMap<>(); + publicProperties.put("location", product); + + CloudRequestEngine requestEngine = mock(CloudRequestEngine.class); + when(requestEngine.getPublicProperties()).thenReturn(publicProperties); + + Pipeline pipeline = mock(Pipeline.class); + when(pipeline.getElement(CloudRequestEngine.class)) + .thenReturn(requestEngine); + + engine.addPipeline(pipeline); + return engine; + } + + /** + * Process the given cloud response. + */ + private CloudGeoData process( + GeoLocationCloudEngine engine, + String response) { + CloudRequestData cloudData = mock(CloudRequestData.class); + when(cloudData.getJsonResponse()).thenReturn(response); + + FlowData flowData = mock(FlowData.class); + when(flowData.getFromElement(any(CloudRequestEngine.class))) + .thenReturn(cloudData); + + CloudGeoData data = new CloudGeoDataDefault( + loggerFactory.getLogger(CloudGeoData.class.getSimpleName()), + flowData, + engine, + MissingPropertyServiceDefault.getInstance()); + engine.processEngine(flowData, data); + return data; + } +} 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 3336d8ddc..34a070f5f 100644 --- a/location/geo-location.cloud/src/test/java/fiftyone/geolocation/flowelements/GeoLocationCloudEngineTests.java +++ b/location/geo-location.cloud/src/test/java/fiftyone/geolocation/flowelements/GeoLocationCloudEngineTests.java @@ -49,7 +49,6 @@ import java.util.Map; import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; @@ -103,17 +102,23 @@ public void ProductMissing_ProcessLogsAndThrowsNothing() throws Exception { * With the Location product included, a response that holds no * part for it leaves the element data without values and throws * nothing, as the Processing section of the cloud aspect engine - * specification says. + * specification says. Reading a property then throws, as for any + * property the response leaves out. */ @Test - public void ProductPresent_ResponseWithoutPart_NoValues() + public void ProductPresent_ResponseWithoutPart_PropertyIsMissing() throws Exception { try (GeoLocationCloudEngine engine = buildEngine("location", "town")) { CloudGeoData data = process( engine, "{ 'ip': { 'registeredname': 'Example' }, " + "'javascriptProperties': [] }"); - assertFalse(data.getTown().hasValue()); + try { + data.getTown(); + fail("A PropertyMissingException should have been thrown"); + } catch (PropertyMissingException e) { + // The response held nothing for the Location product. + } } } diff --git a/location/geo-location/src/test/java/fiftyone/geolocation/data/tests/ValueTests.java b/location/geo-location/src/test/java/fiftyone/geolocation/data/tests/ValueTests.java index 060e6625c..ce3edd363 100644 --- a/location/geo-location/src/test/java/fiftyone/geolocation/data/tests/ValueTests.java +++ b/location/geo-location/src/test/java/fiftyone/geolocation/data/tests/ValueTests.java @@ -67,9 +67,26 @@ public static void valueTypes(Wrapper wrapper) throws Exception { assertNotNull("Value of " + property.getName() + " is null. ", value); assertTrue("Value of '" + property.getName() + "' was of type " + value.getClass().getSimpleName() + + " but should have been AspectPropertyValue.", + value instanceof AspectPropertyValue); + AspectPropertyValue propertyValue = + (AspectPropertyValue) value; + // A property can have no value at these coordinates, such + // as Suburb. Its type cannot be read then, so only that it + // carries a message is checked. + if (propertyValue.hasValue() == false) { + assertNotNull("Property '" + property.getName() + + "' has no value and no message.", + propertyValue.getNoValueMessage()); + continue; + } + assertTrue("Value of '" + property.getName() + + "' was of type " + + propertyValue.getValue().getClass().getSimpleName() + " but should have been " + expectedType.getSimpleName() + ".", - expectedType.isAssignableFrom(value.getClass())); + expectedType.isAssignableFrom( + propertyValue.getValue().getClass())); } } }