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

Geometry null and bbox #853

Closed
fxi opened this issue Jul 20, 2017 · 10 comments
Closed

Geometry null and bbox #853

fxi opened this issue Jul 20, 2017 · 10 comments
Assignees
Milestone

Comments

@fxi
Copy link

fxi commented Jul 20, 2017

Hi,

Geometry null seems valid according to the geojson spec, but bbox does not like it. This error is thrown when the geometry is null with turf/bbox version 4.5.2;

TypeError: Cannot read property 'type' of null

gJ = {
  "type": "FeatureCollection",
  "features": [
    {
      "type": "Feature",
      "properties": {},
      "geometry": null
    }
  ]
}

turf.bbox(gJ);

Am I missing something ?

Thanks !

@DenisCarriere
Copy link
Member

DenisCarriere commented Jul 20, 2017

Geometry null seems valid according to the geojson spec

Where did you read that?

Also there's a newer GeoJSON specification RFC 7946 and the older deprecated GeoJSON specification.

Both of them say nothing about null being a valid Geometry Object.

New docs

A GeoJSON Geometry object of any type other than "GeometryCollection" has a member with the name "coordinates". The value of the "coordinates" member is an array. The structure of the elements in this array is determined by the type of geometry. GeoJSON processors MAY interpret Geometry objects with empty "coordinates" arrays as null objects.

Older docs

A GeoJSON geometry object of any type other than "GeometryCollection" must have a member with the name "coordinates". The value of the coordinates member is always an array. The structure for the elements in this array is determined by the type of geometry.

@fxi
Copy link
Author

fxi commented Jul 20, 2017

Ok, I did not see the deprecated notice, sorry, but it was there:

http://geojson.org/geojson-spec.html#feature-objects

A feature object must have a member with the name "geometry". The value of the geometry member is a geometry object as defined above or a JSON null value.

Then in the newer version:
https://tools.ietf.org/html/rfc7946#section-3.2

A Feature object has a member with the name "geometry". The value of the geometry member SHALL be either a Geometry object as defined above or, in the case that the Feature is unlocated, a JSON null value.

And I use geojsonhint to do the validation, but it did not catch this:

https://jsfiddle.net/d9zob2c5/1/

But maybe there is something obvious I've missed..

@DenisCarriere
Copy link
Member

DenisCarriere commented Jul 20, 2017

A feature object must have a member with the name "geometry". The value of the geometry member is a geometry object as defined above or a JSON null value.

🤓 Heh... I guess it does seem valid... Learn something everyday :)

I don't think any TurfJS modules test or support null geometry. @turf/bbox would be one of MANY modules that would not support null defined as a geometry.

@morganherlocker @tmcw any thoughts on if we should be handling this?

If we do chose to fix this, we would need to change @turf/meta to continue if the geometry is null (code breaks here)

@morganherlocker
Copy link
Member

@morganherlocker @tmcw any thoughts on if we should be handling this?

My opinion would be to explicitly ignore this part of the spec, similarly to how we have ignored a couple other dark corners of the spec such as the elevation coordinate or (sometimes) GeometryCollections. I'm not set on this opinion, but the idea of a null geometry really seems to break most of the common assumptions about geographic data.

@stebogit
Copy link
Collaborator

@DenisCarriere we might just add a note about this to the README file in the "Data in Turf" section.

@tmcw
Copy link
Collaborator

tmcw commented Jul 20, 2017

I'm definitely of the GeoJSON spec completism camp, hence the turf/meta work: the fewer exceptions we make the better. I don't think the overhead of doing so is egregious, and we already have similar cases, like a FeatureCollection with 0 features should have the same output as a Feature with null geometry.

@DenisCarriere
Copy link
Member

I'm open to all suggestions, every argument so far has been completely valid. Ideally we should follow the GeoJSON spec as much as possible (within reason). How about we start with a "simple" change of @turf/meta with the additional supports of null geometry and lets see how much it impacts the overall test case.

I'll submit a PR sometime tomorrow related to this, we can continue this discussion on the PR.

@DenisCarriere DenisCarriere self-assigned this Jul 21, 2017
@DenisCarriere DenisCarriere added this to the 4.6.0 milestone Jul 21, 2017
@DenisCarriere
Copy link
Member

we might just add a note about this to the README file in the "Data in Turf" section.

Good idea, whichever direction we go with, we should still include a note in that section.

CC: @stebogit

@andrewharvey
Copy link
Contributor

andrewharvey commented Jul 26, 2017

I actually ran into this just now with getGeomType which breaks when geometry is null. I went to fix it and was going to submit a PR but noticed that it's probably better to fix this across the whole of turf rather than just a function at a time (writing the test case, I then realised feature won't create a feature with null geometry, hence complicating tests for this, so all round support would be 👍 ).

I'm in the camp that turf support valid GeoJSON with null geometries and it should never crash due to this.

@DenisCarriere if you could link to your PR, we can discuss or I can try to also PR further null geometry support.

but the idea of a null geometry really seems to break most of the common assumptions about geographic data.

There are many use cases for a null geometry, most of the time I see it used when you simply don't know the location yet or the feature doesn't have a location.

@stebogit
Copy link
Collaborator

Fixed in #866

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants