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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 10 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
de2d858
use coalesce to provide default values for aggregate functions that c…
rausnitz Oct 31, 2018
0bfab65
Merge branch 'master' into aggregate-coalesce
tanner0101 Nov 1, 2018
de8b12c
keep decodable to avoid breaking change
rausnitz Nov 5, 2018
84e074b
Merge branch 'aggregate-coalesce' of https://github.com/rausnitz/flue…
rausnitz Nov 5, 2018
f983464
fix bug caused by string describing an optional
rausnitz Nov 5, 2018
3b076ed
sqlite coalesce requires >1 argument
rausnitz Nov 5, 2018
d66b18f
use numeric literal
rausnitz Nov 5, 2018
d602fd2
make new protocol requirement to avoid breaking changes
rausnitz Nov 5, 2018
f68e3d5
count will never be null in sql so there is no need for default
rausnitz Nov 5, 2018
703ccb0
bind value to avoid possibility of sql injection
rausnitz Feb 23, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions Sources/Fluent/Query/QuerySupporting.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -249,6 +249,15 @@ public protocol QuerySupporting: Database {
/// Special "all fields" query key.
static var queryKeyAll: QueryKey { get }

/// Creates an aggregate-type (computed) query key, falling back on a default value if the aggregate functions returns NULL.
///
/// - parameters:
/// - method: Aggregate method to use.
/// - field: Keys to aggregate. Can be zero.
/// - default: Value to fall back on if the aggregate function returns NULL.
static func queryAggregate<D>(_ aggregate: QueryAggregate, _ fields: [QueryKey], default: D) -> QueryKey
where D: Decodable

/// Creates an aggregate-type (computed) query key.
///
/// - parameters:
Expand Down
40 changes: 19 additions & 21 deletions Sources/Fluent/QueryBuilder/QueryBuilder+Aggregate.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,18 +17,7 @@ extension QueryBuilder {
/// - default: Optional default to use.
/// - returns: A `Future` containing the sum.
public func sum<T>(_ field: KeyPath<Result, T>, default: T? = nil) -> Future<T> where T: Decodable {
return self.count().flatMap { count in
switch count {
case 0:
if let d = `default` {
return self.connection.map { _ in d }
} else {
throw FluentError(identifier: "noSumResults", reason: "Sum query returned 0 results and no default was supplied.")
}
default:
return self.aggregate(Database.queryAggregateSum, field: field)
}
}
return aggregate(Database.queryAggregateSum, field: field, default: `default`)
}

/// Returns the average of all entries for the supplied field.
Expand All @@ -37,9 +26,10 @@ extension QueryBuilder {
///
/// - parameters:
/// - field: Field to average.
/// - default: Optional default to use.
/// - returns: A `Future` containing the average.
public func average<T>(_ field: KeyPath<Result, T>) -> Future<T> where T: Decodable {
return aggregate(Database.queryAggregateAverage, field: field)
public func average<T>(_ field: KeyPath<Result, T>, default: T? = nil) -> Future<T> where T: Decodable {
return aggregate(Database.queryAggregateAverage, field: field, default: `default`)
}

/// Returns the minimum value of all entries for the supplied field.
Expand All @@ -48,9 +38,10 @@ extension QueryBuilder {
///
/// - parameters:
/// - field: Field to find min for.
/// - default: Optional default to use.
/// - returns: A `Future` containing the min.
public func min<T>(_ field: KeyPath<Result, T>) -> Future<T> where T: Decodable {
return aggregate(Database.queryAggregateMinimum, field: field)
public func min<T>(_ field: KeyPath<Result, T>, default: T? = nil) -> Future<T> where T: Decodable {
return aggregate(Database.queryAggregateMinimum, field: field, default: `default`)
}

/// Returns the maximum value of all entries for the supplied field.
Expand All @@ -59,9 +50,10 @@ extension QueryBuilder {
///
/// - parameters:
/// - field: Field to find max for.
/// - default: Optional default to use.
/// - returns: A `Future` containing the max.
public func max<T>(_ field: KeyPath<Result, T>) -> Future<T> where T: Decodable {
return aggregate(Database.queryAggregateMaximum, field: field)
public func max<T>(_ field: KeyPath<Result, T>, default: T? = nil) -> Future<T> where T: Decodable {
return aggregate(Database.queryAggregateMaximum, field: field, default: `default`)
}

/// Perform an aggregate action on the supplied field. Normally you will use one of
Expand All @@ -71,13 +63,19 @@ extension QueryBuilder {
///
/// - parameters:
/// - method: Aggregate method to use.
/// - field: Field to find max for.
/// - field: Field to find aggregate for.
/// - type: `Decodable` type to decode the aggregate value as.
/// - default: Optional default to use.
/// - 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>
public func aggregate<D, T>(_ method: Database.QueryAggregate, field: KeyPath<Result, T>, as type: D.Type = D.self, default: D?) -> Future<D>
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

where D: Decodable
{
return _aggregate(Database.queryAggregate(method, [Database.queryKey(Database.queryField(.keyPath(field)))]))
guard let d = `default` else {
return _aggregate(Database.queryAggregate(method, [Database.queryKey(Database.queryField(.keyPath(field)))]))
}

return _aggregate(Database.queryAggregate(method, [Database.queryKey(Database.queryField(.keyPath(field)))], default: d))

}

/// Returns the number of results for this query.
Expand Down
16 changes: 16 additions & 0 deletions Sources/FluentSQL/SQL+QuerySupporting.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,22 @@ extension QuerySupporting where
QueryKey: SQLSelectExpression,
QueryKey.Expression == QueryKey.Expression.Function.Argument.Expression
{
/// See `QuerySupporting`.
public static func queryAggregate<C>(_ name: QueryAggregate, _ fields: [QueryKey], default: C) -> QueryKey
where C: Codable
{
let args: [QueryKey.Expression.Function.Argument] = fields.compactMap { expr in
if expr.isAll {
return .all
} else if let (expr, _) = expr.expression {
return .expression(expr)
} else {
return nil
}
}
return .expression(.coalesce(.function(.function(name, args)), .bind(.encodable(`default`))), alias: .identifier("fluentAggregate"))
}

/// See `QuerySupporting`.
public static func queryAggregate(_ name: QueryAggregate, _ fields: [QueryKey]) -> QueryKey {
let args: [QueryKey.Expression.Function.Argument] = fields.compactMap { expr in
Expand Down