From 1fd3e93fcf47f50ca9e8a157ae99dea993367e11 Mon Sep 17 00:00:00 2001 From: James Rosewell Date: Tue, 10 Nov 2026 08:36:42 +0000 Subject: [PATCH 1/5] Test that a null in the Location cloud response has no value A property the cloud response gives as null must have no value, with the text under the property's name followed by nullreason as its message. The tests read the property by name, from the Element Data's map that the JSON Builder reads, and from the typed getter. A property with a value must give that value, and a property the response leaves out must have no value. Mockito is added to the geo-location.cloud tests to stand in for the Cloud Request Engine, the Pipeline and the Flow Data. --- location/geo-location.cloud/pom.xml | 4 + .../GeoLocationCloudEngineNullValueTests.java | 185 ++++++++++++++++++ 2 files changed, 189 insertions(+) create mode 100644 location/geo-location.cloud/src/test/java/fiftyone/geolocation/flowelements/GeoLocationCloudEngineNullValueTests.java diff --git a/location/geo-location.cloud/pom.xml b/location/geo-location.cloud/pom.xml index 4195dafd5..17679707e 100644 --- a/location/geo-location.cloud/pom.xml +++ b/location/geo-location.cloud/pom.xml @@ -55,5 +55,9 @@ junit junit + + org.mockito + mockito-core + diff --git a/location/geo-location.cloud/src/test/java/fiftyone/geolocation/flowelements/GeoLocationCloudEngineNullValueTests.java b/location/geo-location.cloud/src/test/java/fiftyone/geolocation/flowelements/GeoLocationCloudEngineNullValueTests.java new file mode 100644 index 000000000..b4d9ccde9 --- /dev/null +++ b/location/geo-location.cloud/src/test/java/fiftyone/geolocation/flowelements/GeoLocationCloudEngineNullValueTests.java @@ -0,0 +1,185 @@ +/* ********************************************************************* + * 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.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.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. Each + * test reads the property by name, from the map the JSON Builder reads, and + * from the typed getter, and expects the same from all three. + */ +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 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 NoEntry_NoValue() throws Exception { + try (GeoLocationCloudEngine engine = buildEngine()) { + CloudGeoData data = process(engine, NO_SUBURB_RESPONSE); + + assertNoValue("Unknown", data.get("Suburb")); + assertNoValue("Unknown", data.asKeyMap().get("Suburb")); + assertNoValue("Unknown", data.getSuburb()); + } + } + + 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; + } +} From 7bd5e4d61a094b11d571ed3b5415f31d0d97bebb Mon Sep 17 00:00:00 2001 From: James Rosewell Date: Tue, 10 Nov 2026 08:42:23 +0000 Subject: [PATCH 2/5] Give a null in the Location cloud response no value, with its nullreason The Location cloud engine put each value from the response into the Element Data unwrapped. org.json reads a JSON null as JSONObject.NULL, which is not null, so a property the cloud gave as null held JSONObject.NULL when read by name or from the map, and the JSON Builder wrote it as null with no nullreason. Only the typed getters gave no value, through a separate map of reasons that the engine handed to the Element Data. The engine now stores every property as an AspectPropertyValue, as the Device Detection and IP Intelligence cloud engines do. A JSON null, or no entry, is no value with the text under the property's name followed by nullreason as its message, or Unknown when the response gives none. CloudGeoDataDefault no longer wraps values itself, so its map of reasons, setNoValueReasons on it and on CloudGeoData, and its getters, which only repeated those of GeoDataDefault, are removed. The live value test expected the unwrapped value. It now expects an AspectPropertyValue, and a reason where there is no value, as the Device Detection cloud value test does. --- .../geolocation/data/CloudGeoData.java | 5 +- .../geolocation/data/CloudGeoDataDefault.java | 135 +----------------- .../flowelements/GeoLocationCloudEngine.java | 72 +++++++--- .../geolocation/data/tests/ValueTests.java | 18 ++- 4 files changed, 72 insertions(+), 158 deletions(-) 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..1cbe50799 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 @@ -23,12 +23,9 @@ package fiftyone.geolocation.data; import fiftyone.geolocation.core.data.GeoData; -import java.util.Map; /** - * - * @author joseph + * The Location Element Data from the cloud. */ public interface CloudGeoData extends GeoData { - public 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..45422163a 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,20 @@ 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 every + * property 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"); - } - - @Override - public AspectPropertyValue getAddress() { - return getValueAsString("address"); - } - - 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 098faac53..0188c1c70 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; @@ -112,39 +114,29 @@ protected void processEngine(FlowData data, CloudGeoData aspectData) { Map methodData = new TreeMap<>(String.CASE_INSENSITIVE_ORDER); for (AspectPropertyMetaData property : getProperties()) { + String key = property.getName().toLowerCase(); + 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( @@ -158,6 +150,40 @@ 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, or no entry, 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 if there is 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/src/test/java/fiftyone/geolocation/data/tests/ValueTests.java b/location/geo-location/src/test/java/fiftyone/geolocation/data/tests/ValueTests.java index 060e6625c..1abcd3280 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,25 @@ 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; + // The cloud can have no value for a property at these + // coordinates, such as Suburb, and then gives the reason. + if (propertyValue.hasValue() == false) { + assertNotNull("Property '" + property.getName() + + "' has no value and no reason was given.", + 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())); } } } From 95a50a7aa37e4eb2dd0b18e8368ef8756d1edd83 Mon Sep 17 00:00:00 2001 From: James Rosewell Date: Tue, 10 Nov 2026 10:49:00 +0000 Subject: [PATCH 3/5] Say what the live Location value test checks for a property with no value The comment described what the cloud does. The test only checks that a property with no value carries a message, because its type cannot be read, and the engine always sets a message. The comment and the assertion message now say that. --- .../java/fiftyone/geolocation/data/tests/ValueTests.java | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) 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 1abcd3280..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 @@ -71,11 +71,12 @@ public static void valueTypes(Wrapper wrapper) throws Exception { value instanceof AspectPropertyValue); AspectPropertyValue propertyValue = (AspectPropertyValue) value; - // The cloud can have no value for a property at these - // coordinates, such as Suburb, and then gives the reason. + // 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 reason was given.", + "' has no value and no message.", propertyValue.getNoValueMessage()); continue; } From 9ca0cade039dc6c4cb1a0658a4c04245bfa2eb29 Mon Sep 17 00:00:00 2001 From: James Rosewell Date: Tue, 10 Nov 2026 18:01:14 +0000 Subject: [PATCH 4/5] Test that a Location property the response leaves out is missing A property the cloud response leaves out altogether must throw PropertyMissingException when read, as the Missing Properties section of the properties specification says and as .NET does. A response with no Location part leaves out every property, so the no part test now expects the exception rather than no value. A property given as null with no nullreason must have no value with the message Unknown, which no test covered once the left out case changed. CloudGeoData.setNoValueReasons stays, deprecated, so callers still compile. A test calls it and checks that the messages the engine set are unchanged. --- .../GeoLocationCloudEngineNullValueTests.java | 55 +++++++++++++++++-- .../GeoLocationCloudEngineTests.java | 13 +++-- 2 files changed, 59 insertions(+), 9 deletions(-) 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 index b4d9ccde9..8bfe09c1b 100644 --- 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 @@ -32,6 +32,7 @@ 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; @@ -44,6 +45,7 @@ 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; @@ -51,9 +53,11 @@ /** * 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. Each - * test reads the property by name, from the map the JSON Builder reads, and - * from the typed getter, and expects the same from all three. + * 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 { @@ -67,6 +71,10 @@ public class GeoLocationCloudEngineNullValueTests { "'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': [] }"; @@ -97,9 +105,11 @@ public void Value_HasValue() throws Exception { } @Test - public void NoEntry_NoValue() throws Exception { + public void NullValueWithoutReason_NoValueUnknown() throws Exception { try (GeoLocationCloudEngine engine = buildEngine()) { - CloudGeoData data = process(engine, NO_SUBURB_RESPONSE); + CloudGeoData data = process( + engine, + NULL_SUBURB_WITHOUT_REASON_RESPONSE); assertNoValue("Unknown", data.get("Suburb")); assertNoValue("Unknown", data.asKeyMap().get("Suburb")); @@ -107,6 +117,41 @@ public void NoEntry_NoValue() throws Exception { } } + @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 " + 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. + } } } From 750dd4717c20b11067a58ac293ca56db4429a3c2 Mon Sep 17 00:00:00 2001 From: James Rosewell Date: Tue, 10 Nov 2026 18:03:41 +0000 Subject: [PATCH 5/5] Leave out a Location property the response leaves out, and keep setNoValueReasons The Location cloud engine gave a property the response leaves out no value with the message Unknown. It now stores no entry for that property, so reading it throws PropertyMissingException, as the Missing Properties section of the properties specification says and as .NET does. A property the response gives as null still has no value, with its nullreason as the message. CloudGeoData.setNoValueReasons and its implementation in CloudGeoDataDefault are back, deprecated, so callers still compile. The method has no effect, because the engine sets each message on the value. --- .../geolocation/data/CloudGeoData.java | 10 ++++++++++ .../geolocation/data/CloudGeoDataDefault.java | 18 +++++++++++++++--- .../flowelements/GeoLocationCloudEngine.java | 14 ++++++++++---- 3 files changed, 35 insertions(+), 7 deletions(-) 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 1cbe50799..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 @@ -23,9 +23,19 @@ package fiftyone.geolocation.data; import fiftyone.geolocation.core.data.GeoData; +import java.util.Map; /** * The Location Element Data from the cloud. */ public interface CloudGeoData extends GeoData { + + /** + * 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 45422163a..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 @@ -28,16 +28,28 @@ import fiftyone.pipeline.engines.data.AspectPropertyMetaData; import fiftyone.pipeline.engines.flowelements.AspectEngine; import fiftyone.pipeline.engines.services.MissingPropertyService; +import java.util.Map; import org.slf4j.Logger; /** - * The Location Element Data from the cloud. The cloud engine stores every - * property as an AspectPropertyValue, so the getters of GeoDataDefault return - * them as they are. + * 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 { public CloudGeoDataDefault(Logger logger, FlowData flowData, AspectEngine engine, MissingPropertyService missingPropertyService) { super(logger, flowData, engine, missingPropertyService); } + + /** + * 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 void setNoValueReasons(Map value) { + } } 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 15a00a619..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 @@ -144,6 +144,11 @@ protected void processEngine(FlowData data, CloudGeoData aspectData) { 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"): @@ -186,12 +191,13 @@ public static Object tryToGet(JSONObject jsonObj, String key) { private static final String UNKNOWN_NULL_REASON = "Unknown"; /** - * Wrap a value read from the response. A JSON null, or no entry, is no - * value, with the reason the response gives under the property's name - * followed by nullreason. + * 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 if there is none + * @param value the value read, null or JSONObject.NULL where the + * response has none * @return the property value */ private static AspectPropertyValue toAspectPropertyValue(