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

Feature/join on val and on val or on val #2746

Merged
merged 4 commits into from Oct 3, 2018

Conversation

Projects
None yet
3 participants
@wubzz
Copy link
Collaborator

wubzz commented Aug 1, 2018

Relates to #2707 - See discussion

Allows join using parameters instead of having to rely on Raw.

Before

this.on('p.id', '=', 'price.post_id').andOn('price.meta_key', knex.raw('"_regular_price"'))

After

this.on('p.id', '=', 'price.post_id').andOnVal('price.meta_key', '_regular_price')

wubzz added some commits Aug 1, 2018

@wubzz wubzz requested a review from tgriesser Aug 1, 2018

@tgriesser
Copy link
Owner

tgriesser left a comment

Cool I think this looks pretty solid!

Minor, but can we also add a test for the argument syntax, e.g. for arrow functions:

.onVal((q) => {
  q.onVal('price.meta_key', '_regular_price').andOnVal(
    'price_meta_key',
    '_regular_price'
  );
})
.orOnVal((q) => {
  q.onVal('price_meta.key', '_regular_price');
});
'_regular_price'
);
})
.orOnVal(function() {

This comment has been minimized.

@tgriesser

tgriesser Aug 7, 2018

Owner

Why does this need the extra closure here?

This comment has been minimized.

@wubzz

wubzz Aug 7, 2018

Collaborator

It doesn't necessarily. It's just personal preference. I usually prefer keeping conditions within bounds of closures so that future additions don't accidentally end up in the wrong closure.

Calling without function would also yield valid syntax.

@tgriesser

This comment has been minimized.

Copy link
Owner

tgriesser commented Aug 7, 2018

Also based on your comment on #2707:

What I don't really like is having to call .onVal() with an additional .onVal() + .andOnVal() to get it wrapped, but I guess it's the same for normal join functions, so maybe it's ok..

Could we just mirror object syntax from where and clauses...

.onVal({
  'price.meta_key': '_regular_price',
  'price_meta_key': '_regular_price'
})
.orOnVal('price_meta.key', '_regular_price')
@wubzz

This comment has been minimized.

Copy link
Collaborator

wubzz commented Aug 7, 2018

@tgriesser

Could we just mirror object syntax from where and clauses...

Object syntax is supported already, but just like where clauses this does not generate a closure. So the closure syntax requires functions.

knex('table').where({col1: '1', col2: '2'}).orWhere('col3', '4').toString()
//"select * from `table` where `col1` = '1' and `col2` = '2' or `col3` = '4'"

wubzz added some commits Aug 7, 2018

Merge remote-tracking branch 'tgriesser/master' into feature/join-onV…
…al-andOnVal-orOnVal

# Conflicts:
#	test/unit/query/builder.js
@tgriesser

This comment has been minimized.

Copy link
Owner

tgriesser commented Aug 8, 2018

Oh that's weird, I thought it did. 🤔

Just curious, does the behavior of the join differ with or without the explicit grouping via parens?

@wubzz

This comment has been minimized.

Copy link
Collaborator

wubzz commented Aug 8, 2018

I can't speak for other databases but Postgres is 'smart enough' to understand these priorities in most cases. For example the test in this PR Postgres would yield the same join result with or without grouping via parens.

@tgriesser

This comment has been minimized.

Copy link
Owner

tgriesser commented Aug 17, 2018

Cool, I think this looks good. @elhigu any thoughts here?

Going to wait to see if there's anything else that should go out with 0.15.x before merging this, will hopefully have this out soon though.

@elhigu

This comment has been minimized.

Copy link
Collaborator

elhigu commented Aug 18, 2018

Looks like some breaking changes has already been merged in after last release so I don't think that 0.15 will have any additional patch versions. No other thoughts about this than that. I suppose this one is good.

On objection side this had been resolved by having lit() helper function, which is used to tell query builder that passed value should be treated as value instead of identifier.

Like: .on('column', lit('Im value'))

We got a bit annoyed about having so many different variants of basically the same method, like .where(), .orWhere(), .whereRef(), .orWhereRef() + .andXXXXXX´ variants so having ref(indentifier)andlit(val)` helpers allowed us to get rid of having to tell parameter type by calling different function name :)

No extra thoughts about this. PR look fine to me.

@tgriesser tgriesser merged commit 0b417e5 into tgriesser:master Oct 3, 2018

1 of 2 checks passed

coverage/coveralls Coverage decreased (-0.3%) to 85.192%
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

tgriesser added a commit that referenced this pull request Oct 3, 2018

tgriesser added a commit that referenced this pull request Oct 3, 2018

Revert "onVal -> onColumn for #2746"
This reverts commit b83fc4d.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment