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

Should GeoObject 'id' be a GUID instead of an integer #29

Closed
craigappl opened this issue Oct 30, 2018 · 10 comments
Closed

Should GeoObject 'id' be a GUID instead of an integer #29

craigappl opened this issue Oct 30, 2018 · 10 comments
Labels
enhancement New feature or request

Comments

@craigappl
Copy link
Contributor

craigappl commented Oct 30, 2018

I'm reviewing the spec and see that there's an ID that's generated which is defined as an integer. Should this be a GUID?

https://github.com/terraframe/common-geo-registry-specification/blob/master/1.0/registry-interface/rest-api.yaml#L425

Searching in the properties, I see the "uid". Maybe this is an issue of my understanding. How do you define this id?

The RESTful document cites a UID https://georegistry.geoprism.net:8443/registry/getGeoObject?uid=fc07a3f5-0696-32b1-a4cc-38df1f000507

@craigappl
Copy link
Contributor Author

I also think we should consider the type of integer and the limits associated with that.

@justinlewis justinlewis added the bug label Nov 1, 2018
@justinlewis
Copy link
Contributor

You're right! That 1st ID property was a mistake. That should not be there. The only id prop is the 'uid' prop that is in the GeoJSON properties object. This also addresses your concern about using an integer because the 'uid' prop is a string id. I'll change this.

@justinlewis
Copy link
Contributor

Removed: 8b75307

@craigappl
Copy link
Contributor Author

@justinlewis I think we need to reopen this issue. My interpretation of the GeoJSON spec Feature Object https://tools.ietf.org/html/rfc7946#section-3.2 is that we should have an id.

Text:
If a Feature has a commonly used identifier, that identifier SHOULD be included as a member of the Feature object with the name "id", and the value of this member is either a JSON string or number.

@justinlewis justinlewis reopened this Nov 9, 2018
@justinlewis
Copy link
Contributor

justinlewis commented Nov 9, 2018

That will work. We can move the current 'uid' prop out of properties up to the top level and rename it to 'id'. As the spec said in your quote above this would be good practice. One reason for caution with using the text based uid is that we've had issues with MapboxGL when those top level ID's are not integers. But since we don't want to bind a spec to a specific technology this is less of a valid concern. I thought you might be interested to know this for your efforts ahead.

So, just to be clear. We will move the uid her:
{
"type": "Feature",
"geometry": {
"type": "Point"
},
"properties": {
"uid": "string", <<<---- this
"code": "string",
"type": {...},
"status": {...},
"localizedDisplayLabel": "string"
}
}

To here and rename it to 'id' like this:

{
"type": "Feature",
"id": "string", <<<---- here AND renamed
"geometry": {
"type": "Point"
},
"properties": {
"code": "string",
"type": {...},
"status": {...},
"localizedDisplayLabel": "string"
}
}

@justinlewis justinlewis added enhancement New feature or request and removed bug labels Nov 9, 2018
@ekigamba
Copy link
Contributor

ekigamba commented Nov 9, 2018

@justinlewis What was the issue on Mapbox with using top level ID's that are not integers? Could you kindly explain. It would save us a lot of trouble while working with it

@justinlewis
Copy link
Contributor

justinlewis commented Nov 9, 2018

@ekigamba - I can't remember what our exact implementation issues where but it related to the way vector tiles are handled within MapboxGL. Data ingested into MapboxGL whether GeoJSON or Vector Tiles are stored in MapboxGL as Vector Tiles. That means GeoJSON is parsed into a VT format. A quick search uncovered these tickets but I'm sure there are more tickets than just what I listed here:

mapbox/mapbox-gl-js#2716
mapbox/mapbox-gl-js#6651
mapbox/mapbox-gl-js#7049

I agree with @craigappl that it's likely a good idea to have that unique id in the top level. However, the current spec enables you to create a temporary integer id to maintain simple state for presentation purposes. But I realize that isn't the best approach for defining a standard.

@ekigamba
Copy link
Contributor

Thanks @justinlewis

@craigappl
Copy link
Contributor Author

From 3 Dec GeoWidget Call:
Move unique id into the top level - Mapbox GL app developed, string based ids caused errors in mapbox. Apprehensive to add this string to top level. Handful of issues related to this still open
Options -
leave as is
Add as a string to the top level
Ona adds own unique id to the top level and manage it themselves
Ona - implemented string based top level id, before object gets to the CGR. UID in property is the UID coming back from from the CGR
Geoobject adapter -when caching happens, UID generated by the CGR can be cached as well. Validation - any object that tries to add itself with GUID not issues by the CGR will fail

@MelihCelik00
Copy link

@ekigamba - I can't remember what our exact implementation issues where but it related to the way vector tiles are handled within MapboxGL. Data ingested into MapboxGL whether GeoJSON or Vector Tiles are stored in MapboxGL as Vector Tiles. That means GeoJSON is parsed into a VT format. A quick search uncovered these tickets but I'm sure there are more tickets than just what I listed here:

mapbox/mapbox-gl-js#2716 mapbox/mapbox-gl-js#6651 mapbox/mapbox-gl-js#7049

I agree with @craigappl that it's likely a good idea to have that unique id in the top level. However, the current spec enables you to create a temporary integer id to maintain simple state for presentation purposes. But I realize that isn't the best approach for defining a standard.

I also need that featureId but I still couldn't find any workaround. Are there any developments to solve this issue? Mapbox and frontend have different featureIds which made me crazy not to able to solve this issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants