Bug 46070 - SQLResultSet.rowsAffected not cleared
Summary: SQLResultSet.rowsAffected not cleared
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Minor
Assignee: Chris Dumez
URL: http://www.w3.org/TR/webdatabase/#dom...
Keywords:
Depends on:
Blocks:
 
Reported: 2010-09-19 20:14 PDT by Chris K
Modified: 2021-05-17 14:04 PDT (History)
11 users (show)

See Also:


Attachments
HTML page demonstrating error (1.35 KB, text/html)
2010-09-19 20:14 PDT, Chris K
no flags Details
Patch (8.99 KB, patch)
2012-10-01 02:56 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (9.14 KB, patch)
2012-10-01 08:24 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (8.78 KB, patch)
2012-10-01 23:30 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (8.75 KB, patch)
2012-10-02 04:46 PDT, Chris Dumez
andersca: review-
andersca: commit-queue-
Details | Formatted Diff | Diff
Patch (take #2) (7.85 KB, patch)
2012-10-04 23:06 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris K 2010-09-19 20:14:36 PDT
Created attachment 68047 [details]
HTML page demonstrating error

After performing an INSERT (for which rowsAffected is correctly set to 1), a subsequent SELECT will also return a result set with rowsAffected set to 1.  rowsAffected should be set to 0 after a SELECT, as explicitly stated in the spec:

"The rowsAffected attribute must return the number of rows that were changed by the SQL statement. If the statement did not affected any rows, then the attribute must return zero. For "SELECT" statements, this returns zero (querying the database doesn't affect any rows)."
Comment 1 Chris Dumez 2012-09-28 04:53:35 PDT
I can reproduce the issue with latest WebKit.
Comment 2 Chris Dumez 2012-10-01 02:56:40 PDT
Created attachment 166436 [details]
Patch
Comment 3 Chris Dumez 2012-10-01 03:00:33 PDT
sqlite3_changes() documentation:
http://www.sqlite.org/c3ref/changes.html
Comment 4 Andreas Kling 2012-10-01 07:07:44 PDT
Comment on attachment 166436 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=166436&action=review

> Source/WebCore/platform/sql/SQLiteStatement.cpp:98
> +static inline bool queryMayCauseChanges(const String& query)
> +{
> +    return query.startsWith("INSERT", false) || query.startsWith("UPDATE", false) || query.startsWith("DELETE", false);
> +}

This doesn't look very robust. What if there's a whitespace character before the INSERT/UPDATE/DELETE for example?
Comment 5 Chris Dumez 2012-10-01 08:24:20 PDT
Created attachment 166478 [details]
Patch

Take Andreas Kling's feedback into consideration. Thanks for spotting this.
Comment 6 Kenneth Rohde Christiansen 2012-10-01 13:48:36 PDT
Comment on attachment 166478 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=166478&action=review

> LayoutTests/storage/websql/execute-sql-rowsAffected.html:4
> +function log(message)

why not include the common js tools, like other tests? fast/js/resources/js-test-pre.js

> LayoutTests/storage/websql/execute-sql-rowsAffected.html:23
> +    if (resultSet.rowsAffected == expectedRowsAffected)

use shouldBe(

> Source/WebCore/platform/sql/SQLiteDatabase.cpp:333
> +    // m_lastStatementMayCauseChanges is true if the last statement was
> +    // amongst INSERT, UPDATE, or DELETE. This check is needed because

was either?
Comment 7 Chris Dumez 2012-10-01 23:30:05 PDT
Created attachment 166615 [details]
Patch

Take Kenneth's feedback into consideration.
Comment 8 Chris Dumez 2012-10-02 04:46:45 PDT
Created attachment 166668 [details]
Patch

Slight Layout test simplification.
Comment 9 Anders Carlsson 2012-10-04 16:46:06 PDT
Comment on attachment 166668 [details]
Patch

This seems super fragile. There must be a better way to do this in SQLite than to check the SQL syntax.
Comment 10 Chris Dumez 2012-10-04 22:15:11 PDT
Since the issue seems to be with the parsing of the SQL statements, I can propose a second implementation:

It is possible to use sqlite3_total_changes() [1] instead of sqlite3_changes(). I can store the total changes count before each statement and return (sqlite3_total_changes() - m_lastChangesCount) in lastChanges(). With this approach, there would be no need to parse the SQL statements.

I will prepare another patch with this new approach unless someone has a better proposal.

[1] http://www.sqlite.org/c3ref/total_changes.html
Comment 11 Chris Dumez 2012-10-04 23:06:29 PDT
Created attachment 167258 [details]
Patch (take #2)

This should be more robust in my opinion.
Comment 12 Chris Dumez 2012-10-08 08:19:57 PDT
Any feedback on my new patch?
Comment 13 Kenneth Rohde Christiansen 2012-10-10 05:29:23 PDT
Comment on attachment 167258 [details]
Patch (take #2)

Looks simpler and OK to me
Comment 14 WebKit Review Bot 2012-10-10 05:37:53 PDT
Comment on attachment 167258 [details]
Patch (take #2)

Clearing flags on attachment: 167258

Committed r130891: <http://trac.webkit.org/changeset/130891>
Comment 15 WebKit Review Bot 2012-10-10 05:37:57 PDT
All reviewed patches have been landed.  Closing bug.