olivierphi / Node-DBI

A SQL database abstraction layer strongly inspired by the PHP Zend Framework Zend_Db API, with support of multiple Node.js database engines
https://github.com/DrBenton/Node-DBI
93 stars 22 forks source link

DBSelect.orWhere #3

Closed kalifg closed 12 years ago

kalifg commented 12 years ago

specifies whether to use OR instead of AND for the current clause.

Added the function DBSelect.orWhere to give it parity with ZendDB.

Modified the function DBSelect.assemble to take advantage of this feature.

olivierphi commented 12 years ago

Hey, this is a simple but efficient feature ! :-) Before merging, could you add some unit tests about this new feature in "test/db-select.js" ?

Thanks a lot !

olivierphi commented 12 years ago

Thank you for the unit test !

Shouldn't we add parentheses around each condition, like Zend_Db does ?

Zend_Db_Select automatically puts parentheses around each expression you specify using the where() or orWhere() > methods. This helps to ensure that Boolean operator precedence does not cause unexpected results. http://framework.zend.com/manual/en/zend.db.select.html

This would prevent problems with queries like this one (from the same Zend_Db documentation page) :

$select = $db->select()
             ->from('products',
                    array('product_id', 'product_name', 'price'))
             ->where("price < $minimumPrice OR price > $maximumPrice")
             ->where('product_name = ?', $prod);
kalifg commented 12 years ago

I was just working on that! :)

The problem with automatically grouping the OR clauses is what if you want the normal precedence rules?

ie, you want a AND b OR c AND d to be (a AND b) OR (c AND d), not a AND (b OR c) AND d

I was also thinking that to avoid the hack of putting ORs inside the string , I could code something so that we could do something like this:

dbSelect.from('products', ['product_id', 'product_name', 'price'])
    .whereGroup()
    .where('price < ?', minimumPrice)
    .orWhere('price > ?', maximumPrice)
    .whereGroupClose()
    .where('product_name = ?', prod)

We could keep a grouping count to make sure that no invalid sql strings are generated upon assemble. This should allow arbitrary grouping structures. What do you think?

kalifg commented 12 years ago

I went ahead pushed the new functionality, with unit tests. Please let me know if there is something you'd like changed. This is the best way I can think of to offer full functionality, and it doesn't change the behavior of any existing code to boot.

olivierphi commented 12 years ago

Hi ! Well, it seems simple and efficient! I would have a last request : as you are a native english speaker, would you mind adding some documentation about these new whereGroup() / whereGroupClose() statements in the README? You would do that far better than me, with my poor english level :-) You can also add your name in the package.json "authors" field, if you want to.

kalifg commented 12 years ago

Certainly, I'd be happy to! I had no idea you weren't a native english speaker, you do very well.

I will let you know when I have that finished.

olivierphi commented 12 years ago

Thank you very much for this new functionnality! I will just add automatic parentheses around all WHERE clauses, to allow a simple where("price < ? OR price > ?") for simple SQL queries, then I will merge your Pull Request.

kalifg commented 12 years ago

Good idea! I get what you are saying about that now.

kalifg commented 12 years ago

Do you need anything else from me before merging this?

olivierphi commented 12 years ago

No no, it's ok. I just wanted to add the automatic parentheses around all WHERE clauses, but I don't have enough time these days :-) So... this is merge time!

Thank you very much for this new feature!

kalifg commented 12 years ago

Oh, I'm sorry, I forgot you had said that. I would be happy to add that for you!

Michael

On Aug 14, 2012, at 3:10 AM, Olivier Philippon notifications@github.com wrote:

No no, it's ok. I just wanted to add the automatic parentheses around all WHERE clauses, but I don't have enought time these days :-) So... this is merge time!

Thank you very much for this new feature!

— Reply to this email directly or view it on GitHub.

olivierphi commented 12 years ago

Well, I thought I would do it ; but if you want to, it would be great ! :-)