Make WordPress Core

Opened 3 years ago

Closed 11 months ago

Last modified 11 months ago

#22023 closed enhancement (fixed)

Remove UNIQUE for slug in wp_terms

Reported by: nacin Owned by: boonebgorges
Milestone: 4.1 Priority: high
Severity: normal Version: 2.3
Component: Taxonomy Keywords: has-patch 4.1-early
Focuses: Cc:


To set us up for future changes to the taxonomy API, we should remove the UNIQUE key for 'slug' for wp_terms. Said future changes include:

  • Splitting shared terms on update (#5809)
  • Stop creating shared terms (#21950)
  • Forcibly split remaining shared terms
  • Merge wp_terms and wp_term_taxonomy (I can dream, right?)

The term_exists() check should prevent duplicate terms from ever being inserted (before, of course, we fix #5809 and #21950). But, this needs unit tests, particularly because there is a case where term_exists() breaks. See #17689, which blocks this ticket.

Attachments (4)

22023.patch (464 bytes) - added by hotchkissconsulting 2 years ago.
22023.term_exists-tests.diff (1.9 KB) - added by simonwheatley 16 months ago.
Beef up tests for term_exists
22023.term_exists-tests.02.patch (6.9 KB) - added by boonebgorges 12 months ago.
22023.db-error.png (51.0 KB) - added by ocean90 11 months ago.

Download all attachments as: .zip

Change History (60)

comment:1 @scribu3 years ago

  • Cc scribu added

comment:2 @hsatterwhite3 years ago

  • Cc whsatterwhite@… added

comment:3 @knutsp3 years ago

  • Cc knut@… added

comment:4 @toscho3 years ago

  • Cc info@… added

comment:5 @webord3 years ago

  • Cc webord.net@… added

comment:6 @sirzooro3 years ago

  • Cc sirzooro added

comment:7 @goblindegook3 years ago

  • Cc goblindegook added

comment:8 @webord3 years ago

For the version 3.5 we would need:

  • Remove the UNIQUE from slug on wp_terms
  • Improvments on term_exists()

Just checking so we can delivery something on 3.5

comment:9 @082net3 years ago

  • Cc 082net added

comment:10 @intoxstudio3 years ago

  • Cc jv@… added

comment:11 @wonderboymusic3 years ago

  • Keywords needs-patch reporter-feedback added

Is this part of 3.5 or 3.6?

comment:12 @scribu3 years ago

  • Keywords reporter-feedback removed
  • Milestone changed from 3.5 to Future Release
  • Type changed from defect (bug) to enhancement

No unit tests; #17689 not fixed yet. Punting.

comment:13 @nacin3 years ago

Yeah, I thought this was punted a while ago.

comment:14 @aaroncampbell3 years ago

  • Cc aaroncampbell added

comment:15 @aaroncampbell3 years ago

  • Milestone changed from Future Release to 3.6

comment:16 @adamsilverstein3 years ago


comment:17 @sc0ttkclark3 years ago

  • Cc lol@… added

comment:18 @tomdxw3 years ago

  • Cc tom@… added

comment:19 @tomdxw3 years ago

  • Cc tom@… removed

comment:20 @Viper007Bond3 years ago

  • Cc Viper007Bond added

comment:21 @greenshady3 years ago

  • Cc justin@… added

comment:22 @ryan2 years ago

  • Milestone changed from 3.6 to Future Release

comment:23 @nacin2 years ago

  • Milestone changed from Future Release to 3.7

Fixing #5809 requires this to be fixed to get the ball rolling for http://make.wordpress.org/core/2013/07/28/potential-roadmap-for-taxonomy-meta-and-post-relationships/. #17689 must be fixed first.

comment:24 @nacin2 years ago

  • Milestone changed from 3.7 to 3.8

We can do #5809 and this in the same release.

@hotchkissconsulting2 years ago

comment:25 @hotchkissconsulting23 months ago

  • Keywords has-patch added; needs-patch removed

There's a patch now. A teeny, tiny one.

comment:26 @wonderboymusic23 months ago

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

comment:27 @maorb22 months ago

  • Cc maor@… added

comment:28 @travisnorthcutt17 months ago

  • Keywords 4.0-early added; 3.9-early removed

Just a hair late for 3.9-early, now ;-)

comment:29 @ircbot16 months ago

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

comment:30 @helen16 months ago

  • Milestone changed from Future Release to 4.0

#17689 is in, let's do this pronto.

comment:31 follow-up: @ircbot16 months ago

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

comment:32 @simonwheatley16 months ago

I'm not clear on the testing requirement here; there are tests newly added by #17689 (test_duplicate_name) which indirectly exercise term_exists, but I'm happy to write more direct testing if that's required.

comment:33 in reply to: ↑ 31 @simonwheatley16 months ago

Replying to ircbot:

This ticket was mentioned in IRC in #wordpress-dev by simonwheatley. View the logs.
@nacin – "I don't think the current API in any way allows for multiple duplicate term slugs to sneak in the database, except possibly by race condition. which I am concerned about."

I've been thinking about Nacin's concerns above, and AFAIK it's not possible to have a UNIQUE constraint across the wp_term and wp_term_taxonomy tables. If this (race conditions allowing duplicate terms) is a big enough concern to block this ticket, and we need to block the possibility of duplicate terms with a constraint at the database level, then one alternative is to lose the wp_terms table (as mentioned in the potential taxonomy roadmap) so we can have a UNIQUE constraint in the restructured wp_term_taxonomy on taxonomy and slug (and name, perhaps). This is obviously (somewhat understating things) a big change… but we're considering some other structure changes, so may be a good time to get it in? I imagine we'd also want to have time and resource to test both core and existing plugins?

@simonwheatley16 months ago

Beef up tests for term_exists

comment:34 follow-up: @simonwheatley16 months ago

Additional tests for term_exists:

  • Don't use rand_str in test_term_exists_known or test_term_exists_unknown, instead specify a term name (allows for debugging tests more easily if the term name is in DB already)
  • Check optional $taxonomy parameter
  • Check optional `$parent parameter
  • Incidentally check inserting a term with a parent
  • Check specifying $term as integer other than 0 (in this case 99999)
  • No need to assert the term deletion (we're testing term deletion functionality elsewhere)

comment:35 @ircbot16 months ago

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

comment:36 @ircbot16 months ago

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

comment:37 in reply to: ↑ 34 @ericmann16 months ago

Replying to simonwheatley:

Those additional tests look good, and I'm definitely a fan of removing uses of rand_str (actually seen that cause collisions in rare cases).

Looking forward, let's put together a list of further areas that need testing. No one needs to actually write the tests yet, just pull together a list of what we need to test so we can divvy up work and get started. Thoughts?

comment:38 @johnbillion15 months ago

  • Keywords 4.1-early added; 4.0-early removed
  • Milestone changed from 4.0 to Future Release
  • Priority changed from normal to high

Let's do this in 4.1 early so we can get lots of nice tests in place and testing done.

comment:39 @aaroncampbell13 months ago

  • Milestone changed from Future Release to 4.1

comment:40 @boonebgorges12 months ago

term_exists-tests.02.patch is an exhaustive set of tests for term_exists(), as well as some tests that show the duplicate-term failure conditions for wp_insert_term().

comment:41 @nacin12 months ago

These tests look great.

comment:42 @boonebgorges12 months ago

In 29798:

Improve unit test coverage for duplicate term creation.

These include an exhaustive set of tests for term_exists(), as well as tests
for wp_insert_term() that demonstrate failure when attempting to create a
duplicate term.

Props simonwheatley for an initial patch.

See #22023.

comment:43 @boonebgorges12 months ago

The tests committed in r29798 are all passing. As long as all term creation passes through wp_insert_term() and uses term_exists(), I'm pretty confident that we're close to being ready to remove the UNIQUE key.

comment:44 @ircbot12 months ago

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

comment:45 @boonebgorges12 months ago

See #29589, an edge case where term_exists() fails.

comment:46 @boonebgorges12 months ago

In 29830:

Improve unit test coverage for wp_insert_term().

See #5809, #22023.

comment:47 @boonebgorges12 months ago

In 29875:

Improve unit test coverage for wp_update_term().

See #5809, #22023.

comment:48 @boonebgorges12 months ago

  • Owner set to boonebgorges
  • Status changed from new to accepted


comment:49 @boonebgorges12 months ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 30056:

Remove UNIQUE key from 'slug' column of terms table.

Each slug is a unique and beautiful snowflake, but let's enforce that
uniqueness elsewhere.

Props hotchkissconsulting.
Fixes #22023.

comment:50 @boonebgorges11 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

I neglected to write the proper upgrade routine for existing installs to remove the UNIQUE index. Reopening.

comment:51 @boonebgorges11 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 30121:

Bump db_version and add upgrade routine for schema change in [30056].

Fixes #22023.

@ocean9011 months ago

comment:52 @ocean9011 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Not sure why, but I got 22023.db-error.png.

comment:53 @slackbot11 months ago

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

comment:54 @boonebgorges11 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 30134:

Fix database schema update introduced in [30121].

The index must be manually dropped before dbDelta() can add the new index
without throwing a notice.

Fixes #22023.

comment:55 @boonebgorges11 months ago

In 30238:

In wp_insert_term(), clean up accidental duplicate terms after insert.

In [30056], the UNIQUE index was removed from the 'slug' column of wp_terms.
While we have numerous checks in place to avoid the creation of unwanted
duplicate term+term_taxonomy pairs, it's possible that in certain edge cases -
such as during the lag caused by database replication, or a race condition
involving near-simultaneous creation of more than one term - we'll end up
unwittingly inserting two identical rows.

The current changeset minimizes this risk by introducing a failsafe mechanism
into wp_insert_term(). After a term and term_taxonomy are INSERTed, we check
to see whether the term just created is a duplicate of an existing term; if so,
we delete the new one and keep the old one. This prevents problems caused by
replication lag, because SELECT queries that take place after an INSERT will
hit the master database; it mitigates race conditions by enforcing that the
term that was created first (ie, the one with the lowest term_id) is
always the "canonical" one.

Props nacin, markjaquith.
See #22023, #5809.

comment:56 @boonebgorges11 months ago

In 30239:

Enforce ORDER BY and LIMIT clauses in term_exists() queries.

In case of edge cases where a duplicate term might exist in a replicated
database for a split second, these explicit query clauses ensure that
term_exists() will always recognize the oldest matched term as the
canonical one. See [30238] for background.

Props pento.
See #22023, #5809.

Note: See TracTickets for help on using tickets.