[jwrosewell]

The problem

The full build of #93 failed once on Windows Java 8 because of a live cloud answer, in this job (attempt 1 of run 42722443022).

FiftyoneValueTests.ValueTests_Cloud_ValueTypes:42 Value of 'Suburb' was of type Null but should have been String.

The cloud gave Suburb as JSON null for the test coordinates. A property the cloud gives as null should have no value, with the text under suburbnullreason as its message. The Location cloud Element Data held org.json.JSONObject$Null instead, whose simple name is the Null in the message. There is no issue for this defect.

The cause

At rc 9846076, where the defect was found, GeoLocationCloudEngine.processEngine put each value into the Element Data unwrapped. For a property that is not a list or JavaScript it stored geoObj.opt(...), and org.json's opt returns the stored JSONObject.NULL for a JSON null, which is not Java null. The engine then handed the Element Data a separate map of reasons (lines 138 to 147) through setNoValueReasons, which only the typed getters used, through CloudGeoDataDefault.tryGetValue and setValue. So, for a property the cloud gave as null,

1. get("Suburb") and asKeyMap() gave JSONObject.NULL, which the live test read at line 61,
2. the JSON Builder, which reads asKeyMap() and writes nullreason only for an AspectPropertyValue with no value (JsonBuilderElement, lines 284 to 298), wrote "suburb": null with no suburbnullreason,
3. getSuburb() did give no value with the reason.

A property the response left out was stored as Java null, so get gave null, the typed getters gave no value with an empty message, and the JSON Builder wrote no entry.

The Device Detection and IP Intelligence cloud engines test with isNull and store an AspectPropertyValue with the nullreason text, so they do not have the JSONObject.NULL defect. CloudAspectEngineBase.createAPVMap, which the property keyed engines use, is given JSONObject.toMap(), and toMap turns JSONObject.NULL into Java null before line 293 checks it. The dependency:tree step of an earlier green run, run 42785280464, shows the Location cloud engine builds against org.json:json:jar:20231013, the version linked here.

What the specification says

All links are to specifications rc at 093bf3e.

The Processing section of the Cloud Aspect Engine page describes how each value is read.

3. Populate the instance with the value from the raw JSON.
4. If there is no value in the JSON, Look for a JSON Property with the same name suffixed with nullreason. Use this value to set the NoValueMessage on the instance.

The same page says, at lines 84 and 85, "Where the response JSON has no portion for the Engine's Data Key, the Cloud Aspect Engine leaves the Element Data without values and raises no error."

The Null values section of the Properties page says what no value is.

Where 'no value' has been assigned, attempting to access the value MUST throw an error/exception with a customizable message explaining why the value is not set.

The Missing Properties section of the same page covers a property the response leaves out.

When the user attempts to access a Property that is defined for an Engine, but does not exist in the result set for some reason, the system MUST throw an error/exception with a message explaining why the Property is not available.

The Process section of the JSON Builder Element page says "A Property that has been assigned 'no value' MUST be written as 'null', with the nullreason meta-Property in the table below."

What .NET does

On 51degrees-dotnet rc at 56a5d60, GeoLocationCloudEngine.ProcessCloudEngine passes the response's part to CreateAPVDictionary, which loops over the response's entries (line 561) and wraps each in an AspectPropertyValue. A JSON null, which the CloudJsonConverter reads as null, gets the nullreason text as its message (line 605), or Unknown when there is none (line 611). A property the response leaves out gets no entry, and reading it throws PropertyMissingException from the missing property service (AspectDataBase.GetAs, lines 314 to 327).

What changed

Two points were decided on 10 November. A property the response leaves out throws PropertyMissingException, as .NET does and as the Missing Properties section says. setNoValueReasons stays, deprecated, so no caller breaks in a patch release.

• GeoLocationCloudEngine.processEngine skips a property the response leaves out (line 149), so the Element Data has no entry for it and reading it throws PropertyMissingException. A response with no Location part therefore leaves every property out, raises no error while processing, and throws when a property is read. Every other property is read as before and stored through the new toAspectPropertyValue. A value becomes an AspectPropertyValue holding it. A JSON null becomes no value with the nullreason text as its message, or Unknown when the response gives none, as .NET does and as Java's createAPVMap does (lines 304 to 311).
• The Device Detection and IP Intelligence cloud engines differ from Location in two ways, which this draft does not change. They store an entry with no value for a property the response leaves out, because isNull is true for no entry. They also leave the message null where the response gives no nullreason (getNoValueReason in Device Detection and in IP Intelligence), where Location gives Unknown.
• CloudGeoDataDefault no longer wraps values itself, so its map of reasons and its getters, which only repeated those of GeoDataDefault through that map, are removed. setNoValueReasons stays on it (lines 45 to 55) and on CloudGeoData, marked deprecated. The method has no effect, because the engine now sets each message on the value, and nothing in this repository calls it.
• The live ValueTests.valueTypes expected the unwrapped value. It now expects an AspectPropertyValue, and checks the type of the value inside it, as the Device Detection cloud value test does. A property with no value, as Suburb had on the failed job, now passes, because its type cannot be read then. For such a property the test checks only that it carries a message, and the engine always sets one.
• GeoLocationCloudEngineTests.ProductPresent_ResponseWithoutPart_PropertyIsMissing, added by Build a cloud engine whose product the resource key lacks, and log that once #84 as ProductPresent_ResponseWithoutPart_NoValues, expected no value for a property when the response has no Location part. It now expects PropertyMissingException, following the first decision.
• mockito-core is added to the geo-location.cloud test dependencies, the same block Build a cloud engine whose product the resource key lacks, and log that once #84 added, so the merge of rc leaves one copy. The parent pom manages its version.

For the release notes

• get(name) and asKeyMap() on the Location cloud Element Data now give an AspectPropertyValue for each property the response holds, as the Device Detection and IP Intelligence cloud Element Data do. Code that read the unwrapped value by name must check hasValue() and call getValue(). The typed getters such as getTown() keep their signatures.
• A Location property the cloud gives as null with a nullreason now has that text as its message when read by name, as it already had from the typed getters, and the JSON Builder writes it as null with that nullreason.
• A Location property the cloud gives as null with no nullreason now has the message Unknown, by name and from the typed getters, where the typed getters gave an empty message and reading by name gave JSONObject.NULL. The JSON Builder writes it as null with nullreason Unknown, where it wrote null with no nullreason.
• A Location property the response leaves out now throws PropertyMissingException when read, by name or from the typed getters. Before, reading by name gave null and the typed getters gave no value with an empty message. The JSON Builder still writes no entry for it.
• CloudGeoData.setNoValueReasons is deprecated and has no effect.
• The JSON Builder changes follow from the lines quoted above and are not tested in this pull request.

How to verify

mvn -pl location/geo-location.cloud -am install -DskipTests
mvn -pl location/geo-location.cloud surefire:test -Dtest=GeoLocationCloudEngineNullValueTests,GeoLocationCloudEngineTests

GeoLocationCloudEngineNullValueTests feeds the engine a response through a mocked Cloud Request Engine, and reads each property by name, from asKeyMap(), which the JSON Builder reads, and from the typed getter.

Test	Response	Expected
NullValue_NoValueWithNullReason	suburb null, suburbnullreason given	no value, message is the suburbnullreason text
NullValueWithoutReason_NoValueUnknown	suburb null, no suburbnullreason	no value, message Unknown
Value_HasValue	town is Reading	value Reading
NoEntry_PropertyIsMissing	no suburb entry	no entry in asKeyMap(), and get and getSuburb() throw PropertyMissingException
SetNoValueReasons_ChangesNothing	suburb null, suburbnullreason given, then setNoValueReasons with another message	the message is still the suburbnullreason text

The red and green runs used a temporary workflow on a tmp/ branch that builds the named modules on Ubuntu with Java 17 and runs the named tests, the live ones with the bespoke resource key.

• Red. Run 42871052117 at 9ca0cad, the tests on the previous fix with rc ff6e4c0 merged in, failed as expected, in two jobs. As committed, the tests do not compile, because the previous fix had removed setNoValueReasons.

  GeoLocationCloudEngineNullValueTests.java:[138,17] cannot find symbol
    symbol:   method setNoValueReasons(java.util.Map<java.lang.String,java.lang.String>)
  

  With that one test removed, the two tests for a property the response leaves out fail.

  FAIL GeoLocationCloudEngineNullValueTests#NoEntry_PropertyIsMissing: java.lang.AssertionError
  FAIL GeoLocationCloudEngineTests#ProductPresent_ResponseWithoutPart_PropertyIsMissing: A PropertyMissingException should have been thrown
  2 of 9 tests failed
  

  The earlier red run for the JSON null case, run 42784353899 at 1fd3e93 on rc 9846076, failed with Expected an AspectPropertyValue but found org.json.JSONObject$Null.
• Green. Run 42871402845 at 750dd47, this draft's head, passed, with the live Location value tests.

  PASS GeoLocationCloudEngineNullValueTests#NullValue_NoValueWithNullReason
  PASS GeoLocationCloudEngineNullValueTests#NoEntry_PropertyIsMissing
  PASS GeoLocationCloudEngineNullValueTests#NullValueWithoutReason_NoValueUnknown
  PASS GeoLocationCloudEngineNullValueTests#SetNoValueReasons_ChangesNothing
  PASS GeoLocationCloudEngineNullValueTests#Value_HasValue
  PASS GeoLocationCloudEngineTests#DiscoveryNotComplete_LoadsTheMetadataLater
  PASS GeoLocationCloudEngineTests#ProductMissing_ProcessLogsAndThrowsNothing
  PASS GeoLocationCloudEngineTests#ProductPresent_ResponseWithoutPart_PropertyIsMissing
  PASS GeoLocationCloudEngineTests#ProductMissing_PropertyIsMissing
  PASS GeoLocationCloudEngineTests#ProductMissing_BuildsWithEmptyMetadata
  PASS FiftyoneValueTests#ValueTests_Cloud_AvailableProperties
  PASS FiftyoneValueTests#ValueTests_Cloud_ValueTypes
  PASS FiftyoneValueTests#ValueTests_Cloud_TypedGetters
  0 of 13 tests failed
  

• Full build. Run 42871895314, common-ci's nightly-pull-request.yml as a dry run on rc ff6e4c0, passed. All nine non-macOS jobs succeeded (Ubuntu Java 8, 11, 17 and 21, Ubuntu ARM Java 17, and Windows Java 8, 11, 17 and 21), and Configure printed Left out 6 macOS entries. Every Setting 'PullRequestSha' line reads 750dd4717c20b11067a58ac293ca56db4429a3c2, this draft's head. In every job GeoLocationCloudEngineNullValueTests ran 5 tests, GeoLocationCloudEngineTests 5 and the live FiftyoneValueTests 3, with no failures or errors.

macOS was left out of this draft's full build under the 9 November decision. The rc head is built with macOS before it merges into main.

Other drafts

This branch has rc ff6e4c0 merged in, which holds #84, #85, #86, #89, #92, #93 and #94, all now merged. The merge had no conflicts. Of those, only #84 and #86 changed the Location cloud engine, and this draft changes one of #84's tests, as above.

What remains

Nothing. The rc head is built with macOS before it merges into main.