vulnerability to PROJ6/GDAL3

[rsbivand]

Running revdeps from sp (sp (my github fork) with development rgdal from R-Forge):

> packageVersion("sp")
[1] ‘1.3.4’
> packageVersion("rgdal")
[1] ‘1.5.3’
> rgdal::rgdal_extSoftVersion()
          GDAL GDAL_with_GEOS           PROJ             sp 
       "3.0.2"         "TRUE"        "6.2.1"        "1.3-2" 

a test fails:

#filter is to disable tests that rely on external servers
> test_check("jsonlite", filter="toJSON|fromJSON|libjson|serializeJSON")
Loading required package: jsonlite
── 1. Failure: Advanced S4 serialization (@test-serializeJSON-S4.R#38)  ────────
`out` not identical to `meuse`.
Attributes: < Component "proj4string": Attributes: < Names: 1 string mismatch > >
Attributes: < Component "proj4string": Attributes: < Length mismatch: comparison on first 2 components > >
Attributes: < Component "proj4string": Attributes: < Component 2: 1 string mismatch > >

══ testthat results  ═══════════════════════════════════════════════════════════
[ OK: 1473 | SKIPPED: 0 | WARNINGS: 221 | FAILED: 1 ]
1. Failure: Advanced S4 serialization (@test-serializeJSON-S4.R#38) 

Error: testthat unit tests failed

See:

http://rgdal.r-forge.r-project.org/articles/PROJ6_GDAL3.html
r-spatial/sf#1231
r-spatial/sf#1187
r-spatial/sf#1146
r-spatial/discuss#28

for background. See:

r-spatial/discuss#28 (comment)

for a way of testing fixes in a docker container contributed by Jakub Nowosad.

[jeroen]

Hmm I'm seeing this (but perhaps the issue is that homebrew currently ships gdal 2.4.2 + proj 6.3.0.

Loading required package: jsonlite
── 1. Error: Advanced S4 serialization (@test-serializeJSON-S4.R#33)  ────────────────────────────
rgdal version mismatch
Backtrace:
 1. sp::`coordinates<-`(`*tmp*`, value = ~x + y)
 2. sp::`coordinates<-`(`*tmp*`, value = ~x + y)
 3. sp::SpatialPointsDataFrame(...)
 8. sp::CRS(as.character(NA))

══ testthat results  ═════════════════════════════════════════════════════════════════════════════
[ OK: 1482 | SKIPPED: 0 | WARNINGS: 0 | FAILED: 1 ]
1. Error: Advanced S4 serialization (@test-serializeJSON-S4.R#33)

[rsbivand]

Maybe. GDAL 2 should match PROJ 5, but can be built with PROJ 6 if a compile flag is set. GDAL 3 requires PROJ >= 6. There is a lot of volatility at the moment, I'm afraid, but it isn't going to get better, and attempts in sf and *sp' workflows, although advancing, are not yet stable. I'm testing here GDAL 3.0.2 released and PROJ 6.2.1 released, and developing with GDAL 3.1 master and PROJ 6.3.0 released. There are also weird interactions between C++ objects in GDAL and PROJ for the most recent code (vanishing projection definition tree nodes ...), so both sp and sf are transitioning to WKT2-2018/9 from PROJ strings as CRS descriptors. It is all very messy. Please scan the sf issues for an impression of how muddled things are at the moment.

[rsbivand]

Problem still present with current rgdal from R-Forge, PROJ >= 6 & GDAL >= 3. The match should be PROJ < 6 & GDAL < 3; PROJ >= 6 & GDAL >= 3.

The specific problem is that serializeJSON() and unserializeJSON() do not handle comments, which are used in sp with new PROJ/GDAL to piggy-back WKT2_2019 onto an S4 object with a Proj4 string in it:

library(jsonlite)
data(meuse, package = 'sp', envir = environment())
sp::coordinates(meuse) <- ~x+y
sp::proj4string(meuse) <- sp::CRS("+init=epsg:28992")
out <- jsonlite::unserializeJSON(jsonlite::serializeJSON(meuse))
inherits(out, "SpatialPointsDataFrame")
# [1] TRUE
isS4(out)
# [1] TRUE
all.equal(out, meuse)
# [1] "Attributes: < Component “proj4string”: Attributes: < Names: 1 string mismatch > >"                         
# [2] "Attributes: < Component “proj4string”: Attributes: < Length mismatch: comparison on first 2 components > >"
# [3] "Attributes: < Component “proj4string”: Attributes: < Component 2: 1 string mismatch > >"  
> all.equal(slot(out, "proj4string"), slot(meuse, "proj4string"))
[1] "Attributes: < Names: 1 string mismatch >"                         
[2] "Attributes: < Length mismatch: comparison on first 2 components >"
[3] "Attributes: < Component 2: 1 string mismatch >"                   
slot(out, "proj4string")
# CRS arguments:
# +proj=sterea +lat_0=52.1561605555556 +lon_0=5.38763888888889
# +k=0.9999079 +x_0=155000 +y_0=463000 +ellps=bessel +units=m +no_defs 
slot(meuse, "proj4string")
# CRS arguments:
#  +proj=sterea +lat_0=52.1561605555556 +lon_0=5.38763888888889
# +k=0.9999079 +x_0=155000 +y_0=463000 +ellps=bessel +units=m +no_defs 
str(comment(slot(meuse, "proj4string")))
# chr "PROJCRS[\"Amersfoort / RD New\",\n    BASEGEOGCRS[\"Amersfoort\",\n        DATUM[\"Amersfoort\",\n            E"| __truncated__
str(comment(slot(out, "proj4string")))
# NULL

I can't see a non-invasive way to detect and pass through the comment() WKT2_2019 string; I think pack() would need to do that.

By the way, sp polygon classes also use comment() to kludge Simple Features exterior/interior ring structures, so if support for sp may matter going forward, support for comments may be useful in those cases too.

Why comments? Because adding or changing slots in a class definition breaks objects stored serialized, often in data/ folders, but also say in gadm.org. So I had to go with comments as a kludge both for SF-rings and now for WKT2_2019 strings, not elegant, but hopefully the least disruptive option.

[jeroen]

The reason that the Comment slot does not get serialized is that it is missing from the CRS class definition. So is that intentional?

> getClassDef('CRS')
Class "CRS" [package "sp"]

Slots:

Name:   projargs
Class: character

[rsbivand]

Briefly, yes, intentional, as with Polygons objects. Changing the S4 definition breaks stored serialized objects. More later with links to motivation. Roger Bivand Falsensvei 32 5063 Bergen søn. 5. apr. 2020, 11.37 skrev Jeroen Ooms <notifications@github.com>:

…

The reason that the Comment slot does not get serialized is that it is missing from the CRS class definition. So is that intentional? > getClassDef('CRS')Class "CRS" [package "sp"] Slots: Name: projargsClass: character — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#303 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACNZ3BDHBAWTJ4OBPHVNFK3RLBGNLANCNFSM4KEVRCHA> .

[jeroen]

OK, we can't include the Comment attribute in the json, because if we reinstantiate the CRS object in the unserializeJSON step that yields an error:

invalid name for slot of class “CRS”: comment

I'll just work around it in the unit test for now. This is not a very important part of jsonlite anyway.

[rsbivand]

I agree, muting the test until things stabilize is a rational choice - we can't guard against further changes in PROJ/GDAL necessitating further representation changes. Up to now, the Proj4 string has also been able to contain a specified operation (+towgs84=) to transform to WGS84/EPSG:4326. This may now also go away, as may +nadgrids=, as both are now seen as operation specifications, not coordinate reference system specifications.

If you can test "all-but" the sp slot with the "CRS" object, that should work, but for now, simply muting tests of spatial objects makes a lot of sense.

My earlier point about breaking stored serialized S4 objects by changing the definition referred to r-spatial/sf#1187 (comment), applying to objects stored for example by https://gadm.org.

[jeroen]

So do I have to push out a new release of jsonlite for you to be able to release a new rgdal?

[rsbivand]

Not really, I've submitted rgdal 1.5-8 rev 990 to CRAN, and am waiting for reactions. I think you can release when it suits you, but the submitted rgdal will I think break jsonlite with released sp. Thanks for your understanding.