Bug 377432: Fix "Building Schema object from database" to handle existing foreign keys#7
Conversation
|
Just FYI, I haven't tested this yet, beyond making sure the patch applies. Visual inspection looks good. Dylan mentioned he had a 4.2 schema he was having trouble upgrading to harmony and suspected this patch might help it, so that might be a good test case. |
| if (!defined $info_cache->{$col_data->{'TABLE_NAME'}}) { | ||
| $info_cache->{$col_data->{'TABLE_NAME'}} = { }; | ||
| } | ||
| $info_cache->{$col_data->{'TABLE_NAME'}}->{$col_data->{'COLUMN_NAME'}} = $col_data; |
There was a problem hiding this comment.
Is there a more generalised, or more DBI-idiomatic, way of implementing this caching? You know what they say about the 2 or 3 hard problems in computer science.
Otherwise, this change looks good to me and I approve.
CyberShadow
left a comment
CyberShadow
left a comment
There was a problem hiding this comment.
- Please rebase
- Please split the change into atomic commits
- Would it be feasible to add a test?
| /blib | ||
| /Bugzilla-*.*/ | ||
| /local | ||
| .DS_Store |
There was a problem hiding this comment.
Unless Bugzilla or some test creates .DS_Store files, I don't think this should be in here at all. IMO operating system specific detritus belongs in users' global git ignores, so we don't have to burden every open-source project's ignore files with rules for every operating system out there.
|
|
||
| if (!$self->_bz_schema->columns_equal($current_def, $new_def)) { | ||
| if (!$current_def) { | ||
| $self->bz_add_column($table, $name, $new_def, $set_nulls_to); | ||
| } elsif (!$self->_bz_schema->columns_equal($current_def, $new_def)) { |
| my $col_data; | ||
| while ($col_data = $info_sth->fetchrow_hashref) { | ||
| last if $col_data->{'COLUMN_NAME'} eq $column; | ||
| my $info_cache = Bugzilla->request_cache->{'column_info_cache'} || |
There was a problem hiding this comment.
If the addition of caching is an orthogonal optimization, it should be in its own commit.
Otherwise, the commit message should explain the addition.
| # Don't want to modify the source def before we explicitly set it below. | ||
| # This is just us being extra-cautious. | ||
| my $column_def = dclone($self->get_column_abstract($table, $column)); | ||
| my $column_def = $self->get_column_abstract($table, $column); | ||
| die "Tried to set an fk on $table.$column, but that column doesn't exist" | ||
| if !$column_def; | ||
| # Don't want to modify the source def before we explicitly set it below. | ||
| # This is just us being extra-cautious. | ||
| $column_def = dclone($column_def); |
There was a problem hiding this comment.
Does change this have a function other than optimizing away a possibly-unnecessary dclone? (If not, it would be nice if it was in a separate commit.)
|
Just FYI, the original patch here was posted to the Bugzilla bug 7 years ago and the person who posted it is no longer involved with Bugzilla, so if anyone wants to take on updating this you're more than welcome. |
justdave
force-pushed
the
bug-377432-build-schema-foreign-keys
branch
from
f252543 to
8c1e1d4
Compare
|
The test fails are because I flubbed one of the merges, should have that fixed shortly. |
justdave
force-pushed
the
bug-377432-build-schema-foreign-keys
branch
from
8c1e1d4 to
6e494e7
Compare
…ting foreign keys
justdave
force-pushed
the
bug-377432-build-schema-foreign-keys
branch
from
6e494e7 to
441c482
Compare
|
OK, got it to build :-) I guess the test for this is to blow away the bz_schema table and see if it rebuilds correctly? |
5 participants
https://bugzilla.mozilla.org/show_bug.cgi?id=377432