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

Allow casting when using named bindings. #1890

Merged
merged 6 commits into from Feb 5, 2017

Conversation

Projects
None yet
2 participants
@mathewdgardner
Contributor

mathewdgardner commented Feb 2, 2017

I have had lots of trouble using named bindings when writing raw queries in the past but recently I found that most of my trouble occurs if I am casting. This PR expands the regex used for searching for identifies and values and adds a little bit of logic around it.

This may be a naive approach but I'm open to critique.

Before Examples:

SELECT foo::TEXT 
FROM <table>
WHERE bar = :bar;

Currently, this will yield an error complaining about undefined bindings. What's happening behind the scenes is ::TEXT is being picked up as a named binding but of course it is missing from the given bindings object.

SELECT :foo::TEXT 
FROM <table>;

Currently, this will result in :foo: being thought of as an identifier instead of a value being given and casted.

After some digging I found that this undocumented approach works in the current version:

SELECT foo:\:TEXT 
FROM <table>
WHERE bar = :bar;

While this does not:

SELECT :foo:\:TEXT 
FROM <table>

After Examples:

These work as expected.

SELECT foo::TEXT 
FROM <table>
WHERE bar = :bar;

SELECT :foo::TEXT 
FROM <table>;

I mostly work on Postgres and so my knowledge of other RDBMS's is very limited. As I mentioned above I am open to suggestions for improvement.

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Feb 2, 2017

Hi, thanks for looking into this I'll give more careful look to the implementation a bit later..

On the first glance looks like you have used template strings in tests... sadly we still cannot use them in tests, since they are ran also with node 0.12. So this causes test failure.

Also if I understood correctly your implementation checks just against :: operator... so this doesn't solve all the issues with intended colons in queries.

There is a bug report about the same issue, which has also proposal for the solution to the problem that will work in more general way to all colons in the query #1887 (comment)

@mathewdgardner

This comment has been minimized.

Contributor

mathewdgardner commented Feb 2, 2017

@elhigu Thanks for looking into this. I'm not aware of any intended uses of : besides using named bindings for identifiers and values, can you provide an example so I can better understand?

Edit: I realized timestamps have colons and possibly any other raw input strings.

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Feb 2, 2017

@mathewdgardner do you think you could change the implementation to work so that only those named bindings and converted to positional which are found from bindings object?

@mathewdgardner

This comment has been minimized.

Contributor

mathewdgardner commented Feb 2, 2017

@elhigu I'll take a stab at it.

@mathewdgardner

This comment has been minimized.

Contributor

mathewdgardner commented Feb 5, 2017

@elhigu Let me know if this implementation works you or if I need to squash.

I expanded the regex to either match a named binding identifier that is not followed by a casting or a named binding value otherwise. Casts will still be picked up by the regex but it will only use the given bindings for replacement. If a binding given is undefined, it will still pass it along; that behavior is unchanged.

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Feb 5, 2017

So... casting :column:::text won't work I suppose, but one has to do it like :comlumn: ::text? I think that is ok. A bit inconsistent is that now in case of :var match eagerly :var: match eagerly, but :var:: does not match eagerly but matches to :var.

We could make special cases for :column:::bigint and :value::bigint cases in addition to only replace stuff that has been given in bindings, but to me current solution is good enough.

Would be great to add a paragraph about how named bindings work in these special cases where syntax is a bit unambiguous.

Code looks good. Consider those last comments if you like to do some more tuning and let me know. If you feel this is ready in current form I can already merge this in.

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Feb 5, 2017

Awesome thanks a lot!

@elhigu elhigu merged commit f198c7d into tgriesser:master Feb 5, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@elhigu

This comment has been minimized.

Collaborator

elhigu commented Feb 5, 2017

added issue about documentation

@mathewdgardner mathewdgardner deleted the mathewdgardner:cast-wtih-named-bindings branch Feb 6, 2017

elhigu added a commit to elhigu/knex that referenced this pull request Feb 15, 2017

Allow casting when using named bindings. (tgriesser#1890)
* Allow casting when using named bindings.

* Remove template string in test.

* Only replace matches that are member to the bindings object.

* Slight refactor for tests.

* Simplify regex.

* Allow casting on named identifiers.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment