Merge pull request #87 from xsist10/misc_cleanup

Misc cleanup

src/Engine/Check/Column/LowCardinalityExpensiveStorage.php

@@ -33,7 +33,7 @@ public function run($entity): ?Report
         }
 
         // We can ignore tables under a thousand records. Optimization should focus on larger tables.
-        $num_rows = $entity->getTable()->information_schema->table_rows;
+        $num_rows = $entity->getTable()->getNumRows();
         if ($num_rows < 1_000) {
             return new Report(
                 $this,

src/Engine/Check/Index/LowCardinality.php

@@ -19,7 +19,7 @@ public function run($entity): ?Report
     {
         // If the index is unique, then the cardinality is as high as it can be
         // Or if the table is empty, then it won't have any cardinality
-        if ($entity->isUnique() || !$entity->getTable()->information_schema->table_rows) {
+        if ($entity->isUnique() || !$entity->getTable()->getNumRows()) {
             return new Report(
                 $this,
                 $entity,

src/Engine/Check/Table/EmptyTable.php

@@ -18,7 +18,7 @@ public function supports($entity): bool
 
     public function run($entity): ?Report
     {
-        if (!empty($entity->information_schema->table_rows)) {
+        if ($entity->getNumRows()) {
             return new Report(
                 $this,
                 $entity,
@@ -55,7 +55,7 @@ public function run($entity): ?Report
             ];
 
             // Is this table in a weird tablespace?
-            if (is_null($entity->getTablespace())) {
+            if ($entity->getTablespaceType() === "System") {
                 // If so, we can't rely on data free to give us an indication of
                 // the usage of this table.
                 $messages[] = "This table is in a shared tablespace so this doesn't mean much.";

src/Engine/Entity/Column.php

@@ -12,7 +12,7 @@
 class Column implements Entity
 {
     protected string $name;
-    protected Table $table;
+    protected ?Table $table = null;
     public InformationSchema $information_schema;
     public int $cardinality = 0;
 
@@ -43,7 +43,7 @@ public function setTable(Table $table): void
         $this->table = $table;
     }
 
-    public function getTable(): Table
+    public function getTable(): ?Table
     {
         return $this->table;
     }
@@ -265,18 +265,14 @@ public function getCardinality(): int
 
     public function getCardinalityRatio(): float
     {
-        if (isset($this)) {
-            $information_schema = $this->getTable()->information_schema;
-            if (!$information_schema) {
-                return 0;
-            }
-
-            // Identify the cardinality as a ratio of the size of the table
-            // Cardinality in older version of MySQL aren't distinct per column
-            $table_size = $information_schema->table_rows;
-            return $this->getCardinality() ? ($table_size / $this->getCardinality()) : 0;
+        if (!$this->getTable()) {
+            return 0;
         }
 
-        return 0;
+        // Identify the cardinality as a ratio of the size of the table
+        // Cardinality in older version of MySQL aren't distinct per column
+        return $this->getCardinality()
+            ? $this->getTable()->getNumRows() / $this->getCardinality()
+            : 0;
     }
 }

src/Engine/Entity/Table.php

@@ -219,6 +219,13 @@ public function setInnoDbTable(InnoDbTable $innodb_table): void
         $this->innodb_table = $innodb_table;
     }
 
+    public function getTableSpaceType(): ?string
+    {
+        return $this->innodb_table
+            ? $this->innodb_table->space_type
+            : null;
+    }
+
     public function getTableSpace(): ?Tablespace
     {
         if (!$this->innodb_table) {
@@ -230,6 +237,13 @@ public function getTableSpace(): ?Tablespace
         return $database->getTablespace($this->innodb_table->space);
     }
 
+    public function getNumRows(): int
+    {
+        return $this->information_schema
+            ? $this->information_schema->table_rows
+            : 0;
+    }
+
     public function isVirtual(): bool
     {
         // If we don't have an information_schema, we'll have to guess

src/Engine/Entity/Table/InnoDbTable.php

@@ -4,6 +4,13 @@
 
 namespace Cadfael\Engine\Entity\Table;
 
+/**
+ * Class InnoDbTable (readonly)
+ *
+ * Stores a representation of information_schema.innodb_tables or information_schema.innodb_sys_tables
+ * @package Cadfael\Engine\Entity\Table
+ * @codeCoverageIgnore
+ */
 class InnoDbTable
 {
     public int $table_id;

src/Engine/Entity/Tablespace.php

@@ -6,6 +6,13 @@
 
 use Cadfael\Engine\Entity;
 
+/**
+ * Class Tablespace (readonly)
+ *
+ * Stores a representation of information_schema.innodb_tablespaces or information_schema.innodb_sys_tablespaces
+ * @package Cadfael\Engine\Entity
+ * @codeCoverageIgnore
+ */
 class Tablespace implements Entity
 {
     /**

tests/Engine/Check/Index/IndexPrefixTest.php

@@ -25,12 +25,21 @@ public function setUp(): void
 
         // We need:
         // * A unique index
-        $this->uniqueIndex = $builder->name('unique_index')->isUnique(true)->generate();
-        $this->uniqueIndex->getColumns()[0]->setCardinality(1);
+        $this->uniqueIndex = $builder
+            ->name('unique_index')
+            ->isUnique(true)
+            ->generate();
+        $this->uniqueIndex
+            ->getColumns()[0]
+            ->setCardinality(1);
 
         // * An index without string columns
-        $this->nonStringIndex = $builder->name('non_string_index')->generate();
-        $this->nonStringIndex->getColumns()[0]->setCardinality(1);
+        $this->nonStringIndex = $builder
+            ->name('non_string_index')
+            ->generate();
+        $this->nonStringIndex
+            ->getColumns()[0]
+            ->setCardinality(1);
 
         // * An index with string columns that are under 12 characters
         $this->shortStringIndex = $builder
@@ -56,7 +65,7 @@ public function setUp(): void
 
         // * An index with string columns that are over 12 characters and is not prefixed
         $this->longStringIndex = $builder
-            ->name('long_string_zero_cardinality_index')
+            ->name('long_string_high_cardinality_index')
             ->setColumn((new ColumnBuilder())->varchar(255)->generate())
             ->generate();
         $column = $this->longStringIndex->getColumns()[0];

tests/Engine/Check/Table/EmptyTableTest.php

@@ -2,6 +2,8 @@
 declare(strict_types=1);
 
 use Cadfael\Engine\Check\Table\EmptyTable;
+use Cadfael\Engine\Entity\Table\InnoDbTable;
+use Cadfael\Engine\Entity\Tablespace;
 use Cadfael\Engine\Report;
 use Cadfael\Tests\Engine\BaseTest;
 use Cadfael\Tests\Engine\Check\ColumnBuilder;
@@ -36,35 +38,67 @@ public function providerTableDataForRun() {
             ->primary()
             ->auto_increment()
             ->generate();
-        $table = $this->createEmptyTable();
+        $table = $this->createEmptyTable(['TABLE_NAME' => 'empty_table']);
         $table->setColumns($column);
 
         $column = $builder->int(10)
             ->unsigned()
             ->primary()
             ->auto_increment()
             ->generate();
-        $table_with_inserts = $this->createEmptyTable();
+        $table_with_inserts = $this->createEmptyTable(['TABLE_NAME' => 'empty_table_with_inserts']);
         $table_with_inserts->setColumns($column);
         $auto_increment = $table_with_inserts->getSchemaAutoIncrementColumn();
         $auto_increment->auto_increment = 10;
 
+        $table_with_data_free = $this->createEmptyTable(['TABLE_NAME' => 'empty_table_with_data_free', 'DATA_FREE' => 110]);
+        $table_with_data_free_in_tablespace = $this->createEmptyTable(['TABLE_NAME' => 'empty_table_with_data_free_in_tablespace', 'DATA_FREE' => 110]);
+        $table_with_data_free_in_tablespace->setInnoDbTable(InnoDbTable::createFromInformationSchema([
+            'TABLE_ID' => 2425,
+            'SPACE' => 0,
+            'NAME' => 'test/test',
+            'FLAG' => 161,
+            'N_COLS' => 5,
+            'ROW_FORMAT' => 'Dynamic',
+            'ZIP_PAGE_SIZE' => 0,
+            'SPACE_TYPE' => 'System',
+            'INSTANT_COLS' => 0
+        ]));
+
         return [
             [
                 $table,
-                Report::STATUS_WARNING
+                Report::STATUS_WARNING,
+                [ 'Table contains no records.' ]
             ],
             [
                 $table_with_inserts,
-                Report::STATUS_CONCERN
+                Report::STATUS_CONCERN,
+                [
+                    "Table is empty but previously had records inserted.",
+                    "It is possible it is used as a some form of queue or has had all records deleted."
+                ]
             ],
             [
-                $this->createEmptyTable([ "DATA_FREE" => 110 ]),
-                Report::STATUS_CONCERN
+                $table_with_data_free,
+                Report::STATUS_CONCERN,
+                [
+                    "Table is empty but has allocated free space.",
+                    "It is possible it is used as a some form of queue or has had all records deleted."
+                ]
             ],
             [
-                $this->createTable(),
-                Report::STATUS_OK
+                $table_with_data_free_in_tablespace,
+                Report::STATUS_CONCERN,
+                [
+                    "Table is empty but has allocated free space.",
+                    "This table is in a shared tablespace so this doesn't mean much."
+                ]
+            ],
+            [
+                $this->createTable(['TABLE_NAME' => 'table_with_rows']),
+                Report::STATUS_OK,
+                []
             ],
         ];
     }
@@ -75,15 +109,31 @@ public function providerTableDataForRun() {
     public function testSupports($table, $result)
     {
         $check = new EmptyTable();
-        $this->assertEquals($check->supports($table), $result, "Ensure that the supports for $table returns $result");
+        $this->assertEquals(
+            $result,
+            $check->supports($table),
+            "Ensure that the supports for $table returns $result"
+        );
     }
 
     /**
      * @dataProvider providerTableDataForRun
      */
-    public function testRun($table, $result)
+    public function testRun($table, $result, $message)
     {
         $check = new EmptyTable();
-        $this->assertEquals($check->run($table)->getStatus(), $result, "Ensure that the run for $table returns status $result");
+        $report = $check->run($table);
+
+        $this->assertEquals(
+            $result,
+            $report->getStatus(),
+            "Ensure that the run for $table returns status $result")
+        ;
+
+        $this->assertEquals(
+            $message,
+            $report->getMessages(),
+            "Ensure that the run for $table returns expected message")
+        ;
     }
 }

tests/Engine/Entity/ColumnTest.php

@@ -179,4 +179,13 @@ public function test__nonIntergetColumnCalling__getCapacity()
         $this->expectException(InvalidColumnType::class);
         $this->stringColumn->getCapacity();
     }
+
+    public function test__getCardinalityRatio()
+    {
+        $this->assertEquals(
+            0,
+            $this->invalidColumn->getCardinalityRatio(),
+            "Invalid column should return a zero cardinality"
+        );
+    }
 }