ngageoint / geopackage-js

GeoPackage JavaScript Library
http://ngageoint.github.io/geopackage-js/
MIT License
304 stars 78 forks source link

GeoJSON id from feature id property #177

Closed juancromero-cotesa closed 2 years ago

juancromero-cotesa commented 2 years ago

Take in account de id property on queryForGeoJSONIndexedFeaturesWithBoundingBox method to create GeoJSON with id property from feature property

danielbarela commented 2 years ago

The spirit of this change, to not lose the id value of a feature row, is correct, however I believe that the implementation could be different. If there is both a _feature_id and an id value in the feature row values, the last one through the loop would win. This presents issues with legacy systems that we interact with. The _feature_id being set as the geoJson.id is probably not optimal either, however, legacy systems depend on this and so it must stay.

What I would suggest is to move the || key.toLowerCase() === 'id' from line 368, to line 370 so that the id value will be set as a property in the geoJson properties. This will make it so we do not lose information, and then if a system has a reason to take the id property from the geoJson properties and make it the actual id of the geoJson object, then that system can handle that case while the iteration is taking place.

So my suggestion would be this for lines 368 - 374

if (key.toLowerCase() === '_feature_id') {
  geoJson.id = featureRow.values[key] as string | number;
} else if (key.toLowerCase() === 'id') {
  geoJson.properties[key] = featureRow.values[key];
} else if (key.toLowerCase() === '_properties_id') {
  geoJson.properties[key.substring(12)] = featureRow.values[key];
} else {
  geoJson.properties[columnMap[key].displayName] = featureRow.values[key];
}
juancromero-cotesa commented 2 years ago

Right Daniel!, without loosing the id property the developer has the chance to use as he wants and the code don't break legacy code. I think it's a good solution.

Thanks for all

juancromero-cotesa commented 2 years ago

Done!

juancromero-cotesa commented 2 years ago

Hi Daniel,can you merge this pr? Thanks

danielbarela commented 2 years ago

Merged. Sorry I was on vacation and just got back today.

juancromero-cotesa commented 2 years ago

Thanks a lot Daniel, great job !