WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 20 months ago

#10905 closed defect (bug) (invalid)

Do not allow duplicate (post_id, meta_key, meta_value) rows in meta tables

Reported by: scribu Owned by: ryan
Milestone: Priority: normal
Severity: normal Version: 2.9
Component: Database Keywords: needs-patch
Focuses: Cc:

Description

Having these two rows in a meta table shouldn't be allowed:

meta_id		post_id		meta_key	meta_value
1000		1		foo		bar
1001		1		foo		bar


Attachments (1)

10905.diff (571 bytes) - added by scribu 4 years ago.
add duplicate check for add_metadata()

Download all attachments as: .zip

Change History (26)

comment:1 follow-up: scribu5 years ago

  • Keywords needs-patch added
  • Milestone changed from 2.9 to Future Release

When trying to add a UNIQUE index to post_id, meta_key and meta_value I get an error:

#1170 - BLOB/TEXT column 'meta_value' used in key specification without a key length

So this will have to be resolved through the *_metadata() API.

comment:2 follow-up: scribu5 years ago

  • Summary changed from Add UNIQUE(post_id, meta_key, meta_value) constraint to meta tables to Do not allow duplicate (post_id, meta_key, meta_value) rows in meta tables

comment:3 in reply to: ↑ 2 sivel5 years ago

Can you comment on what changed on your view in regards to this very discussion you had on the wp-hackers list.

http://lists.automattic.com/pipermail/wp-hackers/2009-February/025019.html

Seems that your last comment was:

"Ok, I suppose it wouldn't be a good idea. Nevermind."

comment:4 scribu4 years ago

The reason why I said that then, was this reply:

[wp-hackers] On 2/26/09, aesqe <aesqe at skyphe.org> wrote:

i think this is the right behavior - how else would a plugin like
flutter have grouped custom fields?

i don't know how many of you are familiar with flutter, but here's an
example:

let's say you have a group called reviews, and that group can be duplicated.

let's say that it has three fields:

  • reviewer_name
  • reviewer_magazine
  • review_text

and if two journalists from the same magazine write a review about that
particular post (which is actually a book, or a movie, etc)

then too, you'd have the same situation:

*meta_id* *post_id* *meta_key * *meta_value*

001 001 reviewer_magazine bar
002 001 reviewer_magazine bar

and flutter has its own table in which it stores relations between
custom fields and groups, so this works jus fine.

But after I thought about it more, it just hit me that that's not a good reason at all. There are better ways to achieve what Flutter is doing, without having the same data duplicated over and over again.

comment:5 scribu4 years ago

If we fix this, #11465 would also go away.

comment:6 in reply to: ↑ 1 ; follow-up: hakre4 years ago

Replying to scribu:

When trying to add a UNIQUE index to post_id, meta_key and meta_value I get an error:

#1170 - BLOB/TEXT column 'meta_value' used in key specification without a key length

Can you please provide the SQL you used to create the key? That would be interesting.

So this will have to be resolved through the *_metadata() API.

Let's see. It might work with SQL, maybe you have just forgotten to use some more parameters / options in the SQL Statement.

comment:7 hakre4 years ago

  • Keywords reporter-feedback added

comment:8 aesqe4 years ago

I've actually changed my opinion on this matter since the above-mentioned discussion, there definitely are better ways to do what Flutter is doing with grouped custom fields (saving values as array comes to mind first) :)

comment:9 in reply to: ↑ 6 scribu4 years ago

  • Keywords reporter-feedback removed

Replying to hakre:

Replying to scribu:

When trying to add a UNIQUE index to post_id, meta_key and meta_value I get an error:

#1170 - BLOB/TEXT column 'meta_value' used in key specification without a key length

Can you please provide the SQL you used to create the key? That would be interesting.

ALTER TABLE `wp_postmeta` ADD UNIQUE (
`post_id` ,
`meta_key` ,
`meta_value`
);

It was generated by phpMyAdmin, so I don't think the SQL is wrong.

comment:10 mrmist4 years ago

The SQL is wrong..

ALTER TABLE wp_postmeta ADD UNIQUE (

post_id ,
meta_key ,
meta_value (40)
);

Would work.

It's because meta_value is a blob. You have to specify the length of the part you want to index. The example above would index the first 40 chars, you could go longer but it probably isn't necessary.

comment:11 mrmist4 years ago

Actually reading up that length is bytes not chars so (40) is a lot.

comment:12 scribu4 years ago

Hm, didn't know you could do that.

Anyway, that doesn't help: checking only a fragment of the value doesn't guarantee it's the same value.

comment:13 mrmist4 years ago

Nope, but it's the only way to index such columns.

The alternative (to the index) I guess is to introduce a check before such a row is inserted or updated, to see if it would produce a duplicate.

scribu4 years ago

add duplicate check for add_metadata()

comment:14 scribu4 years ago

  • Keywords has-patch added; needs-patch removed

Should add a check on update_metadata() too. This is how I think it should work:

If by performing the update you end up with duplicates, delete the old value and return true.

comment:15 scribu4 years ago

  • Milestone changed from Future Release to 3.0

comment:16 scribu4 years ago

Related: #11683

comment:17 arena4 years ago

Not sure the duplicates (meta_key, meta_value) should not be allowed as it is supported today !

A. To complete the api, following functions should be added

1. update_metadata_by_id($meta_type, $meta_id, $meta_value, $meta_key = false)

2. delete_metadata_by_id($meta_type, $meta_id) ( see function delete_meta() )

3. get_metadata_by_id($meta_type, $meta_id)

B. For code consistency, make a definitive choice between $type_column or $column

C. add_metadata should return inserted id :

	$result = $wpdb->insert( $table, array(
		$type_column => $object_id,
		'meta_key' => $meta_key,
		'meta_value' => $meta_value
	) );

	if ($result) {
		$result = $wpdb->insert_id;
		wp_cache_delete($object_id, $meta_type . '_meta');
		do_action( "added_{$meta_type}_meta", $wpdb->insert_id, $object_id, $meta_key, $meta_value );
	}

	return $result;
}

comment:18 arena4 years ago

  • Cc arena added

comment:19 hakre4 years ago

Having more concrete working API functions would indeed help to figure things out properly. I like arena's suggestion.

comment:20 nacin4 years ago

  • Milestone changed from 3.0 to 3.1

Per the latest patch, I don't think we expose meta_id anywhere in the API, at least not prominently.

They should probably be unique across post_id, meta_key, meta_value, but we're too late in the dev cycle to be making this change at this point.

comment:21 Denis-de-Bernardy4 years ago

  • Keywords needs-patch added; has-patch removed

See also #15049, which got closed as dup, with further discussion points.

Btw, the unique index is not an interesting one. It's too large - better use in_array() for this kind of stuff.

comment:22 nacin3 years ago

  • Milestone changed from Awaiting Triage to Future Release

Agreed with Denis - avoid the index here if we want to do this.

comment:23 moskjis3 years ago

Thanks to scribu: pointed out this ticket.
I have a related ticket: #16267. If this one (#10905) is implemented, #16267 isn't needed.
Although, I would like to have the possibility of duplicates. And, possibility of multiple meta values with one key.
Besides, if multiple values with one key are allowed, duplicates shouldn't be denied.
Example:
What if you want to change the key_1 order in this example:

[key_1] => 'Happy'
[key_1] => 'Sad'
[key_2] => 'Gregory'
[my_key] => 'Steve'

Both values will be same for a bit (if you don't put some temp value there).

Last edited 20 months ago by SergeyBiryukov (previous) (diff)

comment:24 wonderboymusic20 months ago

is anyone still pining for this?

comment:25 scribu20 months ago

  • Milestone Future Release deleted
  • Resolution set to invalid
  • Status changed from new to closed

Well, the API already covers this:

  1. add_metadata() has a $unique parameter (don't know if it had it 3 years ago)
  2. update_metadata() will never produce duplicates

So, not much else to do here.

Last edited 20 months ago by scribu (previous) (diff)
Note: See TracTickets for help on using tickets.