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

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

Merged
merged 4 commits into from Oct 3, 2018

Conversation

@wubzz
Copy link
Member

@wubzz 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 wubzz requested a review from tgriesser Aug 1, 2018
Copy link
Member

@tgriesser 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
Member

Why does this need the extra closure here?

This comment has been minimized.

@wubzz

wubzz Aug 7, 2018
Author Member

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
Copy link
Member

@tgriesser 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
Copy link
Member Author

@wubzz 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'"
@tgriesser
Copy link
Member

@tgriesser 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
Copy link
Member Author

@wubzz 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
Copy link
Member

@tgriesser 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
Copy link
Member

@elhigu 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 knex:master Oct 3, 2018
1 of 2 checks passed
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
Cellule added a commit to Cellule/knex that referenced this pull request Apr 14, 2021
Related knex#2746

I wasn't sure exactly which arguments it really supports, I confirmed that these 2 versions work
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants