search

rgeo / rgeo-activerecord

RGeo ActiveRecord extensions and tools for spatial connection adapters
Other
89 stars 64 forks source link

WKT parsing failures on spatial columns should add to `errors` instead of exception #3

Closed haxney closed 13 years ago

haxney commented 13 years ago

I am writing some tests of the validation, and I am running into the problem that trying to set a spatial column to an invalid value raises an exception before validators can run. For example, I have a spec like this:

post :create, :model => {:pos => 'POINT(1 )'}
should have(1).error_on(:pos)

which tries to create a Model with the invalid WKT string "POINT(1 )". I expect to get an entry in model.errors, but an exception is raised, causing the spec to fail. There should be a way to have this problem show up as an entry in errors rather than an exception, since Rails has an easier time dealing with the former.

As a workaround, I overrode the pos= method with the following:

def pos=(pos_)
  begin
    super pos_
  rescue RGeo::Error::ParseError
    errors.add(:pos, "has an invalid format")
  end
end

Which simply turns the exception into an entry in errors.

dazuma commented 13 years ago

Yeah, I think you're right. I'll fix this in the adapters. Thanks!

haxney commented 13 years ago

Cool! Man, you're a responsive maintainer! :)

dazuma commented 13 years ago

Hey, I was looking at this and I realized I can't quite reproduce what you're seeing. Can you post the exception that gets raised (including the stack trace if you have it)?

haxney commented 13 years ago

Sorry I didn't get back sooner, I've been busy.

Anyway, here's the stack trace I get when running it from RSpec.

     Failure/Error: post :create, :event => {:pos => 'POINT(1 1'}
     RGeo::Error::ParseError:
       :end expected but nil found.
     # /usr/lib/ruby/gems/1.9.1/gems/rgeo-0.3.1/lib/rgeo/wkrep/wkt_parser.rb:431:in `_expect_token_type'
     # /usr/lib/ruby/gems/1.9.1/gems/rgeo-0.3.1/lib/rgeo/wkrep/wkt_parser.rb:305:in `_parse_point'
     # /usr/lib/ruby/gems/1.9.1/gems/rgeo-0.3.1/lib/rgeo/wkrep/wkt_parser.rb:233:in `_parse_type_tag'
     # /usr/lib/ruby/gems/1.9.1/gems/rgeo-0.3.1/lib/rgeo/wkrep/wkt_parser.rb:163:in `parse'
     # /usr/lib/ruby/gems/1.9.1/gems/activerecord-mysql2spatial-adapter-0.3.2/lib/active_record/connection_adapters/mysql2spatial_adapter/spatial_column.rb:106:in `convert_to_geometry'
     # /usr/lib/ruby/gems/1.9.1/gems/activerecord-mysql2spatial-adapter-0.3.2/lib/active_record/connection_adapters/mysql2spatial_adapter/spatial_column.rb:78:in `type_cast'
     # /usr/lib/ruby/gems/1.9.1/gems/activerecord-3.0.7/lib/active_record/attribute_methods/dirty.rb:83:in `field_changed?'
     # /usr/lib/ruby/gems/1.9.1/gems/activerecord-3.0.7/lib/active_record/attribute_methods/dirty.rb:57:in `write_attribute'
     # /usr/lib/ruby/gems/1.9.1/gems/activerecord-3.0.7/lib/active_record/attribute_methods/write.rb:14:in `pos='
     # /usr/lib/ruby/gems/1.9.1/gems/activerecord-3.0.7/lib/active_record/base.rb:1559:in `block in attributes='
     # /usr/lib/ruby/gems/1.9.1/gems/activerecord-3.0.7/lib/active_record/base.rb:1555:in `each'
     # /usr/lib/ruby/gems/1.9.1/gems/activerecord-3.0.7/lib/active_record/base.rb:1555:in `attributes='
     # /usr/lib/ruby/gems/1.9.1/gems/activerecord-3.0.7/lib/active_record/base.rb:1407:in `initialize'
     # ./app/controllers/events_controller.rb:38:in `new'
     # ./app/controllers/events_controller.rb:38:in `create'
     # /usr/lib/ruby/gems/1.9.1/gems/actionpack-3.0.7/lib/action_controller/metal/implicit_render.rb:5:in `send_action'
     # /usr/lib/ruby/gems/1.9.1/gems/actionpack-3.0.7/lib/abstract_controller/base.rb:150:in `process_action'
     # /usr/lib/ruby/gems/1.9.1/gems/actionpack-3.0.7/lib/action_controller/metal/rendering.rb:11:in `process_action'
     # /usr/lib/ruby/gems/1.9.1/gems/actionpack-3.0.7/lib/abstract_controller/callbacks.rb:18:in `block in process_action'
     # /usr/lib/ruby/gems/1.9.1/gems/activesupport-3.0.7/lib/active_support/callbacks.rb:436:in `_run__595098887__process_action__153318358__callbacks'
     # /usr/lib/ruby/gems/1.9.1/gems/activesupport-3.0.7/lib/active_support/callbacks.rb:410:in `_run_process_action_callbacks'
     # /usr/lib/ruby/gems/1.9.1/gems/activesupport-3.0.7/lib/active_support/callbacks.rb:94:in `run_callbacks'
     # /usr/lib/ruby/gems/1.9.1/gems/actionpack-3.0.7/lib/abstract_controller/callbacks.rb:17:in `process_action'
     # /usr/lib/ruby/gems/1.9.1/gems/actionpack-3.0.7/lib/action_controller/metal/instrumentation.rb:30:in `block in process_action'
     # /usr/lib/ruby/gems/1.9.1/gems/activesupport-3.0.7/lib/active_support/notifications.rb:52:in `block in instrument'
     # /usr/lib/ruby/gems/1.9.1/gems/activesupport-3.0.7/lib/active_support/notifications/instrumenter.rb:21:in `instrument'
     # /usr/lib/ruby/gems/1.9.1/gems/activesupport-3.0.7/lib/active_support/notifications.rb:52:in `instrument'
     # /usr/lib/ruby/gems/1.9.1/gems/actionpack-3.0.7/lib/action_controller/metal/instrumentation.rb:29:in `process_action'
     # /usr/lib/ruby/gems/1.9.1/gems/actionpack-3.0.7/lib/action_controller/metal/rescue.rb:17:in `process_action'
     # /usr/lib/ruby/gems/1.9.1/gems/actionpack-3.0.7/lib/abstract_controller/base.rb:119:in `process'
     # /usr/lib/ruby/gems/1.9.1/gems/actionpack-3.0.7/lib/abstract_controller/rendering.rb:41:in `process'
     # /usr/lib/ruby/gems/1.9.1/gems/actionpack-3.0.7/lib/action_controller/metal/testing.rb:12:in `process_with_new_base_test'
     # /usr/lib/ruby/gems/1.9.1/gems/actionpack-3.0.7/lib/action_controller/test_case.rb:412:in `process'
     # /usr/lib/ruby/gems/1.9.1/gems/actionpack-3.0.7/lib/action_controller/test_case.rb:47:in `process'
     # /usr/lib/ruby/gems/1.9.1/gems/actionpack-3.0.7/lib/action_controller/test_case.rb:355:in `post'
     # ./spec/controllers/events_controller_spec.rb:76:in `block (5 levels) in <top (required)>'
     # /usr/lib/ruby/gems/1.9.1/gems/rspec-core-2.6.3/lib/rspec/core/example.rb:48:in `instance_eval'
     # /usr/lib/ruby/gems/1.9.1/gems/rspec-core-2.6.3/lib/rspec/core/example.rb:48:in `block in run'
     # /usr/lib/ruby/gems/1.9.1/gems/rspec-core-2.6.3/lib/rspec/core/example.rb:107:in `with_around_hooks'
     # /usr/lib/ruby/gems/1.9.1/gems/rspec-core-2.6.3/lib/rspec/core/example.rb:45:in `run'
     # /usr/lib/ruby/gems/1.9.1/gems/rspec-core-2.6.3/lib/rspec/core/example_group.rb:294:in `block in run_examples'
     # /usr/lib/ruby/gems/1.9.1/gems/rspec-core-2.6.3/lib/rspec/core/example_group.rb:290:in `map'
     # /usr/lib/ruby/gems/1.9.1/gems/rspec-core-2.6.3/lib/rspec/core/example_group.rb:290:in `run_examples'
     # /usr/lib/ruby/gems/1.9.1/gems/rspec-core-2.6.3/lib/rspec/core/example_group.rb:262:in `run'
     # /usr/lib/ruby/gems/1.9.1/gems/rspec-core-2.6.3/lib/rspec/core/example_group.rb:263:in `block in run'
     # /usr/lib/ruby/gems/1.9.1/gems/rspec-core-2.6.3/lib/rspec/core/example_group.rb:263:in `map'
     # /usr/lib/ruby/gems/1.9.1/gems/rspec-core-2.6.3/lib/rspec/core/example_group.rb:263:in `run'
     # /usr/lib/ruby/gems/1.9.1/gems/rspec-core-2.6.3/lib/rspec/core/example_group.rb:263:in `block in run'
     # /usr/lib/ruby/gems/1.9.1/gems/rspec-core-2.6.3/lib/rspec/core/example_group.rb:263:in `map'
     # /usr/lib/ruby/gems/1.9.1/gems/rspec-core-2.6.3/lib/rspec/core/example_group.rb:263:in `run'
     # /usr/lib/ruby/gems/1.9.1/gems/rspec-core-2.6.3/lib/rspec/core/example_group.rb:263:in `block in run'
     # /usr/lib/ruby/gems/1.9.1/gems/rspec-core-2.6.3/lib/rspec/core/example_group.rb:263:in `map'
     # /usr/lib/ruby/gems/1.9.1/gems/rspec-core-2.6.3/lib/rspec/core/example_group.rb:263:in `run'
     # /usr/lib/ruby/gems/1.9.1/gems/rspec-core-2.6.3/lib/rspec/core/command_line.rb:24:in `block (2 levels) in run'
     # /usr/lib/ruby/gems/1.9.1/gems/rspec-core-2.6.3/lib/rspec/core/command_line.rb:24:in `map'
     # /usr/lib/ruby/gems/1.9.1/gems/rspec-core-2.6.3/lib/rspec/core/command_line.rb:24:in `block in run'
     # /usr/lib/ruby/gems/1.9.1/gems/rspec-core-2.6.3/lib/rspec/core/reporter.rb:12:in `report'
     # /usr/lib/ruby/gems/1.9.1/gems/rspec-core-2.6.3/lib/rspec/core/command_line.rb:21:in `run'
     # /usr/lib/ruby/gems/1.9.1/gems/rspec-core-2.6.3/lib/rspec/monkey/spork/test_framework/rspec.rb:5:in `run_tests'
     # /usr/lib/ruby/gems/1.9.1/gems/spork-0.9.0.rc7/lib/spork/run_strategy/forking.rb:13:in `block in run'
     # /usr/lib/ruby/gems/1.9.1/gems/spork-0.9.0.rc7/lib/spork/forker.rb:21:in `block in initialize'
     # /usr/lib/ruby/gems/1.9.1/gems/spork-0.9.0.rc7/lib/spork/forker.rb:18:in `fork'
     # /usr/lib/ruby/gems/1.9.1/gems/spork-0.9.0.rc7/lib/spork/forker.rb:18:in `initialize'
     # /usr/lib/ruby/gems/1.9.1/gems/spork-0.9.0.rc7/lib/spork/run_strategy/forking.rb:9:in `new'
     # /usr/lib/ruby/gems/1.9.1/gems/spork-0.9.0.rc7/lib/spork/run_strategy/forking.rb:9:in `run'
     # /usr/lib/ruby/gems/1.9.1/gems/spork-0.9.0.rc7/lib/spork/server.rb:48:in `run'
     # /usr/lib/ruby/1.9.1/drb/drb.rb:1558:in `perform_without_block'
     # /usr/lib/ruby/1.9.1/drb/drb.rb:1518:in `perform'
     # /usr/lib/ruby/1.9.1/drb/drb.rb:1592:in `block (2 levels) in main_loop'
     # /usr/lib/ruby/1.9.1/drb/drb.rb:1588:in `loop'
     # /usr/lib/ruby/1.9.1/drb/drb.rb:1588:in `block in main_loop

I suppose there should be some part of ActiveRecord which catches these errors and adds the appropriate entry into errors.

A simpler example would be:

::RGeo::Cartesian.preferred_factory.parse_wkt('POINT(1 ')

Which gives

RGeo::Error::ParseError: Numeric expected but nil found.
from /usr/lib/ruby/gems/1.9.1/gems/rgeo-0.3.1/lib/rgeo/wkrep/wkt_parser.rb:431:in `_expect_token_type'

As it should, but this error should be caught and dealt with at the ActiveRecord level.

dazuma commented 13 years ago

Thanks. So I finally got a chance to look into this. It looks like the reason I wasn't able to repro it was because it's not happening with the postgis adapter, which is where I was trying to do my tests. It happens only with the mysql and spatialite adapters.

Regarding errors, I believe the intent of the rails errors hash is to hold validation results. Here we're instead doing a typecast from strings to geometry objects when the attribute is set. Since this isn't happening during validation, I don't think it's right to be mucking with the errors object at that time. Instead, we can match the behavior of the other specialized field types (such as dates) that do typecasts: they just silently degrade the value to nil if the typecast fails. For example, if "my_date" is a date attribute:

obj = MyTable.new
obj.my_date = "2001-01-01"
obj.my_date  # => Mon, 01 Jan 2001
obj.my_date = "hello"
obj.my_date  # => nil
obj.errors   # => {}

This is in fact the behavior of the postgis adapter already. I'll fix the mysql and spatialite adapters to match. I know this means we don't actually get to see the parse exception, but this is how Rails's other field types work, so I think it's best to follow suit.

haxney commented 13 years ago

That seems like a strange behavior to have; I feel like it would be more useful to have some indication mechanism of a failure to typecast. Of course, if that's what Rails does for its other fields, it absolutely makes sense for rgeo-activerecord to do the same. Just a weird decision on Rails' part.

dazuma commented 13 years ago

I agree, probably not the best decision on Rails' part, unless it's using some other error reporting side channel that I'm not aware of. For now it is what it is.

Anyway, the mysql and spatialite adapters have now been updated (version 0.4.0) to return nil rather than raise the exception in this case (among other changes). I'm going to close this issue for now.