rockdai / sql-bricks

Transparent, Schemaless SQL Generation
http://csnw.github.io/sql-bricks
MIT License
210 stars 25 forks source link

Conflict with pg-promise #91

Open kay999 opened 8 years ago

kay999 commented 8 years ago

I'm using sql-bricks together with pg-promise (in fact I'm using sql-bricks-postgres, but the issue is in sql-bricks) which supports named parameters via a $[name]-syntax. So I tried using sql('$[name]') to use this pattern for example in where-expressions.

But this doesn't work because sql-bricks replaces the "$" with "$1". Now I found out that I can use toString({ placeholder:'?%d' }) which makes everything works, but it feels odd and took me some time to discover. And It's annoying to replace all toString( ) with toString({ placeholder:'?%d' })

Is there a better way to solve this? Maybe with some global config which stops sql-bricks to replace '$' with $i++ altogether? Using autmatic numbering is quite risky anyway.

prust commented 8 years ago

@kay999: Yes, I'm surprised no one has mentioned this before. I prefer named parameters over numbered parameters as well; I'd like to add support, I'll see what impact that they will have on the API and the internals.

It's annoying to replace all toString( ) with toString({ placeholder:'?%d' })

Yes, we should expose the default_opts so that users can change these without needing to specify them each time.

prust commented 8 years ago

@kay999: I wrote a test to demonstrate how I think the API should work with support for named parameters:

it('should supported named placeholders', function() {
  var values = {'first_name': 'Fred', 'last_name': 'Flintstone'};
  var result = insert('user', values).toParams({'placeholder': '$[name]'});

  assert.equal(result.text, 'INSERT INTO "user" (first_name, last_name) VALUES ($[first_name], $[last_name])');
  assert.deepEqual(result.values, {'first_name': 'Fred', 'last_name': 'Flintstone'});
});

However, in trying to implement this I realized that it requires a fairly deep change (using name in the placeholder would trigger "named-placeholder mode", which means all values would be stored in an object instead of an array, and when values are accepted from the user they would need to be supplied in key/value form or the key would need to be implied or possibly auto-generated).

I have a feeling that this kind of change would need to happen as part of a deep refactoring along with some other deep changes aimed at simplifying the implementation (in part by making the inputs less flexible).

In the meantime, does it work to use toString({ placeholder: '' })? That might feel a little less odd, as a way to turn off parameter substitution.

kay999 commented 8 years ago

I created a small pull request (#93) which allows changing the defaults. Works for me.

kay999 commented 8 years ago

Ok, thank you, everything works now by using sql.setDefaultOpts({ placeholder:'' }); somewhere.