Inconsistent double quote escaping

[Sharkwald]

When using CamelCased DB object names, there are a number of instances where the SQL generated is not double quote escaped, causing errors to be thrown.

For example, using Chinook:

db > db.Artist.findOne(1, function(err, result) { console.log(err); });
undefined
db > { [error: relation "artist" does not exist]

Places where I've found this issue so far are the table name, the columns and the default order by (i.e. the pk).

I'm not sure whether to include the custom order by, since it's a supplied string, so the delimiting could be placed in the calling code. On the one hand that feels like a potential gotcha, on the other hand, the whole point is to take advantage of postgres' sql engine, which means playing by its rules.

I think I should have a fix for this ready for a PR soon, but I want to ensure it's properly tested first.

PS: This is the first time I've raised an issue & worked on the fix, so please be gentle!

[xivSolutions]

Ahhh, the naming fun begins :-)

@robconery - This is where, in Biggy, we hit information schema on load and snooped/cached a bunch of mappings for db objects (tables/columns). I am interested to see @Sharkwald 's solution. However, do we want to discuss whether we want massive to try to snoop/cache/solve naming discrepancies, or do we want to encourage folks to use node and pg-idiomatic naming, and to match object names to db object names in code?

In Biggy, I felt it was handy to do the snoop/caching, since C# devs most commonly proper-case object names, and don't tend to use underscores in them (i.e. UserGroups vs. user_groups). In a node/pg environment, do we want to build this sort of snoop/cache thing in out of the box?

The fix here is less of a chore than it was in the static type system of C#, but as you know, introduces additional complexity.

What say you, sir?

[xivSolutions]

@Sharkwald btw - Welcome! And, thanks for jumping in! Eager to see your PR :-)

[Sharkwald]

@xivSolutions thanks! I'll be honest, I'm a bit nervous about this, as it's all new to me.

The solution I have is very naive -- I'm just ensuring that any refs to db objects that I've come across are wrapped in double quotes. It's worked in the tests I've written and when I've played about in the REPL against Chinook. You can see the in prog here. Do let me know if this is an unsuitable approach.

[xivSolutions]

Don't be nervous, this is how we learn, and it's exactly how I got started here, not that long ago. :-)

It's ultimately up to Rob, but there is a case to be made either way. It's a valid discussion to have, and potentially a valid fix. :-)

Shoot your PR, and let's see what Rob thinks. The danger in this is that many times solutions for naming bugs solve the easy to see cases, but introduce more subtle, "edge case" naming bugs. On the other hand, a well-crafted solution that tests for all cases solves a common problem when mapping pg objects to code.

[robconery]

Yeah this is a really tough one. Postgres naming schemes go beyond "guidelines" and actually are enforced at a few levels. It simply does not like weird casing.

Let's push a test case and see what we can do. I do not want to slow things down by snooping column info (it was in there and I took it out as it was just ... ugh). I do think we can delimit using "TableName" so that should be OK.

Also the REPL now supports invoking a find() or scripted query without a callback - the output is sent right to the screen HUZZAH.

[robconery]

Done sir :)

[Sharkwald]

Thanks Rob! There's another minor wee issue surrounding the PK being used in the order by when using findOne. I think my PR solves it though.

[xivSolutions]

I think we need to make a decision here. We can decide we support camel cased and other non-pg style db object names (spaces, casing, etc) and delimit everything, or that we don't. As it sits right now, we have some code which delimits the table name, and some which doesn't. Unfortunately, it doesn;t stop with table names. Databases which have cased table names are very likely to have cased column names as well.

It is pretty easy to wrap object names in delimiters everywhere (I would just add a delimitedTableName to the table object in massive), but this leads to two issues:

• If you need to output that SQL anywhere (say, a script) that sh$%T is UGLY.
• Where do we stop? If we allow cased table names, what about column names? etc.

FWIW, this came up as I am working on implementing the schema stuff. Thoughts?

[robconery]

I think there's only one more place we need to add quotes to yes? Let's just add that and we'll stop there :). I think casing in the Node world isn't as bad as the .NET world.

[xivSolutions]

The issue @Sharkwald was describing is when you have a backend with cased database objects. This does create a problem.

Proper casing on js objects, I agree isn't much of a problem. Looks to me like he pulled down the default chinook pg db, which is all proper-cased in the db itself. The version of Chinook you were using in here previously (and in Biggy) previously I had modified for pg to use compatible (lower-cased/underscored) names.

My question was whether we want to support databases with non-standard casing in the back end.

I'm inclined to say we shouldn't, and encourage folks to adhere to pg standards.

[Sharkwald]

@xivSolutions that's the version of Chinook I used, yeah.

That's kind of why I'm pro supporting databases with non standard casing; it means that if you're a developer without full control of your app's DB schema, you may or may not be able to use MassiveJS OOTB.

I'm slowly updating my fork to support stuff like Add/Update/Delete operations on vanilla Chinook Tables, and so far, it's generally a matter of keeping things like the table names and PK names delimited.

[xivSolutions]

The challenge will be with inserts and updates, in cases where column names are proper cased on the back-end. We'll need to delimit them in the insert and update sql. Not a huge problem, but it does add some complexity (not to mention some butt-ugly generated sql ;-)

In my recent push, I bolted a delimitedTableName property onto the table prototype, some that things would work with the schema stuff I was working on. then I accessed that instead of putting delimiters in all the sql statements. See HERE.

It's a little funky, but makes it easier to manage the quoted SQL code.

[robconery]

Help me understand the problem? We have quotes going out, now we need quotes going in (which makes perfect sense). This is a formatting issue which should be a matter of adding quotes. Is there more to this?

[Sharkwald]

I may be missing the wood for the trees here, but so far just adding quotes to the generated sql is doing the job for the Ins/Upd/Del functions. Using the new delimitedTableName, Where.forTable() and adding a new delimitedPrimaryKeyName() are keeping things relatively clean. So far the only additional places requiring further intervention are the preparation of the insert and update statements, and they're not especially onerous, e.g. update now contains:

_.each(fields, function(value, key) {
  f.push(util.format('"%s" = $%s', key, (++seed)));
  parameters.push(value);
});

Which isn't the end of the world, surely?

[paulxtiseo]

Not sure if this issue is related or should be its own ticket, but I have a function in postgresql as:

CREATE OR REPLACE FUNCTION "getOrderByOperator"(IN opid bigint)

Now, MassiveJS gives me a db.getOrderByOperator to call. But, when I call it, it errors with [error: function getorderbyoperator(unknown) does not exist]. True, it does not exist, but only because it's (I'm assuming) lowercasing the whole thing rather than using it as-is.

Is this fixed? Fixable? Is there a workaround short of constraining to snake casing or abandoning MassiveJS altogether?

[xivSolutions]

Pretty sure this is the same thing - escaping the loadFunctions() method , but if you could make it its own ticket would be great. I can push a fix in the next day or so (I'm travelling atm), or if you want to take a shot at it and shoot a PR is cool also. Basically, need to apply escapes to the loadFunctions() method.

Nice catch.