Initial ref syntax #270
Initial ref syntax #270
Conversation
…ing and stringifying to handle raw and non-object wrapped json string values
|
This stuff requires psql >= 9.5 gotta set travis to use it also... tests might have some problems with node < 4 . I'll check those a bit later... |
elhigu
force-pushed
the
initial-ref-syntax
branch
5 times, most recently
from
2604726
to
8478fe1
Compare
elhigu
force-pushed
the
initial-ref-syntax
branch
from
8478fe1
to
543179d
Compare
|
That looks fantastic but is missing documentation ? |
|
@kapouer yes it is, I wanted sami to check this out before writing it to have kind of final code ready first. |
| .select(['id', ref('jsonObject:attr').as('foo')]) | ||
| .groupBy([ref('jsonObject:attr'), 'id']) | ||
| .having('id', '>=', ref('jsonObject:attr').castInt()) | ||
| // knex doesnt seem to support nested having |
There was a problem hiding this comment.
outdated comment, it just didn't support raw as a first parameter for having.
| }); | ||
| }); | ||
|
|
||
| it('should update nicely', function () { |
There was a problem hiding this comment.
need to update more descriptive test case name
| }); | ||
| }); | ||
|
|
||
| it('should patch nicely', function () { |
There was a problem hiding this comment.
bad name here as well
| const ret = super.call(builder, args); | ||
| // treat select(['1','2','3']) and select('1','2','3') the same way | ||
| const normalizedArgs = (args.length === 1 && Array.isArray(args[0])) ? args[0] : args; | ||
| const ret = super.call(builder, normalizedArgs); | ||
| const selections = _.flatten(this.args); | ||
|
|
There was a problem hiding this comment.
_.flatten does exacly the same normalization that you have added above. You could simply move the _.flatten call before the super call.
| const joinClauseBuilder = new JoinBuilder(knex); | ||
| func.call(joinClauseBuilder, joinClauseBuilder); | ||
| joinClauseBuilder.buildInto(knexQueryBuilder); | ||
|
|
||
| } else { | ||
| // This case is for function argument `join` operation and other methods that |
| json[attr] = JSON.parse(value); | ||
| } catch (err) { | ||
| // json column might contain plain single string which is not wrapped to array / object | ||
| } |
There was a problem hiding this comment.
Do we really need this try-catch?
There was a problem hiding this comment.
Looks like it was necessary, because pg driver converts json-string to non quoted string.
| @@ -261,7 +267,7 @@ export default class Model extends ModelBase { | |||
| const attr = jsonAttr[i]; | |||
| const value = json[attr]; | |||
|
|
|||
| if (_.isObject(value)) { | |||
| if (_.isObject(value) && !(value instanceof KnexRaw)) { | |||
| json[attr] = JSON.stringify(value); | |||
There was a problem hiding this comment.
Is this needed after the UpdateOperation stuff is moved to onBuild?
There was a problem hiding this comment.
Yes. I added test case, where json column value is updated with knex.raw and it fails even without reference builder stuff.
This failed because circular data structure:
return BoundModel.query()
.update({
jsonArray: BoundModel.knex().raw('to_jsonb(??)', ['name'])
}).then(function (result) {
expect(result).to.be(4);
});| args[0][key] = loweredValue; | ||
| } | ||
| }); | ||
|
|
There was a problem hiding this comment.
This should be moved to onBuild and the references should be stashed in splitQueryProps function as discussed.
| // convert reference builders to knex.raw | ||
| args[i] = args[i].map(arg => { | ||
| return arg instanceof ReferenceBuilder ? knex.raw(...arg.toRawArgs()) : arg; | ||
| }) |
There was a problem hiding this comment.
Is this needed since you normalize the select args into a flat array?
There was a problem hiding this comment.
.groupBy() with multiple arguments will fail without this.
|
Fixed everything! |
Adds support to easy places to be able to use ref syntax instead of knex.raw... check test cases which parts are now supported.
Stuff that should be done nicer and implemented later on: