Storm model query created invalid sql

[Mark Lakewood]

With the following model query

store = storm_store.find(models.customer.Store, models.customer.Store.id == contract.store_id).one()
from storm.tracer import debug
debug(True)
billing_address = storm_store.find(models.customer.Address, models.customer.Address.store_id == store.id,
                                                                                                    models.customer.Address.address_type_id == 2).one()

Causes the following sql to be used to query the database. As you can see the where clause is invalid.

SELECT customer_address.address,
        customer_address.address_type_id,
        customer_address.city,
        customer_address.country,
        customer_address.created_audit_id,
        customer_address.date_deleted,
        customer_address.deleted_audit_id,
        customer_address.fax,
        customer_address.id,
        customer_address.last_modified_audit_id,
        customer_address.phone,
        customer_address.postcode,
        customer_address.state,
        customer_address.store_id,
        customer_address.timezone
        FROM customer_address
        WHERE customer_address.store_id = %s AND customer_address AND customer_address.store_id = %s AND customer_address.address_type_id = %s', (0, 0, 2)

If I dont have the one() function call at the end, the query is accurate

Below is my model

"""
Module that contains the model that represents the 1...nth address of a customer
"""

from storm.locals import Unicode, Int, Reference, DateTime, Storm
import pytz

from api.models.common import AuditedModel

#pylint:disable=W0622,W0613,R0902,R0903
def validate_state(object, attr, value):
    """
    Validate that a value passed into the state is a valid state.
    """
    state_list = [u'ACT', u'NSW', u'VIC', u'QLD', u'WA', u'SA', u'TAS', u'NT']

    if value not in state_list:
        raise ValueError("Expected one of u'ACT', u'NSW', u'VIC', u'QLD', u'WA', u'SA', u'TAS', u'NT' but got '%s'" % value)
    else:
        return value

def validate_address_type(object, attr, value):
    """
    Validate that the address type is one of address_types
    """

    address_type = [u'Physical', u'Billing', u'Shipping', u'Mailing']

    if value not in address_type:
        raise ValueError("Expected one of u'Physical', u'Billing', u'Shipping', u'Mailing' but got '%s'" % value)
    else:
        return value

def validate_timezone(object, attr, value):
    """
    Validate the timezone string
    """
    if value not in pytz.country_timezones('AU'):
        raise ValueError("The timezone '%s' passed does not exist in pytx.country_timezones('AU')" % value)
    else:
        return value

class Address(AuditedModel):
    """
    An instance of an address for a store
    """

    __storm_table__ = 'customer_address'

    id = Int(primary=True)
    phone = Unicode()
    fax = Unicode()
    address = Unicode()
    city = Unicode()
    state = Unicode(validator=validate_state)
    postcode = Unicode()
    country = Unicode()
    store_id = Int()
    timezone = Unicode(validator=validate_timezone)

    date_deleted = DateTime()
    store = Reference('Address.store_id', 'Store.id')

    address_type_id = Int()
    address_type = Reference('Address.address_type_id', 'AddressType.id')

class AddressType(Storm):
    """
    Lookup table for address type
    """
    __storm_table__ = 'customer_address_type'

    id = Int(primary=True)
    type = Unicode(validator=validate_address_type)

the following is the create table in postgres

CREATE TABLE customer_address
(
  id bigint NOT NULL DEFAULT nextval(('customer_address_id_seq'::text)::regclass),
  phone text NOT NULL,
  fax text,
  address text NOT NULL,
  city text NOT NULL,
  state text NOT NULL,
  postcode text NOT NULL,
  country text NOT NULL,
  created_audit_id numeric(20,0),
  deleted_audit_id numeric(20,0),
  last_modified_audit_id numeric(20,0),
  date_deleted timestamp without time zone,
  store_id bigint NOT NULL,
  timezone text NOT NULL,
  address_type_id bigint NOT NULL,
  CONSTRAINT pk_customer_address PRIMARY KEY (id),
  CONSTRAINT fk_customer_address_common_audit_data_created FOREIGN KEY (created_audit_id)
      REFERENCES common_audit_data (id) MATCH SIMPLE
      ON UPDATE NO ACTION ON DELETE NO ACTION,
  CONSTRAINT fk_customer_address_common_audit_data_deleted FOREIGN KEY (deleted_audit_id)
      REFERENCES common_audit_data (id) MATCH SIMPLE
      ON UPDATE NO ACTION ON DELETE NO ACTION,
  CONSTRAINT fk_customer_address_common_audit_data_last_modified FOREIGN KEY (last_modified_audit_id)
      REFERENCES common_audit_data (id) MATCH SIMPLE
      ON UPDATE NO ACTION ON DELETE NO ACTION,
  CONSTRAINT fk_customer_address_customer_address_type FOREIGN KEY (address_type_id)
      REFERENCES customer_address_type (id) MATCH SIMPLE
      ON UPDATE NO ACTION ON DELETE NO ACTION,
  CONSTRAINT fk_customer_address_customer_store FOREIGN KEY (store_id)
      REFERENCES customer_store (id) MATCH SIMPLE
      ON UPDATE NO ACTION ON DELETE NO ACTION,
  CONSTRAINT uq_store_id_address_type_id UNIQUE (store_id, address_type_id)
)

System:
Python 2.6.5
storm 0.19
psycopg2 2.4.2
postgres 9.0
ubuntu 10.04

I have the workaround, ie using it without the one() call but thought I should file a bug anyway.

[James Henstridge (jamesh)]

The code fragment you've listed in the description looks a bit weird to me:

    store = store.find(models.customer.Store, models.customer.Store.id == contract.store_id).one()

When this is executed, presumably "store" is a storm.store.Store instance. After it executes, it will be a models.customer.Store instance. While there is nothing stopping you doing this kind of thing in Python, it does make it more difficult to debug or understand the code.

So in the next query, you're executing the models.customer.Store.find() method instead of storm.store.Store.find(). You haven't included any information about that class in your code snippet, so it is difficult to tell what it might be doing.

Looking at the resulting SQL though, it appears that the first argument is being treated as part of the where clause (hence the table name appearing bare). Storm does have a few find() methods that act similar to this by not taking the first argument of Store.find() such as ResultSet.find() and BoundReferenceSet.find() (both of which use the same class list as the container they act on). Perhaps you used one of those?

[Mark Lakewood (munderwood)]

Hi,

Sorry about the confusion. I needed to take out some company naming conventions and missed that i'd renamed the storm store to the result of store.

As you can see both finds() are performed on a storm store. The first one returns a result set from the find and so does the second. Its the second query that causes the incorrect sql.

[James Henstridge (jamesh)]

Okay. But the query being generated still doesn't really match up with the Python code you've included. For the following query:

    SELECT customer_address.address, ...
        FROM customer_address
        WHERE customer_address.store_id = 0 AND customer_address AND customer_address.store_id = 0 AND customer_address.address_type_id = 2

Your Python code snippet does not account for the first "customer_address.store_id = 0" bit of the where clause. I would expect the code that generated this query to look more like the following:

    result = storm_store.find(Address, store_id=0)
    # ResultSet.find() does not take a class list first, so this introduces the bare table reference
    result2 = result.find(Address, store_id=0, address_type_id=2)

Or if you have an "addresses" ReferenceSet attached to your store model using the customer_address.store_id foreign key, maybe something like:

    # the store object may be acquired some other way.
    store = storm_store.get(Store, 0)
    # again, the first argument here would cause the bare table reference
    result = store.addresses.find(Address, store_id=store.id, address_type_id=2)

So again, are you absolutely sure that your snippet accurately represents the code that generates the bad SQL?

[Mark Lakewood (munderwood)]

Hi,

So we have two storm queries

store = storm_store.find(models.customer.Store, models.customer.Store.id == contract.store_id).one()

and

billing_address = storm_store.find(models.customer.Address, models.customer.Address.store_id == store.id,
                                                                                                    models.customer.Address.address_type_id == 2).one()

These have nothing to do with each other, apart from that I need to get the address of the store. To do that I first get the store, in the first find. this returns a model because of the one() method. Then I use the id of the store model to add a filter to the query on the next find. Hence the second query matches up with the SQL that I posted. It is doing a select on customer_address where the store_id == 0 and the address_type_id == 2. The bit that doesnt work is that when I then call the one() function on that find query, storm inserts and empty where table like so

WHERE customer_address.store_id = %s AND customer_address AND customer_address.store_id = %s AND customer_address.address_type_id = %s', (0, 0, 2)

see the "AND customer_address AND"

when I do not call one() on the result of the find, then the SQL is correctly constructed.

 I hope this clarifies this, and im sorry for the typos at the start that made it hard to understand.

Cheers

Mark

[James Henstridge (jamesh)]

Could you try putting together a short self contained example of this bug? As I said earlier, the snippets you've included here do not look like they would generate the queries you are seeing.

Note that the find() method does not generate any queries itself: instead it is method calls on the returned ResultSet object (e.g. one()). So when you say that the problem does not occur when you do not call one(), that would still indicate that there is a problem with how the ResultSet in question was constructed.