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

Fixed GeoBoundingBox BottomLeft args order #1610

Open
wants to merge 1 commit into
base: release-branch.v7
Choose a base branch
from

Conversation

pistatium
Copy link

Hello.

The order of BottomLeft arguments in GeoBoundingBoxQuery was swapped, which has been corrected.

before: "bottom_left":[40.01,-71.12],"top_right":[-74.1,40.73]}

after: bottom_left":[-71.12,40.01],"top_right":[-74.1,40.73]}

@olivere
Copy link
Owner

olivere commented Apr 14, 2022

@pistatium Can you check if that fixes all cases as described in #1600?

@pistatium
Copy link
Author

I see...

bottom_right is used in the document in the order lat,lon.

https://www.elastic.co/guide/en/elasticsearch/reference/current/query-dsl-geo-bounding-box-query.html#_lat_lon_as_array_2

So this seems to be the correct order for BottomLeft.
BottomLeft is also called from BottomRightFromGeoPoint and BottomRightFromGeoHash, but this one is used just like the other methods and has no problem.

Please let me know if there are any other assumptions that need to be made.

@chchaffin
Copy link

Won't this fix break existing clients?

@@ -65,7 +65,7 @@ func (q *GeoBoundingBoxQuery) BottomRightFromGeoHash(bottomRight string) *GeoBou

// BottomLeft position from longitude (left) and latitude (bottom).
func (q *GeoBoundingBoxQuery) BottomLeft(bottom, left float64) *GeoBoundingBoxQuery {
q.bottomLeft = []float64{bottom, left}
q.bottomLeft = []float64{left, bottom}
Copy link

@chchaffin chchaffin May 3, 2022

Choose a reason for hiding this comment

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

Clients, like me, already swapped this and are using this in production. This fix will break unsuspecting clients that think this is a bug fix. If this change is accepted then it would warrant a major version change.

Copy link
Owner

Choose a reason for hiding this comment

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

I think this falls into the bucket of a very unfortunate bug on my side. However, if you were to fix all of these bugs by creating a new major version, we would probably be way beyond version 100 now. So I think we will fix it in a minor version.

If you fixed it, you are probably aware of the bug. Thankfully, Go modules won't silently update your dependency to the latest version automatically. However, I would be very thankful for a way to post a warning when the Go module is updated, but I'm not aware of anything like that. Changelogs are good, but people generally don't read them. A final option would be to remove BottomLeft and replace it with e.g. ButtomLeft2 to make people aware of the issue, but that also seems rather awkward to me.

So: It will be a minor version update, and watch out when you update.

Choose a reason for hiding this comment

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

I think awkward beats breaking backwards compatibility, but that's just my opinion. I'd opt to make a new exposed function and deprecate the old one. This will certainly be a headache for some one down the road otherwise. It's your repo, so your call.

Choose a reason for hiding this comment

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

I second @chchaffin - deprecate the old function and fix the bug in a new function. This is technically a bug fix, but anyone still using the current, bugged version of this function will have swapped the order to compensate, so there's going to be a lot more breakage if this goes out as a minor or patch version.

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.

4 participants