iframe-proxy

HarshMN2345 · 2026-09-22T09:25:24Z

What does this PR do?

The relationship column update was the only one of the 18 TablesDB column update routes shaped columns/:key/<type> instead of columns/<type>/:key. This makes it match its siblings and keeps the shipped path working as an httpAlias, the same treatment the equivalent Databases attributes route already has.

Correction to an earlier version of this description. It claimed this also fixed the collision where a column literally named relationship was unreachable via its typed route. It does not, and I should not have asserted that without checking. Verified empirically by registering every real route and calling Router::match directly:

Router::match enumerates param-position combinations in first-registered order and takes the first hit. The alias /columns/:key/relationship has params at indices {2,4,6}; /columns/string/:key has {2,4,7}. Index 6 is registered first (by account.php's /v1/account/sessions/oauth2/callback/:provider/:projectId, before any module loads), so the alias always wins. That is unfixable here while both paths must remain routable — the two shapes are genuinely ambiguous for exactly the key relationship, and the alias has to stay because TablesDB already shipped with it.

So this is a consistency fix only: canonical path added, legacy path preserved.

Test Plan

composer lint on all changed files → {"tool":"pint","result":"passed"} each.
Router-level before/after as tabulated above.
The e2e URL helper no longer special-cases relationship, so the existing relationship PATCH assertions in DatabasesBase now exercise the canonical path. MigrationsBase keeps PATCHing the old path as a raw literal, covering the alias.
Added testUpdateRelationshipAttributeAliasPrecedence, which pins the collision as it actually behaves (the typed route 404s for a column named relationship) so a future change to alias precedence is a visible test failure rather than a silent shift.

Not run: no e2e executed here (no Docker stack). CI should cover tests/e2e/Services/TablesDB and tests/e2e/Services/Databases with --filter=testOneToManyRelationship, plus tests/e2e/Services/Migrations --filter=Relationship.

Related PRs and Issues

Fixes 🐛 Bug Report: Update Relationship endpoint is inconsistent #9022

Checklist

Have you read the Contributing Guidelines on issues?
If the PR includes a change to an API's metadata (desc, label, params, etc.), does it also include updated API specs and example docs? — the route path changes, so specs need regenerating before merge

…p/:key The relationship column update was the only one of the 18 column update routes shaped columns/:key/<type> instead of columns/<type>/:key. Make it match its siblings and keep the shipped path working as an httpAlias, the same treatment the equivalent Databases attributes route already has. The e2e URL helper no longer special-cases relationship, so the existing relationship PATCH assertions in DatabasesBase now exercise the canonical path on the TablesDB suites. MigrationsBase keeps PATCHing the old path as a raw literal, which now covers the alias.

greptile-apps · 2026-09-22T09:28:30Z

Confidence Score: 4/5

The route change itself appears compatible, but the implementation-coupled regression test violates an explicit repository testing requirement and should be replaced before merging.

Findings

Test Freezes Broken Routing ▶
Routing Regression Lacks Coverage ▶

Fix with agent prompt

### Issue 1
tests/e2e/Services/Databases/DatabasesBase.php:1865-1878
This test hard-codes the router’s current first-registration precedence and requires the known broken 404 response instead of testing an API contract users should rely on. It would fail if routing were improved so the string-column update succeeded, while passing as long as this defect remains. The repository requires tests to assert observable behavior rather than mirror implementation details, so this test must be replaced before merging with compatibility coverage for the canonical relationship route and legacy alias.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

### Issue 2
tests/e2e/Traits/DatabasesUrlHelpers.php:27-33
The updated tests exercise the canonical relationship route and its legacy alias, but not the collision this change is meant to fix. Add an observable E2E test that creates a string column keyed `relationship`, PATCHes `/columns/string/relationship`, and verifies that the string-column update persisted. Without this coverage, the alias could regain precedence and dispatch the request to the wrong action without a test failing.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Summary

This PR makes /columns/relationship/:key the canonical TablesDB relationship-column update path while retaining the shipped /:key/relationship path as an HTTP alias.

Simplifies the shared URL helper to use the standard type-then-key ordering.
Keeps migration coverage on the legacy alias.
Adds a regression test documenting the existing route collision, but that test improperly locks in internal router precedence and broken behavior.

_{Reviews (3) · Last reviewed commit: "Merge branch 'main' into fix/issue-9022"}

greptile-apps · 2026-09-22T09:28:33Z

        $resource = $this->getSchemaResource();
        $base = "{$this->getContainerUrl($databaseId, $containerId)}/{$resource}";
-        // For relationship updates, the URL pattern is /attributes/{key}/relationship
-        // For other attribute updates, the URL pattern is /attributes/{type}/{key}
-        if ($type === 'relationship' && $key) {
-            $base .= "/{$key}/{$type}";
-        } else {
-            if ($type) {
-                $base .= "/{$type}";
-            }
-            if ($key) {
-                $base .= "/{$key}";
-            }
+        if ($type) {
+            $base .= "/{$type}";
+        }
+        if ($key) {
+            $base .= "/{$key}";


Routing Regression Lacks Coverage

The updated tests exercise the canonical relationship route and its legacy alias, but not the collision this change is meant to fix. Add an observable E2E test that creates a string column keyed relationship, PATCHes /columns/string/relationship, and verifies that the string-column update persisted. Without this coverage, the alias could regain precedence and dispatch the request to the wrong action without a test failing.

Prompt To Fix With AI

This is a comment left during a code review. Path: tests/e2e/Traits/DatabasesUrlHelpers.php Line: 27-33 Comment: **Routing Regression Lacks Coverage** The updated tests exercise the canonical relationship route and its legacy alias, but not the collision this change is meant to fix. Add an observable E2E test that creates a string column keyed `relationship`, PATCHes `/columns/string/relationship`, and verifies that the string-column update persisted. Without this coverage, the alias could regain precedence and dispatch the request to the wrong action without a test failing. --- For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

github-actions · 2026-09-22T09:33:54Z

✨ Benchmark results

Comparing main (before) → fix/issue-9022 (after).

Metric	Before	After	Change
🚀 Requests/sec	`205.42`	`207.25`	⚪ `+0.9%`
⏱️ Latency P50	`83.75 ms`	`83.2 ms`	⚪ `-0.7%`
⏱️ Latency P95	`198.87 ms`	`197.46 ms`	⚪ `-0.7%`

Per-scenario breakdown & investigation details

Metrics below reflect the current branch (after). Δ P95 compares against the base.

Scenario	P50 (ms)	P95 (ms)	Requests	RPS	Δ P95 (ms)
API total	`83.2`	`197.46`	`12,939`	`207.25`	`-1.41`
Account	`162.1`	`310.1`	`681`	`11.31`	`-2.63`
TablesDB	`79.64`	`141.26`	`7,037`	`114.81`	`-9.85`
Storage	`76.74`	`165.79`	`3,405`	`57.48`	`-7.38`
Functions	`125.66`	`250.78`	`1,816`	`31.3`	`+1.13`

Top API waits (after)

API request	Max wait (ms)
functions.delete	`614.2`
account.name.update	`589.92`
account.prefs.update	`479.49`
functions.create	`441.2`
functions.variables.update	`423.28`

…routes A column keyed `relationship` is the one key where the legacy `columns/:key/relationship` alias and the typed `columns/<type>/:key` routes describe the same URL. The router resolves that URL through the alias, so the request lands on the relationship update keyed `string` rather than the string update keyed `relationship`. Nothing covered that overlap, so a change to either path could silently move the dispatch. Assert the resolution both surfaces actually have.

greptile-apps · 2026-09-22T12:21:22Z

+        $response = $this->client->call(Client::METHOD_PATCH, $this->getSchemaUrl($databaseId, $containerId, 'string', 'relationship'), array_merge([
+            'content-type' => 'application/json',
+            'x-appwrite-project' => $this->getProject()['$id'],
+            'x-appwrite-key' => $this->getProject()['apiKey'],
+        ]), [
+            'required' => false,
+            'default' => 'plain',
+        ]);
+
+        // The legacy `/:key/relationship` alias outranks the typed route, so the request
+        // reaches the relationship update keyed `string` instead of the string update
+        // keyed `relationship`. Dropping the alias is what would flip this to a 200.
+        $this->assertEquals(404, $response['headers']['status-code']);
+        $this->assertEquals($this->getSchemaParam() . '_not_found', $response['body']['type']);


Test Freezes Broken Routing

This test hard-codes the router’s current first-registration precedence and requires the known broken 404 response instead of testing an API contract users should rely on. It would fail if routing were improved so the string-column update succeeded, while passing as long as this defect remains. The repository requires tests to assert observable behavior rather than mirror implementation details, so this test must be replaced before merging with compatibility coverage for the canonical relationship route and legacy alias.

Context Used: Call out and harshly judge implementation-coupled tests. We don't mirror source code, configuration, or version pins in assertions. We test observable behavior; use linters for syntax and schema checks. (source)

Prompt To Fix With AI

This is a comment left during a code review. Path: tests/e2e/Services/Databases/DatabasesBase.php Line: 1865-1878 Comment: **Test Freezes Broken Routing** This test hard-codes the router’s current first-registration precedence and requires the known broken 404 response instead of testing an API contract users should rely on. It would fail if routing were improved so the string-column update succeeded, while passing as long as this defect remains. The repository requires tests to assert observable behavior rather than mirror implementation details, so this test must be replaced before merging with compatibility coverage for the canonical relationship route and legacy alias. **Context Used:** Call out and harshly judge implementation-coupled tests. We don't mirror source code, configuration, or version pins in assertions. We test observable behavior; use linters for syntax and schema checks. ([source](https://app.greptile.com/review/custom-context?memory=instruction-0)) --- For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

greptile-apps Bot reviewed Sep 22, 2026

View reviewed changes

HarshMN2345 and others added 2 commits September 22, 2026 16:25

Merge branch 'main' into fix/issue-9022

1efecf3

greptile-apps Bot reviewed Sep 22, 2026

View reviewed changes

ArnabChatterjee20k approved these changes Sep 22, 2026

View reviewed changes

HarshMN2345 merged commit 49d74cc into main Sep 22, 2026
52 checks passed

ChiragAgg5k mentioned this pull request Sep 23, 2026

chore: prepare 2.3.0 release #13857

Merged

Sunbelt Computer Software

PL/B Language Development and Support

Navigation Menu

Repository navigation

fix(tablesdb): move relationship column update to columns/relationship/:key - #13822

fix(tablesdb): move relationship column update to columns/relationship/:key#13822
HarshMN2345 merged 3 commits into
mainfrom
fix/issue-9022

HarshMN2345 commented Sep 22, 2026 •

edited

Loading

Uh oh!

greptile-apps Bot commented Sep 22, 2026 •

edited

Loading

Uh oh!

greptile-apps Bot Sep 22, 2026

Uh oh!

github-actions Bot commented Sep 22, 2026 •

edited

Loading

Uh oh!

greptile-apps Bot Sep 22, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Path	Before	After
`/columns/string/relationship`	relationship update, `key="string"`	unchanged
`/columns/relationship/:key`	no match	relationship update, `key`
`/columns/:key/relationship`	relationship update, `key`	unchanged

Sunbelt Computer Software

PL/B Language Development and Support

Conversation

HarshMN2345 commented Sep 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Test Plan

Related PRs and Issues

Checklist

Uh oh!

greptile-apps Bot commented Sep 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Confidence Score: 4/5

Findings

Summary

Uh oh!

greptile-apps Bot Sep 22, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Sep 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✨ Benchmark results

Uh oh!

greptile-apps Bot Sep 22, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

HarshMN2345 commented Sep 22, 2026 •

edited

Loading

greptile-apps Bot commented Sep 22, 2026 •

edited

Loading

github-actions Bot commented Sep 22, 2026 •

edited

Loading