davidgtonge / backbone_query

A lightweight query api for Backbone Collections
MIT License
378 stars 29 forks source link

Fixed compound queries #9, AMD compatibility #8 #10

Closed Rob--W closed 11 years ago

Rob--W commented 11 years ago

This set of changes are offering the following features:

This set of changes is divided in four commits, so that it's easier to see which changes were made to bring the specific feature.

davidgtonge commented 11 years ago

Thanks for these, I'm working on the library this afternoon. I wont be able to do a straight merge, but I'll give you credit for the features added.

Rob--W commented 11 years ago

The git diff in the second commit ( 4889a96 ) might not be very clear. That's because every line had to be indented in order to wrap it in a module.

These are the changes made in commit "Made module AMD/NodeJS/CJS/browser-compatible #8":

  1. Indent every line in the original code by two spaces (except for the license header).
  2. Prepend the following:

    ((define) -> define 'backbone-query', (require, exports) ->
    _ = require('underscore')
    Backbone = require('backbone')
  3. Near the bottom, remove the following code:

    # If used on the server, then Backbone and Underscore are loaded as modules
    unless typeof require is 'undefined'
    _ ?= require 'underscore'
    Backbone ?= require 'backbone'
  4. At the end of the file, remove unless typeof exports is 'undefined', and remove two spaces before the next line (exports.Quer...):

    unless typeof exports is "undefined"
    exports.QueryCollection = Backbone.QueryCollection
  5. Append the following code:

    ).call this, if typeof define == 'function' and define.amd then define else (id, factory) ->
    unless typeof exports is 'undefined'
     factory ((id) -> require id), exports
    else
     # Load Underscore and backbone. No need to export QueryCollection in an module-less environment
     factory ((id) -> this[if id == 'underscore' then '_' else 'Backbone']), {}
    return
davidgtonge commented 11 years ago

Thanks for the detailed comment. Apologies for the misunderstanding earlier, there were 2 main issues that you found:

  1. Nested $or, $and, $not, $nor queries were not working
  2. Implicit $and queries were missed if there was a $or, $not, $nor query.

I've nearly finished a refactor which includes your fixes and improves performance of the library.

Thanks again for your contribution

Rob--W commented 11 years ago

I see that you've incorporated most changes, except for this one in `backbone-query.coffee. Did you find any reason to do so?

These lines of code should be obsolete, because treatment of $and, $or, etc. ought to be equal regardless of the query position (your previous and current code use different methods to handle $and, ... at the root and in subqueries).

davidgtonge commented 11 years ago

HI Rob, I'll have a look at the code again, but there is likely to be a performance penalty in doing that. The reason for dealing with $and, $or etc separately for "root" queries is that we can make use _.reduce. Essentially that bit of code ensures that if a model is filtered out by an $and query then it is not even passed on to the following $or query.

Personally I've never used a nested $or in any of my projects. I'd be interested to see an example of how it would be used?

Thanks again for your help

Rob--W commented 11 years ago

Have you considered moving these lines to process_query, so that non-root queries also receive the optimization?

Here's an example of a query which is based on my current query.

The document consists of several properties, and contains a list of embedded documents of a different kind.

Given a user ID and a name, all documents which are either created by this user or assigned to this user, AND available to me, are returned.

owner: userid
$or:
  some_set
    $elemMatch:
      role: "participant"
      $or:
        user: userid
        alias: name
    is_assigned_to_me: $computed: true
# the function at the last line has the following meaning:

$or:
  public: true
  some_set:
    $elemMatch:
      role: "participant"
      $or:
        user: my_userid
        alias: my_name
# if these queries were combined, then they'd be joined at the root using $and, for instance:

#implicit or explicit AND:
$and:
  <first query without last line>
  $and:
    <second query>
# which gives the same result as the first query *with* the last line.
davidgtonge commented 11 years ago

Thanks for the example.

I understand now that the nested compound queries are for $elemMatch queries. I'll have a look at optimising those queries to use ._reduce.

I'm not sure about the indentation that you have though? As I understand your query I would write it like this: (please tell me if I've misunderstood though)

$or:
    owner: userid
    public: true
    some_set: 
      $elemMatch: 
        role:"participant" 
        $or:
          user:$any:[my_userid, userid]
          name: $any: [my_name, name]

This query should match models with either: (owner equals userid) OR (public equals true) OR (a document in some_set has a role of participant AND (a user field with my_userid or userid OR a name field with of my_name or name)

If the user / name field is not an array then you would change $any to $contains.

Rob--W commented 11 years ago

The second query was an illustration of the is_assigned_to_me method. What I wanted to show -as you've correctly noticed- is that $or etc might be useful in $elemMatch. Don't take my example literally, because I attempted to show the structure of the query without revealing the actual query.

davidgtonge commented 11 years ago

Ok, I understand, I'm working on a way to bring compound queries to $elemMatch

davidgtonge commented 11 years ago

Hi Rob, I've pushed a change that now allows compound queries to be performed on $elemMatch queries. Please can you test it out. Thanks