Skip to content
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

Add escapeLiteral and escapeIdentifier functions #396

Closed
wants to merge 4 commits into from

Conversation

rpedela
Copy link
Contributor

@rpedela rpedela commented Jul 11, 2013

I needed to be able to escape SQL identifiers (table names, column names, etc), so I added escapeLiteral and escapeIdentifier. There is a native and JS version. Below is an example usage.

var identifier = client.escapeIdentifier('my_table_name');
var literal = client.escapeLiteral('my_literal_value');

I am unsure the bindings C code is 100% correct. Particularly how errors are handled. The functions are synchronous so not sure I did the right thing for error handling. As far as I know, the native functions do not perform any I/O only string manipulation which is why I made them synchronous.

I would appreciate it if someone double checked my C to JS port of PQescapeLiteral and PQescapeIdentifier starting on line 3238.
http://doxygen.postgresql.org/fe-exec_8c_source.html

I added tests for both JS and native, but I am unable to run the native tests. It tells me that it cannot find node-gyp, any ideas?

@rpedela
Copy link
Contributor Author

rpedela commented Jul 12, 2013

Okay I fixed the tests. Just need a code review now.

@brianc
Copy link
Owner

brianc commented Jul 15, 2013

Sorry for not getting to this over the weekend. Had friends & family in town. With that out of the way...

This pull request is one of the best pull requests I've ever received. You used a style in line with what's currently in the code base, you kept feature parity between native and JavaScript bindings, and most importantly you provided a huge amount of test coverage. ❤️ ❤️ ❤️

Thank you so much! 👍 I'll merge & release a new minor version today.

@brianc
Copy link
Owner

brianc commented Jul 15, 2013

Cannot find node-gyp? Do npm i -g node-gyp to install it globally.

@brianc
Copy link
Owner

brianc commented Jul 15, 2013

As far as the error handling in C++ land goes, you did well. The only thing you missed is MallocCString can return NULL if the call to malloc fails. I've put in code to handle this edge case. Everything else looks perfectamundo.

@brianc
Copy link
Owner

brianc commented Jul 15, 2013

Merged this via command-line

@brianc brianc closed this Jul 15, 2013
@rpedela
Copy link
Contributor Author

rpedela commented Jul 15, 2013

Oh yeah, forgot about checking NULL. It has been awhile since I have touched C/C++. :)

Thanks for merging!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants