refactor(insights): drop CTA key field

`key` was a leftover from when CTAs were embedded JSON — there's no remaining reason to require analyzers to invent a within-insight identifier. The execution layer is gone (no `cta.key` event format), insights are immutable from the user side (analyzers re-ingest by delete + recreate, so idempotent matching never happens), and `label` already covers human-facing identification. The console can group/sort CTAs by `service`+`method` if needed. - Schema: drop `key` attribute and the UNIQUE `(insightInternalId, key)` index from insightCTAs. Required fields are now `label`, `service`, `method` (+ optional `params`). - Validator no longer requires `key`. Drop the dup-key normalization loop in the manager Create endpoint — there's no semantic uniqueness to enforce. - Response model: `InsightCTA` keeps `$id` + standard headers, `insightId` backref, and the four functional fields. - E2E: drop sampleCTA's `$key` parameter, drop the testCreateRejectsDuplicateCTAIds test entirely, rename empty-fields test to testCreateRejectsCTAWithEmptyLabel and update the missing- fields tests to drop `key` from their payloads. - Unit tests rewritten to drop `key`. - Comment on the `insights.ctas` virtual attribute updated to reference the renamed `insightCTAs` collection. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-05-26 13:51:13 +00:00 · 2026-05-06 18:29:40 +12:00
parent c5dfc42a60
commit 0b72dba817
6 changed files with 15 additions and 96 deletions
@@ -2261,7 +2261,7 @@ $platformCollections = [
                'filters' => ['json'],
            ],
            [
-                // Virtual attribute — CTAs live in their own `ctas` collection
+                // Virtual attribute — CTAs live in the `insightCTAs` collection
                // back-referenced by `insightInternalId`. The subQuery filter
                // joins them in at read time, so consumers still see them
                // embedded on the insight response.
@@ -2418,18 +2418,6 @@ $platformCollections = [
                'array' => false,
                'filters' => [],
            ],
-            [
-                // Caller-supplied identifier, unique within the parent insight.
-                '$id' => ID::custom('key'),
-                'type' => Database::VAR_STRING,
-                'format' => '',
-                'size' => Database::LENGTH_KEY,
-                'signed' => true,
-                'required' => true,
-                'default' => null,
-                'array' => false,
-                'filters' => [],
-            ],
            [
                '$id' => ID::custom('label'),
                'type' => Database::VAR_STRING,
@@ -2492,14 +2480,6 @@ $platformCollections = [
                'lengths' => [Database::LENGTH_KEY, 0],
                'orders' => [],
            ],
-            [
-                // Enforce per-insight key uniqueness at the DB layer.
-                '$id' => ID::custom('_key_insight_key'),
-                'type' => Database::INDEX_UNIQUE,
-                'attributes' => ['insightInternalId', 'key'],
-                'lengths' => [0, Database::LENGTH_KEY],
-                'orders' => [],
-            ],
        ],
    ],
 ];
@@ -8,7 +8,7 @@ class CTAs extends Validator
 {
    public const MAX_COUNT_DEFAULT = 16;

-    protected string $message = 'Value must be an array of CTA descriptors. Each entry must define `key`, `label`, `service`, `method`, and an optional `params` object.';
+    protected string $message = 'Value must be an array of CTA descriptors. Each entry must define `label`, `service`, `method`, and an optional `params` object.';

    public function __construct(protected int $maxCount = self::MAX_COUNT_DEFAULT)
    {
@@ -45,7 +45,7 @@ class CTAs extends Validator
                return false;
            }

-            foreach (['key', 'label', 'service', 'method'] as $required) {
+            foreach (['label', 'service', 'method'] as $required) {
                if (!isset($entry[$required]) || !\is_string($entry[$required]) || $entry[$required] === '') {
                    return false;
                }
@@ -85,7 +85,7 @@ class Create extends Action
            ->param('title', '', new Text(256), 'Short, human-readable title.')
            ->param('summary', '', new Text(4096, 0), 'Markdown summary describing the insight.', true)
            ->param('payload', null, new Nullable(new JSON()), 'Type-specific structured payload.', true)
-            ->param('ctas', [], new CTAsValidator(), 'Array of call-to-action descriptors. Each must contain `key` (unique within the insight), `label`, `service`, `method`, and an optional `params` object.', true)
+            ->param('ctas', [], new CTAsValidator(), 'Array of call-to-action descriptors. Each must contain `label`, `service`, `method`, and an optional `params` object.', true)
            ->param('analyzedAt', null, new Nullable(new DatetimeValidator()), 'Time the insight was analyzed in ISO 8601 format. Defaults to now.', true)
            ->inject('response')
            ->inject('project')
@@ -129,18 +129,10 @@ class Create extends Action
            $reportInternalId = $report->getSequence();
        }

-        $seen = [];
        $normalizedCTAs = [];

        foreach ($ctas as $cta) {
-            $key = (string) $cta['key'];
-            if (isset($seen[$key])) {
-                throw new Exception(Exception::GENERAL_ARGUMENT_INVALID, 'CTA `key` values must be unique within an insight.');
-            }
-            $seen[$key] = true;
-
            $normalizedCTAs[] = [
-                'key' => $key,
                'label' => (string) $cta['label'],
                'service' => (string) $cta['service'],
                'method' => (string) $cta['method'],
@@ -182,7 +174,6 @@ class Create extends Action
                'projectId' => $project->getId(),
                'insightInternalId' => $insight->getSequence(),
                'insightId' => $insight->getId(),
-                'key' => $cta['key'],
                'label' => $cta['label'],
                'service' => $cta['service'],
                'method' => $cta['method'],
@@ -34,12 +34,6 @@ class InsightCTA extends Model
                'default' => '',
                'example' => '5e5ea5c16897e',
            ])
-            ->addRule('key', [
-                'type' => self::TYPE_STRING,
-                'description' => 'Caller-supplied identifier, unique within the parent insight.',
-                'default' => '',
-                'example' => 'createIndex',
-            ])
            ->addRule('label', [
                'type' => self::TYPE_STRING,
                'description' => 'Human-readable label for the CTA, used in UI.',
@@ -84,10 +84,9 @@ trait InsightsBase
     *  - `documentsDB`  → service `documentsDB`,  method `createIndex` (params use collectionId/attributes)
     *  - `vectorsDB`    → service `vectorsDB`,    method `createIndex` (params use collectionId/attributes)
     */
-    protected function sampleCTA(string $key = 'createIndex', string $engine = 'tablesDB'): array
+    protected function sampleCTA(string $engine = 'tablesDB'): array
    {
        $base = [
-            'key' => $key,
            'label' => 'Create missing index',
            'method' => 'createIndex',
        ];
@@ -169,7 +168,7 @@ trait InsightsBase
            'title' => 'Missing index on collection orders',
            'summary' => 'Queries against `orders.status` are scanning the full collection.',
            'payload' => ['databaseId' => 'main', 'engine' => $engine],
-            'ctas' => [$this->sampleCTA('createIndex', $engine)],
+            'ctas' => [$this->sampleCTA($engine)],
        ];

        if ($reportId !== null) {
@@ -349,7 +348,6 @@ trait InsightsBase
        $this->assertSame('orders', $insight['body']['parentResourceId']);
        $this->assertSame('Missing index on collection orders', $insight['body']['title']);
        $this->assertCount(1, $insight['body']['ctas']);
-        $this->assertSame('createIndex', $insight['body']['ctas'][0]['key']);
        $this->assertSame($insightId, $insight['body']['ctas'][0]['insightId']);
        $this->assertSame('Create missing index', $insight['body']['ctas'][0]['label']);
        $this->assertSame('tablesDB', $insight['body']['ctas'][0]['service']);
@@ -475,7 +473,7 @@ trait InsightsBase
        $this->assertSame('report_not_found', $insight['body']['type']);
    }

-    public function testCreateRejectsDuplicateCTAIds(): void
+    public function testCreateRejectsCTAWithEmptyLabel(): void
    {
        $insight = $this->createInsight([
            'insightId' => ID::unique(),
@@ -484,25 +482,7 @@ trait InsightsBase
            'resourceId' => 'main',
            'title' => 'Should not be created',
            'ctas' => [
-                ['key' => 'dup', 'label' => 'A', 'service' => 'databases', 'method' => 'createIndex'],
-                ['key' => 'dup', 'label' => 'B', 'service' => 'databases', 'method' => 'createIndex'],
-            ],
-        ]);
-
-        $this->assertSame(400, $insight['headers']['status-code']);
-        $this->assertSame('general_argument_invalid', $insight['body']['type']);
-    }
-
-    public function testCreateRejectsCTAWithEmptyFields(): void
-    {
-        $insight = $this->createInsight([
-            'insightId' => ID::unique(),
-            'type' => 'databaseIndex',
-            'resourceType' => 'databases',
-            'resourceId' => 'main',
-            'title' => 'Should not be created',
-            'ctas' => [
-                ['key' => '', 'label' => 'Has empty id', 'service' => 'databases', 'method' => 'createIndex'],
+                ['label' => '', 'service' => 'databases', 'method' => 'createIndex'],
            ],
        ]);

@@ -518,7 +498,7 @@ trait InsightsBase
            'resourceId' => 'main',
            'title' => 'Should not be created',
            'ctas' => [
-                ['key' => 'createIndex', 'label' => 'Missing method', 'service' => 'tablesDB'],
+                ['label' => 'Missing method', 'service' => 'tablesDB'],
            ],
        ]);

@@ -534,7 +514,7 @@ trait InsightsBase
            'resourceId' => 'main',
            'title' => 'Should not be created',
            'ctas' => [
-                ['key' => 'createIndex', 'label' => 'Missing service', 'method' => 'createIndex'],
+                ['label' => 'Missing service', 'method' => 'createIndex'],
            ],
        ]);

@@ -546,7 +526,6 @@ trait InsightsBase
        $ctas = [];
        for ($i = 0; $i < 17; $i++) {
            $ctas[] = [
-                'key' => 'cta-' . $i,
                'label' => 'CTA ' . $i,
                'service' => 'databases',
                'method' => 'createIndex',
@@ -28,7 +28,6 @@ class CTAsTest extends TestCase
        $validator = new CTAs();

        $this->assertTrue($validator->isValid([[
-            'key' => 'createIndex',
            'label' => 'Create missing index',
            'service' => 'tablesDB',
            'method' => 'createIndex',
@@ -44,7 +43,6 @@ class CTAsTest extends TestCase
        $validator = new CTAs();

        $this->assertTrue($validator->isValid([[
-            'key' => 'createIndex',
            'label' => 'Create missing index',
            'service' => 'tablesDB',
            'method' => 'createIndex',
@@ -55,10 +53,9 @@ class CTAsTest extends TestCase
    {
        $validator = new CTAs();

-        $this->assertFalse($validator->isValid([['key' => 'x']]));
-        $this->assertFalse($validator->isValid([['key' => 'x', 'label' => 'y']]));
-        $this->assertFalse($validator->isValid([['key' => 'x', 'label' => 'y', 'service' => 'tablesDB']]));
-        $this->assertFalse($validator->isValid([['key' => 'x', 'label' => 'y', 'method' => 'createIndex']]));
+        $this->assertFalse($validator->isValid([['label' => 'x']]));
+        $this->assertFalse($validator->isValid([['label' => 'x', 'service' => 'tablesDB']]));
+        $this->assertFalse($validator->isValid([['label' => 'x', 'method' => 'createIndex']]));
    }

    public function testRejectsEntryWithEmptyStrings(): void
@@ -66,8 +63,7 @@ class CTAsTest extends TestCase
        $validator = new CTAs();

        $this->assertFalse($validator->isValid([[
-            'key' => '',
-            'label' => 'Create missing index',
+            'label' => '',
            'service' => 'tablesDB',
            'method' => 'createIndex',
        ]]));
@@ -78,8 +74,7 @@ class CTAsTest extends TestCase
        $validator = new CTAs();

        $this->assertFalse($validator->isValid([[
-            'key' => 123,
-            'label' => 'Create missing index',
+            'label' => 123,
            'service' => 'tablesDB',
            'method' => 'createIndex',
        ]]));
@@ -90,7 +85,6 @@ class CTAsTest extends TestCase
        $validator = new CTAs();

        $this->assertFalse($validator->isValid([[
-            'key' => 'createIndex',
            'label' => 'Create missing index',
            'service' => 'tablesDB',
            'method' => 'createIndex',
@@ -113,7 +107,6 @@ class CTAsTest extends TestCase
        $entries = [];
        for ($i = 0; $i < 4; $i++) {
            $entries[] = [
-                'key' => 'cta-' . $i,
                'label' => 'Label ' . $i,
                'service' => 'tablesDB',
                'method' => 'createIndex',
@@ -131,7 +124,6 @@ class CTAsTest extends TestCase
        $entries = [];
        for ($i = 0; $i < 3; $i++) {
            $entries[] = [
-                'key' => 'cta-' . $i,
                'label' => 'Label ' . $i,
                'service' => 'tablesDB',
                'method' => 'createIndex',
@@ -146,7 +138,6 @@ class CTAsTest extends TestCase
        $validator = new CTAs();

        $entry = [
-            'key' => 'createIndex',
            'label' => 'Create missing index',
            'service' => 'tablesDB',
            'method' => 'createIndex',
@@ -161,7 +152,6 @@ class CTAsTest extends TestCase
        $validator = new CTAs();

        $this->assertFalse($validator->isValid([[
-            'key' => 'createIndex',
            'label' => 'Create missing index',
            'service' => '',
            'method' => 'createIndex',
@@ -173,25 +163,12 @@ class CTAsTest extends TestCase
        $validator = new CTAs();

        $this->assertFalse($validator->isValid([[
-            'key' => 'createIndex',
            'label' => 'Create missing index',
            'service' => 'tablesDB',
            'method' => '',
        ]]));
    }

-    public function testRejectsEntryWithEmptyLabel(): void
-    {
-        $validator = new CTAs();
-
-        $this->assertFalse($validator->isValid([[
-            'key' => 'createIndex',
-            'label' => '',
-            'service' => 'tablesDB',
-            'method' => 'createIndex',
-        ]]));
-    }
-
    public function testDefaultMaxCountIsSixteen(): void
    {
        $validator = new CTAs();
@@ -201,7 +178,6 @@ class CTAsTest extends TestCase
        $entries = [];
        for ($i = 0; $i < 16; $i++) {
            $entries[] = [
-                'key' => 'cta-' . $i,
                'label' => 'Label ' . $i,
                'service' => 'tablesDB',
                'method' => 'createIndex',
@@ -211,7 +187,6 @@ class CTAsTest extends TestCase
        $this->assertTrue($validator->isValid($entries));

        $entries[] = [
-            'key' => 'cta-16',
            'label' => 'Label 16',
            'service' => 'tablesDB',
            'method' => 'createIndex',