#533 ✓resolved
Jeremy Nicoll

Proper exception needed for when SQL executions are run as queries

Reported by Jeremy Nicoll | August 18th, 2008 @ 02:37 PM | in 0.10.2

When I run a query like so: DataMapper.repository(:default).adapter.query("DELETE FROM user_categories")

I get the following error:

undefined method fields' for nil:NilClass

    from /opt/local/lib/ruby/gems/1.8/gems/dm-core-0.9.4/lib/dm-core/adapters/data_objects_adapter.rb:97:in `query'
    from /opt/local/lib/ruby/gems/1.8/gems/dm-core-0.9.4/lib/dm-core/adapters/data_objects_adapter.rb:182:in `with_reader'
    from /opt/local/lib/ruby/gems/1.8/gems/dm-core-0.9.4/lib/dm-core/adapters/data_objects_adapter.rb:168:in `with_connection'
    from /opt/local/lib/ruby/gems/1.8/gems/dm-core-0.9.4/lib/dm-core/adapters/data_objects_adapter.rb:178:in `with_reader'
    from /opt/local/lib/ruby/gems/1.8/gems/dm-core-0.9.4/lib/dm-core/adapters/data_objects_adapter.rb:94:in `query'

I'm thinking that people will want to run SQL queries directly that don't necessarily return column values.

Comments and changes to this ticket

  • Dan Kubb (dkubb)

    Dan Kubb (dkubb) August 18th, 2008 @ 06:27 PM

    • State changed from “new” to “invalid”

    That's because DataMapper::Adapters::DataObjectsAdapter#query is only meant for SELECT queries.

    For DELETE, UPDATE, and DDL type queries you want to use DataMapper::Adapters::DataObjectsAdapter#execute.

  • Jeremy Nicoll

    Jeremy Nicoll August 18th, 2008 @ 08:32 PM

    Oh... ok. Well, at least I know now, and sorry for the hassle!

  • Adam French

    Adam French August 18th, 2008 @ 10:40 PM

    • Title changed from “Error thrown on certain SQL queries” to “Proper exception needed for when SQL executions are run as queries”
    • State changed from “invalid” to “open”

    This should really raise a sane exception, rather than erroring out and revealing a DM stack-trace.

    Ideally, the statement should get intercepted before being run so that it never makes it down to the data-store if it won't return results...but not entirely sure how to accomplish that without string inspection and other nastiness.

    Even more ideally, manual query and execute statements should be implicitly wrapped in a transaction. This way, if an exception like this does get thrown, the statement gets rolled back and no damage is done outside of notifying the user that his manual query/execution went awry.

    I guess that might damage the user's ego, actually.

  • Dan Kubb (dkubb)

    Dan Kubb (dkubb) August 18th, 2008 @ 11:11 PM

    Sure, its a possibility to wrap it in a transaction although if someone skips testing the code and runs it for the first time in production, they sort of deserve what they get. Also, if there's already a transaction in progress then some DB's won't allow nested transactions either.

    I think the real reason this error occurred in the first place is that the method name has poor affordance. If it were named something like select(), rather than the ambiguous query(), I think it would've been far more obvious what it does.

    I'd propose we add a select() method, deprecate query(). For a few releases we can have query() proxy to select() with a warning telling people to use select() directly.

  • Jeremy Nicoll

    Jeremy Nicoll August 19th, 2008 @ 09:59 AM

    Personally, the type of behavior I would expect from running a method named query would be to have structs returned if there are columns and nil returned if there are no columns returned. Having a method named query would be easier to remember as a general all around "send this to your database" type of method, IMHO.

  • Adam French

    Adam French August 21st, 2008 @ 08:49 AM

    @dkubb, I like your option much better than the voodoo I proposed. How about a hybrid of the two? Changing the name of the method to select in order to clarify it's use and raising a real exception instead of a Undefined Method On NilClass after it runs.

    As far as implicit transactions go, I would hope anyone talking directly to their data-store would be doing so in an explicit transaction anyway. Adding and implicit one sounds like it wouldn't be fully supported by the least common denominator of data-stores.

    What do you think?

  • Dan Kubb (dkubb)

    Dan Kubb (dkubb) January 8th, 2009 @ 04:46 AM

    • State changed from “open” to “confirmed”
    • Assigned user cleared.
  • Dan Kubb (dkubb)

    Dan Kubb (dkubb) June 16th, 2009 @ 04:18 PM

    • Assigned user set to “Dan Kubb (dkubb)”
    • Milestone set to 0.10.1
  • Dan Kubb (dkubb)

    Dan Kubb (dkubb) June 16th, 2009 @ 10:40 PM

    • State changed from “confirmed” to “accepted”
  • Dan Kubb (dkubb)

    Dan Kubb (dkubb) October 4th, 2009 @ 09:33 PM

    • Milestone changed from 0.10.1 to 0.10.2

    [project:id#20609 not-tagged:"0.10.0" not-tagged:"0.10.1" milestone:id#51895 bulk edit command]

  • Dan Kubb (dkubb)

    Dan Kubb (dkubb) October 5th, 2009 @ 12:43 PM

    • State changed from “accepted” to “resolved”

    (from [6dba5de43655662a4a1cce03eca3859381cb32d8]) Rename DataObjectsAdapter#query to #select

    • Added docs and specs for DataObjectsAdapter#select
    • Added docs and specs for DataObjectsAdapter#execute
    • Changed usage of DataObjects::Result#to_it to #affected_rows, since it is more explicit
    • Added pending specs for inconsistent behavior for DataObjects::Result#insert_id for every adapter aside from do_postgres. Need to ask Dirkjan to review and add shared spec to DO, then can be removed from dm-core specs

    [#533 state:resolved] http://github.com/datamapper/dm-core/commit/6dba5de43655662a4a1cce0...

Please Sign in or create a free account to add a new ticket.

With your very own profile, you can contribute to projects, track your activity, watch tickets, receive and update tickets through your email and much more.

New-ticket Create new ticket

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »

People watching this ticket

Tags

Referenced by

Pages