fix wrapIdentifier not being called in postgres alter column

[heisian]

#2606

[kibertoad]

@heisian No tests?..

[heisian]

What would I test? There's an unlimited amount of possibilities for wrapIdentifier since it's a user-customizable function.

Should I test camelCase to snake_case conversion? That's what I wanted this fix for. But who's to say that same conversion would be used anywhere else (even though we know it is)? For all we know, someone else out there could be using wrapIdentifier to add prefixes to all their column operations.

IMO, and unless it's proved differently, whatever tests already exist to ensure that existing functionality is the same as before is what matters here.

[elhigu]

@heisian you added code that causes wrapIdentifier to be called for the query, so you should test that wrapIndetifier is actually called after this change. There is no need to test the whole infinite universe.

You can do it by passing custom wrapIndentifier function to configuration, then create .alter() query and see that the custom function was called. I usually test it by setting it to return .toUpperCase() version of passed data.

Kind of the same way I wrote runkit problem verification to the #2606

[heisian]

@elhigu OK, how about this?

I'm really wondering, though, if there's a way to pass the value from the first time wrapIdentifier is called when invoking table.string('email') rather than having to call it again explicitly within the postgres TableCompiler.

I poked around and experimented w/ src/formatter.js but didn't really come to anything solid, and maybe there's the argument that it's better to keep it localized to this specific case since modifying code outside of this context could introduce problems elsewhere.

[elhigu]

How does this test test that wrapIdentifier was called correctly?

[elhigu]

ah right, that spy was set on wrapIdentifier function... original naming of that spy should probably have been done better :) 👍 This is good.

[elhigu]

maybe this should be something like this.formatter.wrapAsIdentifier(col.getColumnName()) (EDIT: fixed called formatter method)

[heisian]

I did try that when running tests as well - I'll use that method instead.

[heisian]

@elhigu nevermind, I remember why I didn't end up using it, it seems that wrapAsIdentifier doesn't get the queryContext from columnBuilder - this.formatter.builder has the queryContext from table.queryContext and not alter().queryContext.

If I do process.stdout.write(JSON.stringify(this.formatter.builder.queryContext())) the output is: table context. And not alter context.

So any other ideas other than calling wrapIdentifier as-is?

[heisian]

When modifying line 49 as you suggested:

[elhigu]

Maybe allowing to pass the object holding the context to the this.formatter.wrapAsIdentifier()? I would be surprised if that same problem you encountered is not accidentally done some elsewhere.

[heisian]

What seems to happen is:

1. wrapIdentifier is called because of table.queryContext
2. wrapIdentifier is called because of table.string('someColumn').queryContext
3. The results of 2) do not get passed into .alter()

The alter block is given this.formatter containing the query context of the table, instead of the column, which is why wrapAsIdentifier doesn't work in this case.

So maybe we need to find where we can pass it the this.formatter from 2) instead of 1).

[elhigu]

I think adding comment to the place where wrapIdentifier is called instead of formatter is enough for now. We really need to do some bigger refactoring to knex internals at some point.

[heisian]

alrighty, comment added, LMK if OK

[elhigu]

Thanks!

[heisian]

thank you!