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

Fix GeometryCollection::getAll extension method #3295

Merged
merged 2 commits into from
Nov 19, 2020

Conversation

jpolchlo
Copy link
Contributor

@jpolchlo jpolchlo commented Sep 24, 2020

Overview

As noted in #3289, we have some problems dealing with GeometryCollections. This bug was traced to the implementation of getAll which used reflection to extract the contained geometries that matched a desired type. This was intended to be used to extract specific types or subclasses (gc.getAll[Geometry] would be useful). The problem was with gc.getAll(GeometryCollection), since MultiPoint, MultiLineString, and MultiPolygon are subclasses of GeometryCollection. Therefore, gc.getAll[GeometryCollection] would return all Multi* and GeometryCollection entities. This caused problems for methods like reproject, which would duplicate Multi* elements.

This PR, in it's first form, is for discussion purposes. The simple solution presented here is that getAll loses it's subclass semantic. This is the most straightforward, requiring no additional changes, but it does break the semantics, and therefore, technically must wait for GT 4. A more laborious change could be implemented, keeping the subclass semantics of the old getAll implementation, but reimplementing reproject and other functions that don't correctly differentiate types. This latter solution is more work, but may be preferred to maintain certain functionality.

In all cases, GT 3 broke with the model of GT 2 and down where the contents of geometry collections were explicitly compartmentalized, so we might have some latitude for bending semver rules, as this PR's content can be seen as a bugfix of a broken API rather than a change of interface.

Signed-off-by: jpolchlo [email protected]

Checklist

  • ./CHANGELOG.md updated, if necessary. Link to the issue if closed, otherwise the PR.
  • Unit tests added for bug-fix or new feature

Notes

Closes #3289

@pomadchin pomadchin self-requested a review September 26, 2020 00:11
@pomadchin
Copy link
Member

@jpolchlo could you update the changelog?

@pomadchin
Copy link
Member

Hm, I also have a feeling that changes intorduced in this PR #3288 are not neccesary now. I think it makes sense to revert them to keep the code consistent?

@jpolchlo
Copy link
Contributor Author

Happy to update the changelog, but I was/am waiting for discussion as to whether this is the right solution. If you and others are happy with what's presented here, then I'll note the changes and we can move to merge.

Copy link
Member

@pomadchin pomadchin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Justin, PR looks good 💯 // I rebased it to resolve CHANGELOG conflicts // merging it once CI is happy

@pomadchin pomadchin force-pushed the fix/geometrycollection-getall branch from 6b59e70 to 0412a57 Compare November 18, 2020 23:27
@pomadchin pomadchin merged commit e545d6d into locationtech:master Nov 19, 2020
@pomadchin pomadchin mentioned this pull request Nov 19, 2020
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GeometryCollection vector.Reproject incorrect behavior
3 participants