jtwebman / bookshelf-scopes

Giving you Rails like scopes in Bookshelf.js.
MIT License
14 stars 3 forks source link

Scopes are not insulated from being influenced strangely by one another #2

Closed mrhwick closed 6 years ago

mrhwick commented 6 years ago

In my current use case, I want to support multi-tenancy in my data model. For example, all of my models inherit from a base model which declares this scope:

inOrganization: function(qb, orgID) {
    qb.where(`organization_id`, orgID);
},

I might then declare a second scope on a particular model, like so:

inactive: (qb) => {
    const currentTime = new Date();
    qb.where("start_date", ">", currentTime);
    qb.orWhere("end_date", "<=", currentTime);
},

If I call both of these scopes, bookshelf-scopes does not compose them in such a way that the query builders do not influence one another. The query will include all three WHERE clauses on the same level in the generated query:

SELECT * FROM my_model 
WHERE organization_id = {orgID} 
    AND start_date > {current_time} 
    OR end_date <= {current_time}

It would be nice if bookshelf-scopes would automatically nest these query builders so that scopes are composed in a way that they do not influence one another. In my above scenario, a more useful generated query might look something like this:

SELECT * FROM my_model 
WHERE (organization_id = {orgID})
    AND (start_date > {current_time} 
        OR end_date <= {current_time})
jtwebman commented 6 years ago

Yep the examples are maybe not perfect as in Knex.js (what bookshelf uses under the hood) you would need to write the two functions like so to get this nesting or at least I think so, been a while now.

inOrganization: function(qb, orgID) {
  qb.where(function() {
    this.where(`organization_id`, orgID);
  });
},
inactive: (qb) => {
  qb.where(function() {
    const currentTime = new Date();
    this.where("start_date", ">", currentTime);
    this.orWhere("end_date", "<=", currentTime);
  });
},
mrhwick commented 6 years ago

That looks right if you wanted to workaround the current implementation by manually creating the nested query builders.

jtwebman commented 6 years ago

Sorry not sure there is another way around this with Knex. I am open to any PR's if you find away.