Fix error when polygon gets returned from DB #58

cschell · 2017-07-11T16:02:36Z

Like the OP in #46 I get errors when receiving Polygon columns from the database. The error was due to a wrong initialization call to the GeometryField: Since it inherits from Array the first parameter needs to be an integer.

coveralls · 2017-07-11T16:06:18Z

Coverage decreased (-2.6%) to 93.12% when pulling bf70442 on cschell:fix_46 into 4093de0 on mongoid:master.

coveralls · 2017-07-11T16:55:07Z

Coverage decreased (-1.06%) to 94.668% when pulling 313a0ac on cschell:fix_46 into 4093de0 on mongoid:master.

cschell · 2017-07-11T16:59:51Z

I have no idea why Travis is failing, but to me it doesn't look like my changes have anything to do with it...

coveralls · 2017-07-11T17:07:25Z

Coverage decreased (-1.06%) to 94.668% when pulling e790751 on cschell:fix_46 into 4093de0 on mongoid:master.

dblock · 2017-07-12T19:42:15Z

I'm looking at the build. Either way this needs a test please.

dblock · 2017-07-12T19:59:45Z

Build fixed in #59, you can rebase this.

coveralls · 2017-07-13T08:49:47Z

Coverage increased (+0.02%) to 95.745% when pulling f73f8ad on cschell:fix_46 into 4093de0 on mongoid:master.

coveralls · 2017-07-13T08:49:47Z

Coverage increased (+0.02%) to 95.745% when pulling f73f8ad on cschell:fix_46 into 4093de0 on mongoid:master.

coveralls · 2017-07-13T08:49:47Z

Coverage increased (+0.02%) to 95.745% when pulling f73f8ad on cschell:fix_46 into 4093de0 on mongoid:master.

dblock · 2017-07-13T10:40:46Z

This contains the other diff, can you please rebase it:

git checkout master
git pull upstream master
git checkout fix_46
git rebase master
git push origin fix_46 -f

Could I please ask you to write another spec that looks a little more like the problem the OP in #46 is having? I am not convinced this fixes that :)

dblock · 2017-07-13T10:42:35Z

spec/mongoid/geospatial/fields/polygon_spec.rb

@@ -38,6 +38,12 @@
      expect(geom.radius_sphere(10)[1]).to be_within(0.001).of(0.001569)
    end

+    it 'should not fail to demongoize BSON documents' do


I would make it just "should demongoize BSON dociuments", nitpick.

dblock · 2017-07-13T10:43:13Z

spec/mongoid/geospatial/fields/polygon_spec.rb

@@ -38,6 +38,12 @@
      expect(geom.radius_sphere(10)[1]).to be_within(0.001).of(0.001569)
    end

+    it 'should not fail to demongoize BSON documents' do
+      expect do
+        Mongoid::Geospatial::Polygon.demongoize(BSON::Document.new)


Compare the result instead of expecting no exception. Otherwise this hides the actual functionality.

Yeah you're right, this only describes a symptom we don't want to have, not the functionality we do want to have. The problem ist that even after some digging I didn't really understand what the original intention here was.

At some point Mongoid::Geospatial::Polygon.demongoize gets called and I assumed it's goal was to convert the data structure we're getting from the database to something we later want to work with in our ruby application.

In #46 the OP describes a working example where he passes coordinates as an array to Polygon.new. This works, because Array.new([:a, :b]) == [:a, :b]. But in the failing case a BSON::Document gets passed and Array.new(BSON::Document.new()) doesn't work and my solution worked, because it kind of converts the BSON::Document to a GeometryField – something that is already done by Geospatial::Point, by the way, which is why I didn't encounter the problem until I introduced Polygons into my database.

But after diving deeper into this and playing around with the debugger, I don't think this is the correct solution either, because this BSON::Document is not simply a list of coordinates, but more of a GeoJSON structure:

From: ~/.rbenv/versions/2.4.1/lib/ruby/gems/2.4.0/gems/mongoid-geospatial-5.0.0/lib/mongoid/geospatial/geometry_field.rb @ line 87 Mongoid::Geospatial::GeometryField.demongoize: 85: def demongoize(obj) 86: require "pry" => 87: binding.pry 88: new_field = GeometryField.new 89: obj && new_field.push(obj) 90: end [1] pry(Mongoid::Geospatial::Polygon)> obj => {"type"=>"Polygon", "coordinates"=> [[[11.422888309021834, 47.92921072316257], [11.423735962886049, 47.92914725536383], [11.423670397256993, 47.929033575045906], [11.422963074813495, 47.92919023324661], [11.422888309021834, 47.92921072316257]]]}

After going through the steps of demongoize it looks like this:

[8] pry(Mongoid::Geospatial::Polygon)> new_field = GeometryField.new => [] [9] pry(Mongoid::Geospatial::Polygon)> obj && new_field.push(*obj) => [["type", "Polygon"], ["coordinates", [[[11.422888309021834, 47.92921072316257], [11.423735962886049, 47.92914725536383], [11.423670397256993, 47.929033575045906], [11.422963074813495, 47.92919023324661], [11.422888309021834, 47.92921072316257]]]]]

So this gets passed on and works for my use case and is the reason why reducing demongoize's contents to obj worked for the OP, but as I realized this is more of a coincidence than the right sollution. That's because GeometryFields clearly are not designed to work with GeoJSONy arrays since the instance-methods expect a simple array with coordinates.

So my solution indeed only solves the symptom, not the real problem. The array with the coordinates needs to be extracted out of the BSON::Document before it enters GeometryField.demongoize. So how come that doesn't happen, do you have any ideas?

Uhm I'll be frank I don't understand all the details here, maybe @estolfo can help us out?

Either way I think @cschell you could really help by writing a failing test case for the actual scenario, even if the fix only works coincidentally (aka this is not the best or the right fix).

This spec shows the reasons why the reported exception in mongoid#46 gets thrown.

mongoid-bot · 2017-07-13T19:34:44Z

	1 Warning
⚠️	Unless you’re refactoring existing code, please update CHANGELOG.md.

	1 Message
📖	We really appreciate pull requests that demonstrate issues, even without a fix. That said, the next step is to try and fix the failing tests!

Here's an example of a CHANGELOG.md entry:

* [#58](https://github.com/mongoid/mongoid-geospatial/pull/58): Fix error when polygon gets returned from db - [@cschell](https://github.com/cschell).

Generated by 🚫 danger

coveralls · 2017-07-13T19:35:16Z

Coverage decreased (-0.09%) to 95.632% when pulling b76a009 on cschell:fix_46 into 9d25a5a on mongoid:master.

coveralls · 2017-07-13T19:35:16Z

Coverage decreased (-0.09%) to 95.632% when pulling b76a009 on cschell:fix_46 into 9d25a5a on mongoid:master.

coveralls · 2017-07-13T19:35:16Z

Coverage decreased (-0.09%) to 95.632% when pulling b76a009 on cschell:fix_46 into 9d25a5a on mongoid:master.

cschell · 2017-07-13T19:51:29Z

Okay, I reset my changes, since I'm convinced they're not helping, and created a spec that reproduces the failure. That being said, the spec claims that the GeometryField should be able to deal with BSON::Documents, but I'm not sure if that's actually true and instead it has never been intended to get confronted with BSON::Documents in the first place.

I also built a minimal script that reproduces the failure.

dblock · 2017-07-13T20:29:10Z

spec/mongoid/geospatial/fields/polygon_spec.rb

+
+      geom = Mongoid::Geospatial::Polygon.new(document)
+
+      expect([*geom]).to eq(corrdinates)


Typo: coordinates

coveralls · 2017-07-13T20:57:46Z

Coverage decreased (-0.09%) to 95.632% when pulling c8cdd7a on cschell:fix_46 into 9d25a5a on mongoid:master.

dblock reviewed Jul 13, 2017

View reviewed changes

Add spec to demonstrate mongoid#46

b76a009

This spec shows the reasons why the reported exception in mongoid#46 gets thrown.

cschell force-pushed the fix_46 branch from f73f8ad to b76a009 Compare July 13, 2017 19:32

dblock reviewed Jul 13, 2017

View reviewed changes

Fix typo

c8cdd7a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix error when polygon gets returned from DB #58

Fix error when polygon gets returned from DB #58

cschell commented Jul 11, 2017 •

edited

Loading

coveralls commented Jul 11, 2017

coveralls commented Jul 11, 2017

cschell commented Jul 11, 2017

coveralls commented Jul 11, 2017

dblock commented Jul 12, 2017

dblock commented Jul 12, 2017

coveralls commented Jul 13, 2017

coveralls commented Jul 13, 2017

coveralls commented Jul 13, 2017

dblock commented Jul 13, 2017

dblock Jul 13, 2017

dblock Jul 13, 2017

cschell Jul 13, 2017

dblock Jul 13, 2017

dblock Jul 13, 2017

mongoid-bot commented Jul 13, 2017 •

edited

Loading

coveralls commented Jul 13, 2017

coveralls commented Jul 13, 2017

coveralls commented Jul 13, 2017

cschell commented Jul 13, 2017

dblock Jul 13, 2017

coveralls commented Jul 13, 2017 •

edited

Loading


		geom = Mongoid::Geospatial::Polygon.new(document)

		expect([*geom]).to eq(corrdinates)

Fix error when polygon gets returned from DB #58

Are you sure you want to change the base?

Fix error when polygon gets returned from DB #58

Conversation

cschell commented Jul 11, 2017 • edited Loading

coveralls commented Jul 11, 2017

coveralls commented Jul 11, 2017

cschell commented Jul 11, 2017

coveralls commented Jul 11, 2017

dblock commented Jul 12, 2017

dblock commented Jul 12, 2017

coveralls commented Jul 13, 2017

coveralls commented Jul 13, 2017

coveralls commented Jul 13, 2017

dblock commented Jul 13, 2017

dblock Jul 13, 2017

Choose a reason for hiding this comment

dblock Jul 13, 2017

Choose a reason for hiding this comment

cschell Jul 13, 2017

Choose a reason for hiding this comment

dblock Jul 13, 2017

Choose a reason for hiding this comment

dblock Jul 13, 2017

Choose a reason for hiding this comment

mongoid-bot commented Jul 13, 2017 • edited Loading

coveralls commented Jul 13, 2017

coveralls commented Jul 13, 2017

coveralls commented Jul 13, 2017

cschell commented Jul 13, 2017

dblock Jul 13, 2017

Choose a reason for hiding this comment

coveralls commented Jul 13, 2017 • edited Loading

cschell commented Jul 11, 2017 •

edited

Loading

mongoid-bot commented Jul 13, 2017 •

edited

Loading

coveralls commented Jul 13, 2017 •

edited

Loading