brianc / node-sql

SQL generation for node.js
MIT License
1.05k stars 191 forks source link

Immutable queries or node.copy() #54

Open spion opened 11 years ago

spion commented 11 years ago

Right now an sql Node mutates itself when sql operations are chained to it. I found out about this the hard way, trying something like this:

var query1 = user.where(...);
var query2 = query1.and(...);
// trying to execute query1 gives the results of executing query2

Could node operations be made to return new nodes instead of mutating the exiting one?

I imagine this might break backward-compatibility in cases like this:

var query2 = user.where(...);
query2.and(...)
// executing query2 yields different results before and after the change

Personally I think that the benefits of immutable queries are worth the breakage, but if its a problem, node.copy() could be implemented instead:

var query1 = user.where(...);
var query2 = query1.copy().and(...);
// query1 and query2 are now different.

What do you think?

brianc commented 11 years ago

Yeah I thought immutable queries would be cool too, or a query.copy() thing. I'm not sure I understand what you mean about breaking backwards compatibility though. Could show a fuller example of backwards compat breakage?

spion commented 11 years ago

The breakage occurs when people don't chain their queries and don't re-assign the return result to the original variable:

REPL-friendly example:

> var user = require('sql').define({name: 'Users', columns: ['id', 'name', 'age']});
> var query = user.where({name: 'John'});
> query.toQuery().text;
'SELECT * FROM "Users" WHERE ("Users"."name" = $1)'

So far so good, but what if we add another where clause?

> query.where(user.age.equals(30));
> query.toQuery().text;
'SELECT * FROM "Users" WHERE (("Users"."name" = $1) AND ("Users"."age" = $2))'

This will break after the change. If queries are made immutable, the original query will not change.

To change the original query users will have to re-assign to the variable, e.g.

> query = query.where(user.age.equals(30));
> query.toQuery().text;
'SELECT * FROM "Users" WHERE (("Users"."name" = $1) AND ("Users"."age" = $2))'
brianc commented 11 years ago

Ohhh yes I totally see what you're saying now. I definitely think it's the way to go. I'm cool with bumping the version to v1.0 too since it's a breaking change in a way. Are you interested in implementing this? There's pretty good test coverage so you can so whatever you want refactor-wise as long as the tests pass. :)

spion commented 11 years ago

I'll give it a try. There are a lot of return this; points in Node and the other classes, but I'm sure that once I change a few key ones like add() there wont be too many changes remaining. Not quite sure what the performance penalty of making copies will be, but I guess the only way to find out is to do it and then make some benchmarks :)

caseywebdev commented 11 years ago

So is the decision for node.copy() or returning a new node for every step in the chain? I can see it going either way. I'm inclined to say returning a new node for every step is the best solution, even if it's not going to be as fast. I don't think the performance is going to be a huge issue.

brianc commented 11 years ago

I'm with you @caseywebdev. If you wanted to use node.copy() you could use the library in its current form & just wrap your "base query" in a function:

var usersWithoutEmail = function() {
  return users.where(users.email.isNull());
}

usersWithoutEmail().and(users.name.like('%blah'));

Using all immutable would yield something cool like this:

users.withoutEmail = users.where(users.email.isNull());
users.withoutEmail.and(users.name.like('%blah'));

which is way hotter

spion commented 11 years ago

I gave this a try, and concluded that its not really easy. A lot of places in the code assume mutable objects. For example if I change Node#add and Node#addAll to return new nodes instead of mutating this then in Query#select there is a problem:

    else {
      select = this._select = new Select();
      this.add(select);
    }
    <snip>
    select.addAll(args); 

To make the last line work after adding immutability I would have to find and update the reference found in the nodes array. Basically, its not easy to make a copy of a node - it may contain multiple references to the same object and they will all need to be carefully copied.

Looks like its going to be a pretty huge refactor that requires familiarity with the entire code base. I'm not sure I'm up to it.

brianc commented 11 years ago

understandable - I might take a crack at it while I'm on vacation

spion commented 11 years ago

Hmm I just got another idea - maybe it will be easier to make separate copy methods for every class.

Node#copy will do something like copy.nodes = this.nodes.map(function(n) { return n.copy(); });

Query#copy will call Node#copy first, then set up the rest of the references and copy additional fields

Then to implement immutable queries on top of that, every method will first crete a copy, then call either Node#_mutableAdd or Node#_mutableAddAll or otherwise modify the fields of the copy directly, and finally will return that copy...

I'll also give this a try and see where it leads...

brianc commented 11 years ago

:+1:

diwu1989 commented 11 years ago

The other piece of the code that can be made cleaner after nodes become immutable is the whole Aliasing concept.

sebasgarcep commented 7 years ago

Do we already have immutable quieries or .copy()?