Itemref properties are used to map foreign key columns to the object corresponding to the target table row. However so far adding an Itemref property meant hiding the underlying table column(s) with annoying side-effects: * The type of the underlying property is unknown which is not an issue for strings and integers but rules out using booleans and timestamps in foreign keys since they need conversion from / to the database format. * Filtering on an Itemref property is possible by passing the target object, but not by using the underlying property(s). This makes inequality filters (e.g. >= JobId) awkward or impossible. * This also makes it impossible to filter on a partial foreign key. For instance if one had a TaskFailures table matching known failures to the tasks (JobId, TaskNo) they appear in, one would be unable to get a collection with all the entries for a given job since it's impossible to filter on just the JobId column.
So this modifies Itemref properties to come in addition to the underlying column instead of replacing them. This means declaring all the underlying columns, with their proper property descriptors, which makes it possible to access them the normal way and to use them in filters, while still being able to access the corresponding object using the extra Itemref 'virtual' property. It also means filtering out the new properties if they should not be shown in the GUI. Another consequence is that Itemref properties cannot be keys: only the underlying columns can. Finally the underlying columns should not be modified directly as that would cause them to be inconsistent with the corresponding Itemref property but this is not enforced.
Signed-off-by: Francois Gouget fgouget@codeweavers.com --- testbot/lib/ObjectModel/DBIBackEnd.pm | 26 ++++++++++++------- testbot/lib/ObjectModel/Item.pm | 25 +++++------------- .../ObjectModel/ItemrefPropertyDescriptor.pm | 17 +++++++----- testbot/lib/WineTestBot/CGI/Sessions.pm | 1 + testbot/lib/WineTestBot/Jobs.pm | 3 +++ testbot/lib/WineTestBot/PendingPatches.pm | 1 + testbot/lib/WineTestBot/StepsTasks.pm | 1 + testbot/lib/WineTestBot/Tasks.pm | 1 + testbot/lib/WineTestBot/UserRoles.pm | 3 ++- testbot/web/JobDetails.pl | 6 ++--- testbot/web/index.pl | 11 +++----- 11 files changed, 50 insertions(+), 45 deletions(-)
diff --git a/testbot/lib/ObjectModel/DBIBackEnd.pm b/testbot/lib/ObjectModel/DBIBackEnd.pm index f4d16836b..e837d47f5 100644 --- a/testbot/lib/ObjectModel/DBIBackEnd.pm +++ b/testbot/lib/ObjectModel/DBIBackEnd.pm @@ -219,15 +219,23 @@ sub ColNameToDb($) { my ($PropertyDescriptor) = @_;
- my $ColNames = $PropertyDescriptor->GetColNames(); - if (@$ColNames != 1) - { - # This would require building a more complex where clause, something like - # ((col1 = item1.val1 AND col2 = item1.val2...) OR - # (col1 = item2.val1 AND col2 = item2.val2...) OR ...) - # So it is not supported. Instead one should build the where clause by - # hand from the underlying columns. - die "cannot filter on the ". $PropertyDescriptor->GetName() ." Itemref because it has a multi-column key: @$ColNames"; + my $ColNames; + if ($PropertyDescriptor->GetClass() eq "Itemref") + { + $ColNames = $PropertyDescriptor->GetRefColNames(); + if (@$ColNames != 1) + { + # This would require building a more complex where clause, something like + # ((col1 = item1.val1 AND col2 = item1.val2...) OR + # (col1 = item2.val1 AND col2 = item2.val2...) OR ...) + # So it is not supported. Instead one should build the where clause by + # hand from the underlying columns. + die "cannot filter on the ". $PropertyDescriptor->GetName() ." Itemref because it has a multi-column key: @$ColNames"; + } + } + else + { + $ColNames = $PropertyDescriptor->GetColNames(); } return $ColNames->[0]; } diff --git a/testbot/lib/ObjectModel/Item.pm b/testbot/lib/ObjectModel/Item.pm index 593b039ff..b07b76066 100644 --- a/testbot/lib/ObjectModel/Item.pm +++ b/testbot/lib/ObjectModel/Item.pm @@ -292,7 +292,7 @@ sub AUTOLOAD { $self->{IsModified} = 1; $self->{Itemrefs}{$PropertyName} = undef; - foreach my $ColName (@{$PropertyDescriptor->GetColNames()}) + foreach my $ColName (@{$PropertyDescriptor->GetRefColNames()}) { $self->{ColValues}{$ColName} = undef; } @@ -305,7 +305,7 @@ sub AUTOLOAD # Note that the foreign key names ($RefColNames) typically don't # match the Item's primary key column names. But their order and # count must match the primary key values. - my $RefColNames = $PropertyDescriptor->GetColNames(); + my $RefColNames = $PropertyDescriptor->GetRefColNames(); my ($_ColNames, $ColValues) = $Item->GetMasterKey(); foreach my $ColIndex (0..$#{$RefColNames}) { @@ -315,8 +315,8 @@ sub AUTOLOAD } elsif (! defined($self->{Itemrefs}{$PropertyName})) { - my $ColNames = $PropertyDescriptor->GetColNames(); - my @KeyComponents = map { $self->{ColValues}{$_} || "" } @$ColNames; + my $RefColNames = $PropertyDescriptor->GetRefColNames(); + my @KeyComponents = map { $self->{ColValues}{$_} || "" } @$RefColNames; my $Collection = &{$PropertyDescriptor->GetCreator()}($self); my $Key = $Collection->CombineKey(@KeyComponents); $self->{Itemrefs}{$PropertyName} = $Collection->GetItem($Key); @@ -455,32 +455,21 @@ sub Compare($$) { next if (!$PropertyDescriptor->GetIsKey());
- my $ColNames = $PropertyDescriptor->GetColNames(); + my $ColName = $PropertyDescriptor->GetColNames()->[0]; if ($PropertyDescriptor->GetClass() eq "Basic") { - my $ColName = $ColNames->[0]; my $ColType = $PropertyDescriptor->GetType(); my $Cmp = ($ColType eq "N" or $ColType eq "S" or $ColType eq "DT") ? $self->{ColValues}{$ColName} <=> $B->{ColValues}{$ColName} : $self->{ColValues}{$ColName} cmp $B->{ColValues}{$ColName}; return $Cmp if ($Cmp); } - elsif ($PropertyDescriptor->GetClass() eq "Enum") + else { - my $ColName = $ColNames->[0]; + # Detailrefs and Itemrefs cannot be keys so this is an Enum. my $Cmp = $self->{ColValues}{$ColName} cmp $B->{ColValues}{$ColName}; return $Cmp if ($Cmp); } - else - { - # A Detailref cannot be a key so this is an Itemref. Note that its - # underlying column types are unknown so treat them as strings. - foreach my $ColName (@$ColNames) - { - my $Cmp = $self->{ColValues}{$ColName} cmp $B->{ColValues}{$ColName}; - return $Cmp if ($Cmp); - } - } } return 0; } diff --git a/testbot/lib/ObjectModel/ItemrefPropertyDescriptor.pm b/testbot/lib/ObjectModel/ItemrefPropertyDescriptor.pm index cba4f9af1..d79e1d25a 100644 --- a/testbot/lib/ObjectModel/ItemrefPropertyDescriptor.pm +++ b/testbot/lib/ObjectModel/ItemrefPropertyDescriptor.pm @@ -32,11 +32,11 @@ our @EXPORT = qw(CreateItemrefPropertyDescriptor);
sub _initialize($$$) { - my ($self, $Creator, $ColNames) = @_; + my ($self, $Creator, $RefColNames) = @_;
$self->{Class} = "Itemref"; $self->{Creator} = $Creator; - $self->{ColNames} = $ColNames; + $self->{RefColNames} = $RefColNames;
$self->SUPER::_initialize(); } @@ -50,9 +50,14 @@ sub GetCreator($)
sub GetColNames($) { - my ($self) = @_; + #my ($self) = @_; + return []; +}
- return $self->{ColNames}; +sub GetRefColNames($) +{ + my ($self) = @_; + return $self->{RefColNames}; }
sub ValidateValue($$$) @@ -69,8 +74,8 @@ sub ValidateValue($$$)
sub CreateItemrefPropertyDescriptor($$$$$$) { - my ($Name, $DisplayName, $IsKey, $IsRequired, $Creator, $ColNames) = @_; - return ObjectModel::ItemrefPropertyDescriptor->new($Name, $DisplayName, $IsKey, $IsRequired, $Creator, $ColNames); + my ($Name, $DisplayName, $IsKey, $IsRequired, $Creator, $RefColNames) = @_; + return ObjectModel::ItemrefPropertyDescriptor->new($Name, $DisplayName, $IsKey, $IsRequired, $Creator, $RefColNames); }
1; diff --git a/testbot/lib/WineTestBot/CGI/Sessions.pm b/testbot/lib/WineTestBot/CGI/Sessions.pm index 39634e3ef..d61c6e0ff 100644 --- a/testbot/lib/WineTestBot/CGI/Sessions.pm +++ b/testbot/lib/WineTestBot/CGI/Sessions.pm @@ -71,6 +71,7 @@ sub CreateItem($)
my @PropertyDescriptors = ( CreateBasicPropertyDescriptor("Id", "Session id", 1, 1, "A", 32), + CreateBasicPropertyDescriptor("UserName", "User", !1, 1, "A", 40), CreateItemrefPropertyDescriptor("User", "User", !1, 1, &CreateUsers, ["UserName"]), CreateBasicPropertyDescriptor("Permanent", "Permanent session", !1, 1, "B", 1), ); diff --git a/testbot/lib/WineTestBot/Jobs.pm b/testbot/lib/WineTestBot/Jobs.pm index 1eb7bb7eb..091420d45 100644 --- a/testbot/lib/WineTestBot/Jobs.pm +++ b/testbot/lib/WineTestBot/Jobs.pm @@ -508,13 +508,16 @@ sub CreateItem($)
my @PropertyDescriptors = ( CreateBasicPropertyDescriptor("Id", "Job", 1, 1, "S", 10), + CreateBasicPropertyDescriptor("BranchName", "Branch", !1, 1, "A", 20), CreateItemrefPropertyDescriptor("Branch", "Branch", !1, 1, &CreateBranches, ["BranchName"]), + CreateBasicPropertyDescriptor("UserName", "Author", !1, 1, "A", 40), CreateItemrefPropertyDescriptor("User", "Author", !1, 1, &CreateUsers, ["UserName"]), CreateBasicPropertyDescriptor("Priority", "Priority", !1, 1, "N", 1), CreateEnumPropertyDescriptor("Status", "Status", !1, 1, ['new', 'staging', 'queued', 'running', 'completed', 'badpatch', 'badbuild', 'boterror', 'canceled']), CreateBasicPropertyDescriptor("Remarks", "Remarks", !1, !1, "A", 128), CreateBasicPropertyDescriptor("Submitted", "Submitted", !1, !1, "DT", 19), CreateBasicPropertyDescriptor("Ended", "Ended", !1, !1, "DT", 19), + CreateBasicPropertyDescriptor("PatchId", "Patch id", !1, !1, "N", 10), # Somehow mod_perl sometimes fails to find CreatePatches() if not given the # fully qualified name, but never has any trouble with the other Create*() # functions. diff --git a/testbot/lib/WineTestBot/PendingPatches.pm b/testbot/lib/WineTestBot/PendingPatches.pm index 118c74900..c65aabdce 100644 --- a/testbot/lib/WineTestBot/PendingPatches.pm +++ b/testbot/lib/WineTestBot/PendingPatches.pm @@ -65,6 +65,7 @@ sub CreateItem($)
my @PropertyDescriptors = ( CreateBasicPropertyDescriptor("No", "Part no", 1, 1, "N", 2), + CreateBasicPropertyDescriptor("PatchId", "Patch id", !1, 1, "N", 10), CreateItemrefPropertyDescriptor("Patch", "Submitted via patch", !1, 1, &CreatePatches, ["PatchId"]), ); my @FlatPropertyDescriptors = ( diff --git a/testbot/lib/WineTestBot/StepsTasks.pm b/testbot/lib/WineTestBot/StepsTasks.pm index 5970c872e..794c73eb7 100644 --- a/testbot/lib/WineTestBot/StepsTasks.pm +++ b/testbot/lib/WineTestBot/StepsTasks.pm @@ -181,6 +181,7 @@ my @PropertyDescriptors = ( CreateBasicPropertyDescriptor("TaskNo", "Task", !1, 1, "N", 2), CreateBasicPropertyDescriptor("Type", "Step type", !1, 1, "A", 32), CreateBasicPropertyDescriptor("Status", "Status", !1, 1, "A", 32), + CreateBasicPropertyDescriptor("VMName", "VM", !1, 1, "A", 20), CreateItemrefPropertyDescriptor("VM", "VM", !1, 1, &CreateVMs, ["VMName"]), CreateBasicPropertyDescriptor("Timeout", "Timeout", !1, 1, "N", 4), CreateBasicPropertyDescriptor("FileName", "File", !1, !1, "A", 100), diff --git a/testbot/lib/WineTestBot/Tasks.pm b/testbot/lib/WineTestBot/Tasks.pm index a7d9e820e..8e9386299 100644 --- a/testbot/lib/WineTestBot/Tasks.pm +++ b/testbot/lib/WineTestBot/Tasks.pm @@ -354,6 +354,7 @@ sub CreateItem($) my @PropertyDescriptors = ( CreateBasicPropertyDescriptor("No", "Task", 1, 1, "N", 2), CreateEnumPropertyDescriptor("Status", "Status", !1, 1, ['queued', 'running', 'completed', 'badpatch', 'badbuild', 'boterror', 'canceled', 'skipped']), + CreateBasicPropertyDescriptor("VMName", "VM", !1, 1, "A", 20), CreateItemrefPropertyDescriptor("VM", "VM", !1, 1, &CreateVMs, ["VMName"]), CreateBasicPropertyDescriptor("Timeout", "Timeout", !1, 1, "N", 4), CreateBasicPropertyDescriptor("Missions", "Missions", !1, 1, "A", 256), diff --git a/testbot/lib/WineTestBot/UserRoles.pm b/testbot/lib/WineTestBot/UserRoles.pm index e01cd9d28..fcdbc8e04 100644 --- a/testbot/lib/WineTestBot/UserRoles.pm +++ b/testbot/lib/WineTestBot/UserRoles.pm @@ -58,7 +58,8 @@ sub CreateItem($) }
my @PropertyDescriptors = ( - CreateItemrefPropertyDescriptor("Role", "Role", 1, 1, &CreateRoles, ["RoleName"]), + CreateBasicPropertyDescriptor("RoleName", "Role", 1, 1, "A", 20), + CreateItemrefPropertyDescriptor("Role", "Role", !1, 1, &CreateRoles, ["RoleName"]), ); my @FlatPropertyDescriptors = ( CreateBasicPropertyDescriptor("UserName", "Username", 1, 1, "A", 40), diff --git a/testbot/web/JobDetails.pl b/testbot/web/JobDetails.pl index 329ab150c..36bba0d04 100644 --- a/testbot/web/JobDetails.pl +++ b/testbot/web/JobDetails.pl @@ -50,7 +50,7 @@ sub DisplayProperty($$) my ($self, $PropertyDescriptor) = @_;
my $PropertyName = $PropertyDescriptor->GetName(); - return $PropertyName =~ /^(?:Id|PreviousNo|Type|FileType|Missions)$/ ? "" : + return $PropertyName =~ /^(?:Id|PreviousNo|Type|VM|FileType|Missions)$/ ? "" : $PropertyName eq "Started" ? ("ro", "timetipdate") : $self->SUPER::DisplayProperty($PropertyDescriptor); } @@ -89,10 +89,10 @@ sub GenerateDataView($$$)
my $StepTask = $Row->{Item}; my $PropertyName = $Col->{Descriptor}->GetName(); - if ($PropertyName eq "VM") + if ($PropertyName eq "VMName") { print "<a href='#k", $StepTask->GetKey(), "'>", - $self->escapeHTML($StepTask->VM->Name), "</a>"; + $self->escapeHTML($StepTask->VMName), "</a>"; return; } if ($PropertyName eq "FileName") diff --git a/testbot/web/index.pl b/testbot/web/index.pl index 1083d3b57..8b4af53c0 100644 --- a/testbot/web/index.pl +++ b/testbot/web/index.pl @@ -39,14 +39,9 @@ sub DisplayProperty($$) my ($self, $PropertyDescriptor) = @_;
my $PropertyName = $PropertyDescriptor->GetName(); - if ($PropertyName eq "Patch" || - ($PropertyName eq "Branch" && - ! CreateBranches()->MultipleBranchesPresent)) - { - return !1; - } - - return $PropertyName eq "Submitted" ? ("ro", "timetipdate") : + return $PropertyName =~ /^(?:Branch|PatchId|Patch)$/ ? "" : + $PropertyName eq "Submitted" ? ("ro", "timetipdate") : + ($PropertyName eq "Branch" and !CreateBranches()->MultipleBranchesPresent) ? "" : $self->SUPER::DisplayProperty($PropertyDescriptor); }