NullPointerException on CRSWKT.tableDefinition test

[rouault]

On the attached GeoPackage uint16.gpkg.zip, the ETS 1.2 fails with

Test Name: | table Definition
-- | --
Test Description: | See OGC 12-128r13: Requirement 115 \| Details ↗Class Name:CRSWKTMethod Name:tableDefinitionPath:org/opengis/cite/gpkg12/extensions/crswkt/CRSWKT
Test Result: | CANTTELL
Reason of Failure: | java.lang.NullPointerException

As far as I can tell, the definition of gpkg_spatial_ref_sys is correct with the following DDL

CREATE TABLE "gpkg_spatial_ref_sys" (srs_name TEXT NOT NULL,srs_id INTEGER NOT NULL PRIMARY KEY,organization TEXT NOT NULL,organization_coordsys_id INTEGER NOT NULL,definition TEXT NOT NULL,description TEXT, definition_12_063 TEXT NOT NULL)

[dstenger]

@jyutzler Can you please take a look at the reported problem?

[jyutzler]

@rouault According to http://www.geopackage.org/spec121/#gpkg_spatial_ref_sys_cols_crs_wkt if you are using this extension, you need to add default values to your definition columns. Meanwhile, this pull request eliminates the NPE that was triggered when these values are null.

[rouault]

you need to add default values to your definition columns.

Oh I missed that. Thanks !

[bosborn]

Those defaults were removed in the current working 1.3 draft. Should these tests be future looking by allowing 1.2 and current draft support?

Adding the non nullable definition_12_063 column to an existing gpkg_spatial_ref_sys table with entries will require a default value. I assume that default value is now undefined (not necessarily 'undefined'). Or when the extension is added, is the expectation to populate a valid definition_12_063 value for each SRS entry?

[jyutzler]

Aah, you're right. I totally forgot about that. (Thank you for keeping me honest!) I removed the test entirely in https://github.com/opengeospatial/ets-gpkg12/pull/87/commits/3d78307a72e3290c5111c7a58454d01646e468bf.

The rule is that you have to have at least one definition populated but in theory there are some SRSs that cannot be adequately defined using 12-063. I don't actually know of any but we have to allow for that possibility regardless.

[bosborn]

Sounds good, thanks.

So a gpkg_spatial_ref_sys initially created with the crs wkt extension may not have a default value.

If the crs wkt extension is added to an existing gpkg_spatial_ref_sys table, an undefined default value is required ('', 'undefined', etc) for sqlite.

ALTER TABLE ADD COLUMN
If a NOT NULL constraint is specified, then the column must have a default value other than NULL.

And due to the sqlite alter limitations, the only way to remove the default value would be following the Making Other Kinds Of Table Schema Changes section (new table, copy data, delete table, rename table, etc).

[bosborn]

Now that the defaults are gone, it might be worth considering changing the definition_12_063 column to be a nullable field.

If the crs wkt extension exists, check if a SRS has a non null definition_12_063 value to use. If the crs wkt extension does not exist or has a null or unparseable definition_12_063 value, default to the definition value.

[jyutzler]

Well, we could make the column nullable, but we conventionally use undefined for that. As we've seen with this ticket, unexpected nulls in places can make things break and that is something we want to avoid.

[dstenger]

@keshav-nangare Can you please do a technical review of this fix (including a test of test suite)?

[keshavnangare]

@dstenger

According to spec the changes are fine and tested successfully with the local docker environment. The test "table Definition" passed successfully.