New subject: [tools 2/2] testbot/orm: Remove parameters from Create{Detail,Item}refPropertyDescriptor().

June 8, 2022

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(a)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);
 }
 
-- 
2.30.2

    

[tools 1/2] testbot/orm: Turn Itemrefs into virtual properties.

Francois Gouget

Francois Gouget

tags

participants (1)