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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement GROUP BY ... HAVING #69

Merged
merged 5 commits into from Oct 23, 2019
Merged

Implement GROUP BY ... HAVING #69

merged 5 commits into from Oct 23, 2019

Conversation

vijfhoek
Copy link
Contributor

I started working on fixing #58, but ran into the issue that reusing code by inheriting SQLPredicateBuilder is really possible, for two reasons:

  1. SQLPredicateBuilder defines functions where and orWhere, while we want them to be called having and orHaving, and Swift generics don't seem to be able to rename functions at will (it's not C++ after all 馃槈)
  2. More importantly, SQLSelectBuilder already inherits SQLPredicateBuilder, so that really makes it impossible.

To quickly recap the issue, we basically want this:

db.select().column("*")
    .from("planets")
    .where(...)
    .groupBy("color")
    .having(...)

to translate to SELECT * FROM `planets` WHERE ... GROUP BY `color` HAVING ...

I can see 4 possible solutions for this:

  1. The simplest: Just copying over the where/orWhere functions from SQLPredicateBuilder, renaming them to having/orHaving and calling it a day (as I have done in this draft PR, for now)

  2. Duplicating SQLPredicateBuilder.swift, calling it SQLHavingBuilder.swift (or something), changing the necessary references and again calling it a day (not much unlike option 1)

  3. Making groupBy return a SQLGroupByBuilder instead of Self, which then inherits SQLPredicateBuilder itself too, which results in this syntax:

    db.select().column("*")
        .from("planets")
        .where(...)
        .groupBy("color")
        .where(...)

    I really don't like it...

  4. Using dynamicCallables, new in Swift 5! I have never used them, and I only know them from a few blog posts, but looking this one I have a good feeling it's possible to solve our problem.

    But... Doing this will make the code base much more complex, perhaps unnecessarily so. I imagine there are plenty of footguns that come for free with this solution, and perhaps just taking the easy way out (option 1) is the best solution.

SELECT is, as far as I know, the only statement that even supports GROUP BY, and by extension HAVING, so should we be worried so much? If you agree, I'll just add documentation to the PR and we'll be good.

@tanner0101 tanner0101 added the enhancement New feature or request label Oct 22, 2019
@tanner0101 tanner0101 added this to In Progress in Vapor 4 via automation Oct 22, 2019
Copy link
Member

@tanner0101 tanner0101 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this PR!

I think the approach you chose here is the best option. We could try to break out some of the re-usability into a type like SQLBinaryExpressionsBuilder, but that would only reduce code dupe by a tiny bit. Most of the problem is needing all the various having overloads and as you mention I don't see any way around that.

Re: dynamic callable, unless I'm mistaken, I think the key path variant only works w/ members at the moment. I don't think there is a type safe version of dynamic callable yet. So that would mean making db.select().asdf() valid code that doesn't fail until runtime.

@vijfhoek
Copy link
Contributor Author

Re: Re: dynamic callable, I could indeed not find any confirmation if there was a way to insure type safety for dynamic callables, so I already was afraid of that.

Anyway, tomorrow I'll properly document the functions I added, and then I'll undraft!

@vijfhoek vijfhoek marked this pull request as ready for review October 23, 2019 11:49
@vijfhoek vijfhoek changed the title [DRAFT] Implement GROUP BY ... HAVING using copypasta Implement GROUP BY ... HAVING Oct 23, 2019
@tanner0101
Copy link
Member

tanner0101 commented Oct 23, 2019

Looks like some small conflicts were introduced from merging #68

Copy link
Member

@tanner0101 tanner0101 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thanks!

@tanner0101 tanner0101 merged commit e2ec979 into vapor:master Oct 23, 2019
Vapor 4 automation moved this from In Progress to Done Oct 23, 2019
@vijfhoek vijfhoek deleted the having branch October 23, 2019 23:39
@tanner0101 tanner0101 mentioned this pull request Apr 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Vapor 4
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants