Rename the "tags" table to "tag"

RESOLVED FIXED in Bugzilla 4.2

Status

()

Bugzilla
Database
--
enhancement
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: Max Kanat-Alexander, Assigned: Max Kanat-Alexander)

Tracking

Bugzilla 4.2
Dependency tree / graph
Bug Flags:
approval +
approval4.2 +
blocking4.2 +

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Assignee)

Description

7 years ago
For consistency with how our other multi-select-style fields work, we should rename the "tags" table to "tag".

Comment 1

7 years ago
Does this need to be done before 4.2rc1? If yes, it should block (it's a DB schema change), else it must be retargetted to 5.0.
(Assignee)

Comment 2

7 years ago
Yeah, I would rather not release this with the current table name.
Flags: blocking4.2+
Assignee: database → sdaugherty
Status: NEW → ASSIGNED
Status update: This is almost done, having issues with the table rename in checksetup.pl
(Assignee)

Comment 4

7 years ago
Just this should work:

$dbh->bz_rename_table('tags', 'tag');
Created attachment 539141 [details] [diff] [review]
almost working

Work in progress so far, almost works but the bz_rename_table is called too late by checksetup if called in the recommended place.
(Assignee)

Comment 6

7 years ago
Comment on attachment 539141 [details] [diff] [review]
almost working

You'll also need to rename the index.

Ah, put bz_rename_table before _migrate_user_tags.
Created attachment 539443 [details] [diff] [review]
patch for bug 637648

Ok. Working patch now! :)
Attachment #539141 - Attachment is obsolete: true
Attachment #539443 - Flags: review?(mkanat)

Comment 8

7 years ago
Comment on attachment 539443 [details] [diff] [review]
patch for bug 637648

Please leave the _populate_bug_see_also_class() subroutine where it is currently. There is no need to move it, and you are going to make code history more confusing (because the routine will look like to be created by you, in Bonsai). Please revert that and resubmit another patch.
It's showing up as moved because I moved _migrate_user_tags(); down below _migrate_from_tags_to_tag(); to prevent the possibility of problems caused by both DB migrations hitting at the same time.

Comment 10

7 years ago
I'm not talking about the calls to the subroutines, but the subroutines themselves.
(Assignee)

Comment 11

7 years ago
Comment on attachment 539443 [details] [diff] [review]
patch for bug 637648

This looks a bit more complicated than I expected, in Install::DB. Why couldn't you just move the bz_rename_table and keep everything else in place?
Created attachment 539609 [details] [diff] [review]
cleaned up patch for bug 637648

same patch, but the functions themselves aren't moved around this time, only the calls to them.
Attachment #539443 - Attachment is obsolete: true
Attachment #539443 - Flags: review?(mkanat)
Attachment #539609 - Flags: review?(mkanat)
(Assignee)

Comment 13

7 years ago
Comment on attachment 539609 [details] [diff] [review]
cleaned up patch for bug 637648

Review of attachment 539609 [details] [diff] [review]:
-----------------------------------------------------------------

::: Bugzilla/Install/DB.pm
@@ +647,5 @@
>          {TYPE => 'MEDIUMSERIAL', NOTNULL => 1, PRIMARYKEY => 1});
>  
>      # 2011-01-29 LpSolit@gmail.com - Bug 616185
> +    
> +    _populate_bug_see_also_class();

Why did this move? I'd suggest just inserting your _migrate_from_tags_to_tag above the existing _migrate_user_tags.

@@ +3583,4 @@
>      $dbh->bz_commit_transaction();
>  }
>  
> +sub _migrate_from_tags_to_tag {

I'd call it "_rename_tags_to_tag".

@@ +3593,5 @@
> +    # copy the tags table into tag before dropping tags 
> +    if ($dbh->bz_table_info('tags') and $dbh->bz_table_info('tag')) {
> +        print "Migrating data from 'tags' to 'tag'...\n";
> +        $dbh->do("INSERT INTO tag SELECT * from tags");
> +        $dbh->bz_drop_table("tags");

Ah, this isn't necessary, just check if there is a tags table, and if so, drop the "tag" table and rename it to "tags". You'll also have to rename the index.

Also, just as a note: the SELECT * isn't safe because the schema of "tag" could change in the future, but the schema of "tags" will never change.

@@ +3598,5 @@
> +    }
> +    elsif ($dbh->bz_table_info('tags')) {
> +        # This probably won't be able to be hit, but in case checksetup.pl
> +        # gets changed to do table updates before creating missing tables,
> +        # we'll still do the right thing.'

Oh, you don't need to worry about how checksetup might be changed, so we don't need this block. (In general, we don't include code that isn't used.)
Attachment #539609 - Flags: review?(mkanat) → review-
Created attachment 539615 [details] [diff] [review]
patch again

attempt to address review concerns, this one uses bz_drop_table before bz_rename_table
Attachment #539609 - Attachment is obsolete: true
Attachment #539615 - Flags: review?(mkanat)
(Assignee)

Comment 15

7 years ago
Comment on attachment 539615 [details] [diff] [review]
patch again

Review of attachment 539615 [details] [diff] [review]:
-----------------------------------------------------------------

::: Bugzilla/Install/DB.pm
@@ +647,5 @@
>          {TYPE => 'MEDIUMSERIAL', NOTNULL => 1, PRIMARYKEY => 1});
>  
>      # 2011-01-29 LpSolit@gmail.com - Bug 616185
> +    
> +    _populate_bug_see_also_class();

Why does the diff think you are moving this? Really, don't move this.

@@ +3590,5 @@
> +    # unfortunately this isn't a perfect world, and checksetup creates missing
> +    # tables from the schema before update_table_definitions() ever gets called.
> +    # So, we check to see if both the old and the new of these exist, and then
> +    # copy the tags table into tag before dropping tags 
> +    if ($dbh->bz_table_info('tags') and $dbh->bz_table_info('tag')) {

You don't need to check if the "tag" table exists, we know it does.

@@ +3596,5 @@
> +        # If we get here, it's because the schema created tag as an empty
> +        # table while tags still exists. We'll just get rid of the empty
> +        # tag table so we can do the rename over the top of it.
> +        $dbh->bz_drop_table("tag");
> +        $dbh->bz_rename_table('tags','tag');

You still have to rename the index. That means bz_drop_index and then bz_add_index.

@@ +3598,5 @@
> +        # tag table so we can do the rename over the top of it.
> +        $dbh->bz_drop_table("tag");
> +        $dbh->bz_rename_table('tags','tag');
> +    }
> +    elsif ($dbh->bz_table_info('tags')) {

Don't add code that isn't used (remove this block).
Attachment #539615 - Flags: review?(mkanat) → review-

Comment 16

7 years ago
Stephanie, ping?
(Assignee)

Comment 17

6 years ago
Created attachment 553638 [details] [diff] [review]
v3

Okay, since this is a blocker, I'm going to take it and fix it myself. Also, there was a problem with Sqlite that probably only I would have been able to fix right now, so I had to go and fix that anyhow, as part of this patch. (Otherwise the index disappears from the table entirely when it's being renamed, it's very weird.)
Assignee: sdaugherty → mkanat
Attachment #539615 - Attachment is obsolete: true
Attachment #553638 - Flags: review?(LpSolit)

Comment 18

6 years ago
Comment on attachment 553638 [details] [diff] [review]
v3

>=== modified file 'Bugzilla/Install/DB.pm'

>+sub _rename_tags_to_tag {
>+    my $dbh = Bugzilla->dbh;
>+    # 2011-06-13 sdaugherty@gmail.com - Bug 637648 - Rename the "tags" table
>+    # to "tag".

This comment should go right above the caller, not here, for consistency. Also, we usually never put the bug description in the comment, only its ID.


buglist.cgi crashes, because you forgot to fix Bugzilla/Search.pm around line 2529:
  $args->{full_field} = 'tags.name';

Also, global/field-descs.none.tmpl must also be fixed:
  "tags.name"               => "Tags",


Otherwise looks good.
Attachment #553638 - Flags: review?(LpSolit) → review-
(Assignee)

Comment 19

6 years ago
Created attachment 555869 [details] [diff] [review]
v4

Cool, thanks for the review. Fixed all the things you pointed out.
Attachment #553638 - Attachment is obsolete: true
Attachment #555869 - Flags: review?(LpSolit)

Comment 20

6 years ago
Comment on attachment 555869 [details] [diff] [review]
v4

Looks good now. r=LpSolit
Attachment #555869 - Flags: review?(LpSolit) → review+

Updated

6 years ago
Flags: approval4.2+
Flags: approval+
(Assignee)

Comment 21

6 years ago
Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/trunk/
modified Bugzilla/Bug.pm
modified Bugzilla/DB.pm
modified Bugzilla/Search.pm
modified Bugzilla/User.pm
modified Bugzilla/DB/Schema.pm
modified Bugzilla/DB/Sqlite.pm
modified Bugzilla/Install/DB.pm
modified template/en/default/global/field-descs.none.tmpl
Committed revision 7940.

Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/4.2/
modified Bugzilla/Bug.pm
modified Bugzilla/DB.pm
modified Bugzilla/Search.pm                                                    
modified Bugzilla/User.pm
modified Bugzilla/DB/Schema.pm
modified Bugzilla/DB/Sqlite.pm                                                 
modified Bugzilla/Install/DB.pm
modified template/en/default/global/field-descs.none.tmpl
Committed revision 7913.

Updated

6 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED

Comment 22

6 years ago
Tinderbox now fails with:

-  CONSTRAINT `fk_bug_tag_tag_id_tags_id` FOREIGN KEY (`tag_id`) REFERENCES `tag` (`id`) ON DELETE CASCADE ON UPDATE CASCADE
+  CONSTRAINT `fk_bug_tag_tag_id_tag_id` FOREIGN KEY (`tag_id`) REFERENCES `tag` (`id`) ON DELETE CASCADE ON UPDATE CASCADE

-  KEY `fk_bug_tag_tag_id_tags_id` (`tag_id`)
+  KEY `fk_bug_tag_tag_id_tag_id` (`tag_id`)
(Assignee)

Comment 23

6 years ago
(In reply to Frédéric Buclin from comment #22)
> Tinderbox now fails with:
> [snip]

  Okay. Sounds like FKs don't get properly renamed on some DB when tables get renamed, so we should fix that.

Updated

6 years ago
Blocks: 683644
You need to log in before you can comment on or make changes to this bug.