Add geojson parsing support #655

lognaturel · 2022-01-25T23:21:36Z

This PR adds support for a geojson external secondary instance with:

a .geojson extension
a FeatureCollection at the top level
all features with Point geometry

If the file is parsed successfully, then an external secondary instance is available for use by clients. At that point there's no trace of it having been geojson previously. This matches the XML and CSV external instance implementations.

What has been done to verify that this works as intended?

Ran it in Collect!

Why is this the best possible solution? Were any other approaches considered?

First I looked to Collect to see how json is parsed there. The org.json library is imported so I thought that's what was being used but it's not (getodk/collect#4997). Once I found that out I looked at gson or jackson for parsing. I went with Jackson because it's slightly more broadly used and Gson is in maintenance mode. Some of the folks who worked on Gson are working on moshi at Square but I'd rather go with the venerable Jackson for now.

I then had to pick a parsing strategy. I decided to stream then using data binding for each feature to reduce memory usage (compared to parsing the whole thing in memory). This matches what the CSV parser does.

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

This is isolated. The only risk to existing behavior is that I messed up the conditional that chooses which parser to use.

Do we need any specific form for testing your changes? If so, please attach one.

https://test.getodk.cloud/#/projects/149/forms/geojson

geojson.zip

Does this change require updates to documentation? If so, please file an issue here and include the link below.

getodk/docs#1423

lognaturel · 2022-01-31T16:49:19Z

src/main/java/org/javarosa/core/model/instance/ExternalDataInstance.java

        String path = getPath(instanceSrc);
-        return instanceSrc.contains("file-csv") ?
-            CsvExternalInstance.parse(instanceId, path) : XmlExternalInstance.parse(instanceId, path);
+        return instanceSrc.contains("file-csv") ? CsvExternalInstance.parse(instanceId, path)


I considered pulling this out into a more conventional factory so it could be tested but I don't see a big benefit given how everything else is structured at the moment.

One problem here is that it doesn't look like the new case is covered by any tests. Do we maybe want an outside-in/black-box/smoke test level test for this feature to check everything is in place?

lognaturel · 2022-01-31T19:49:54Z

Apologies for all the force pushes! @seadowg we'd talked about leaving the node-based parsing but I looked at the CSV implementation and it streams so I changed my mind. It seemed like we'd want to go for lower memory usage sooner than later no matter what.

seadowg

This is looking good to me after an initial scan through. There are a few pieces of testing that could be improved but the overall structure is nice and simple and makes sense!

build.gradle

seadowg · 2022-02-07T13:50:41Z

src/main/java/org/javarosa/core/model/instance/ExternalDataInstance.java

        String path = getPath(instanceSrc);
-        return instanceSrc.contains("file-csv") ?
-            CsvExternalInstance.parse(instanceId, path) : XmlExternalInstance.parse(instanceId, path);
+        return instanceSrc.contains("file-csv") ? CsvExternalInstance.parse(instanceId, path)


One problem here is that it doesn't look like the new case is covered by any tests. Do we maybe want an outside-in/black-box/smoke test level test for this feature to check everything is in place?

src/main/java/org/javarosa/core/model/instance/geojson/GeoJsonExternalInstance.java

seadowg · 2022-02-07T13:58:09Z

src/test/java/org/javarosa/core/model/instance/geojson/GeoJsonExternalInstanceTest.java

+        }
+    }
+
+    private static final String GEOMETRY_COLLECTION_GEOJSON = "{\n" +


I feel like the JSON here is difficult enough to read that we should maybe pull out a fixture file or use a JSON framework to put it together inline.

Pulled out files since that addresses your comment about the protected method too. I used the constant because it was easier for me to jump to as I was writing the tests but I also see your point about the visual noise. Too bad we can't use Java 15 """.

Kotlin has multiline strings with support all the way back to Java 6 (https://kotlinlang.org/docs/coding-conventions.html#strings).

src/test/java/org/javarosa/core/model/instance/geojson/GeoJsonExternalInstanceTest.java

src/main/java/org/javarosa/core/model/instance/geojson/GeoJsonExternalInstance.java

seadowg · 2022-02-07T14:11:35Z

src/main/java/org/javarosa/core/model/instance/geojson/GeojsonGeometry.java

+
+    public String getOdkCoordinates() {
+        if (!(type.equals("Point") && coordinates.size() == 2)) {
+            throw new UnsupportedOperationException("Only Points are currently supported");


Not going to be a problem for long, but maybe we should just skip unsupported elements rather than exploding? That would let us test in Collect with .geojson files without worrying about it crashing the app.

I think we likely will have a release with only points. In that case it feels better to fail for reasons similar to the "fast external itemsets" bug you identified -- it's not great if a subset of user features are silently filtered out. I don't feel strongly about it but that was my reasoning for exploding.

That makes sense. Let's use a checked exception then, so there's not a risk of Collect missing this and it resulting in a crash.

Mistake

seadowg

Looking great! As I commented inline, I think we should add a checked exception to deal with the unsupported feature type case.

lognaturel mentioned this pull request Jan 25, 2022

Add select from geojson support getodk/docs#1423

Closed

1 task

lognaturel marked this pull request as draft January 25, 2022 23:51

lognaturel commented Jan 31, 2022

View reviewed changes

lognaturel force-pushed the geojson-jackson branch 3 times, most recently from 734cb1f to 5b34936 Compare January 31, 2022 17:36

lognaturel marked this pull request as ready for review January 31, 2022 17:41

lognaturel requested a review from seadowg January 31, 2022 17:42

lognaturel force-pushed the geojson-jackson branch from 5b34936 to 6d31598 Compare January 31, 2022 19:17

Add geojson parsing support

b33034d

lognaturel force-pushed the geojson-jackson branch from 6d31598 to b33034d Compare January 31, 2022 19:41

seadowg requested changes Feb 7, 2022

View reviewed changes

lognaturel requested a review from seadowg March 2, 2022 22:30

seadowg previously approved these changes Mar 3, 2022

View reviewed changes

seadowg requested changes Mar 3, 2022

View reviewed changes

lognaturel force-pushed the geojson-jackson branch from 45f943f to 1b5e349 Compare March 3, 2022 18:06

Pull test geojson out, increase test coverage

e592593

lognaturel force-pushed the geojson-jackson branch from 1b5e349 to e592593 Compare March 3, 2022 18:09

lognaturel requested a review from seadowg March 3, 2022 18:13

lognaturel mentioned this pull request Mar 3, 2022

Prepare 3.4.0 snapshot #657

Merged

lognaturel merged commit aac2a8a into getodk:master Mar 3, 2022

lognaturel deleted the geojson-jackson branch March 3, 2022 18:22

lognaturel mentioned this pull request May 10, 2022

Increase robustness of geojson parsing by allowing extra properties and null values #681

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add geojson parsing support #655

Add geojson parsing support #655

lognaturel commented Jan 25, 2022 •

edited

lognaturel Jan 31, 2022

seadowg Feb 7, 2022

lognaturel commented Jan 31, 2022 •

edited

seadowg left a comment

seadowg Feb 7, 2022

seadowg Feb 7, 2022

lognaturel Mar 2, 2022 •

edited

seadowg Mar 3, 2022

seadowg Feb 7, 2022

lognaturel Mar 2, 2022

seadowg Mar 3, 2022

seadowg left a comment

Add geojson parsing support #655

Add geojson parsing support #655

Conversation

lognaturel commented Jan 25, 2022 • edited

What has been done to verify that this works as intended?

Why is this the best possible solution? Were any other approaches considered?

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

Do we need any specific form for testing your changes? If so, please attach one.

Does this change require updates to documentation? If so, please file an issue here and include the link below.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lognaturel commented Jan 31, 2022 • edited

seadowg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lognaturel Mar 2, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seadowg left a comment

Choose a reason for hiding this comment

lognaturel commented Jan 25, 2022 •

edited

lognaturel commented Jan 31, 2022 •

edited

lognaturel Mar 2, 2022 •

edited