When writing installation/upgrade code in an extension, if we include a REFERENCES definition in a $dbh->bz_add_column() call, checksetup.pl will crash later when it tries to install all of the foreign keys at one time (at least for MySQL). For example if I include the following code in my Extension.pm file: sub install_update_db { my $dbh = Bugzilla->dbh; $dbh->bz_add_column('bugs', 'some_new_user_role', { TYPE => 'INT3', REFERENCES => { TABLE => 'profiles', COLUMN => 'userid', DELETE => 'SET NULL' } }); } bz_add_column will create the column and also add the new foreign key constraint at the time it is called. The REFERENCES definition is added to the full bz_schema for the bugs table and new column. Later in checksetup.pl bz_setup_foreign_keys will look through bz_schema for the REFERENCES definitions, and if it does not see created => 1, it will attempt to create it again, crashing with the following error: Setting up foreign keys... DBD::mysql::db do failed: Can't write; duplicate key in table '#sql-242_b5' [for Statement "ALTER TABLE bugs ADD CONSTRAINT fk_bugs_qa_contact_profiles_userid FOREIGN KEY (qa_contact) REFERENCES profiles(userid) ON UPDATE CASCADE ON DELETE RESTRICT, ADD CONSTRAINT fk_bugs_reporter_profiles_userid FOREIGN KEY (reporter) REFERENCES profiles(userid) ON UPDATE CASCADE ON DELETE RESTRICT, ADD CONSTRAINT fk_bugs_product_id_products_id FOREIGN KEY (product_id) REFERENCES products(id) ON UPDATE CASCADE ON DELETE RESTRICT, ADD CONSTRAINT fk_bugs_assigned_to_profiles_userid FOREIGN KEY (assigned_to) REFERENCES profiles(userid) ON UPDATE CASCADE ON DELETE RESTRICT, ADD CONSTRAINT fk_bugs_some_new_user_role_profiles_userid FOREIGN KEY (some_new_user_role) REFERENCES profiles(userid) ON UPDATE CASCADE ON DELETE SET NULL, ADD CONSTRAINT fk_bugs_component_id_components_id FOREIGN KEY (component_id) REFERENCES components(id) ON UPDATE CASCADE ON DELETE RESTRICT"] at Bugzilla/DB.pm line 639. Bugzilla::DB::bz_add_fks('Bugzilla::DB::Mysql=HASH(0x7ec9e20)', 'bugs', 'HASH(0x7391e00)', 'HASH(0xb6606e0)') called at Bugzilla/DB.pm line 546 Bugzilla::DB::bz_setup_foreign_keys('Bugzilla::DB::Mysql=HASH(0x7ec9e20)') called at Bugzilla/Install/DB.pm line 746 Bugzilla::Install::DB::update_table_definitions('HASH(0x2768ea8)') called at ./checksetup.pl line 171 Patch coming. dkl

This will fix the reference being created a second time if someone passes REFERENCES to bz_add_column. dkl

Sorry, but bz_add_column() is not the right place to pass REFERENCES. You must define it in the DB schema, and Bugzilla will automatically add missing FKs when it's the right time to do it.

(In reply to Frédéric Buclin from comment #4) > Sorry, but bz_add_column() is not the right place to pass REFERENCES. You > must define it in the DB schema, and Bugzilla will automatically add missing > FKs when it's the right time to do it. please see bug 769829. when checksetup.pl runs and looks at extension code (e.g. component watching) it will add the columns with bz_add_column() and at that time add foreign keys, then later checksetup.pl runs bz_setup_foreign_keys() which ADDS THEM AGAIN.

(In reply to Frédéric Buclin from comment #4) > Sorry, but bz_add_column() is not the right place to pass REFERENCES. You > must define it in the DB schema, and Bugzilla will automatically add missing > FKs when it's the right time to do it. This is mainly for extensions that are adding columns to existing Bugzilla tables so it is not easy to update the ABSTRACT_SCHEMA to add the REFERENCES directly from the install_update_db hook. And according to the code in Bugzilla::DB::Schema, we should not be altering values of tables currently in $abstract_schema, but only to add new tables. http://git.mozilla.org/?p=bugzilla/bugzilla.git;a=blob;f=Bugzilla/DB/Schema.pm;h=ebe2cb4264ac8855d1ed770c732b3363b5162f21;hb=HEAD#l1876 [github] So for using install_update_db, it seems the only way to add a FK is to pass it to bz_add_column. Any other solution? dkl

I disagree to alter the way bz_add_column() works because you are exactly trying to workaround what we want to prevent in the code. Moreover, you seem to add columns to existing tables using bz_add_column() without updating the DB schema accordingly, putting the DB in a weird state. If we want extensions to be able to add columns to existing tables, then this should be done using the db_schema_abstract_schema hook.

(In reply to Frédéric Buclin from comment #7) > If we want extensions to be able to add columns to existing tables, then > this should be done using the db_schema_abstract_schema hook. I am not sure how I get that comment right. Does that mean that in bug 769829 comment #4, the patch in attachment 8504158 [details] [diff] [review] is doing the right thing by specifying: >--- a/extensions/ComponentWatching/Extension.pm >+++ b/extensions/ComponentWatching/Extension.pm >@@ -76,9 +76,10 @@ sub install_update_db { > { > TYPE => 'INT3', > REFERENCES => { >- TABLE => 'profiles', >- COLUMN => 'userid', >- DELETE => 'SET NULL', >+ TABLE => 'profiles', >+ COLUMN => 'userid', >+ DELETE => 'SET NULL', >+ created => 1, > } > } > );

(In reply to Frédéric Buclin from comment #7) > I disagree to alter the way bz_add_column() works because you are exactly > trying to workaround what we want to prevent in the code. Moreover, you seem > to add columns to existing tables using bz_add_column() without updating the > DB schema accordingly, putting the DB in a weird state. > > If we want extensions to be able to add columns to existing tables, then > this should be done using the db_schema_abstract_schema hook. quote: http://www.bugzilla.org/docs/tip/en/html/api/Bugzilla/Hook.html#db_schema_abstract_schema > If you wish to add new columns to existing Bugzilla tables, do that in "install_update_db". quote: http://www.bugzilla.org/docs/tip/en/html/api/Bugzilla/Hook.html#install_update_db > This happens at the very end of all the tables being updated during an installation or upgrade. If you need to modify your custom schema or add new columns to existing tables, do it here. No params are passed. e.g. AFAICT, ComponentWatching does what the docs says, and it still fails (bug 769829) http://bzr.mozilla.org/bmo/4.2/view/8230/extensions/ComponentWatching/Extension.pm The documentation is unclear, what is the proper way to add a column without adding the same foreign key twice (which is what is currently happening for extensions using bz_add_column())?

(In reply to Frédéric Buclin from comment #7) > I disagree to alter the way bz_add_column() works because you are exactly > trying to workaround what we want to prevent in the code. Moreover, you seem > to add columns to existing tables using bz_add_column() without updating the > DB schema accordingly, putting the DB in a weird state. > > If we want extensions to be able to add columns to existing tables, then > this should be done using the db_schema_abstract_schema hook. I do not see the issue with the change I am proposing as it just sets the created => 1 if the db has issued the ALTER TABLE command already to add the foreign key constraint. Later when the code that goes back and adds all of the other foreign keys it just skips that one properly. Hacking db_schema_abstract_schema to add a REFERENCES to an *existing* table goes against what the code docs say to do: http://git.mozilla.org/?p=bugzilla/bugzilla.git;a=blob;f=Bugzilla/DB/Schema.pm;h=ebe2cb4264ac8855d1ed770c732b3363b5162f21;hb=refs/heads/master#l1876 [github] An example of code I would need to write to do what you are proposing would look like this: sub db_schema_abstract_schema { my ($self, $args) = @_; push(@{$args->{'schema'}->{'components'}->{'FIELDS'} }, 'watch_user', { TYPE => 'INT3', REFERENCES => { TABLE => 'profiles', COLUMN => 'userid', DELETE => 'SET NULL', } } ); } sub install_update_db { my $dbh = Bugzilla->dbh; $dbh->bz_add_column('components', 'watch_user'); } Note that you have to add the additional code in install_update_db or otherwise the new field in db_schema_abstract_schema would not be detected automatically as components table already exists. Plus the extra code is redundant if we can just add REFERENCES to the bz_add_column method. dkl

Comment on attachment 8504230 [details] [diff] [review] 1082106_1.patch i don't see any issues with fixing this issue in the way proposed by this patch. if honours the guidelines from our documentation, and avoids ugly code such as comment 10. (In reply to Frédéric Buclin from comment #7) > you seem to add columns to existing tables using bz_add_column() without > updating the DB schema accordingly, putting the DB in a weird state. this isn't correct -- bz_add_column() updates the the db schema. taking this review.

(In reply to Byron Jones ‹:glob› from comment #11) > this isn't correct -- bz_add_column() updates the the db schema. That's not what I said. When you add a new column, you must also update the DB schema yourself.

(In reply to Frédéric Buclin from comment #12) > (In reply to Byron Jones ‹:glob› from comment #11) > > this isn't correct -- bz_add_column() updates the the db schema. > > That's not what I said. When you add a new column, you must also update the > DB schema yourself. I still think you are speaking in terms of columns added to the stock Bugzilla tables that are not part of an extension such as the ones added in Bugzilla/Install/DB.pm. In which case, yes, you would also make the same changes in Bugzilla/DB/Schema.pm for the new column being added. But I am talking about extensions that add columns to existing Bugzilla tables solely to support the extension functions. From what I can tell, adding to the bz_schema data happens automatically at runtime at the end of bz_add_column: sub bz_add_column { [snip] $self->_bz_real_schema->set_column($table, $name, $new_def); $self->_bz_store_real_schema; } } dkl

This causes the new REFERENCES data for the new column to be added later with all other foreign keys. dkl

Once you guys reach a consensus, can you please add: - an example of how an extension would add foreign key constraints to an existing table to the documentation, so that no guess work is required? Apparently looking at existing extensions out there could mislead one into thinking they are doing the correct thing. thank you.

The patch from bug 769829 makes this work, but it doesn't unbork an installation which is already borked. :-| Gerv

(In reply to Gervase Markham [:gerv] from comment #16) > The patch from bug 769829 makes this work, but it doesn't unbork an > installation which is already borked. :-| > > Gerv As this should not affect fields added by core Bugzilla code and only fields added by extension code, I would imagine the developer had already had to work around the issue manually and their DB is no longer borked. I.e. running checksetup.pl on a populated BMO database does not generate any error as the foreign keys are set up correctly. So I am not sure we need to worry with any databases other than new ones. dkl

Comment on attachment 8506249 [details] [diff] [review] 1082106_2.patch Review of attachment 8506249 [details] [diff] [review]: ----------------------------------------------------------------- r=glob

What do you want to document, and where?

Added some text to the bz_add_column perldoc for Bugzilla.pm To ssh://gitolite3@git.mozilla.org/bugzilla/bugzilla.git 1d68680..fa12395 master -> master

(In reply to Frédéric Buclin from comment #19) > What do you want to document, and where? I was referring mostly to the docs that get generated and put online. The equivalent for 5.0 of http://www.bugzilla.org/docs/4.4/en/html/api/Bugzilla/Hook.html (In reply to Damien Nozay [:dnozay] from comment #15) > Once you guys reach a consensus, can you please add: > > - an example of how an extension would add foreign key constraints to an > existing table > > to the documentation, so that no guess work is required? Apparently looking > at > existing extensions out there could mislead one into thinking they are doing > the > correct thing. > > thank you.

glob/dkl/LpSolit: can we backport this to 4.4 or even earlier? It would be useful not to have to custom-patch Bugzillas which are hitting this problem. (E.g. I want to install ComponentWatching on a 4.4 installation.) Gerv

(In reply to Gervase Markham [:gerv] from comment #22) > glob/dkl/LpSolit: can we backport this to 4.4 or even earlier? It would be > useful not to have to custom-patch Bugzillas which are hitting this problem. This is not a security bug, and it's IMO too risky to backport. I don't see why such a fix should land on branches.

i disagree that there's risk involved - the fix is simple, self contained, and addresses a common issue with extensions. sites without extensions will not notice a difference. this is fine for 4.4, but not worth taking on earlier branches.

To ssh://gitolite3@git.mozilla.org/bugzilla/bugzilla.git 9155570..16fa3bf 4.4 -> 4.4

To ssh://gitolite3@git.mozilla.org/bugzilla/bugzilla.git 4407479..89b7de1 5.0 -> 5.0 This appears to be commit 16fa3bf67da86fb9e9826d1422a37b66c0f5ffab on 4.4, checked in by dkl. I mistakenly committed this to 4.2 but then backed it out. Sorry :-(( Perhaps my checkin script needs to make an API call to check for the approval flag, to prevent me making such mistakes in future. Gerv