Skip to content

Joins v2#11121

Open
fogelito wants to merge 186 commits into1.9.xfrom
joins2
Open

Joins v2#11121
fogelito wants to merge 186 commits into1.9.xfrom
joins2

Conversation

@fogelito
Copy link
Contributor

What does this PR do?

(Provide a description of what this PR does and why it's needed.)

Test Plan

(Write your test plan here. If you changed any code, please provide us with clear instructions on how you verified your changes work. Screenshots may also be helpful.)

Related PRs and Issues

  • (Related PR or issue)

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?

# Conflicts:
#	app/init/constants.php
#	composer.json
#	composer.lock
#	src/Appwrite/Migration/Migration.php
#	src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/XList.php
#	src/Appwrite/Platform/Modules/Databases/Http/DocumentsDB/Collections/Documents/Logs/XList.php
#	src/Appwrite/Platform/Modules/Databases/Http/DocumentsDB/Collections/Logs/XList.php
#	src/Appwrite/Platform/Modules/Databases/Http/VectorsDB/Collections/Create.php
#	src/Appwrite/Platform/Modules/Databases/Http/VectorsDB/Collections/Logs/XList.php
#	src/Appwrite/Utopia/Database/Validator/Queries/Base.php
@blacksmith-sh

This comment has been minimized.

@fogelito fogelito closed this Mar 23, 2026
@fogelito fogelito reopened this Mar 23, 2026
@greptile-apps
Copy link
Contributor

greptile-apps bot commented Mar 23, 2026

Greptile Summary

This PR implements "Joins v2" — a new cross-collection join query feature for the Databases API — along with a breaking change to the Query::select() signature (single attribute per call instead of an array), a new V22 backward-compatibility request filter, and a version bump to 1.10.0. The core join logic (resolving join collection IDs, registering them with the database adapter) is well-structured, and the defensive guards added to app/init/database/filters.php are a clean improvement. However, several blocking issues prevent this from being merge-ready:

  • Development branch dependencies: composer.json pins utopia-php/database to dev-joins8 and utopia-php/migration to dev-joins. These must be replaced with stable tagged releases before the PR can land on 1.8.x.
  • Empty PHP files committed: app/controllers/api/teams.php, src/Appwrite/Platform/Modules/Databases/Http/DocumentsDB/Collections/Documents/Logs/XList.php, src/Appwrite/Platform/Modules/Databases/Http/DocumentsDB/Collections/Logs/XList.php, src/Appwrite/Platform/Modules/Databases/Http/VectorsDB/Collections/Create.php, and src/Appwrite/Platform/Modules/Databases/Http/VectorsDB/Collections/Logs/XList.php are all 0-byte files. These will cause fatal errors or silent route omissions at runtime.
  • phpunit.xml stopOnFailure/stopOnError set to true: Halts the suite at the first failure and masks subsequent broken tests in CI.
  • Types.php hardcodes Database::VAR_INTEGER: The DocumentsValidator is constructed with an integer ID type regardless of the active adapter, which is already known to break MongoDB sequence queries (as acknowledged by the adapter-conditional assertion in testQueryBySequenceType).
  • V22 filter silently injects internal attributes: expandSelects appends $id, $sequence, $permissions, $createdAt, $updatedAt for every namespace level when any non-wildcard field is selected, potentially exposing fields the caller did not request.
  • Test retry count reduced: Scope.php drops $maxRetries from 5 to 1, increasing flakiness under parallel test execution.

Confidence Score: 1/5

  • Not safe to merge — development-branch dependencies, multiple empty PHP files, and hardcoded adapter-specific logic are blocking issues.
  • Multiple P0 blockers are present: the entire dependency tree for utopia-php/database and utopia-php/migration points at untagged development branches, five committed PHP files are completely empty (which will cause runtime fatals or silent route loss), and phpunit.xml is configured to hide downstream test failures. The new join feature logic itself looks architecturally sound, but the PR is clearly in a work-in-progress state and is not ready for the 1.8.x production branch.
  • composer.json (dev deps), app/controllers/api/teams.php (empty), the four empty files under Databases/Http/DocumentsDB and VectorsDB, phpunit.xml (stop-on-failure), and src/Appwrite/Utopia/Database/Validator/Queries/Types.php (hardcoded VAR_INTEGER).

Important Files Changed

Filename Overview
composer.json Both utopia-php/database and utopia-php/migration are pinned to in-progress dev-joins8 / dev-joins branches instead of stable releases — must not be merged to production.
app/controllers/api/teams.php New file that is completely empty (0 bytes / no PHP tag); will silently omit all team routes or break autoloading at runtime.
src/Appwrite/Platform/Modules/Databases/Http/DocumentsDB/Collections/Documents/Logs/XList.php Completely empty file (along with three other sibling empty PHP files); will cause class-not-found errors if any code references these expected classes.
phpunit.xml Changed stopOnFailure and stopOnError from false to true, which halts the test suite at the first failure and hides any subsequent broken tests in CI.
src/Appwrite/Utopia/Database/Validator/Queries/Types.php New validator class replacing the old Queries wrapper; correctly gates allowed query types but hardcodes Database::VAR_INTEGER for the DocumentsValidator, which may break MongoDB sequence-based queries.
src/Appwrite/Utopia/Database/Validator/Queries/Base.php Refactored to extend the new Types class and use QueryContext; attribute handling is cleaner, internal attributes are correctly appended to both the context document and the allowed-attributes list.
src/Appwrite/Utopia/Request/Filters/V22.php New backward-compat filter that converts old multi-value select(["a","b"]) into per-attribute select("a") calls; expandSelects silently injects all five internal attributes at every namespace level, which may expose fields the caller did not request.
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/XList.php Added join query support: fetches and validates the join collection, maps it to its internal table ID, and registers it with addJoinCollection. Authorization is correctly skipped for the lookup but access is gated on enabled + API key / privileged user checks.
src/Appwrite/Migration/Migration.php Version 1.10.0 maps to the same V24 migration as 1.9.0 — no new migration class created. Intentional only if the joins feature requires no schema changes.
tests/e2e/Scopes/Scope.php Reduced test user creation max retries from 5 to 1; increases flakiness risk under parallel test execution where ID collisions are explicitly expected.

Comments Outside Diff (4)

  1. app/controllers/api/teams.php, line 1 (link)

    P0 Empty PHP file will break autoloading / routing

    This file is completely empty (0 bytes). If the framework scans or includes app/controllers/api/teams.php for route definitions, an empty file will silently omit all team-related routes. This appears to be an accidental commit — if teams routes have been migrated to the new module structure, this placeholder file should either contain a <?php stub with a comment, or not be committed at all.

  2. src/Appwrite/Platform/Modules/Databases/Http/DocumentsDB/Collections/Documents/Logs/XList.php, line 1 (link)

    P0 Multiple empty PHP files committed

    This file (and the following sibling files) are completely empty:

    • src/Appwrite/Platform/Modules/Databases/Http/DocumentsDB/Collections/Documents/Logs/XList.php
    • src/Appwrite/Platform/Modules/Databases/Http/DocumentsDB/Collections/Logs/XList.php
    • src/Appwrite/Platform/Modules/Databases/Http/VectorsDB/Collections/Create.php
    • src/Appwrite/Platform/Modules/Databases/Http/VectorsDB/Collections/Logs/XList.php

    PHP files without a <?php opening tag are essentially dead code. Any class autoloader targeting these paths will either throw a fatal parse error or silently skip the expected class definition, causing ClassNotFoundException at runtime. These appear to be placeholders for not-yet-implemented features and should not be committed until they have actual implementations.

  3. tests/e2e/Scopes/Scope.php, line 1894 (link)

    P1 Retry count reduced from 5 to 1 may cause flaky tests

    Lowering $maxRetries from 5 to 1 means test user creation will be attempted only once. Under parallel execution (which the comment above explicitly mentions), this significantly increases the chance of a collision-based failure causing a test to fail without any retry. The original value of 5 was likely chosen to handle such races.

  4. src/Appwrite/Migration/Migration.php, line 657 (link)

    P1 Version 1.10.0 reuses the existing V24 migration class

    1.10.0 is mapped to V24, the same migration class used for 1.9.0. If the Joins v2 feature requires any schema changes (new collection indexes, new attributes, column type changes), those won't be applied during the upgrade path from ≤ 1.9.0 → 1.10.0 because the migration already ran for 1.9.0. Please confirm this is intentional and that no schema migration is needed for the joins feature.

Reviews (1): Last reviewed commit: "lock" | Re-trigger Greptile

@@ -61,7 +61,7 @@
"utopia-php/compression": "0.1.*",
"utopia-php/config": "1.*",
"utopia-php/console": "0.1.*",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P0 Dev-branch dependencies must not be merged

Both utopia-php/database and utopia-php/migration are pointing at in-progress development branches (dev-joins8 and dev-joins) rather than stable tagged releases. This is expected during active development, but these must be replaced with proper stable semver releases before this PR can be merged into the 1.8.x (production) branch.

"utopia-php/database": "dev-joins8 as 5.3.15",
"utopia-php/migration": "dev-joins as 1.8.0",

The composer.lock also reflects "minimum-stability": "dev" and explicit dev stability flags, which would carry through to any environment that installs from this lockfile.

Comment on lines 6 to 7
colors="true"
processIsolation="false"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 stopOnFailure/stopOnError set to true hides downstream test failures

Setting these to true means the test runner halts at the first failing assertion, so any subsequent broken tests will never be reported in CI. This makes it harder to get a complete picture of test health in a PR. The prior false values allowed all tests to run to completion so every failure was visible at once.

If this is intentional (e.g. to speed up a specific debug run), it should be reverted before merging.

Suggested change
colors="true"
processIsolation="false"
stopOnFailure="false"
stopOnError="false"

@fogelito fogelito changed the base branch from 1.8.x to 1.9.x March 23, 2026 15:25
@blacksmith-sh

This comment has been minimized.

@blacksmith-sh

This comment has been minimized.

@blacksmith-sh

This comment has been minimized.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants