New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make ordering consistent across all dialects #882

Merged
merged 6 commits into from Sep 17, 2013

Conversation

2 participants
@janmeier
Member

janmeier commented Sep 3, 2013

This PR gives consistent ordering / grouping semantics across all dialects. It also provides support for multiple order columns for all dialects (closes #817 ) and support for functions in order / group by (closes #783)

The semantics are described below

something.find({
  order: [
    'name',
    // will return `name`
    'username DESC', 
    // will return `username DESC` -- i.e. don't do it!
    ['username', 'DESC'], 
    // will return `username` DESC
    sequelize.fn('max', sequelize.col('age')), 
    // will return max(`age`)
    [sequelize.fn('max', sequelize.col('age')), 'DESC'], 
    // will return max(`age`) DESC
    [sequelize.fn('otherfunction', sequelize.col('col1'), 12, 'lalala'), 'DESC'], 
    // will return otherfunction(`col1`, 12, 'lalala') DESC
    [sequelize.fn('otherfunction', sequelize.fn('awesomefunction', sequelize.col('col'))), 'DESC'] 
    // will return otherfunction(awesomefunction(`col`)) DESC, This nesting is potentially infinite!
    [{ raw: 'otherfunction(awesomefunction(`col`))' }, 'DESC']
    // This won't be quoted, but direction will be added
    ]
})

something.find({
  order: 'name, username DESC'
}) // Will generate 'ORDER BY name, username DESC', since we leave a single order string alone

The distinction is basically - if order / group is a single string, we leave it alone, if it is an array we handle each element. The logic for what to do with strings/arrays/objects is common to all dialects, so I've added that in the abstract QG (which has moved from being solely an interface def, to now containing one actual implementation, and a lot of interface defs)

Previously, a string passed to order would be escaped from pg, but not for mysql (as in id DESC would be "id" DESC for PG, but id desc for mysql). Now it is id DESC for all dialects, to allow for a way of passing a verbatim qoute string. This means that we slightly change semantics / backwards compat., but I would argue that it is okay in this case, for the sake of making the api consistent. Furthermore, two things can happen when the order string is no longer escaped in PG:

  1. Nothing, if the string is all lowercase
  2. PG Syntax error, because case-sensitive strings MUST be quoted if they were created with quotes. In that case it is an easy fix to change to the "right" syntax.

Futhermore, all grouping strings were escaped - I've removed that feature, to make the API consistent

@durango

This comment has been minimized.

Member

durango commented Sep 5, 2013

// will return otherfunction(col1, col2) DESC

What if we wanted to use literal strings for our custom functions? :P

@janmeier

This comment has been minimized.

Member

janmeier commented Sep 5, 2013

That is not supported with this scheme. We could say that cols are quoted, args are escaped, but what if we want to combine the two :P ? sparse arrays?:

{
    cols: [undefined, 'col1'],
    args: ['string literal', undefined]
}

to yield function('string literal', col)

@durango

This comment has been minimized.

Member

durango commented Sep 6, 2013

No, tbh I think we should have a small instace called SequelizeColumn or something and check it's instance?

Something like args: [Sequelize.Col('col1'), 'string literal']

I've been thinking about these kind of concepts for a long time but with the impementation of node-sql in Sequelize this may already exist (sort of / kind of). Either way I suppose we can worry about this later down the road when we start utilizing node-sql more :)

@janmeier

This comment has been minimized.

Member

janmeier commented Sep 12, 2013

Updated with the fn / col syntax suggested by @durango :)

@durango

This comment has been minimized.

Member

durango commented Sep 12, 2013

Muuuuuuuch better, this changes everything too btw :) I'll merge later tonight when I get off of work and/or tomorrow on my day off.

durango added a commit that referenced this pull request Sep 17, 2013

Merge pull request #882 from janmeier/ordering
Make ordering consistent across all dialects

@durango durango merged commit 8dfed0a into sequelize:master Sep 17, 2013

1 check passed

default The Travis CI build passed
Details

@janmeier janmeier deleted the janmeier:ordering branch Sep 18, 2013

@janmeier janmeier referenced this pull request Sep 19, 2013

Closed

Bugfix/group count hack #907

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment