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

Fix bug on filter for empty group #616

Merged
merged 2 commits into from
Mar 5, 2019
Merged

Conversation

code28
Copy link

@code28 code28 commented Mar 4, 2019

When using the QueryBuilder and a filter group with or, the resulting SQL statement is incorrect, if no filters are provided.

Since this is a mix between an issue and a proposition of a solution, I'm first describing the issue:

Steps to reproduce

User.query(on: req).group(.or) { query in
    // No filters here...
}.all()

Expected behavior

No filters, so no restrictions on the query, so the statement should be:
SELECT * FROM "User" WHERE [].

Actual behavior

I get the following error: [ ERROR ] SQLiteError.error: near ")": syntax error (SQLiteStatement.swift:15), caused by the SQL statement SELECT * FROM "User" WHERE () [].

My solution

Since Fluent 4 seems to work quite differently, I chose to apply this PR on version 3. This feels a bit "hacky", and I'm not sure if it should be at another point inside the query building stack, but this is a solution to the error.

But... why?

Why should you use .group without any filters? Because sometimes you loop over an array of filters, which could be empty.

@calebkleveter calebkleveter added the bug Something isn't working label Mar 4, 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.

This fix LGTM, thanks. To get CI passing, we need to change this line in circle CI config to checkout branch 1 not master: https://github.com/vapor/fluent/blob/3/circle.yml#L42

Additionally, could you open an issue against https://github.com/vapor/fluent-kit to ensure we fix this in the Fluent 4 code, too?

@code28
Copy link
Author

code28 commented Mar 5, 2019

This fix LGTM, thanks. To get CI passing, we need to change this line in circle CI config to checkout branch 1 not master: https://github.com/vapor/fluent/blob/3/circle.yml#L42

Do you mean branch 3 or indeed 1? And I assume, it should be changed for mysql and sqlite as well..?

Additionally, could you open an issue against https://github.com/vapor/fluent-kit to ensure we fix this in the Fluent 4 code, too?

Sure!

@tanner0101
Copy link
Member

PostgreSQL was a new package for Vapor 3, so its stable version is 1.0: https://github.com/vapor/fluent-postgresql/tree/1

@code28
Copy link
Author

code28 commented Mar 5, 2019

PostgreSQL was a new package for Vapor 3, so its stable version is 1.0: https://github.com/vapor/fluent-postgresql/tree/1

Oops, that one was stupid, sorry. Obviously didn't read the circle.yml correctly... 🙈

@tanner0101 tanner0101 merged commit 3776a0f into vapor:3 Mar 5, 2019
@penny-coin
Copy link

Hey @code28, you just merged a pull request, have a coin!

You now have 1 coins.

@tanner0101
Copy link
Member

Thanks :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants