Output proper exception message for database RowCountExceptions [#2171359]

Background

#2146733: Select queries should not use rowCount() to calculate number of rows made rowCount() available for (classed) INSERT, UPDATE, and DELETE queries only, according to PDOStatement::rowCount() docs.
#2167507: Fix rowCount query usage in pgsql and sqlite drivers fixes remaining usages of rowCount() in the pgsql and sqlite database drivers.

Problem

The newly introduced RowCountException does not expose a message to developers.
However, some application code, e.g. the installer, is catching any exception and just prints the exception message (→ an empty string).

Proposed solution

Adjust RowCountException directly to add a generic, but yet, helpful exception message for developers.

Comment	File	Size	Author
#18	rowcount.jpg	66.82 KB	dcrocks
#17	interdiff.txt	915 bytes	sun
#17	drupal8.rowcountexception-dx.17.patch	1.04 KB	sun
#17
#1	rowcount.jpg	46.15 KB	dcrocks
	drupal8.rowcountexception-dx.0.patch	877 bytes	sun

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

dcrocks CreditAttribution: dcrocks commented 11 January 2014 at 13:54

File	Size
rowcount.jpg	46.15 KB

Installed current clone of D8, then this patch. After selecting SQLite for database saw the error message as expected. Not quite sure what an inexperienced user would do with this but better than before.

Comment #2

sun

German

Karlsruhe

CreditAttribution: sun commented 11 January 2014 at 17:22

Thanks for testing it! :)

As any other exception, only developers should encounter them, and for that target audience, the message should make sense and provides a helpful pointer? (even in the slightly weird way the installer outputs it)

Comment #3

dcrocks CreditAttribution: dcrocks commented 11 January 2014 at 17:32

Status:

Needs review

» Reviewed & tested by the community

Then lets prevent others from rubbing their head about this one.

Comment #4

mradcliffe

he/him

English

CreditAttribution: mradcliffe commented 11 January 2014 at 19:13

Works for me.

Comment #5

chx CreditAttribution: chx commented 13 January 2014 at 03:35

Status:

Reviewed & tested by the community

» Needs work

Nope. The message should say that for SELECT, rowCount is illegal and for UPDATE/INSERT/DELETE consider using db_update() or db_insert() or db_delete(). The return affected is for database drivers only and the chances of people writing rowCount against a SELECT is way way way higher than they are writing database drivers...

Comment #6

chx CreditAttribution: chx commented 13 January 2014 at 03:36

See https://drupal.org/comment/8275875#comment-8275875 for more:

Calling code should never have to specify the return mode. That's an internal property callers should never have to think about.

Comment #7

chx CreditAttribution: chx commented 13 January 2014 at 03:55

Actually, db_update and db_delete already returns the number of affected row, for insert the affected is 1 unless it was a multi insert where affected is not defined. So pretty sure an external rowCount() is a botched attempt on a SELECT.

Comment #8

sun

German

Karlsruhe

CreditAttribution: sun commented 13 January 2014 at 04:12

The error scenario can be triggered in more situations than anticipated.

The major failures being fixed in #2167507: Fix rowCount query usage in pgsql and sqlite drivers are caused by UPDATE queries being performed as a db_query()/Query.

Even though I understand the original intentions of #2146733: Select queries should not use rowCount() to calculate number of rows, the actual implementation does not account at all for the actual query statement being executed. → The exception is thrown for perfectly valid PDO statements that are not SELECT queries, whereas the PDO docs clearly make an exception on SELECT queries only.

If the original goal of #2146733 was to force everyone to use (Drupal) Insert, Update, and Delete query statements, then I think the condition was implemented in the completely wrong spot. If that is the actual goal, then Database\Connection::query() should perform this check instead:

if (preg_match('@^(INSERT|UPDATE|DELETE)@i', $query)) {
  throw new \InvalidArgumentException('Invalid query attempt. Use insert(), update(), or delete() instead.');
}

But as long as that isn't the case, direct insert/update/delete queries are valid, and consumers are able to retrieve the count of affected rows of such insert/update/delete queries by passing the Database::RETURN_AFFECTED constant.

In turn, the currently proposed exception message appears to resemble the API within the exact technical constraints and intentions.

That is, of course, unless the intentions are different to the actual implementation. ;)

Comment #9

chx CreditAttribution: chx commented 13 January 2014 at 04:29

We are not parsing SQL strings. We are not. That's not going to happen.

> direct insert/update/delete queries are valid

Nope. They were not since DBTNG got committed to core. There's a lengthy comment explaining this but let me reiterate:when a consumer runs a data change query it has no idea what database it will run on and the structured builder allows the driver to do whatever manipulations necessary (for example blob processing) . The drivers do not have this problem -- they do know what database they run on and so they can execute whatever they fancy.

Comment #10

Crell CreditAttribution: Crell commented 13 January 2014 at 04:39

chx is correct here. While running a raw update or insert query is not something we can realistically prevent (we considered doing so and rejected it), it's not a supported operation. If you want to run query("UPDATE ...") and get an affected rows back... eh, not our problem. We don't support that.

I'm not sure off hand what return mode is legal for DDL statements, but those have an API as well instead of raw queries so again, not supported.

Suggested exception text: "rowCount() is not supported on select queries. Use a count query instead, such as that returned by Select::getCountQuery()."

Comment #11

mradcliffe

he/him

English

CreditAttribution: mradcliffe commented 13 January 2014 at 14:35

I don't advocate using db_query() either. However I understand sun because db_update() didn't support everything I needed in a narrow use case.

If for whatever reason someone needed to do this then they have the following options:

Override the database service with a sub-class of Connection, and use a sub-class of Statement. :-)
Don't use DBTNG, use PDO.
Hack core by apply the patch.

Comment #12

Damien Tournoud CreditAttribution: Damien Tournoud commented 13 January 2014 at 14:42

chx is correct here. While running a raw update or insert query is not something we can realistically prevent (we considered doing so and rejected it), it's not a supported operation. If you want to run query("UPDATE ...") and get an affected rows back... eh, not our problem. We don't support that.

That doesn't make sense.

If you write a raw UPDATE or INSERT query, you can expect to be on your own. But you can also except that all the standard PDO mechanisms are going to work.

IMO, we should just proxy whatever PDO returns in the case ->rowCount() is called on a statement directly created by ->query().

Comment #13

Damien Tournoud CreditAttribution: Damien Tournoud commented 13 January 2014 at 14:45

> direct insert/update/delete queries are valid

Nope. They were not since DBTNG got committed to core. [...]

I do agree that direct insert/update/delete are not *supported in a portable manner*. AKA: you are on your own, this is uncharted territory. Use at your own risk.

That said, as long as we don't explicitly disallow them (which I said multiple time I think would be a good idea), we need to provide at least the same degree of functionality as naked PDO would.

Comment #14

sun

German

Karlsruhe

CreditAttribution: sun commented 14 January 2014 at 00:44

If you write a raw UPDATE or INSERT query, you can expect to be on your own.

not supported in a portable manner

That was my understanding as well.

Especially because of what @mradcliffe mentioned — Drupal's Database query features can be too limiting, so in a custom function of a custom.module on a custom site, I should be allowed to fully leverage my database engine's features without having to write a completely new database driver?

In light of that, I wonder whether the proposed message isn't accurate?

rowCount() is supported for DELETE, INSERT, or UPDATE statements only. Consider to use the Database::RETURN_AFFECTED option.

Perhaps as a compromise, how about the following?

rowCount() is supported for DELETE, INSERT, or UPDATE statements only. Either use the Database::RETURN_AFFECTED option or use a Delete, Insert, or Update query object instead.

I.e., to get the count of affected rows for a raw query, pass that option. Or use a query object instead.

Comment #15

chx CreditAttribution: chx commented 14 January 2014 at 02:34

rowCount() is supported for DELETE, INSERT, or UPDATE statements only. Running these statements in db_query is not portable across database engines and is not supported. Use the structured query builders instead. If the structured query builders are not sufficient, the Database::RETURN_AFFECTED option can be used to get the number of rows affected.

Comment #16

Crell CreditAttribution: Crell commented 15 January 2014 at 06:41

#15 would make this the most detailed and descriptive exception string in all of Drupal. I have to admit, that has a soft spot for me. :-)

If we go that route, though, it should say what to do with Database::RETURN_AFFECTED. As is, #15 says to use it but not how, where, what key, etc. Something like "... set the 'return' option to Database::RETURN_AFFECTED to get the number of affected rows."

Comment #17

sun

German

Karlsruhe

CreditAttribution: sun commented 16 January 2014 at 16:54

Status:

Needs work

» Needs review

File	Size
drupal8.rowcountexception-dx.17.patch	1.04 KB

interdiff.txt	915 bytes

Works for me.

rowCount() is supported for DELETE, INSERT, or UPDATE statements performed with structured query builders only, since they would not be portable across database engines otherwise. If the query builders are not sufficient, set the 'return' option to Database::RETURN_AFFECTED to get the number of affected rows.

Comment #18

dcrocks CreditAttribution: dcrocks commented 16 January 2014 at 19:51

Status:

Needs review

» Reviewed & tested by the community

File	Size
rowcount.jpg	66.82 KB

Tested on a copy of Dev 8.x created after #2167507: Fix rowCount query usage in pgsql and sqlite drivers and before #2146733: Select queries should not use rowCount() to calculate number of rows. Output as expected.

Comment #19

webchick

she/they

English

Vancouver 🇨🇦

CreditAttribution: webchick commented 17 January 2014 at 22:16

Status:

Reviewed & tested by the community

» Needs review

Hm. I was going to commit this until I looked at the screenshot. I'd have no idea how to resolve that myself. How about a file/line number in there somewhere?