Bug 46070 – SQLResultSet.rowsAffected not cleared

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Chris K]

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)."

[Chris Dumez]

I can reproduce the issue with latest WebKit.

[Chris Dumez]

Created attachment 166436 [details]
Patch

[Chris Dumez]

sqlite3_changes() documentation:
http://www.sqlite.org/c3ref/changes.html

[Andreas Kling]

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?

[Chris Dumez]

Created attachment 166478 [details]
Patch

Take Andreas Kling's feedback into consideration. Thanks for spotting this.

[Kenneth Rohde Christiansen]

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?

[Chris Dumez]

Created attachment 166615 [details]
Patch

Take Kenneth's feedback into consideration.

[Chris Dumez]

Created attachment 166668 [details]
Patch

Slight Layout test simplification.

[Anders Carlsson]

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.

[Chris Dumez]

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

[Chris Dumez]

Created attachment 167258 [details]
Patch (take #2)

This should be more robust in my opinion.

[Chris Dumez]

Any feedback on my new patch?

[Kenneth Rohde Christiansen]

Comment on attachment 167258 [details]
Patch (take #2)

Looks simpler and OK to me

[WebKit Review Bot]

Comment on attachment 167258 [details]
Patch (take #2)

Clearing flags on attachment: 167258

Committed r130891: <http://trac.webkit.org/changeset/130891>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.