Data Sanitization of enumerable arguments in [FMDatabase executeQuery:withArgumentsInArray:]

[groue]

Hi,

This commit provides data sanitization of NSArray, NSSet and NSOrderedSet arguments in [FMDatabase executeQuery:withArgumentsInArray:], and solves two problems:

1. FMDB 2.1 requires users to provide as many question marks in the query as there are elements, and this is tedious when the enumerable object has a variable length.
2. FMDB 2.1 surprises users who do not know well the internals of sqlite, and expect enumerables to be transparently expanded, as in famous ORMs such as ActiveRecord.

For example:

// Note the single question mark bound to an NSArray object.
ids = @[@1, @2, @3];
[db executeQuery:@"SELECT * FROM table WHERE id IN (?)" withArgumentsInArray:@[ids]]

There are a few caveats, though. They are documented in the method implementation (see attached code).

I hope you'll find this patch useful, and that we'll improve it together.

[ccgus]

No comment on this yet, because I'm still thinking about it. (Though if I do accept something like this, the ? placeholder would have to be changed to something else. In sqlite, ? has a well defined meaning already, and shouldn't represent multiple placeholders)

[groue]

Though if I do accept something like this, the ? placeholder would have to be changed to something else.

When a user binds a collection (array, set) to a ? placeholder, fmdb will, today, bind the description string of the collection: the end user does not get what he wants - without any warning or error.

The binding of collections is, today, meaningless.

You can see my contribution as a way to fill this hole.

Would you want to reserve the binding of collections to something else in the future, I would understand your questionning. I claim that my proposal is, at least, natural and unsurprising, not mentioning helpful.

[groue]

Hi @ccgus. I've added support for ? expansion to executeUpdate:withArgumentsInArray:

BOOL success = [db executeUpdate:@"UPDATE table SET column = ? WHERE id IN (?)" withArgumentsInArray:@[value, ids]];

I hope you will agree it's convenient...

[groue]

What's up?

[robertmryan]

@groue I like the fact that even if you can't handle ?NNN syntax, you at least check for it. Same with ? in string literals. Nice.

But, I can't help but think this might be better suited as an external category. It seems like such a deviation of standard SQLite binding syntax that it doesn't feel like it belongs in the core FMDB, to me. And there have been other pull requests that attempt to parse SQL (for very different purposes), and I think those are best suited as separate categories, too.

By the way, if this pull request gets merged, I'd suggest also updating the comments in the .h so it bears a warning about the non-standard SQL syntax as well as the limitation of ?NNN and string literals. That way, the limitations/issues will be documented in the class framework reference documentation.

[groue]

A light of hope ;-) All right, I'll perform the requested cleanup in the next days.

[robertmryan]

@groue You really should wait to hear from Gus (who, as I understand, is moving and is therefore occupied with other matters right now) before investing any time here. I'm just an interested observer, with no authority to say what pull requests get merged and which don't. I was just giving you my two cents.

[ccgus]

So I've had a couple of personal requests for this as well (and yes, maybe as a category?). But what should the syntax be for the placeholder? '??' maybe? '@?', ? Any ideas?

[ahti]

Two more ideas:

SELECT * FROM table WHERE id IN (?...)
SELECT * FROM table WHERE id IN ([?])

I kind of like ?... because it stands out a bit more than the others, but ?? is nice and clean, and may be easier to remember.

[newyankeecodeshop]

I like [?] since it matches up well with the new literal syntax and JSON.
A comment regarding the code changes: I think it would be preferrable to test that the arguments conforms to the NSFastEnumeration protocol, instead of using [isKindOfClass:] and testing for specific collections. This allows custom collections, NSEnumerators, and potentially new Foundation collections to be supported without having to change the logic.

[groue]

A bit of context:

• I don't use any fancy wrapper around fmdb.
• I write SQL queries, and I use the IN operator.
• Expanding collections into ?,?,... is a chore.
• Therefore, I use this PR every time I use fmdb.

It looks like you are not convinced about the utility of this PR.

When you invent a new syntax, a syntax that nobody knows about, a syntax that will be documented in some burried side of the documentation, you are making the feature nearly undiscoverable, the use case an ultra-minor one, and people will keep on expanding their arrays into ?,?,... by hand.

A special syntax would kill the spirit of this PR, which is helping people who fill SQL with Objective-C objects.

We are all trying to have a sensible and solid discussion, right? I hope you will see the previous point as valid.

---

Reminder: today, when a user binds a collection to a ? placeholder, fmdb will replace it with the description string of the collection:

(lldb) po [@[@"1",@"2"] description]
(
    1,
    2
)

This string is itself interpreted by sqlite as '(\n 1, 2\n)'.

It can not be what the end user, the developer, wants.

I'm disappointed that none of you has yet agreed on this.

This is thus a programmer error: it actually deserves an exception.

I'm disappointed that none of you has seen this point.

This leaves the ? syntax free for any use, as long as it is bound to a collection.

I'm disappointed that none of you has gone to this logical conclusion, or, at least, has agreed that it it indeed a valid point.

You invoke the "respect of the sqlite syntax". Except that sqlite doesn't say anything about collection binding.

---

As a conclusion, I claim that ? is free for collections, and that a special syntax could work, but is not very good because most end users won't see it.

Do whatever you want with this.

[groue]

@ccgus wrote:

So I've had a couple of personal requests for this as well (and yes, maybe as a category?). But what should the syntax be for the placeholder? '??' maybe? '@?', ? Any ideas?

@ahti wrote:

Two more ideas:

SELECT * FROM table WHERE id IN (?...) 
SELECT * FROM table WHERE id IN ([?])

I kind of like ?... because it stands out a bit more than the others, but ?? is nice and clean, and may be easier to remember.

@newyankeecodeshop wrote:

I like [?] since it matches up well with the new literal syntax and JSON.

You have my answer above.

@newyankeecodeshop wrote:

A comment regarding the code changes: I think it would be preferrable to test that the arguments conforms to the NSFastEnumeration protocol, instead of using [isKindOfClass:] and testing for specific collections. This allows custom collections, NSEnumerators, and potentially new Foundation collections to be supported without having to change the logic.

This is an interesting proposal. It may need to be refined. Let's look at how other libraries identify collections:

My own GRMustache library (a template engine) doesn't check NSFastEnumeration conformance, but checks whether objects respond to countByEnumeratingWithState:objects:count:, excluding NSDictionary. Testing for the selector instead of the protocol is closer to the Obj-C spirit: the runtime invokes methods, and the compiler uses protocols for type checking.

Handlebars-objc (another template engine) has a special case for NSArray, NSOrderedSet and NSSet, plus checks for both NSFastEnumeration conformance and the objectAtIndex: and objectAtIndexedSubscript: selectors. The last test leaves out NSSet, which is why it is tested separately. It don't quite know their motivation for such a complex test. This may require a little investigation.

[ahti]

@ccgus have you come to a conclusion regarding this issue? I keep running into it again and again. Imho, this is really a quite big usability issue with FMDB.

[ccgus]

I haven't had a chance play with it yet, sorry. Just don't pass collection types into a prepared statement for now.

[groue]

Now that I have shipped a Swift toolkit for SQLite (heavily influenced by fmdb), I have a better understanding of @ccgus' reluctance to accept this. FMDB does not focus at all on reusable SQLite statements, and I missed them. But reusable statements are a big drag for transparent support for array bindings.

@ccgus : it was a strong decision to hide reusable statements from fmdb API. And I have never missed them, even though I knew that such a concept existed. So this decision of yours was probably the good one. Do you remember why you took this path?

[ccgus]

Do you mean the decision to "hide" reusable statements, or not accepting this PR? I don't think I'm hiding reusable statements, I just have them off by default because I think the programmer should know what she/he getting into before turning it on.

As for not accepting the PR - there's just a little "code smell" about it that's tripping warnings in my head.

[groue]

Do you mean the decision to "hide" reusable statements, or not accepting this PR?

Definitely the decision to "hide" reusable statements, which I do not question at all.

I just have them off by default because I think the programmer should know what she/he getting into before turning it on.

If I understand well your sentence, there is a fmdb API to reusable statements that the programmer can turn on. Is it true?

[ccgus]

FMDatabase's - (void)setShouldCacheStatements:(BOOL)value;

[groue]

Thanks for the info, @ccgus.