Repository navigation
Geojson driver and bytes buffer - #423
Merged
sgillies merged 1 commit intoFeb 20, 2017
Merged
Conversation
3 similar comments
Member
|
@youngpm this PR is great. Would you be willing to aim it at the main-1.7 branch so I can get it into a 1.7.4 release? 1.8 is still a little ways out. |
This issue is related to GDAL's GeoJSON driver. When attempting to open a buffer of geojson with a BytesCollection where the vsimem filepath has no extension, the GeoJSON driver will read the first 6000 bytes of the buffer and look for certain keys that indicate that the file is GeoJSON, ESRI JSON or TopoJSON. If those keys are not present, it assumes the driver can't open the file. The test here is against valid GeoJSON where these special keys have been moved past the first 6000 bytes. The fix employed is to append a .json extension to the vsimem filepath in the situation where the user has specifically requested the GeoJSON driver. The driver, when this extension is present, skips the 6000 byte test and assumes it can open it.
youngpm
force-pushed
the
geojson-driver-and-bytes-buffer
branch
from
February 20, 2017 05:59
c901437 to
594b782
Compare
Contributor
Author
|
👍 The appveyor test failed with this switch to the maintenance branch, but I think from looking at the history it was already failing. |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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 addresses the issue discussed in #422.
By default, the BytesCollection class maps the buffer to a filename that has no extension. GDAL will happily check each driver in its registry to see if it can open
/vsimem/<somerandomhash>. The GeoJSON driver has logic in it to quickly see if it should open the file, as you can see here. In that snippet, you can see that if passed a file path whose extension is not recognized as json, it will go ahead and check the first 6000 bytes to see if its json-like by searching for the presence of certain keys that one would expect to find in GeoJSON, ESRI JSON, or TopoJSON.The newly minted
tests/data/grenada.geojsonfile as seen here is an example of valid GeoJSON where the keys that GDAL is searching for are found past the first 6000 bytes. This causes the GeoJSON driver to be skipped and a failure to occur when trying to open it with fiona despite of its validity.The fix is to append an appropriate extension to the
vsimempath; I have done so only in the case when theGeoJSONdriver is explicitly called out. The new test demnstrates both that the expected exception is raised when trying to read this file when the GeoJSON driver is not specified, and that we succeed when it is.What do you think? My feelings won't be hurt if this is too esoteric of a change. In #422, a hack is described where you can feed the json straight to
fiona.Collectionclass directly, but that work around is not guaranteed to work in the future.