WordPress.org

Make WordPress Core

Opened 8 years ago

Closed 7 years ago

Last modified 7 years ago

#26106 closed defect (bug) (wontfix)

Passing empty array to wpdb::update() generates invalid SQL

Reported by: Viper007Bond Owned by: wonderboymusic
Milestone: Priority: normal
Severity: normal Version:
Component: Database Keywords: dev-feedback
Focuses: Cc:

Description

I wanted to update all rows in a database table, so I did this:

$wpdb->update(
	'custom_table',
	array(
		'column' => 'value',
	),
	array()
);

However this generates a query like this:

UPDATE `custom_table` SET `column` = 'value' WHERE

So we either need some validation and bail if it's empty or better, we should drop the WHERE, allowing queries like mine.

Attachments (3)

26106.diff (724 bytes) - added by UmeshSingla 7 years ago.
Allow to update all rows in a table
26106.2.diff (621 bytes) - added by UmeshSingla 7 years ago.
Allow to update all rows in a table
26106.3.diff (830 bytes) - added by SergeyBiryukov 7 years ago.

Download all attachments as: .zip

Change History (22)

#1 follow-up: @nacin
8 years ago

Seems dangerous to allow an unbounded UPDATE with ease. Perhaps we could allow for something like exactly array( '1' => '1' ) (as in, WHERE 1=1) as an under-the-hood, explicit way of doing an unbounded UPDATE.

#2 in reply to: ↑ 1 ; follow-up: @Viper007Bond
8 years ago

Replying to nacin:

Seems dangerous to allow an unbounded UPDATE with ease. Perhaps we could allow for something like exactly array( '1' => '1' ) (as in, WHERE 1=1) as an under-the-hood, explicit way of doing an unbounded UPDATE.

That could work, and infact that's what I tried before I realized that the first 1 would be treated as a column name.

#3 @wonderboymusic
7 years ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to 4.0

#4 in reply to: ↑ 2 @UmeshSingla
7 years ago

Replying to Viper007Bond:

Replying to nacin:

Seems dangerous to allow an unbounded UPDATE with ease. Perhaps we could allow for something like exactly array( '1' => '1' ) (as in, WHERE 1=1) as an under-the-hood, explicit way of doing an unbounded UPDATE.

That could work, and infact that's what I tried before I realized that the first 1 would be treated as a column name.

There is a workaround for 1 being treated as column name, but a query something like that could screw the whole database like I ended up corrupting my whole user meta, if no precautions are taken.

@UmeshSingla
7 years ago

Allow to update all rows in a table

@UmeshSingla
7 years ago

Allow to update all rows in a table

#5 @wonderboymusic
7 years ago

  • Owner set to wonderboymusic
  • Resolution set to fixed
  • Status changed from new to closed

In 28814:

In $wpdb->update(), prevent explosions when $where is empty.

Adds unit tests.

Props UmeshSingla, wonderboymusic.
Fixes #26106

#6 @UmeshSingla
7 years ago

  • Keywords needs-patch removed

#7 @UmeshSingla
7 years ago

@wonderboymusic, We are keeping the update query unbounded, I mean you've removed the 1=1 part from patch, is it intentional?

#8 @nacin
7 years ago

  • Keywords dev-feedback added

The 1=1 part doesn't matter as to whether it is unbounded. That was just a suggestion as to how you could make it "explicit" in that you wanted an unbounded query.

To be honest, I'm still very scared about this. One bad variable and suddenly you've updated all rows. I don't love this as an API change and would like to see if anyone else agrees for a possible 4.0 revert.

Two of the longest downtime events of WordPress.com in the last 4-5 years were, to my recollection, as follows:

  • A bad variable name/assumption that overwrote entire options tables.
  • An unbounded query that overwrote the entire domain mapping table.

This fix makes it trivial to combine both of these events into one. Let's not, please.

#9 @nacin
7 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

#10 @nacin
7 years ago

In this case, the update() method still rejects any $where that is not an array. So this requires an explicit empty array() to be passed. Is that really enough to prevent issues like this?

$where = array();
if ( something ) {
   $where[] = ...;
} elseif ( something else ) {
   $where[] = ...;
}

$wpdb->update( $table, $data, $where ); // oops, $where = array()

Maybe I'm just assuming too little of developers, but it's very possible that at least one plugin is making this mistake right now and we're going from a quiet SQL error to wiping the entire table with the 4.0 update.

I'd probably rather see separate update_all() and delete_all() methods or something.

#11 @helen
7 years ago

Revert sounds good to me. Empty array() is definitely not explicitly asking for an unbounded query.

#12 @pento
7 years ago

+1 for revert. I agree with the original premise of the ticket (that there should be a way to do unbounded updates), but this change makes it too easy for someone to make a disastrous mistake.

#13 @nacin
7 years ago

In 29664:

DB: Revert [28814] and require a WHERE for wpdb::update().

see #26106.

#14 @nacin
7 years ago

  • Milestone 4.0 deleted
  • Resolution set to wontfix
  • Status changed from reopened to closed

I left the unit tests here to enforce the design decision. If anyone tries to change this five years from now, the tests will fail, and they'll be led here.

#15 @markjaquith
7 years ago

100% agree with revert. Yikes. That code scenario Nacin outlined is entirely plausible.

#16 @Viper007Bond
7 years ago

As the reporter, I agree with the revert as well. pento sums it up nicely -- should be possible but not easy.

#17 @SergeyBiryukov
7 years ago

$wpdb->show_errors(false) does not work as expected in [29664]:

.............................................................   61 / 2504 (  2%)
...........................SS...SSSSSS.............S...SS....  122 / 2504 (  4%)
........SS................S...........SS...SSSSSS............  183 / 2504 (  7%)
.S...SS............SS..........................S.............  244 / 2504 (  9%)
.............................................................  305 / 2504 ( 12%)
.WordPress database error You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near '' at line 1 for query UPDATE `wptests_posts` SET `post_name` = 'burrito' WHERE  made by PHPUnit_TextUI_Command::main, PHPUnit_TextUI_Command->run, PHPUnit_TextUI_TestRunner->doRun, PHPUnit_Framework_TestSuite->run, PHPUnit_Framework_TestSuite->run, PHPUnit_Framework_TestSuite->runTest, PHPUnit_Framework_TestCase->run, PHPUnit_Framework_TestResult->run, PHPUnit_Framework_TestCase->runBare, PHPUnit_Framework_TestCase->runTest, ReflectionMethod->invokeArgs, Tests_DB->test_empty_where_on_update

26106.3.diff fixes that.

This ticket was mentioned in IRC in #wordpress-dev by SergeyBiryukov. View the logs.


7 years ago

#19 @SergeyBiryukov
7 years ago

In 29701:

Properly suppress errors in test_empty_where_on_update(), which expects an empty WHERE clause.

see #26106.

Note: See TracTickets for help on using tickets.