Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug 377432: Fix "Building Schema object from database" to handle existing foreign keys #7

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Bug 377432: Fix "Building Schema object from database" to handle existing foreign keys #7

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f1d3db0
Bug 377432: Fix "Building Schema object from database" to handle exis…
andershol Jan 25, 2018
940efa0
Bug 1433029 - make git ignore .DS_Store files anywhere, not just in root
justdave Jan 28, 2018
f252543
Merge branch 'master' into bug-377432-build-schema-foreign-keys
dylanwh Jan 28, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,14 +18,14 @@
/localconfig.*
/index.html
/error_reports
/.DS_Store
/template_cache
/Makefile
/MYMETA.*
/pm_to_blib
/blib
/Bugzilla-*.*/
/local
.DS_Store
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

.vagrant/
version.json
__lbheartbeat__
Expand Down
5 changes: 3 additions & 2 deletions Bugzilla/DB.pm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -656,8 +656,9 @@ sub bz_alter_column {
my ($self, $table, $name, $new_def, $set_nulls_to) = @_;

my $current_def = $self->bz_column_info($table, $name);

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)) {
Comment on lines -659 to +661
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

# You can't change a column to be NOT NULL if you have no DEFAULT
# and no value for $set_nulls_to, if there are any NULL values
# in that column.
Expand Down
48 changes: 39 additions & 9 deletions Bugzilla/DB/Mysql.pm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -836,18 +836,48 @@ sub _bz_raw_column_info {
# DBD::mysql does not support selecting a specific column,
# so we have to get all the columns on the table and find
# the one we want.
my $info_sth = $self->column_info(undef, undef, $table, '%');

# Don't use fetchall_hashref as there's a Win32 DBI bug (292821)
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'} ||
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the addition of caching is an orthogonal optimization, it should be in its own commit.

Otherwise, the commit message should explain the addition.

(Bugzilla->request_cache->{'column_info_cache'} = {});

if (!defined $info_cache->{$table}) {
# Don't use fetchall_hashref as there's a Win32 DBI bug (292821)
my $info_sth = $self->column_info(undef, undef, $table, '%');
while (my $col_data = $info_sth->fetchrow_hashref) {
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;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
}

if (!defined $col_data) {
return undef;
if (!defined Bugzilla->request_cache->{'column_fk_cache'}) {
my $fk_cache = Bugzilla->request_cache->{'column_fk_cache'} = {};
my $fk_sth = $self->foreign_key_info(undef, undef, undef, undef, undef, undef);
while (my $fk = $fk_sth->fetchrow_hashref) {
if (defined $fk->{'PKCOLUMN_NAME'}) {
if (!defined $fk_cache->{$fk->{'FKTABLE_NAME'}}) {
$fk_cache->{$fk->{'FKTABLE_NAME'}} = { };
}
$fk_cache->{$fk->{'FKTABLE_NAME'}}->{$fk->{'FKCOLUMN_NAME'}} = $fk;
}
}
}
my $fk_cache = Bugzilla->request_cache->{'column_fk_cache'};

if (defined $info_cache->{$table}->{$column}) {
my $col_data = $info_cache->{$table}->{$column};
if (defined $fk_cache->{$table}->{$column}) {
my $fk = $fk_cache->{$table}->{$column};
$col_data->{'REFERENCES'} = {
TABLE => $fk->{'PKTABLE_NAME'},
COLUMN => $fk->{'PKCOLUMN_NAME'},
DELETE => '',
created => 1,
}
}
return $col_data;
}
return $col_data;
return undef;
}

=item C<bz_index_info_real($table, $index)>
Expand Down
7 changes: 4 additions & 3 deletions Bugzilla/DB/Schema.pm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2847,11 +2847,12 @@ Sets the C<REFERENCES> item on the specified column.

sub set_fk {
my ($self, $table, $column, $fk_def) = @_;
# 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);
Comment on lines -2850 to +2855
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

if ($fk_def) {
$column_def->{REFERENCES} = $fk_def;
}
Expand Down
3 changes: 3 additions & 0 deletions Bugzilla/DB/Schema/Mysql.pm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -273,6 +273,9 @@ sub column_info_to_column {
my $col_name = $column_info->{COLUMN_NAME};

my $column = {};
if (defined $column_info->{REFERENCES}) {
$column->{REFERENCES} = $column_info->{REFERENCES};
}

($column->{NOTNULL} = 1) if $column_info->{NULLABLE} == 0;

Expand Down