Saving metadata fails (randomly) if equal value already exists

[JVel]

When using js rest api to save page.

Duplicate value test in wp-includes/rest-api/fields/class-wp-rest-meta-fields.php line 303 fails and causes error "Could not update meta value in database."

if ( $old_value[0] === $meta_value ) {

Problem seems to be that $old_value is not going through wp_slash but new $meta_value is.

I get it working by changing code to test against $value instead.

if ( $old_value[0] === $value ) {

[pilou69]

Having an issue with this too in 4.8.2.

It's not random, it always happens when the meta is not modified and has any characters that must be slashed.

We found 3 ways to work around this:

1. Apply modification above, but it overrides the core :(

   if ( $old_value[0] === $value ) {
   

2. Register meta fields as single => false. But that seems very ugly.

3. Create custom endpoint:

create a class extending WP_REST_Meta_Fields, and override update_meta_value function with patch applied above

extend WP_REST_Posts_Controller (WP_REST_{$type}_Controller),)

override in you custom endpoint controller
$this->meta with your Class with the patch

[dcavins]

Improve reliability of updating meta via REST API.

[dcavins]

I too ran into this problem today and was able to identify two cases where WP_REST_Meta_Fields::update_meta_value() was failing to interpret the values as identical, but update_metadata() was returning false (because the values were considered equal by that function), so the REST request was getting a 500 error back. These two cases are problematic:

• Text containing characters that are slashed or converted to entities, like " or <.
• Booleans, like a checkbox, that, when unchecked, pass the integer 0.

In the first case, the comparison in WP_REST_Meta_Fields::update_meta_value() tries to strict compare < to <, and, in the second case, the string '0' (all saved meta is strings) is compared to the incoming sanitized value of (int) 0.

I've created a diff with tests showing these failures and a proposed patch to improve the comparison in WP_REST_Meta_Fields::update_meta_value() so that it catches more cases that update_metadata() or $wpdb->query() will consider equal.

If there are other cases that I can write tests for, please describe the situation in detail in your comments. I'd like to find all failures and make sure that the fix works for all of them.

Thanks for your comments!

[dcavins]

Also note that #42810 is related.

[MattGeri]

I'm encountering this issue too. I've looked at the patch and my suggestion would be that you remove the sanitization from the old value as it's already unslashed and sanitized. Sanitization and unslashing should only be applied to $meta_value.

So the conditional could read something like this, to satisfy all the usecases and tests:

if ( $old_value[0] === sanitize_meta( $meta_key, wp_unslash( $meta_value ), $meta_type ) ) {

Let me know your thoughts and if you agree but can't make the changes to the patch, I'd be happy to.

[dcavins]

Replying to MattGeri:
Thanks for your comments. Your change is close to what TimothyBlynJacobs suggested in https://core.trac.wordpress.org/attachment/ticket/42810/42810.diff

Can you offer a use case that fails in his previous patch, so that we can explain the necessity of adding wp_unslash()?

I'm fuzzy on the details, but it seems like one of my test cases was failing with the previous patch, probably the '0' (from saved meta) === 0 (incoming sanitized POST) case, which is why I added the sanitize step to the saved value as well. It's easy to test, but hard to remember something from a few months ago.

Thanks!

[MattGeri]

No worries. I think it was actually the other test that would have been failing.

The issue is that $old_value[0] gets returned unslashed from get_metadata, but $meta_value is wrapped in slashes earlier in the method. So it's trying to compare slashed against unslashed data, which the other solution does not take in to account.

The data in meta.php > update_metadata() gets unslashed on line 167 this is before the same comparison as the rest function happens to check if an update should occur.

Edit: So an example usecase would be text with a single quote in it.

[dcavins]

Update tests to use new REST fake server. Add test of single-slash case.

[dcavins]

Hi @MattGeri- I've tested your suggestion, and it works for textured text, but fails when updating booleans (test_update_value_return_success_response_when_updating_boolean_values_no_change() here: https://core.trac.wordpress.org/attachment/ticket/42069/42069.2.diff)

I've added a test that includes a single quote per your suggestion. Any other feedback is welcome--it'd be great to have comprehensive tests of cases we know fail.

Without the change, all three of my test cases fail. :(

[dcavins]

Update tests to use new REST fake server. Add test of single-quote case.

[flixos90]

I cannot replicate this issue unfortunately. The tests you wrote all passed for me without actually using the changes in the code. @dcavins @MattGeri @adamsilverstein Can you provide some more background on your environment? I wonder if it's some PHP version-related detail or similar.

Regarding the patch, I just uploaded 42069.diff​ with optimized tests using a data provider, which reduces the necessary code and at the same time tests a lot more combinations. There's one test for checking updating a value with exactly the same value, and another one for checking updating a value with another value that essentially means the same (like 0/1 or false/true for booleans).
My new patch only includes the tests as they passed without the code change. I'm not at all saying that change is wrong, it just did not fix a problem for me, so I excluded it for now. Of course, as we determine where exactly this problem is coming from, we can add the fix back as needed. Please use the new patch going forward and iterate from there.

[adamsilverstein]

@flixos90

Nice work on the unit test cleanup! I pushed the changes from 42069.diff​ to a branch/fork to run in travis and see some meta tests failing, surprising these don't fail for you locally. are you sure the new tests are running, possibly you need to run build before testing?

​https://travis-ci.org/adamsilverstein/wordpress-develop-fork/builds/396971510

[flixos90]

@adamsilverstein I did make sure all of this is there. Must be something in my setup, maybe I can dive in in a couple days. Did you check whether bringing back the actual code change makes all those tests pass? My general feeling is, if so, we can commit this. Those tests should catch enough of these cases.

[dcavins]

Thanks very much for the unit test expansion, @flixos90! I haven't seen or used the dataProvider functionality and look forward to simplifying many of my unit tests in the future.

I also tested the new tests, and I'm not seeing any failures on my local machine, either. However, when I create a build, I'm still seeing a 500 response code when updating textured text via REST, so something must have changed in the test environment that is masking the failures.

Thanks again for taking a look at this issue.

[MattGeri]

@flixos90 This is still very much an issue for us in a production environment. I've run your tests locally and some of the tests fail for me (as expected). By applying the patch I'm attaching 42069.1.2.diff​ (combination of your tests + a modified fix from above), it fixes the issue and the tests pass. Not too sure why they're passing on your local without the changes applied, but I've got multiple use cases where this bug breaks saving meta because it's not unslashed.

I do however seem to get 2 unrelated test fails when I run them. The dataProvider is somehow passing the test_bad_auth meta field in to tests that expect a 200 response, and the update fails. I'll try look in to this when I get some free time.

@dcavins In the patch I'm attaching 42069.1.2.diff​, I've applied what I suggested above (only sanitize and unslash the $meta_value coming in and not the $old_value) and the tests pass. I'm not sure why you were only able to get the tests (and boolean use cases) to pass by sanitizing the old and new values. We shouldn't need to sanitize the old value as the original method in meta.php doesn't.

[dcavins]

Hi @MattGeri-

Thanks for your interest in this problem. I took a minute to test your patch, and meta of the type boolean or integer is still failing. Example meta definitions:

// Boolean
register_meta( 'post', 'featured_item', array(
        'sanitize_callback' => 'absint',
        'type' => 'boolean',
        'description' => 'test',
        'single' => true,
        'show_in_rest' => true,
) );

// Integer
register_meta( 'post', 'post_order', array(
        'sanitize_callback' => 'absint',
        'type' => 'integer',
        'description' => 'test',
        'single' => true,
        'show_in_rest' => true,
) );

The way-down-deep issue for this case is that if you pass in an integer, like 1, and the strictly equal check doesn't catch the equivalency, it will also pass the check in update_metadata() (so at least it won't return false at that point) and be passed to $wpdb->update(). $wpdb->update() relies on $wpdb->query() for the actual insert, and the returned value is the number of rows affected, which is 0. This causes update_metadata() to return false at this point: https://core.trac.wordpress.org/browser/tags/4.9.8/src/wp-includes/meta.php#L251

My patch catches this case because both the old value and new value are cast to be the same type by the sanitization functions. Now that I know where the integer/boolean case is failing, I see that there are more ways to achieve this result, like relaxing the strictness of the check in WP_REST_Meta_Fields::update_meta_value():

if ( sanitize_meta( $meta_key, wp_unslash( $meta_value ), $meta_type ) == $old_value[0] ) {
        return true;
}

Alternatively, we could cast the incoming value as a string, because we know all meta values retrieved by get_metadata() are strings:

if ( (string) sanitize_meta( $meta_key, wp_unslash( $meta_value ), $meta_type ) === $old_value[0] ) {
        return true;
}

Thanks again for your thoughts on this problem.

[MattGeri]

@dcavins Thanks for the insight. The unit tests provided by @flixos90 do not seem to cover that use case (at least on my local) as they all passed with my patch. If they also pass your side, could you add a test case which would cover the issue?

Relaxing the strictness would not be an option as it's against the code standards. Typecasting as string seems to be viable but we should provide a failing unit test first.

[dcavins]

Ah, the test setup was broken--the meta registration was moved in one of the patch updates. I'll upload a new patch that corrects the test setup and also moves to the (string) sanitize_meta( $meta_key, wp_unslash( $meta_value ), $meta_type ) === $old_value[0] comparison check.

With the corrected tests (and the old comparison code in place), 9 out of 15 assertions fail:

1) WP_Test_REST_Post_Meta_Fields::test_update_value_return_success_with_same_value with data set #0 ('test_boolean_update', 0)
Failed asserting that 500 matches expected 200.

2) WP_Test_REST_Post_Meta_Fields::test_update_value_return_success_with_same_value with data set #2 ('test_boolean_update', false)
Failed asserting that 500 matches expected 200.

3) WP_Test_REST_Post_Meta_Fields::test_update_value_return_success_with_same_value with data set #4 ('test_boolean_update', '')
Failed asserting that 400 matches expected 200.

4) WP_Test_REST_Post_Meta_Fields::test_update_value_return_success_with_same_value with data set #5 ('test_boolean_update', '0')
Failed asserting that 500 matches expected 200.

5) WP_Test_REST_Post_Meta_Fields::test_update_value_return_success_with_same_value with data set #7 ('test_textured_text_update', 'She said, "What about the > 1...chen?"')
Failed asserting that 500 matches expected 200.

6) WP_Test_REST_Post_Meta_Fields::test_update_value_return_success_with_same_value with data set #8 ('test_textured_text_update', 'He's about to do something rash...')
Failed asserting that 500 matches expected 200.

7) WP_Test_REST_Post_Meta_Fields::test_update_value_return_success_with_similar_value with data set #0 ('test_boolean_update', 0, '')
Failed asserting that 400 matches expected 200.

8) WP_Test_REST_Post_Meta_Fields::test_update_value_return_success_with_similar_value with data set #2 ('test_boolean_update', false, 0)
Failed asserting that 500 matches expected 200.

9) WP_Test_REST_Post_Meta_Fields::test_update_value_return_success_with_similar_value with data set #4 ('test_boolean_update', '', false)
Failed asserting that 500 matches expected 200.

7 of these failures are the expected 500 when update_metadata() fails. 2, though, are "Bad Request" 400 errors, caused by passing an empty string as the update value to the test, and these are unaffected by the changes, so I'm assuming that they happen upstream of the actual update routine.

With the new comparison in place, there are only the two "Bad Request" failures:

1) WP_Test_REST_Post_Meta_Fields::test_update_value_return_success_with_same_value with data set #4 ('test_boolean_update', '')
Failed asserting that 400 matches expected 200.

/Users/dcavins/Sites/develop.git.wordpress.org/tests/phpunit/tests/rest-api/rest-post-meta-fields.php:1239

2) WP_Test_REST_Post_Meta_Fields::test_update_value_return_success_with_similar_value with data set #0 ('test_boolean_update', 0, '')
Failed asserting that 400 matches expected 200.

Thanks again for the help with getting this change accepted.

[dcavins]

Correct problem in test setup; use string typecast to improve equivalency check.

[boonebgorges]

Quoting from https://core.trac.wordpress.org/ticket/42810#comment:10:

I'd like to flag this as a Gutenberg blocker, because of the following situation:

1. You have a post with more than one block that uses post meta attributes
2. One or more of those blocks save their data as a JSON-encoded string, as recommended in the documentation ​https://wordpress.org/gutenberg/handbook/block-api/attributes/#considerations
3. You save the post without modifying the data in one or more of the blocks

Because the REST API uses *slashed* data to compare with the existing values, but get_metadata() returns *unslashed* data, most JSON-encoded strings will look "new" to the REST API https://core.trac.wordpress.org/browser/tags/4.9.8/src/wp-includes/rest-api/fields/class-wp-rest-meta-fields.php?marks=299,301#L287 But update_metadata() correctly compares unslashed-to-unslashed values, so returns false. Not only does this cause an "Updating failed" notice in Gutenberg, but because of the way that the update_value() method returns early out of the foreach() loop, blocks further down the page are not processed. https://core.trac.wordpress.org/browser/tags/4.9.8/src/wp-includes/rest-api/fields/class-wp-rest-meta-fields.php?marks=131,163-165#L129

[TimothyBlynJacobs]

#42810 was marked as a duplicate.

[boonebgorges]

Responding to @dcavins https://core.trac.wordpress.org/ticket/42810#comment:12:

Hm, the only way I could get any of these tests to fail at all was to register_meta(). Otherwise the REST API endpoint was ignoring them. See 42069.4.diff​. With that change, the tests are now failing/passing as expected.

[dcavins]

Replying to boonebgorges:

Responding to @dcavins https://core.trac.wordpress.org/ticket/42810#comment:12:

Hm, the only way I could get any of these tests to fail at all was to register_meta(). Otherwise the REST API endpoint was ignoring them. See 42069.4.diff​. With that change, the tests are now failing/passing as expected.

Yes, the meta was registered in the setup() method in the earlier patches, like https://core.trac.wordpress.org/attachment/ticket/42069/42069.3.diff

The only advantage over the way you did it was that then the metas are created with a sanitization function appropriate to their type. I'll attach a patch that adds your JSON string test that _does_ fail for me, even though my example didn't (a mystery? ha ha). Thanks for being interesting in this issue. It has caused me some problems with moving old admin-ajax-reliant code to using the REST API.

[dcavins]

Add JSON string test.

[dcavins]

Use better meta name from Boone's patch.

[danielbachhuber]

@boonebgorges @adamsilverstein Are you happy with 42069.5.2.diff?

[adamsilverstein]

@danielbachhuber Looks good to me!

[boonebgorges]

I don't think this is ready to commit yet. The test_boolean_update tests are failing for me:

`
There were 2 failures:

1) WP_Test_REST_Post_Meta_Fields::test_update_value_return_success_with_same_value with data set #4 ('test_boolean_update', )
Failed asserting that 400 matches expected 200.

/home/bgorges/sites/wpdev/tests/phpunit/tests/rest-api/rest-post-meta-fields.php:1209

2) WP_Test_REST_Post_Meta_Fields::test_update_value_return_success_with_similar_value with data set #0 ('test_boolean_update', 0, )
Failed asserting that 400 matches expected 200.

/home/bgorges/sites/wpdev/tests/phpunit/tests/rest-api/rest-post-meta-fields.php:1247

`

The problem seems to be that the test_boolean_update meta key is registered with type=boolean (and sanitize_callback=absint, which doesn't seem like a proper match). As such, passing values like 1 or "1" is causing this check to fail https://core.trac.wordpress.org/browser/tags/4.9.8/src/wp-includes/rest-api/fields/class-wp-rest-meta-fields.php#L149 (@dcavins alludes to this above https://core.trac.wordpress.org/ticket/42069#comment:17)

If I understand the design correctly, the strict checks that are coupled with proper use of register_meta() mean that we should not be testing for loosely-equal values in this way. In other words, meta values are not exposed via the REST endpoints unless they're registered with a strict type, and thus we don't support non-strict updates. If that's correct, then I believe the proper patch is 42069.6.diff​.

This could use a review from @dcavins (as I may be misunderstanding the idea behind this part of his patches) as well as a member of the REST API team.

[dcavins]

Thanks @boonebgorges for taking another look at this.

Yes, the extra boolean similar-value tests were included because it appears that meta registered with the boolean type is more permissive than, say, meta registered with the integer type. (The integer type is pretty permissive, though, because it will allow numeric strings.)

If a meta is set to boolean, it seems to be currently possible to pass any boolean-ish value to it successfully, like true, false, 0, 1, "0", or "1" (or any other string, actually) without triggering the mismatched-type 400 error.

The above comments are an aside from Boone's patch, which is the necessary fix. The other tests I had included I agree should be removed because they're requiring the API to cover for poor quality code.

Thanks everyone for your interest in this ticket!

[TimothyBlynJacobs]

I made a couple of changes.

1. Metadata has subtypes now, so we need to take this into account when sanitizing.
2. We weren't handling the slashing of meta key consistently. It needs to be slashed when updating meta, but unslashed for all the other functions. Because of this I changed the code to keep the unslashed versions around instead of continually unslashing them and then slashed as the values went into update_metadata().

[danielbachhuber]

@boonebgorges Are you happy with 42069.7.diff?

[TimothyBlynJacobs]

There was an issue applying the patch, for some reason the new register_post_meta calls were getting inserted into the middle of another test method.

[boonebgorges]

Thanks for catching that in 42069.8.diff​, @TimothyBlynJacobs . I've just added 42069.9.diff​ , which includes the following mods to 8.diff:

• Coding standards fixes
• Added missing @ticket annotation on new test
• Adds a small amount of logic to the test class constructor so that errors are not thrown if the tests are run in isolation (eg using --group 42069)

I think this is ready for commit, which either I can do, or a member of the REST API team.

[danielbachhuber]

@boonebgorges Please do ship it :)

[kadamwhite]

In 43740:

REST API: Slash existing meta values when comparing with incoming meta upates.

When comparing the old and new values for a meta key being set, ensure both values are sanitized using the same logic so that equal values match.

props boonebgorges, dcavins, MattGeri, pilou69, TimothyBlynJacobs.
Fixes #42069.

[danielbachhuber]

Reopening for merge against trunk.

[desrosj]

In 44113:

REST API: Slash existing meta values when comparing with incoming meta upates.

When comparing the old and new values for a meta key being set, ensure both values are sanitized using the same logic so that equal values match.

props boonebgorges, dcavins, MattGeri, pilou69, TimothyBlynJacobs, kadamwhite.

Merges [43740] to trunk.

Fixes #42069.