src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Action.php (1)

129-137: getParentNotFoundException() duplicates getNotFoundException().

Both methods return identical values (COLLECTION_NOT_FOUND / TABLE_NOT_FOUND). Consider reusing getNotFoundException() or removing one method to avoid confusion and maintenance overhead.

-    /**
-     * Get the appropriate parent level not found exception.
-     */
-    protected function getParentNotFoundException(): string
-    {
-        return $this->isCollectionsAPI()
-            ? Exception::COLLECTION_NOT_FOUND
-            : Exception::TABLE_NOT_FOUND;
-    }
+    /**
+     * Get the appropriate parent level not found exception.
+     */
+    protected function getParentNotFoundException(): string
+    {
+        return $this->getNotFoundException();
+    }

src/Appwrite/Utopia/Database/Validator/Attributes.php (2)

163-169: Consider validating enum elements are unique strings.

The enum validation checks for a non-empty elements array but doesn't verify that each element is a string or that elements are unique, which could lead to issues when creating the attribute in the database.

             // Validate enum elements if format is enum
             if (isset($attribute['format']) && $attribute['format'] === APP_DATABASE_ATTRIBUTE_ENUM) {
                 if (!isset($attribute['elements']) || !is_array($attribute['elements']) || empty($attribute['elements'])) {
                     $this->message = "Attribute '" . $attribute['key'] . "' with enum format must have 'elements' array";
                     return false;
                 }
+                foreach ($attribute['elements'] as $element) {
+                    if (!is_string($element)) {
+                        $this->message = "Attribute '" . $attribute['key'] . "' enum elements must be strings";
+                        return false;
+                    }
+                }
+                if (count($attribute['elements']) !== count(array_unique($attribute['elements']))) {
+                    $this->message = "Attribute '" . $attribute['key'] . "' has duplicate enum elements";
+                    return false;
+                }
             }

---

151-161: Default value type is not validated against the attribute type.

The validator checks for conflicts between default and required/array, but does not verify that the default value's type matches the attribute's declared type. For example, { type: 'boolean', default: 'hello' } would pass validation.

Consider adding type validation for default values, or verify that downstream processing handles type mismatches gracefully.

tests/e2e/Services/Databases/Legacy/DatabasesCustomServerTest.php (3)

6783-6959: Confirm async behavior assumptions for attributes/indexes created via collection create

These assertions assume that attributes and indexes are already in 'available' status immediately after the collection create (no sleep / assertEventually like the rest of this file uses for attribute/index creation). If the new multi-create flow still schedules schema work asynchronously, these expectations may be flaky on slower CI or under load.

If the implementation really does block until attributes and indexes are fully built, it would be good to confirm that and maybe add a brief comment here. Otherwise, consider wrapping the /attributes and /indexes checks in assertEventually to poll until all expected keys reach 'available' before asserting their metadata.

---

6960-7111: Nice negative coverage; consider asserting error types and one or two more edge cases

This error suite is useful and focused: invalid type, index referencing unknown attribute, required+default, duplicate keys, and a system-attribute index happy-path. Two optional polish ideas:

• For at least one or two of these cases, also assert the error type (e.g. AppwriteException::GENERAL_ARGUMENT_INVALID / ATTRIBUTE_DEFAULT_UNSUPPORTED) to lock in the contract beyond just 400.
• Optionally add one more index-schema check (e.g. mismatched attributes/orders length or an invalid type constant) to exercise the new Indexes validator a bit more.

Not blocking, but would strengthen regression detection around the new validators.

---

7113-7202: Enum attribute happy-path looks good; optional: add one creation-time failure case

The enum creation flow and the valid/invalid document checks look correct and align with the new "type" = string + "format" = enum + "elements" contract. If you want to harden this area further, you could add a single collection-create failure case here (e.g. enum with empty elements, or format = 'enum' but missing elements) so creation-time enum validation is exercised as well, not only via the update tests above.

Purely optional; current coverage is already solid.

src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Create.php (2)

122-136: Consider clarifying the variable naming for consistency.

The variable $collectionAttributes is later passed to buildIndexDocument as $attributeDocuments (line 133 → line 295). While functionally correct, the naming inconsistency can cause confusion during maintenance.

Consider renaming for clarity:

-        $collectionAttributes = [];
-        $attributeDocuments = [];
+        $attributesForCollection = [];
+        $attributeMetadataDocuments = [];
         foreach ($attributes as $attributeDef) {
             $attrDoc = $this->buildAttributeDocument($database, $collection, $attributeDef, $dbForProject);
-            $collectionAttributes[] = $attrDoc['collection'];
-            $attributeDocuments[] = $attrDoc['document'];
+            $attributesForCollection[] = $attrDoc['collection'];
+            $attributeMetadataDocuments[] = $attrDoc['document'];
         }

Then update the corresponding usages on lines 133, 141, and 157.

---

154-167: Consider aligning collection creation with the transactional pattern used for document operations.

The codebase uses withTransaction() for multi-step document operations (Upsert, Update, Delete, Create), but collection-level operations (Create, Update, Delete) do not. Since collection creation involves multiple dependent steps—creating the collection document, building the schema, and creating attribute/index metadata documents—consider whether wrapping these operations in a transaction would improve consistency safety, consistent with the pattern established for document operations.

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Learnt from: ItzNotABug
Repo: appwrite/appwrite PR: 9693
File: src/Appwrite/Platform/Modules/Databases/Http/Databases/Tables/Update.php:57-59
Timestamp: 2025-06-19T09:20:03.312Z
Learning: In table-related endpoints (such as `src/Appwrite/Platform/Modules/Databases/Http/Databases/Tables/Update.php`), parameter descriptions should use "table" and "row" terminology instead of "collection" and "document" for clarity and consistency.

Learnt from: ItzNotABug
Repo: appwrite/appwrite PR: 9693
File: src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Indexes/Delete.php:85-86
Timestamp: 2025-05-10T04:12:13.037Z
Learning: The class `Appwrite\Platform\Modules\Databases\Http\Databases\Collections\Indexes\Action` defines a method `getGrantParentNotFoundException()` which is used to handle cases where a parent resource (like a database collection) is not found.

src/Appwrite/Utopia/Database/Validator/Attributes.php (1)

136-142: Format validation should be restricted to string types.

String-specific formats (email, enum, ip, url) are validated for all attribute types. This allows invalid combinations like { type: 'integer', format: 'email' } to pass validation.

             // Validate format if provided
-            if (isset($attribute['format']) && $attribute['format'] !== '') {
+            if ($attribute['type'] === Database::VAR_STRING && isset($attribute['format']) && $attribute['format'] !== '') {
                 if (!in_array($attribute['format'], $this->supportedFormats)) {
                     $this->message = "Invalid format for attribute '" . $attribute['key'] . "': " . $attribute['format'];
                     return false;
                 }
             }

src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Create.php (1)

22-22: Remove unused import causing pipeline failure.

StructureException is imported but never used in this file.

-use Utopia\Database\Exception\Structure as StructureException;

tests/e2e/Services/Databases/Legacy/DatabasesCustomServerTest.php (1)

6960-7590: Comprehensive negative‑path validation for attributes, enums, and indexes; consider data‑driven consolidation

The new tests around invalid attribute types/defaults, reserved keys, min/max constraints, enum element/default rules, index key/type/attributes/orders/lengths, and cleanup on failure are thorough and align with the behavior exercised by the per‑attribute tests earlier in this class. From a maintainability standpoint, there’s a fair bit of repetition in the “create DB → create collection with 1 attribute/index → assert 400/201” pattern; over time, you might want to move these cases to a data provider or small helper to keep this already‑large test class from growing harder to navigate. Functionally, the expectations look consistent.

src/Appwrite/Utopia/Database/Validator/Indexes.php (1)

183-201: Inconsistency between isArray() and getType() return values.

isArray() returns false while getType() returns self::TYPE_ARRAY. This is inconsistent—if the validator expects array input (which it does, per line 60), isArray() should return true.

     public function isArray(): bool
     {
-        return false;
+        return true;
     }

src/Appwrite/Utopia/Database/Validator/Attributes.php (1)

355-368: Inconsistency between isArray() and getType() return values.

Same issue as in Indexes.php: isArray() returns false while getType() returns TYPE_ARRAY. The validator expects array input, so isArray() should return true.

     public function isArray(): bool
     {
-        return false;
+        return true;
     }

src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Create.php (1)

331-339: Consider renaming parameter for clarity.

The $attributeDocuments parameter receives $collectionAttributes from the caller (line 149), which are the "collection" documents from buildAttributeDocument. The naming is confusing since the codebase uses "attributeDocuments" for storage documents and "collectionAttributes" for schema documents.

-    protected function buildIndexDocument(Document $database, Document $collection, array $indexDef, array $attributeDocuments, Database $dbForProject): array
+    protected function buildIndexDocument(Document $database, Document $collection, array $indexDef, array $collectionAttributes, Database $dbForProject): array
     {
         $key = $indexDef['key'];
         $type = $indexDef['type'];
         $indexAttributes = $indexDef['attributes'];
         $orders = $indexDef['orders'] ?? [];
         $lengths = $indexDef['lengths'] ?? [];
 
-        $attrKeys = array_map(fn ($a) => $a->getAttribute('key'), $attributeDocuments);
+        $attrKeys = array_map(fn ($a) => $a->getAttribute('key'), $collectionAttributes);

And update remaining references to use $collectionAttributes.

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Learnt from: ItzNotABug
Repo: appwrite/appwrite PR: 9693
File: src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Indexes/Delete.php:85-86
Timestamp: 2025-05-10T04:12:13.037Z
Learning: The class `Appwrite\Platform\Modules\Databases\Http\Databases\Collections\Indexes\Action` defines a method `getGrantParentNotFoundException()` which is used to handle cases where a parent resource (like a database collection) is not found.

Learnt from: ItzNotABug
Repo: appwrite/appwrite PR: 9693
File: src/Appwrite/Platform/Modules/Databases/Http/Databases/Tables/Update.php:57-59
Timestamp: 2025-06-19T09:20:03.312Z
Learning: In table-related endpoints (such as `src/Appwrite/Platform/Modules/Databases/Http/Databases/Tables/Update.php`), parameter descriptions should use "table" and "row" terminology instead of "collection" and "document" for clarity and consistency.

src/Appwrite/Utopia/Database/Validator/Attributes.php (2)

197-271: Defaults for relationship and spatial attributes are neither validated nor explicitly rejected.

The default-value switch only covers string, integer, float, boolean, and datetime types. For relationship and spatial types (point, linestring, polygon), any provided default:

• Passes isValid() without type checking.
• Is forwarded into the attribute documents unmodified.
• Relies on the adapter to error out (or silently ignore), which can lead to confusing server-side failures instead of a clear 4xx validation error.

It would be safer to make the behavior explicit for these types:

• Either disallow non-null defaults (and fail fast with a clear message), or
• Add type-specific validation branches (e.g., shape of a spatial object, nullable semantics for relationships).

For example, if defaults should not be supported:

         // Validate default value matches attribute type
         if (isset($attribute['default'])) {
             switch ($attribute['type']) {
@@
                 case Database::VAR_DATETIME:
                     // existing logic...
                     break;
+
+                case Database::VAR_RELATIONSHIP:
+                case Database::VAR_POINT:
+                case Database::VAR_LINESTRING:
+                case Database::VAR_POLYGON:
+                    $this->message = "Default value is not supported for attribute type '" . $attribute['type'] . "'";
+                    return false;
             }
         }

Aligning this with the actual adapter capabilities will prevent hard-to-debug runtime errors.

---

127-133: Enum elements are not checked against the attribute size, which can yield unsatisfiable enums.

You validate:

• String size bounds for the attribute (lines 127-133).
• Each enum element is a non-empty string and ≤ Database::LENGTH_KEY (lines 281-289).

However, you do not ensure that each enum element’s length is ≤ the attribute’s configured size. This allows definitions where some enum values literally cannot be stored in the column (e.g. size = 4, element "apple").

To avoid creating impossible schemas, consider adding a size check inside the enum loop:

-                foreach ($attribute['elements'] as $elementIndex => $element) {
+                $attrSize = $attribute['size'] ?? APP_DATABASE_ATTRIBUTE_STRING_MAX_LENGTH;
+                foreach ($attribute['elements'] as $elementIndex => $element) {
                     if (!is_string($element) || empty($element)) {
                         $this->message = "Enum element at index $elementIndex for attribute '" . $attribute['key'] . "' must be a non-empty string";
                         return false;
                     }
                     if (strlen($element) > Database::LENGTH_KEY) {
                         $this->message = "Enum element at index $elementIndex for attribute '" . $attribute['key'] . "' exceeds maximum length of " . Database::LENGTH_KEY . " characters";
                         return false;
                     }
+                    if (strlen($element) > $attrSize) {
+                        $this->message = "Enum element at index $elementIndex for attribute '" . $attribute['key'] . "' exceeds attribute size of $attrSize characters";
+                        return false;
+                    }
                 }

This keeps enum definitions consistent with the underlying column size.

Also applies to: 273-299

src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Create.php (2)

82-83: Attributes param description is slightly out of sync with supported attribute types.

AttributesValidator supports additional types like spatial (point, linestring, polygon), but the parameter description only mentions string, integer, float, boolean, datetime, relationship.

To avoid confusion for SDK users and API consumers, consider updating the description to reflect the full set of supported types (or link to a single source of truth in the docs):

-            ->param('attributes', [], new AttributesValidator(), 'Array of attribute definitions to create. Each attribute should contain: key (string), type (string: string, integer, float, boolean, datetime, relationship), size (integer, required for string type), required (boolean, optional), default (mixed, optional), array (boolean, optional), and type-specific options.', true)
+            ->param('attributes', [], new AttributesValidator(), 'Array of attribute definitions to create. Each attribute should contain: key (string), type (string: string, integer, float, boolean, datetime, relationship, point, linestring, polygon), size (integer, required for string type), required (boolean, optional), default (mixed, optional), array (boolean, optional), and type-specific options.', true)

This keeps the public API contract aligned with the validator.

---

122-143: Unify rollback behavior by using cleanup() instead of scattered deleteDocument() calls.

Failure handling around collection creation is mostly correct, but inconsistent:

• In several places (lines 133-136, 139-142, 152-155, 166-176), you call $dbForProject->deleteDocument($databaseKey, $collection->getId()); directly.
• The cleanup() helper (lines 419-435) encapsulates safe rollback of both the DB collection and the metadata document, swallowing any cleanup errors so they don’t mask the original exception.
• Currently, if deleteDocument() itself throws (e.g. connection issue), that secondary error can override the original validation/adapter failure.

To make rollback behavior consistent and less error-prone, consider replacing those direct deleteDocument() calls with cleanup() wherever you’ve already computed $databaseKey and $collectionKey:

-        } catch (\Throwable $e) {
-            $dbForProject->deleteDocument($databaseKey, $collection->getId());
-            throw $e;
-        }
+        } catch (\Throwable $e) {
+            $this->cleanup($dbForProject, $databaseKey, $collectionKey, $collection->getId());
+            throw $e;
+        }

Apply the same pattern for:

• Attribute-building failure (lines 133-136).
• Index limit check (lines 139-142).
• Index-building failure (lines 152-155).
• createCollection() failures (lines 166-176).

cleanup() already guards deleteCollection()/deleteDocument() in try/catch, so calling it before the DB collection exists is safe (the delete will just be ignored), and you avoid leaking cleanup errors to API consumers.

Also applies to: 147-155, 166-176, 181-193, 419-435

tests/e2e/Services/Databases/Legacy/DatabasesCustomServerTest.php (3)

7183-7606: Enum and general attribute edge-case coverage is comprehensive but densely packed

The new tests around enum attributes and general attribute validation edge cases (reserved keys, type/default mismatches, min/max inconsistencies, array+default, invalid format, etc.) provide very thorough coverage of the validator. The only nit is that testCreateCollectionAttributeValidationEdgeCases and testCreateCollectionEnumValidationEdgeCases bundle many unrelated assertions into single methods, which can make failures harder to localize when something breaks. If this grows further, consider splitting into smaller tests or using data providers to keep individual scenarios more isolated, while reusing setup.

---

7608-8011: Index and relationship validation tests are solid; double-check naming consistency for relationship fields

The index validation tests hit the important edge cases (duplicate keys, invalid types, empty attributes, orders/lengths mismatch, invalid order values, and a valid compound index) and look correct. Relationship validation also covers missing relatedCollection, missing/invalid relationType, invalid onDelete, array misuse, bad twoWay, and a valid many-to-one two-way relationship.

One design point to verify: in these multi-create payloads, relationship attributes use relatedCollection + relationType alongside type => Database::VAR_RELATIONSHIP, whereas the dedicated relationship-attribute endpoints elsewhere in the suite use relatedCollectionId and type for the relation kind. If the public API is meant to accept both styles or intentionally diverge here, all good; otherwise, consider aligning field names or documenting the difference clearly to avoid confusion for SDK/CLI users.

---

8013-8280: Datetime/float/missing-field validation tests match existing rules; consider asserting error types for stronger guarantees

The new tests for datetime defaults, float min/max + defaults, and missing required fields in attribute/index definitions correctly mirror the validation rules exercised earlier in the file (e.g., date format constraints, numeric range checks, and mandatory keys in definitions). The HTTP status expectations (400 vs 201) look appropriate and should guard against regressions in the new validators.

If you want these tests to catch more subtle changes in error semantics later, you might also assert the error type (e.g., specific Appwrite exception codes) in a few representative negative cases, similar to other tests in this class. That’s optional but can help ensure the validator fails for the right reasons, not just with a generic 400.

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

tests/e2e/Services/Databases/Legacy/DatabasesCustomServerTest.php (2)

6960-7110: Consider asserting error type payloads in addition to 400 status

The negative tests for invalid attribute/index/relationship definitions currently only assert status-code === 400. That will catch validation vs. success, but won’t fail if the implementation starts returning a different error classification (e.g., wrong type code or a more generic error).

Given these tests are encoding the contract of the new validators, you may want to also assert the specific body['type'] (similar to earlier tests in this class that use AppwriteException constants) for at least a representative subset of these cases (e.g., invalid type, reserved key, min/max violations, enum/relationship/index edge cases). This would tighten regression coverage without much extra noise.

Also applies to: 7182-7605, 7607-7825, 8012-8150, 8152-8279

---

7273-7500: Large “kitchen‑sink” edge‑case tests might benefit from some structuring

testCreateCollectionAttributeValidationEdgeCases, testCreateCollectionEnumValidationEdgeCases, and testCreateCollectionIndexValidationEdgeCases pack many unrelated invalid‑input scenarios into single methods. Functionally this works, but the methods are long and a bit hard to scan, and a single early failure prevents later cases from running.

Consider either:

• Splitting by concern (e.g., reserved keys vs. numeric ranges vs. defaults vs. array/format, and separate enum/index suites), or
• Grouping related payloads into small data‑driven helpers to keep the individual test bodies shorter.

Not urgent, but it would improve readability and future maintenance of this suite.

Also applies to: 7502-7605, 7607-7825

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

src/Appwrite/Utopia/Database/Validator/Attributes.php (1)

184-196: Verify single-bound min/max handling aligns with persistence layer.

The validator now correctly accepts single-bound constraints (only min or only max). Ensure buildAttributeDocument in Create.php persists whichever bound(s) are provided.

src/Appwrite/Utopia/Database/Validator/Attributes.php (1)

114-119: Consider defining reserved keys as a class constant.

Reserved keys are hardcoded inline. Extracting to a class constant would improve maintainability and reusability.

+    /**
+     * @var array<string> Reserved attribute keys that cannot be used
+     */
+    protected const RESERVED_KEYS = ['$id', '$createdAt', '$updatedAt', '$permissions', '$collection'];
+
     ...
-            $reservedKeys = ['$id', '$createdAt', '$updatedAt', '$permissions', '$collection'];
-            if (in_array($attribute['key'], $reservedKeys)) {
+            if (in_array($attribute['key'], self::RESERVED_KEYS)) {

src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Create.php (2)

227-231: Consider documenting the default string size.

The default size of 256 for strings is applied silently when not specified. Consider documenting this default in the parameter description or throwing an error since the validator requires size for string types.

The validator at lines 128-132 in Attributes.php requires size to be set for string types, so this fallback may be unreachable if validation runs first.

---

419-435: Consider adding error logging in cleanup.

Silently swallowing cleanup errors is appropriate to prevent cascading failures, but logging these errors would aid debugging in production.

     protected function cleanup(Database $dbForProject, string $databaseKey, string $collectionKey, string $collectionId): void
     {
         try {
             $dbForProject->deleteCollection($collectionKey);
-        } catch (\Throwable) {
-            // Ignore cleanup errors for collection deletion
+        } catch (\Throwable $e) {
+            // Log but don't throw cleanup errors
+            \error_log("Cleanup failed for collection $collectionKey: " . $e->getMessage());
         }

         try {
             $dbForProject->deleteDocument($databaseKey, $collectionId);
-        } catch (\Throwable) {
-            // Ignore cleanup errors for document deletion
+        } catch (\Throwable $e) {
+            // Log but don't throw cleanup errors
+            \error_log("Cleanup failed for document $collectionId: " . $e->getMessage());
         }
     }

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Create.php (5)

81-82: Consider aligning attributes/indexes param docs with validator capabilities

The attributes and indexes params are correctly wired to AttributesValidator / IndexesValidator, but the descriptions are narrower than what those validators accept (e.g., spatial types, relationship options like relatedCollection, relationType, twoWay, onDelete, numeric min/max). Consider expanding the docstrings so the HTTP/API surface matches the validator feature set and avoids documentation drift.

---

124-135: Relationship & spatial attribute handling: verify two‑way behavior and support flags

The attribute path is generally well-structured: you build collection/attribute documents up front, use buildAttributeDocument() to centralize type/format/default/options logic, validate relationship parents, and bail out early on errors while rolling back the collection document.

Two areas are worth a closer look:

1. For type === Database::VAR_RELATIONSHIP with twoWay enabled, this code only materializes the parent‑side attribute and immediately marks it status => 'available'. If there’s existing logic that backfills the inverse attribute based on new relationship attributes, please confirm it still triggers for attributes created synchronously here (and doesn’t rely on a 'processing' status). Otherwise, two‑way relationships defined at collection‑create time might never get their opposite side.

2. Spatial attributes are currently rejected whenever getAdapter()->getSupportForSpatialIndex() is false. If an adapter can store spatial attribute types but merely lacks spatial index support, this could be overly strict for attribute creation. It’s worth confirming that this flag is intended to gate spatial attribute support, not just spatial index support, or consider separating those concerns.

The isCollectionsAPI()‑based parent-not-found messaging is a nice touch to keep “collection” vs “table” terminology consistent across endpoints. Based on learnings, this aligns well with prior guidance.

Also applies to: 212-309

---

137-155: Index construction & validation are robust; double‑check limit semantics

The index pipeline looks good overall:

• You enforce a hard cap via getLimitForIndexes() before doing any heavier work.
• buildIndexDocument() ensures each index attribute either matches a declared attribute or one of the allowed system fields, rejects relationship attributes, and normalizes lengths / orders (esp. for array attributes via MAX_ARRAY_INDEX_LENGTH).
• You then delegate to IndexValidator with adapter-specific capabilities (getMaxIndexLength(), getInternalIndexesKeys(), spatial/vector support flags, etc.) and surface a contextual invalid-index exception.

Two follow-ups to verify:

• The limit check is based on count($indexes) only. If getLimitForIndexes() is intended to represent the total number of indexes per collection (user + internal), you may need to subtract the internal index count (from getInternalIndexesKeys()) to avoid rejecting valid configurations for adapters with many built‑in indexes.

• IndexValidator is instantiated per index with an empty indexes list ([]), so cross‑index conflicts (duplicate keys, incompatible combinations) depend entirely on IndexesValidator at the request layer. That’s fine if those two validators stay in lockstep, but if their rules ever diverge you might want to pass the already built $collectionIndexes into IndexValidator so it can enforce cross‑index constraints as well.

Also applies to: 316-397

---

132-135: Consider centralizing collection-document rollback logic

On most early error paths (attribute build failure, index count over limit, index build failure, createCollection failures), you manually call deleteDocument($databaseKey, $collection->getId()), while later failures use the dedicated cleanup() helper. To reduce duplication and the risk of future inconsistencies, consider routing these paths through a common helper (possibly cleanup() with an option to skip deleteCollection) so that all rollback logic lives in one place.

Also applies to: 139-141, 152-153, 165-175

---

178-193: Cleanup currently skips attributes/indexes docs; verify createDocuments atomicity

In the post‑creation phase you bulk insert attribute and index documents, and on failure call cleanup() to drop the underlying DB collection and the parent collection document. cleanup() deliberately ignores errors from deleteCollection / deleteDocument, which is reasonable for best‑effort rollback.

However, if createDocuments('attributes' | 'indexes', $docs) can partially succeed before throwing (e.g., per‑document inserts with a mid‑batch duplicate), this sequence will:

• Leave any already‑inserted attribute/index docs in place,
• Remove the parent collection document and drop the physical collection, and
• On retry, likely hit DuplicateExceptions on those orphan docs while the actual collection has been rolled back.

If createDocuments is guaranteed to be all‑or‑nothing at the adapter level, this is safe. Otherwise, consider extending the rollback here to also remove any attribute/index docs that were just created, for example by iterating $attributeDocuments / $indexDocuments and calling deleteDocument('attributes' | 'indexes', $doc->getId()) inside best‑effort try/catch blocks, in addition to the existing cleanup() call.

Also applies to: 405-425

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Learnt from: ItzNotABug
Repo: appwrite/appwrite PR: 9693
File: src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Indexes/Delete.php:85-86
Timestamp: 2025-05-10T04:12:13.037Z
Learning: The class `Appwrite\Platform\Modules\Databases\Http\Databases\Collections\Indexes\Action` defines a method `getGrantParentNotFoundException()` which is used to handle cases where a parent resource (like a database collection) is not found.

Learnt from: ItzNotABug
Repo: appwrite/appwrite PR: 9693
File: src/Appwrite/Platform/Modules/Databases/Http/Databases/Tables/Update.php:57-59
Timestamp: 2025-06-19T09:20:03.312Z
Learning: In table-related endpoints (such as `src/Appwrite/Platform/Modules/Databases/Http/Databases/Tables/Update.php`), parameter descriptions should use "table" and "row" terminology instead of "collection" and "document" for clarity and consistency.

tests/e2e/Services/Databases/Legacy/DatabasesCustomServerTest.php (1)

7294-8300: Extensive attribute/index/relationship validation matrix looks correct but is quite verbose

The new tests for attribute, enum, index, relationship, datetime, float, and “missing required fields” validation cover a wide range of edge cases and failure modes and match the stated goals of the PR. From a behaviour standpoint they look consistent and valuable.

If this area keeps growing, consider extracting small helpers (e.g. “createTestDatabase”, “postCollectionWithPayload”) or data-driven subtests to reduce repetition and make future additions easier to reason about, but this can be deferred.

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Learnt from: ItzNotABug
Repo: appwrite/appwrite PR: 9693
File: src/Appwrite/Platform/Modules/Databases/Http/Databases/Tables/Update.php:57-59
Timestamp: 2025-06-19T09:20:03.312Z
Learning: In table-related endpoints (such as `src/Appwrite/Platform/Modules/Databases/Http/Databases/Tables/Update.php`), parameter descriptions should use "table" and "row" terminology instead of "collection" and "document" for clarity and consistency.

Learnt from: ItzNotABug
Repo: appwrite/appwrite PR: 9693
File: src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Indexes/Delete.php:85-86
Timestamp: 2025-05-10T04:12:13.037Z
Learning: The class `Appwrite\Platform\Modules\Databases\Http\Databases\Collections\Indexes\Action` defines a method `getGrantParentNotFoundException()` which is used to handle cases where a parent resource (like a database collection) is not found.

src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Create.php (7)

82-90: Attributes/indexes params and action signature alignment

The new attributes/indexes params, validators, and the extended action() signature line up correctly with the constructor’s param/inject order, so wiring looks sound. One small UX improvement: the attributes description lists relationship as an allowed type, but inline relationship attributes are rejected later in the method. Consider updating the description to call this out explicitly to avoid surprises.

-            ->param('attributes', [], new ArrayList(new JSON(), APP_LIMIT_ARRAY_PARAMS_SIZE), 'Array of attribute definitions to create. Each attribute should contain: key (string), type (string: string, integer, float, boolean, datetime, relationship), size (integer, required for string type), required (boolean, optional), default (mixed, optional), array (boolean, optional), and type-specific options.', true)
+            ->param('attributes', [], new ArrayList(new JSON(), APP_LIMIT_ARRAY_PARAMS_SIZE), 'Array of attribute definitions to create. Each attribute should contain: key (string), type (string: string, integer, float, boolean, datetime, relationship), size (integer, required for string type), required (boolean, optional), default (mixed, optional), array (boolean, optional), and type-specific options. Relationship attributes cannot be created inline; use the create relationship endpoint instead.', true)

---

122-188: Attribute/index validation and document-building flow looks robust; minor duplication in cleanup

The pre-createCollection pipeline (computing keys, validating attributes, rejecting relationships, building attribute/index docs, and validating indexes against adapter capabilities) is coherent and ensures we bail out before touching the physical collection when definitions are bad. Repeated calls to deleteDocument($databaseKey, $collection->getId()) across several failure branches are correct but a bit duplicated; if this grows further, you might consider a small helper for “delete collection document only” to complement cleanup().

---

190-210: Consider using cleanup() for some createCollection failures to avoid orphaned collections

Passing $collectionKey, $collectionAttributes, and $collectionIndexes into createCollection() looks correct, and you now consistently drop the collection document on all exception paths. If any adapter can partially create the underlying collection (or indexes) before throwing IndexException, LimitException, or a generic \Throwable, you might end up with an orphaned physical collection since cleanup() isn’t invoked here.

If Database::createCollection() is guaranteed to be atomic and leave no collection behind on failure, this is fine. Otherwise, consider using cleanup() for the non-duplicate branches:

-        } catch (DuplicateException) {
-            $dbForProject->deleteDocument($databaseKey, $collection->getId());
-            throw new Exception($this->getDuplicateException());
-        } catch (IndexException $e) {
-            $dbForProject->deleteDocument($databaseKey, $collection->getId());
-            throw new Exception($this->getInvalidIndexException(), $e->getMessage());
-        } catch (LimitException) {
-            $dbForProject->deleteDocument($databaseKey, $collection->getId());
-            throw new Exception($this->getLimitException());
-        } catch (\Throwable $e) {
-            $dbForProject->deleteDocument($databaseKey, $collection->getId());
-            throw $e;
-        }
+        } catch (DuplicateException) {
+            $dbForProject->deleteDocument($databaseKey, $collection->getId());
+            throw new Exception($this->getDuplicateException());
+        } catch (IndexException $e) {
+            $this->cleanup($dbForProject, $databaseKey, $collectionKey, $collection->getId());
+            throw new Exception($this->getInvalidIndexException(), $e->getMessage());
+        } catch (LimitException) {
+            $this->cleanup($dbForProject, $databaseKey, $collectionKey, $collection->getId());
+            throw new Exception($this->getLimitException());
+        } catch (\Throwable $e) {
+            $this->cleanup($dbForProject, $databaseKey, $collectionKey, $collection->getId());
+            throw $e;
+        }

---

212-226: Bulk creation of attribute/index documents with centralized cleanup is a good improvement; verify deleteCollection cascades as expected

The two createDocuments calls, wrapped in a single try with cleanup() on any failure, give you a clear “all-or-nothing” behavior for the schema metadata. This looks solid. One thing to verify: that Database::deleteCollection($collectionKey) reliably removes any attributes/indexes state for this collection (or that such state can’t partially survive a failed bulk insert). If it doesn’t, you might still accumulate orphaned attribute/index docs even after cleanup.

---

228-230: Cache purge is correct; optional reuse of computed keys

Using purgeCachedDocument and purgeCachedCollection with the computed database/collection sequences matches how you build $databaseKey/$collectionKey earlier. For readability, you could reuse those variables here instead of re-concatenating the strings, but that’s purely cosmetic.

---

321-380: buildIndexDocument correctly normalizes array-attribute indexes; consider minor naming/efficiency polish

Index documents are built with normalized lengths and orders, and array attributes are given Database::MAX_ARRAY_INDEX_LENGTH with null order, which lines up with how array indexes are typically treated. Passing the same attribute Document instances that you later feed into IndexValidator keeps things consistent.

Two optional nits:

• The parameter name $attributeDocuments can be slightly confusing since you’re actually passing $collectionAttributes; renaming either the param or the local variable could improve readability.
• If you ever expect many attributes, you could pre-index them by key (assoc array) instead of doing array_search inside the loop, though this is likely premature optimization for most collections.

---

382-402: cleanup() helper is straightforward; parameter naming could be clearer

The cleanup helper safely attempts to drop the underlying DB collection and then the collection document, swallowing any errors. That’s appropriate for a best-effort rollback. The only minor nit is naming: $databaseId and $collectionId here are internal collection names ($databaseKey/$collectionKey), not the logical database/collection IDs. Renaming them (e.g., to $databaseCollectionId / $dbCollectionName) in a follow-up could make call sites and the docblock slightly clearer.

tests/e2e/Services/Databases/Legacy/DatabasesCustomServerTest.php (2)

7294-7521: Split testCreateCollectionAttributeValidationEdgeCases into smaller, focused tests

This method bundles many unrelated attribute edge cases (reserved keys, wrong default types, min/max issues, string length, signed on non‑numeric, arrays, invalid format, and a success case) into one large test. A single failure here will be hard to localize and may short‑circuit checks of later scenarios. Consider splitting into smaller tests (e.g., “ReservedKeys”, “DefaultTypeMismatch”, “RangeValidation”, “ArrayDefaults”, “FormatValidation”) so failures are isolated and easier to debug.

---

7628-7846: Index edge‑case test is comprehensive but monolithic

testCreateCollectionIndexValidationEdgeCases very usefully covers duplicate keys, invalid types, empty attributes, mismatched orders/lengths, invalid order values, and a valid compound index. For maintainability and clearer failure signals, it would be better to break this into multiple tests (e.g., one per invalid pattern plus one success test) rather than a single long method. This also makes it easier to extend with new edge cases later.

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Learnt from: ItzNotABug
Repo: appwrite/appwrite PR: 9693
File: src/Appwrite/Platform/Modules/Databases/Http/Databases/Tables/Update.php:57-59
Timestamp: 2025-06-19T09:20:03.312Z
Learning: In table-related endpoints (such as `src/Appwrite/Platform/Modules/Databases/Http/Databases/Tables/Update.php`), parameter descriptions should use "table" and "row" terminology instead of "collection" and "document" for clarity and consistency.

Learnt from: ItzNotABug
Repo: appwrite/appwrite PR: 9693
File: src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Indexes/Delete.php:85-86
Timestamp: 2025-05-10T04:12:13.037Z
Learning: The class `Appwrite\Platform\Modules\Databases\Http\Databases\Collections\Indexes\Action` defines a method `getGrantParentNotFoundException()` which is used to handle cases where a parent resource (like a database collection) is not found.

src/Appwrite/Utopia/Database/Validator/Indexes.php (1)

122-133: Consider extracting system attributes to a shared constant.

The system attributes ($id, $createdAt, $updatedAt) are defined inline. If these need to be referenced elsewhere or if new system attributes are added, a shared constant would improve maintainability.

+    // Consider defining at class level or using a shared constant
+    protected array $systemAttributes = ['$id', '$createdAt', '$updatedAt'];

Then reference it in the validation:

-            $systemAttrs = ['$id', '$createdAt', '$updatedAt'];
+            $systemAttrs = $this->systemAttributes;

src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Create.php (1)

165-167: Parameter naming mismatch.

buildIndexDocument is called with $collectionAttributes, but the method parameter is named $attributeDocuments. Both are arrays of Document objects, so it works functionally, but the naming is confusing.

Consider renaming the parameter for clarity:

-    protected function buildIndexDocument(Document $database, Document $collection, array $indexDef, array $attributeDocuments): array
+    protected function buildIndexDocument(Document $database, Document $collection, array $indexDef, array $collectionAttributes): array
     {
         ...
-        $attrKeys = array_map(fn ($a) => $a->getAttribute('key'), $attributeDocuments);
+        $attrKeys = array_map(fn ($a) => $a->getAttribute('key'), $collectionAttributes);

And update references in lines 343-344.

tests/e2e/Services/Databases/Legacy/DatabasesCustomServerTest.php (1)

6960-7131: Consider asserting error “type” codes for negative multi-create cases

The error-path scenarios in testCreateCollectionWithAttributesAndIndexesErrors are valuable, but they currently only assert HTTP 400 (and, for the relationship case, a substring of body['message']). To make these tests more resilient to copy updates and to better document expected failure modes, consider also asserting the structured error code in body['type'] for at least the key cases (invalid attribute type, bad index attributes, relationship payload).

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Learnt from: ItzNotABug
Repo: appwrite/appwrite PR: 9693
File: src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Indexes/Delete.php:85-86
Timestamp: 2025-05-10T04:12:13.037Z
Learning: The class `Appwrite\Platform\Modules\Databases\Http\Databases\Collections\Indexes\Action` defines a method `getGrantParentNotFoundException()` which is used to handle cases where a parent resource (like a database collection) is not found.