Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[backport 19.04] mapfishapp: Fix support for uploading KML files and convert to GeoJSON for display #2678

Merged

Conversation

groldan
Copy link
Member

@groldan groldan commented Aug 16, 2019

Backport of #2677 to 19.04

… upload

File upload (convert and download as GeoJSON) was being
handled by the same controller method (toGeoJson), which
was cumbersome and long.

Splitted it out into separate methods:
- toGeoJsonFromMultipart
- toGeoJsonFromURL
It was returning text/html instead of application/json.
Add -Duser.home JVM argument to docker command to
avoid the following kind of warnings on the log
due to the JVM background job not being able to
periodically flush user preferences.

mapfishapp_1 | Aug 14, 2019 4:16:03 PM java.util.prefs.FileSystemPreferences syncWorld
mapfishapp_1 | WARNING: Couldn't flush user prefs: java.util.prefs.BackingStoreException: Couldn't get file lock
Calls to /togeojson were building the wholse GeoJSON
content on a StringBuilder and then to a String.

This patch makes the process streaming, saving the whole
response to a temporary file without loading the full
feature collection in memory nor building the response
in memory, and then streaming to the http response.

Also improves exception handling
Replace the logic to find out the version of a KML
file to use an XML pull parser instead of a SAX one plus
an exception to break the walk. Using exceptions for flow
control is awful.
21-SNAPSHOT required to get the patch that fixes
KML 2.2 Region/LanLonAltBox.
Make /togeojson return `text/html` instead of
`application/json` if such was requested to avoid
a JavaScript warning on the console due to the fact
that Ext.js can't send request headers upon uploading
a file through a form.
* Response content-type: the API returns application/json
as response content type, except if the request explicitly
asks for text/html, which is the case for the Ext.js submitted
form when uploading a file. In this case text/html is returned
to avoid an error/warning on the Javascript client side.

* KML file parsing improvements (KmlFeatureSource):
 - Parsing to FeatureCollection made streaming, it was loading
   all the Features in memory.
 - Added on-the fly reprojection through FeatureCollection decorator
 - Force the default geometry attribute to be the "Geometry" attribute,
   the GeoTools parser returns Features whose schema's default
   geometry attribute is "LookAt", since it occurs before "Geometry",
   and results in no geometry being encoded to GeoJSON, which takes
   the default geometry as the GeoJSON "geometry" property.
 - Remove the "Style" attribute through a feature collection decorator,
   the GeoTools parser sets it to a FeatureTypeImpl instance which
   gets encoded to GeoJSON as FetureTypeImpl.toString(), which makes
   no sense.

Added tests for KML 2.2 and 2.1 parsing and reprojecting.
@groldan groldan force-pushed the bug/GSHDF-207_mapfishapp_kml_19.04 branch from 1839507 to abbd1c8 Compare August 16, 2019 05:43
Making the iterator a Closeable ensures the internal
InputStream gets closed once the iteration is done (provided
the calling code adheres to the contract).
It doesn't make sense to extend a class to override it completely.
FeatureJSON2 extends FeatureJSON and was overriding all its
public methods, whereas only writeFeatureCollection() is modified.
Make the intent of FetureJSON2.writeFeatureCollection()
override clear and the method cleaner.

Do not swallow exceptions while encoding
the feature collection. If thrown, they're caused by
programming erros and shall be propagated.
The encoding of FeatureCollections to GeoJSON was
partially streaming, by means of using a FeatureCollectionEncoder
as the "features" property on a Map<String, Object>. This
encoder, being a JSONStreamAware, encodes feature by feature
to the output stream, but it did so in a not so streaming
fashion: by first converting each Feature to a String and then
writing the String verbatim to the output stream.

Due to the way most of the GeoTools GeoJSON writer is currently
implemented, that resulted in an explosion of objects on the
heap for each Feature (java.io.Writer, String, and other object
instances).

This patch makes FeatureEncoder also a org.json.simple.JSONStreamAware
instead of a simple org.json.simple.JSONAware, which merely converts
a Feature to String, to instead write to a org.json.JSONWriter
directly without incurring in the perf and memory overhead.

To make it optimal, the only missing piece would be adding
the ability to also stream out Geometry serialization, which
is currently handled by the GeoTools GeometryJSON class and builds
a java.util.Map with nested properties, Lists, and Maps to then
serialize it as String.
@groldan groldan force-pushed the bug/GSHDF-207_mapfishapp_kml_19.04 branch from b69a6ea to 48a8fbc Compare August 16, 2019 20:51
@fvanderbiest
Copy link
Member

I'm not sure fd9d22b should be part of this PR since epsg-extension is shipped with 19.04

@groldan
Copy link
Member Author

groldan commented Aug 19, 2019

I'm not sure fd9d22b should be part of this PR since epsg-extension is shipped with 19.04

@fvanderbiest I see, problem is, epsg-extension is also used by extractorapp, and if we keep it in mapfishapp we'll be getting the bellow exception, and it won't be used anyway.
In order to actually include it, we should also port epsg-extension itself and extractorapp to the latest stable geotools version, which was beyond the scope of this PR for 19.04.
That said, it can be done, not sure if it's worth it though. Let me know what's your preference.

WARNING: Can't load a service for category "CRSAuthorityFactory". Cause is "ServiceConfigurationError: org.opengis.referencing.crs.CRSAuthorityFactory: Provider org.geotools.referencing.factory.epsg.CustomCodes could not be instantiated".
java.util.ServiceConfigurationError: org.opengis.referencing.crs.CRSAuthorityFactory: Provider org.geotools.referencing.factory.epsg.CustomCodes could not be instantiated
	at java.util.ServiceLoader.fail(ServiceLoader.java:232)
	at java.util.ServiceLoader.access$100(ServiceLoader.java:185)
	at java.util.ServiceLoader$LazyIterator.nextService(ServiceLoader.java:384)
	at java.util.ServiceLoader$LazyIterator.next(ServiceLoader.java:404)
	at java.util.ServiceLoader$1.next(ServiceLoader.java:480)
	at org.geotools.util.factory.FactoryRegistry.register(FactoryRegistry.java:1041)

@fvanderbiest
Copy link
Member

OK, you're right. Let's keep it simple ...

@fvanderbiest fvanderbiest merged commit a064acb into georchestra:19.04 Aug 20, 2019
@fvanderbiest fvanderbiest added this to the 19.06 milestone Aug 20, 2019
@groldan groldan deleted the bug/GSHDF-207_mapfishapp_kml_19.04 branch August 23, 2019 15:52
@fvanderbiest fvanderbiest modified the milestones: 19.09, 19.12 Oct 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants