Use coalesce to provide default values for aggregate functions that can result in null #582

rausnitz · 2018-10-31T22:15:54Z

Implements the enhancement described in vapor/sql-kit#42

…an result in null

rausnitz · 2018-11-01T13:47:25Z

The tests failed on PostgreSQL with this error:
error: FluentPostgreSQLTests.testBenchmark : threw error "⚠️ [PostgreSQLError.decode: Could not decode Int: 0x0000000000000000 (NUMERIC).]"

The same issue (or very similar) has come up before:
#570
vapor/postgres-kit#109
vapor/fluent-postgres-driver#92

This PR seems to have addressed it vapor/postgres-kit#102 so someone familiar with the past work on this issue might have an idea what to do. @tanner0101 @natanrolnik

tanner0101

Overall the changes look great. Unfortunately, semver is hard.

tanner0101 · 2018-11-01T15:29:38Z

Sources/Fluent/Query/QuerySupporting.swift

@@ -254,7 +254,9 @@ public protocol QuerySupporting: Database {
    /// - parameters:
    ///     - method: Aggregate method to use.
    ///     - field: Keys to aggregate. Can be zero.
-    static func queryAggregate(_ aggregate: QueryAggregate, _ fields: [QueryKey]) -> QueryKey
+    ///     - default: Value to fall back on if the aggregate function returns NULL.
+    static func queryAggregate<C>(_ aggregate: QueryAggregate, _ fields: [QueryKey], default: C?) -> QueryKey


This is, very unfortunately, a semver-major breaking change. QuerySupporting is a public protocol, so changing its definition will cause projects that rely on this protocol to stop compiling.

Some ideas around this:

Protocol finagling
Add this method as a new protocol requirement with a default implementation that tries to fail gracefully. SQL+QuerySupporting.swift will still implement the updated version so everything should work.

The downside here is it makes the code messy and might not work since SQL+QuerySupporting.swift is also just providing extensions. Swift should try to take the more specific extension, but one can never be sure.

Break the API
All of Vapor's Fluent implementations use the SQL+QuerySupporting.swift methods, so we would definitely not break those with this change (obviously as they are being tested as a part of CI). But, it's possible other people have created Fluent driver implementations that could be broken by this change. If we were to merge it as is, we need to give them fair warning.

Either way, we should at least offer an extension with the signature:

static func queryAggregate(_ aggregate: QueryAggregate, _ fields: [QueryKey]) -> QueryKey

To minimize the amount of breakage.

What if we just give C the default value of nil?

@calebkleveter If I understand what you mean, any conformers to the protocol would still be required to add conformance for this new method. Even if we add an extension where it has a default value, that just helps to not break callers. Conformers must provide methods for everything required by the protocol.

Updated with a slightly messy (but stable) way to avoid introducing a breaking change.

tanner0101 · 2018-11-01T15:31:50Z

Sources/Fluent/QueryBuilder/QueryBuilder+Aggregate.swift

    /// - returns: A `Future` containing the aggregate.
-    public func aggregate<D, T>(_ method: Database.QueryAggregate, field: KeyPath<Result, T>, as type: D.Type = D.self) -> Future<D>
-        where D: Decodable
+    public func aggregate<D, T>(_ method: Database.QueryAggregate, field: KeyPath<Result, T>, as type: D.Type = D.self, default: D?) -> Future<D>


Moving from Decodable to Codable is a breaking change as well. I can't see how this would affect anyone, but we need to consider this.

This is now resolved. I'm using .literal instead of .encodable to avoid having to make the value Encodable. Using the String(describing:) initializer feels a little clumsy but it does pass the tests.

… into aggregate-coalesce

tanner0101 · 2018-11-05T20:37:36Z

@rausnitz btw wanted to update you that vapor/postgres-kit#102 is merged and tagged as 1.1.1.

tanner0101

Nice work w/ the protocols. We should be good to merge after this final issue.

rausnitz · 2018-12-17T22:02:19Z

@tanner0101 Can you clarify what the final issue is?

rausnitz · 2019-02-22T18:18:54Z

@tanner0101 How can I go about getting something like this on the agenda for Vapor 4? Once breaking changes aren't a concern, I think it would be much better for functions that use SUM, MAX, MIN, and AVG to return optionals, since those SQL functions can result in NULL. Would be happy to make a contribution but not sure what the process is.

tanner0101 · 2019-02-22T18:36:02Z

Sources/FluentSQL/SQL+QuerySupporting.swift

+            }
+        }
+
+        return .expression(.coalesce(.function(.function(name, args)), .literal(.numeric(String(describing: `default`)))), alias: .identifier("fluentAggregate"))


I think this could risk SQL injection. We need to bind any values the user could input.

tanner0101 · 2019-02-22T18:39:14Z

@rausnitz the final issue is regarding the String(describing:) stuff, I've left a review with more details.

Regarding Fluent 4, the new core, FluentKit, has been moved to https://github.com/vapor/fluent-kit. This package going forward will be an integration package for FluentKit and Vapor. An issue on FluentKit regarding aggregate defaults would be appreciated. It should be easy to fix there since we don't have to worry about breaking changes.

rausnitz · 2019-02-23T03:41:45Z

@tanner0101 I took another approach that at least builds successfully, but the tests are failing so I'm not sure if it'll actually work. On the postgresql test the issue is package at '/root/fluent-postgresql' requires a minimum Swift tools version of 5.0.0 (currently 4.1.0) so I'm not sure if the issues are related to my code changes or changes to the tests.

tanner0101 · 2019-02-26T16:38:14Z

For Fluent Postgres, the checkout branch needs to be changed from master to 1 here: https://github.com/vapor/fluent/blob/3/circle.yml#L42

But they will all fail because of the change from Decodable to Codable breaking conformance.

I think it would be much better for functions that use SUM, MAX, MIN, and AVG to return optionals, since those SQL functions can result in NULL

I also think that would be a better solution and that's what Fluent 4 will do. Is there anything that would prevent us from doing that here? I think we could change AggregateResult<D> to AggregateResult<D?>

tanner0101 · 2019-02-26T16:54:30Z

This is related to: vapor/fluent-kit#5

use coalesce to provide default values for aggregate functions that c…

de2d858

…an result in null

Merge branch 'master' into aggregate-coalesce

0bfab65

tanner0101 reviewed Nov 1, 2018

View reviewed changes

rausnitz added 2 commits November 5, 2018 15:34

keep decodable to avoid breaking change

de8b12c

Merge branch 'aggregate-coalesce' of https://github.com/rausnitz/fluent…

84e074b

… into aggregate-coalesce

rausnitz added 5 commits November 5, 2018 15:42

fix bug caused by string describing an optional

f983464

sqlite coalesce requires >1 argument

3b076ed

use numeric literal

d66b18f

make new protocol requirement to avoid breaking changes

d602fd2

count will never be null in sql so there is no need for default

f68e3d5

tanner0101 requested changes Dec 10, 2018

View reviewed changes

tanner0101 changed the base branch from master to 3 February 22, 2019 18:34

tanner0101 reviewed Feb 22, 2019

View reviewed changes

tanner0101 added the enhancement New feature or request label Feb 22, 2019

tanner0101 added this to In Progress in Vapor 3 via automation Feb 22, 2019

bind value to avoid possibility of sql injection

703ccb0

rausnitz force-pushed the aggregate-coalesce branch from 65b186a to 703ccb0 Compare February 23, 2019 02:50

rausnitz mentioned this pull request Feb 23, 2019

Handling NULL values in aggregate functions vapor/fluent-kit#5

Closed

tanner0101 closed this Mar 6, 2020

Vapor 3 automation moved this from In Progress to Done Mar 6, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use coalesce to provide default values for aggregate functions that can result in null #582

Use coalesce to provide default values for aggregate functions that can result in null #582

rausnitz commented Oct 31, 2018

rausnitz commented Nov 1, 2018

tanner0101 left a comment

tanner0101 Nov 1, 2018 •

edited

Loading

tanner0101 Nov 1, 2018

calebkleveter Nov 1, 2018

tanner0101 Nov 5, 2018

rausnitz Nov 5, 2018

tanner0101 Nov 1, 2018

rausnitz Nov 5, 2018

tanner0101 commented Nov 5, 2018

tanner0101 left a comment

rausnitz commented Dec 17, 2018

rausnitz commented Feb 22, 2019

tanner0101 Feb 22, 2019

tanner0101 commented Feb 22, 2019 •

edited

Loading

rausnitz commented Feb 23, 2019

tanner0101 commented Feb 26, 2019

tanner0101 commented Feb 26, 2019

Use coalesce to provide default values for aggregate functions that can result in null #582

Use coalesce to provide default values for aggregate functions that can result in null #582

Conversation

rausnitz commented Oct 31, 2018

rausnitz commented Nov 1, 2018

tanner0101 left a comment

Choose a reason for hiding this comment

tanner0101 Nov 1, 2018 • edited Loading

Choose a reason for hiding this comment

tanner0101 Nov 1, 2018

Choose a reason for hiding this comment

calebkleveter Nov 1, 2018

Choose a reason for hiding this comment

tanner0101 Nov 5, 2018

Choose a reason for hiding this comment

rausnitz Nov 5, 2018

Choose a reason for hiding this comment

tanner0101 Nov 1, 2018

Choose a reason for hiding this comment

rausnitz Nov 5, 2018

Choose a reason for hiding this comment

tanner0101 commented Nov 5, 2018

tanner0101 left a comment

Choose a reason for hiding this comment

rausnitz commented Dec 17, 2018

rausnitz commented Feb 22, 2019

tanner0101 Feb 22, 2019

Choose a reason for hiding this comment

tanner0101 commented Feb 22, 2019 • edited Loading

rausnitz commented Feb 23, 2019

tanner0101 commented Feb 26, 2019

tanner0101 commented Feb 26, 2019

tanner0101 Nov 1, 2018 •

edited

Loading

tanner0101 commented Feb 22, 2019 •

edited

Loading