-
-
Notifications
You must be signed in to change notification settings - Fork 722
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
Add "LIMIT 1" to fetchOne
requests
#515
Changes from 6 commits
f9868bd
af1fd6b
072fa25
dd139b6
207b90f
088f6c9
20aecd5
7070ded
235a612
e2211ac
1b3f0e8
46791c8
4716f91
693e6da
e3a0935
65fe2bb
21c0175
a128300
3f8047f
a172473
2711b9b
5af1686
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -30,6 +30,10 @@ | |
/// See https://github.com/groue/GRDB.swift#the-query-interface | ||
public struct QueryInterfaceRequest<T> { | ||
var query: SQLSelectQuery | ||
|
||
init(query: SQLSelectQuery) { | ||
self.query = query | ||
} | ||
} | ||
|
||
extension QueryInterfaceRequest : FetchRequest { | ||
|
@@ -39,9 +43,19 @@ extension QueryInterfaceRequest : FetchRequest { | |
/// executed, and an eventual row adapter. | ||
/// | ||
/// - parameter db: A database connection. | ||
/// - parameter hint: A hint about how the query should be prepared. | ||
/// - returns: A prepared statement and an eventual row adapter. | ||
/// :nodoc: | ||
public func prepare(_ db: Database) throws -> (SelectStatement, RowAdapter?) { | ||
public func prepare(_ db: Database, hint: FetchRequestHint?) throws -> (SelectStatement, RowAdapter?) { | ||
let query: SQLSelectQuery | ||
|
||
switch hint { | ||
case .limitOne?: | ||
query = self.query.limit(1) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here we break the following code: let player = try Player.limit(1, offset: 10).fetchOne(db) We have to honor the eventual offset, if present. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Whoops! Should be fixed in the latest commits (and added a test to catch it). |
||
case nil, .primaryKeyOrUnique?: | ||
query = self.query | ||
} | ||
|
||
return try SQLSelectQueryGenerator(query).prepare(db) | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Second question: what is the purpose of keeping
prepare(_:)
in the protocol? Is there any reason for a type to implement both methods? Isn'tprepare(_:hint:)
sufficient?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because the protocol's
public
I had this idea of not wanting to break compatibility with anything already callingprepare(_:)
. Also thoughthint
could be confusing when you don't need it.I'd have preferred to declare it as:
… but you can't have a default argument in a protocol.
Anyway, I'm not too attached to it. Happy to take it out if you'd prefer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All right, I understand. The good news is that we can break things, because we are targeting the next major version. GRDB 4 will break several things (see #479 for the list of changes). So we can keep a single method.
I agree the hint adds a layer of complexity to the API, but I don't expect many users to write their own concrete type which conforms to FetchRequest. This is an advanced technique. If they do, I guess they'll be able to handle it. And we don't want a default value for the hint, because being explicit is better here.
But we have to document clearly that this hint is only an opportunity for optimization, and can safely be ignored.
Towards this goal of clarity, I think we'll also have to look for alternative names for
hint
andFetchRequestHint
, and wonder if we really want an optional. Those currently sound too much like an implementation detail, and not enough like a public API.I don't know... The radical solution is:
But a boolean has little room for further extensions (even if I can't foresee any right now). I agree with your idea of an enum:
But "cardinality" can't really be extended either. Not really an improvement over a boolean. Let's go all general:
I wish we could have merged
FetchRequest.fetchCount
here:But we can't choose this last route, because
fetchCount
is currently an optional method with a non-trivial default implementation: users would now have real difficulties adopting the FetchRequest protocol.Well.. As you see, we have much room in our way to the best design. I'm curious about your ideas as well. Our goal: turn our implementation strategy into a public API that can make sense for people who don't have a clue about our implementation strategy. I mean that most users are utterly indifferent about how the library works, and only care about their needs. Good public APIs acknowledge this hard fact. It may look difficult, but this can also be a funny game :-)
A technique that has constantly helped me finding an answer to this kind of question has been the README. We currently write, below the introduction to the FetchRequest protocol:
We can try to update this paragraph so that it explains how to use the hint. Experience shows that when the documentation looks like it fills its teaching goal, the API is good as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Applying my own advice makes the plain and simple boolean approach look not so bad, after all:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm coming around to the idea of a plain old boolean. Complicating the API for unknown future cases probably isn't worth it. If other scenarios do come up in the future, it can be changed in a way that really makes sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, if we really can't find any extension for the "hint" today, then let's not try to outsmart ourselves. As you say, we'll always be able to revise our opinion in the future, if needed!
We have done our exploration job: if the boolean approach is OK with you too, then I think we can just relax :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool. I've switched it to a boolean and updated the README.