WordPress.org

Make WordPress Core

Opened 6 years ago

Last modified 4 months ago

#27272 reopened defect (bug)

WP meta queries comparing apples to oranges

Reported by: Denis-de-Bernardy Owned by:
Milestone: Priority: normal
Severity: normal Version: 3.8.1
Component: Query Keywords: has-patch
Focuses: Cc:
PR Number:

Description

As seen when hooking into the posts_selection hook while debugging a site:

( (ts_wp_postmeta.meta_key = 'financial_max_annual' AND CAST(ts_wp_postmeta.meta_value AS SIGNED) <= '100000') )

The thing that led to that was:

                $meta_query[] = array(
                    'key'       => $meta_key,
                    'value'     => $max_fee,
                    'compare'   => '<=',
                    'type'      => 'NUMERIC',
                );

Can I suggest that, when we CAST, we do so on both sides?

Attachments (9)

27272.diff (3.0 KB) - added by wonderboymusic 6 years ago.
27272.2.diff (2.9 KB) - added by wonderboymusic 6 years ago.
27272.test.patch (2.2 KB) - added by boonebgorges 5 years ago.
27272_meta.diff (1.9 KB) - added by gitlost 5 years ago.
Patch ( 2nd wonderboy one) updated for 4.1.
27272_test.diff (1.7 KB) - added by gitlost 5 years ago.
Unit tests for patch, large numerics.
27272_meta2.diff (2.0 KB) - added by gitlost 4 years ago.
Refresh for 4.4.
27272_test2.diff (1.7 KB) - added by gitlost 4 years ago.
Refresh for 4.4.
27272_meta3.diff (2.0 KB) - added by gitlost 4 years ago.
Refresh for 4.5.
27272_test3.diff (3.0 KB) - added by gitlost 4 years ago.
Added back original tests from 27272.2.diff.

Download all attachments as: .zip

Change History (32)

#1 @wonderboymusic
6 years ago

  • Keywords needs-patch needs-unit-tests added
  • Milestone changed from Awaiting Review to 3.9

Yes.

@wonderboymusic
6 years ago

#2 @wonderboymusic
6 years ago

27272.diff implements this for SIGNED and UNSIGNED only. Other casts were behaving weirdly.

#3 @nacin
6 years ago

Do we actually need to cast both sides? Could we just make sure that a numeric cast on one side ends up with an integer on the other?

#4 @wonderboymusic
6 years ago

Could use %d and %u as $cast_string for those instead of the explicit cast

#5 @wonderboymusic
6 years ago

27272.2.diff is better - could add more cases as necessary

#6 @Denis-de-Bernardy
6 years ago

  • Version changed from 3.7.1 to 3.8.1

I think I preferred the first patch. Casting both side leaves the door open to doing:

where cast(meta_key as date) > cast(%s as date)

At a conceptual level, I'd suggest the following high-level test case:

  1. Insert post where meta_value = 2
  2. Verify that it's found if the meta value query is >= 1
  3. Verify that it's NOT found if the meta value query is >= 10

#7 @wonderboymusic
6 years ago

I'll incorporate that into a unit test - will ask some others to weigh in on the generated SQL

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


6 years ago

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


6 years ago

#10 @samuelsidler
6 years ago

  • Keywords 4.0-early added
  • Milestone changed from 3.9 to Future Release

This has been like this since meta queries were introduced in 3.1. It was reported fairly late in the cycle. We can deal with it properly in 4.0.

#11 follow-up: @boonebgorges
5 years ago

  • Keywords 4.0-early removed

What is it that we're trying to accomplish with the casting? MySQL does some somewhat intelligent type juggling based on the operators you use https://dev.mysql.com/doc/refman/5.0/en/type-conversion.html. So the kind of example Denis-de-Bernardy describes here https://core.trac.wordpress.org/ticket/27272#comment:6 is actually already supported. (See 27272.patch.) As for things like date, my tests suggest that as long as you're passing the proper 'type' param for casting the 'meta_value' column, MySQL will automatically convert the type of your comparison value. So CAST('2012-04-04' AS DATE) > CAST(meta_value AS DATE) is redundant.

That said, it's possible that I'm missing something. Can we get a unit test that demonstrates how the suggested casting makes a difference in results?

#12 in reply to: ↑ 11 @Denis-de-Bernardy
5 years ago

Replying to boonebgorges:

That said, it's possible that I'm missing something. Can we get a unit test that demonstrates how the suggested casting makes a difference in results?

See comment-6 further up:

  1. insert post meta where value = 2
  2. test that a query where value >= 1 yields the row
  3. test that a query where value >= 10 does NOT yield the row

The test currently fails miserably at step 3, because the SQL it produces is like:

CAST(wp_postmeta.meta_value AS SIGNED) <= '10'

The left-hand side cast is basically ignored, because 10 got sanitized as a string.

Therefor my initial suggestion: rather than introduce potential for problems by not systematically sanitizing things, just cast both sides of the operator when a plugin is explicitly requesting a typed meta query.

#13 @boonebgorges
5 years ago

The test currently fails miserably at step 3

See 27272.test.patch. It demonstrates that these queries do *not* currently fail for types SIGNED, NUMERIC, and DATETIME. Note that the key here is that, in each case, a 'type' is passed with the tax query clause. These same queries will fail if you don't pass an explicit 'type', because the cast will fall back on strings. But as long as you are manually handling the casting on one side of the comparison, MySQL will handle it on the other side.

There is a further question about whether we should be kinder to developers, by assuming that they mean "SIGNED" when they pass a numeric 'value' but don't specify the 'type'. IMO this would probably be a decent enhancement, but I think it's different from what's being suggested here.

#14 @gitlost
5 years ago

Queries will fail though (depending on system) if you trigger the MySQL string to float conversion mentioned in https://dev.mysql.com/doc/refman/5.0/en/type-conversion.html by using large numbers, see appended updated meta patch and test patch.

MySQL does do good type-juggling, but it seems frankly silly to invoke it unnecessarily. Quoting numerics ain't natural!

@gitlost
5 years ago

Patch ( 2nd wonderboy one) updated for 4.1.

@gitlost
5 years ago

Unit tests for patch, large numerics.

@gitlost
4 years ago

Refresh for 4.4.

@gitlost
4 years ago

Refresh for 4.4.

#15 @gitlost
4 years ago

Refreshes for gardening drive.

#16 @gitlost
4 years ago

  • Keywords needs-patch needs-unit-tests removed

#17 @gitlost
4 years ago

  • Keywords has-patch added

@gitlost
4 years ago

Refresh for 4.5.

#18 @gitlost
4 years ago

Refresh for 4.5 Bug Scrub (just removes "Hunk succeeded" messages). 27272_test2.diff still good.

This ticket was mentioned in Slack in #core by chriscct7. View the logs.


4 years ago

@gitlost
4 years ago

Added back original tests from 27272.2.diff.

#20 @gitlost
4 years ago

  • Resolution set to invalid
  • Status changed from new to closed

@boonebgorges per https://wordpress.slack.com/archives/core/p1454693086001759, added back tests that were missed (originally in "27272.2.diff").

#21 @gitlost
4 years ago

  • Resolution invalid deleted
  • Status changed from closed to reopened

Didn't mean to change the resolution or status...

#22 @ericlewis
3 years ago

It seems the original bug reported by this ticket is not a bug (see comment:13). I suggest we close this ticket as worksforme after tying up loose ends, as a few engaging points have been brought up.

  • @boonebgorges wrote a unit test to express that the original bug report is invalid. Was that intended as a prop for confirming the bug wasn't a bug, or should we consider merging that test?
  • There's some thought about using the meta_type do decide which numeric format to use when building the SQL query (@wonderboymusic's attachment:27272.2.diff which @gitlost has kept fresh). This is a separate issue from the original bug report, although related. We should consider using the type of the value passed in the meta query too for a good default that doesn't require a type parameter (basically what @boonebgorges suggested in comment:13). I've opened #36652 to that end.
  • @gitlost reported a bug in its own right in comment:14. @gitlost — can you create a separate ticket and outline the problem?

#23 @gitlost
3 years ago

@ericlewis wilco...

Note: See TracTickets for help on using tickets.