-
Notifications
You must be signed in to change notification settings - Fork 94
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
mapfishapp: Fix support for uploading KML files and convert to GeoJSON for display #2677
Merged
fvanderbiest
merged 20 commits into
georchestra:master
from
groldan:bug/GSHDF-207_mapfishapp_kml
Aug 20, 2019
Merged
mapfishapp: Fix support for uploading KML files and convert to GeoJSON for display #2677
fvanderbiest
merged 20 commits into
georchestra:master
from
groldan:bug/GSHDF-207_mapfishapp_kml
Aug 20, 2019
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
GeoTools backport PR: geotools/geotools/pull/2524. Once travis finishes I'll merge it, then once Jenkins publishes the artifacts this PR should build fine. |
groldan
force-pushed
the
bug/GSHDF-207_mapfishapp_kml
branch
2 times, most recently
from
August 16, 2019 20:54
aefc444
to
908f881
Compare
Excellent ! Can you rebase on top of master, so we don't ship commits from #2676 ? |
Yep, that's the idea, I'll do, thanks for the review |
… 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.
Making the iterator a Closeable ensures the internal InputStream gets closed once the iteration is done (provided the calling code adheres to the contract).
…ximum double precision
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
force-pushed
the
bug/GSHDF-207_mapfishapp_kml
branch
from
August 19, 2019 21:44
0fe4a87
to
b237621
Compare
@fvanderbiest rebased. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR depends on #2676 and the following GeoTools PR to be backported to 21.x: geotools/geotools/pull/2523
Fixes several issues with KML upload to mapfishapp and getting a GeoJSON response:
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):
and results in no geometry being encoded to GeoJSON, which takes the default geometry as the GeoJSON "geometry" property.
Several other fixes and improvements, check the PR's commit list.