Documentation bug for SQLLiteral

[philmitchell]

I wanted to write a select query using an IN clause and eventually found my way to the SQLInterpolation documentation. But even then, there are no examples of how to use a SQLLiteral in a fetch query (only write queries using db.execute(literal:)).

Eventually I came up with:

let query = SQLLiteral("SELECT * FROM player WHERE id IN \(playerIds)")
...
let rows = Row.fetchCursor(db, sql: query.sql, arguments: query.arguments)

This works, but may or may not be a recommended approach.

I'd be happy to submit a PR with small additions to the docs, if you would like. I'm thinking that IN could briefly be shown on the main page under Fetch Queries and perhaps more detail given on the SQLInterpolation page.

Thanks so much for the truly superb library and the generally equally superb docs!!

[groue]

Hello @philmitchell,

You are right, there is no example for using SQLLiteral with fetchCursor/All/One methods in the doc, because those methods do not currently accept those literals. I hope the doc does not say otherwise.

The code you write its perfectly valid:

// Correct
let query = SQLLiteral("SELECT * FROM player WHERE id IN \(playerIds)")
let rows = try Row.fetchCursor(db, sql: query.sql, arguments: query.arguments)

But I guess you did expect fetchCursor/All/One to support SQL interpolation out of the box. Considering the existing support for execute(literal:), this is a pretty reasonable expectation!

This support did not ship eventually, because I did not feel comfortable with adding yet another overload of fetching methods:

• fetchCursor(statement, arguments: ..., adapter: ...)
• fetchCursor(db, sql: ..., arguments: ..., adapter: ...)
• fetchCursor(db, request)
• fetchCursor(db, literal: ..., adapter: ...) -- where will it end?

This inflation is the consequence of an organically grown library over the years. I was shy and full of doubts, and did not dare adding this extra overload.

I thought we could rely on SQLRequest, but did not write it explicitly in the documentation:

// Correct
let request = SQLRequest<Row>("SELECT * FROM player WHERE id IN \(playerIds)")
let rows = try request.fetchCursor(db)

// Correct as well
let query = SQLLiteral("SELECT * FROM player WHERE id IN \(playerIds)")
let request = SQLRequest<Row>(literal: query)
let rows = try request.fetchCursor(db)

The doc introduces SQLRequest in two chapters. In SQL Interpolation and Record Protocols, which sounds too record-focused. And in Custom Requests, which sounds advanced, and does not even talks about SQL interpolation.

As you see, SQLRequest handles both raw rows and SQL interpolation. I wouldn't go as far as saying it is recommended, because your own sample code is perfectly valid. But SQLRequest is the unique "direct" way to use SQL interpolation for fetch queries today.

In conclusion, I agree with you that we have a documentation problem. And many options for the users to choose from (that's another kind of problem). You spent time figuring all this out, and it would be better if other users wouldn't fall in the same trap.

Any suggestion and help for documentation improvements will be warmly welcomed. I'd be happy to discuss with you about what we want to say, lift shadows, and maybe perform necessary API cleanup!

[philmitchell]

Hi Gwendal, thanks so much for filling in some of the blanks :)

I confess I hadn't really noticed SQLRequest and it seems to provide an api that is both simpler and more capable (it accepts SQLiteral and it exposes the prepared statement cache).

How would you feel about consolidating the SQL api docs around SQLRequest?

That would be a natural way to bring both String and SQLLiteral under one umbrella, and personally I think the generic argument is simpler than the family of methods (SQLRequest<T>.fetchCursor instead of Row.fetchCursor, Int.fetchCursor, etc. ...).

If you like that idea I can take a crack at it.

On a related issue, the section on PreparedRequest seems like it might benefit from an update? It relies on the (older?) SelectStatement, etc.. I am wondering if that api might be de-emphasized in favor of the cached:Bool arg of SQLRequest?

Also, a few words about when it is/isn't appropriate to prefer a prepared request might be helpful.

Finally, the big issue w/ GRDB.swift is that it is very long and filled with many different tables of contents that are a bit confusing. What do you think about making it more of a summary page and some of the longest sections into separate pages?

[groue]

Hi Gwendal, thanks so much for filling in some of the blanks :)

You're welcome!

I confess I hadn't really noticed SQLRequest and it seems to provide an api that is both simpler and more capable (it accepts SQLiteral and it exposes the prepared statement cache).

How would you feel about consolidating the SQL api docs around SQLRequest?

That would be a natural way to bring both String and SQLLiteral under one umbrella, and personally I think the generic argument is simpler than the family of methods (SQLRequest<T>.fetchCursor instead of Row.fetchCursor, Int.fetchCursor, etc. ...).

I agree SQLRequest should get more exposure.

Your argument of "simplicity" is subjective, and I don't know what to do with it. I gave you one objective argument above, which is that the list of fetching request overloads is saturated. I don't really need more rationale to explore other solutions.

I'd rather wonder if SQLRequest can really cover every known use cases around SQL requests.

• The basic use cases, fetching rows, values, and records, are OK:

  -try Row.fetchAll(db, sql: "SELECT ...")
  -try String.fetchAll(db, sql: "SELECT ...")
  -try Player.fetchAll(db, sql: "SELECT ...")
  +try SQLRequest<Row>(sql: "SELECT ...").fetchAll(db)
  +try SQLRequest<String>(sql: "SELECT ...").fetchAll(db)
  +try SQLRequest<Player>(sql: "SELECT ...").fetchAll(db)

• Fetching from cached prepared statements is handled as well:

  -let statement = try db.cachedSelectStatement(sql: "SELECT ...")
  -try Row.fetchAll(statement)
  +let request = SQLRequest<Row>(sql: "SELECT ...", cached: true)
  +try request.fetchAll(db)

• Not OK: fetching from non-cached prepared statements:

  -let statement = try db.makeSelectStatement(sql: "SELECT ...")
  -try Row.fetchAll(statement)
  -try Row.fetchAll(statement)

  Some people love prepared statements. Some other need prepared statements. And the cache, which uses the SQL string as its key, is not always helpful. For example, stepping through the results of two distinct statements that use the same SQL query requires to avoid the cache.

• Not OK: changing the type of the fetched values:

  -let request = SQLRequest<Player>(sql: "SELECT ...")
  -try Row.fetchAll(db, request)

I'll have to continue this review in order to evaluate your suggestion.

On a related issue, the section on PreparedRequest seems like it might benefit from an update? It relies on the (older?) SelectStatement, etc.. I am wondering if that api might be de-emphasized in favor of the cached:Bool arg of SQLRequest?

The whole paragraph about FetchRequest could be removed from the README, and replaced with a link to the reference. Before GRDB had its menagery of ready-made requests, FetchRequest could be useful. But now it's a semi-private support protocol.

Also, a few words about when it is/isn't appropriate to prefer a prepared request might be helpful.

You prefer FetchRequest when no ready-made request can fit your needs. It should be rare, now.

Finally, the big issue w/ GRDB.swift is that it is very long and filled with many different tables of contents that are a bit confusing. What do you think about making it more of a summary page and some of the longest sections into separate pages?

This is a good idea. Some sections could indeed be moved out of the main README, even if I like URLs that don't break (because many parts of the README are linked from issues, changelog, etc.)

This is another topic, though.

[philmitchell]

I'd rather wonder if SQLRequest can really cover every known use cases around SQL requests.

Yes, perfect question and very helpful to enumerate the use cases that way. It looks like SQLRequest is less flexible than <T>.fetchAll(), etc., and if that is correct then I'd be happy to organize around that api (presenting alternatives for the less common cases that can't be handled that way).

[groue]

@philmitchell, I'm sorry I didn't answer sooner: I was busy elsewhere. Your focus on SQLRequest does not make me sure it is worth spending your time on a documentation reorganization. To sum up, your less common cases are not everyone else's.

I suggest you take a little more time in order to let GRDB sink into you a little more, and that you try to put yourself in somebody else's shoes. This will help finding the correct documentation balance.

I agree the doc can be improved. I don't agree that SQLRequest should grab all the light.

[philmitchell]

Ok, no problem Gwendal! I really appreciate your incredibly thoughtful approach to both code and documentation, and I'm very grateful for GRDB!

…

On Thu, Oct 3, 2019 at 11:57 PM Gwendal Roué ***@***.***> wrote: @philmitchell <https://github.com/philmitchell>, I'm sorry I didn't answer sooner: I was busy elsewhere. Your focus on SQLRequest does not make me sure it is worth spending your time on a documentation reorganization. To sum up, *your* less common cases are not *everyone else*'s. I suggest you take a little more time in order to let GRDB sink into you a little more, and that you try to put yourself in somebody else's shoes. This will help finding the correct documentation balance. I agree the doc can be improved. I don't agree that SQLRequest should grab all the light. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#617?email_source=notifications&email_token=AAHC2DOYVVHJVQBCASD7ZBTQM3SMVA5CNFSM4IXJ66IKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEAKVMKI#issuecomment-538269225>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAHC2DILMRCEOJTX7AW62ULQM3SMVANCNFSM4IXJ66IA> .

-- Learn Spanish through conversations: https://www.supercocoapp.com <http://supercocoapp.com> Learn Bird Songs, Support Conservation: http://www.larkwire.com

[groue]

I added this issue in the TODO list: 6f5b188

Let's wait for some inspiration to come. Meanwhile, I hope you have the information you needed.