Skip to content

(fix): guard against missing Host header in dispatch#11710

Merged
abnegate merged 3 commits into1.9.xfrom
fix-installer
Mar 31, 2026
Merged

(fix): guard against missing Host header in dispatch#11710
abnegate merged 3 commits into1.9.xfrom
fix-installer

Conversation

@abnegate
Copy link
Copy Markdown
Member

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?

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Mar 31, 2026

📝 Walkthrough

Walkthrough

Adds an optional database migration step to the installer and upgrade flow. Introduces installer step 6 with new UI template, CSS, and JS wiring; extends installer step IDs, step-clamping, and progress handling. The installer POST input now accepts a boolean migrate flag which is forwarded to backend tasks. Installer and upgrade tasks accept and persist the migrate parameter; the install task implements a new migration runner that executes appwrite migrate via Docker, reports progress, and errors on non-zero exit. Also fixes a domain-extraction undefined-offset in app/http.php.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~50 minutes

🚥 Pre-merge checks | ❌ 3

❌ Failed checks (2 warnings, 1 inconclusive)

Check name Status Explanation Resolution
Title check ⚠️ Warning The title focuses on guarding against missing Host header in dispatch, but the changeset primarily adds database migration functionality to the installer and upgrade tasks, with only one file (app/http.php) addressing the titled concern. Update the title to reflect the main changes: adding database migration feature to installer/upgrade flow, or separate migration-related changes into a distinct PR.
Docstring Coverage ⚠️ Warning Docstring coverage is 7.69% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check ❓ Inconclusive The PR description only contains the contribution template with empty sections and does not explain what the changes do, why they are needed, or how they were tested. Provide a meaningful description explaining the database migration feature, its purpose, implementation details, and test coverage for the substantial changes across 14 files.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix-installer

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Mar 31, 2026

Greptile Summary

This PR delivers two distinct changes: the headline fix guarding against a missing Host header in the Swoole dispatch function (app/http.php), and a substantial installer enhancement that adds a new "Database migration" step (step 6) to the upgrade flow, letting users choose whether to run docker compose exec appwrite migrate automatically as part of the upgrade.

Key changes:

  • app/http.php: Adds ?? '' null-coalescing guard on the Host-header array access so requests without a Host: line no longer cause an undefined-index notice/fatal.
  • src/Appwrite/Platform/Tasks/Install.php / Upgrade.php: New runDatabaseMigration method and --migrate flag wire the optional migration step into performInstallation.
  • Installer UI (JS/PHP/CSS): New step 6 card with a toggle, updated step flows, clamp values, and progress-tracking step definitions.
  • tests/: Minimal assertion update to include the new migrate param.

One issue found: When migration fails, runDatabaseMigration sends a detailed STATUS_ERROR SSE event before throwing, but the outer catch block in performInstallation then sends a second, less-informative STATUS_ERROR event for the same step. This could cause the frontend to display a generic error message rather than the actual migration output. See inline comment for a simple fix.

Confidence Score: 5/5

Safe to merge; the core bug fix is correct and the only remaining finding is a minor double-SSE-event on migration failure that does not affect data integrity.

The headline fix (?? '' guard on the Host header) is correct and low-risk. All installer additions are gated behind the upgrade flow. The one issue found (double updateProgress call on migration error) is a UX/display concern rather than a correctness or data-loss problem, and the migration command itself is not affected. All other changes are straightforward wiring and UI additions.

src/Appwrite/Platform/Tasks/Install.php — double STATUS_ERROR event on migration failure.

Important Files Changed

Filename Overview
app/http.php Core fix: adds ?? '' null-coalescing guard on the Host header array access so a request arriving without a Host: line no longer causes an undefined-index notice/error in the dispatch function.
src/Appwrite/Platform/Tasks/Install.php Adds runDatabaseMigration method and wires it into performInstallation. On failure the method sends a STATUS_ERROR SSE event internally before throwing, but the outer catch block unconditionally sends a second STATUS_ERROR event for the same step, potentially overwriting the more-detailed first event with a generic message.
src/Appwrite/Platform/Tasks/Upgrade.php Adds --migrate flag (defaults to true) to the upgrade CLI command and stores it in the shared $migrate property for use by performInstallation.
src/Appwrite/Platform/Installer/Http/Installer/Install.php Adds migrate boolean parameter to the installer HTTP action and forwards it to performInstallation.
app/views/install/installer/templates/steps/step-6.phtml New step-6 card template with a toggle for "run migration automatically" and a manual migration hint.

Reviews (2): Last reviewed commit: "(refactor): rename migrate param and add..." | Re-trigger Greptile

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
app/views/install/installer/js/installer.js (1)

15-27: ⚠️ Potential issue | 🟠 Major

Normalize step mapping is now inconsistent with the server remap rules.

Line 15 introduced a non-monotonic flow ([1, 6, 4, 5]), but Line 22 still applies “first greater step” logic. In upgrade mode, step=2/3 normalizes to 6 client-side, while server-side mapping resolves those to 4, causing panel/init/action-bar desync.

🔧 Suggested fix (align client normalization with server mapping)
-    const normalizeStep = (step) => {
-        const numeric = clampStep(step);
-        if (stepFlow.includes(numeric)) return numeric;
-        if (numeric <= stepFlow[0]) return stepFlow[0];
-        for (let i = 0; i < stepFlow.length; i += 1) {
-            if (numeric < stepFlow[i]) {
-                return stepFlow[i];
-            }
-        }
-        return stepFlow[stepFlow.length - 1];
-    };
+    const normalizeStep = (step) => {
+        const numeric = Number(step);
+        if (Number.isNaN(numeric)) return stepFlow[0];
+
+        // Keep client behavior aligned with server-side View remapping.
+        if (isUpgrade && (numeric === 2 || numeric === 3)) return 4;
+        if (!isUpgrade && numeric === 6) return 4;
+
+        const bounded = Math.max(1, Math.min(6, numeric));
+        if (stepFlow.includes(bounded)) return bounded;
+        return stepFlow[0];
+    };
 
-    const clampStep = (step) => Math.max(1, Math.min(6, step));
+    const clampStep = (step) => {
+        const numeric = Number(step);
+        if (Number.isNaN(numeric)) return 1;
+        return Math.max(1, Math.min(6, numeric));
+    };

Also applies to: 56-56

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@app/views/install/installer/js/installer.js` around lines 15 - 27, The
normalizeStep logic assumes stepFlow is monotonic; when stepFlow is
non-monotonic (e.g. [1,6,4,5]) the "first greater step" logic desynchronizes
from server mapping — update normalizeStep to compute the mapping using a sorted
version of stepFlow so you pick the nearest greater-or-equal step in numeric
order instead of array order: use clampStep(step) as before, create a
sortedStepFlow = [...stepFlow].sort((a,b)=>a-b), then apply the greater-or-equal
logic against sortedStepFlow (return sortedStepFlow[0] if numeric <= first,
iterate sortedStepFlow to find first >= numeric, else return last); also apply
the same change where the same logic appears (the other occurrence around the
later block referenced in the comment). Ensure you still call clampStep and
preserve existing boundary behavior.
🧹 Nitpick comments (1)
src/Appwrite/Platform/Installer/Http/Installer/Install.php (1)

46-46: Consider importing Boolean validator for consistency.

Other validators (Range, Text, WhiteList, Nullable) are imported via use statements, but Boolean uses a fully qualified namespace. For consistency:

♻️ Suggested fix

Add the import at the top of the file with the other validators:

use Utopia\Validator\Boolean;

Then update line 46:

-            ->param('runMigration', false, new \Utopia\Validator\Boolean(true), 'Run database migration after upgrade', true)
+            ->param('runMigration', false, new Boolean(true), 'Run database migration after upgrade', true)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/Appwrite/Platform/Installer/Http/Installer/Install.php` at line 46,
Replace the fully-qualified Boolean validator usage with an import for
consistency: add a use Utopia\Validator\Boolean; alongside the other validators
at the top of the file, then update the param call that currently constructs new
\Utopia\Validator\Boolean(true) (the ->param('runMigration', ...) call) to use
new Boolean(true) instead.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@app/http.php`:
- Around line 105-107: In dispatch(), the Host header parsing (currently using
$lines[1] and trim(explode('Host: ', ...)[1] ?? '') ) is brittle and
case/order-sensitive; fix it by iterating over $lines (or the parsed headers
array), performing a case-insensitive match for the header name (e.g.,
strcasecmp or preg_match for '^Host\s*:'), extracting the header value after the
first colon, trimming whitespace, and stripping any port portion (split on ':'
from the right or parse_url style) before assigning to $domain so $domain
reliably contains the hostname regardless of header order, case, or port. Ensure
this logic lives in dispatch() where $lines and $domain are handled and still
guards against missing headers by defaulting to an empty string.

---

Outside diff comments:
In `@app/views/install/installer/js/installer.js`:
- Around line 15-27: The normalizeStep logic assumes stepFlow is monotonic; when
stepFlow is non-monotonic (e.g. [1,6,4,5]) the "first greater step" logic
desynchronizes from server mapping — update normalizeStep to compute the mapping
using a sorted version of stepFlow so you pick the nearest greater-or-equal step
in numeric order instead of array order: use clampStep(step) as before, create a
sortedStepFlow = [...stepFlow].sort((a,b)=>a-b), then apply the greater-or-equal
logic against sortedStepFlow (return sortedStepFlow[0] if numeric <= first,
iterate sortedStepFlow to find first >= numeric, else return last); also apply
the same change where the same logic appears (the other occurrence around the
later block referenced in the comment). Ensure you still call clampStep and
preserve existing boundary behavior.

---

Nitpick comments:
In `@src/Appwrite/Platform/Installer/Http/Installer/Install.php`:
- Line 46: Replace the fully-qualified Boolean validator usage with an import
for consistency: add a use Utopia\Validator\Boolean; alongside the other
validators at the top of the file, then update the param call that currently
constructs new \Utopia\Validator\Boolean(true) (the ->param('runMigration', ...)
call) to use new Boolean(true) instead.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 7c62fa0b-3076-4293-b011-5b6eebb4864a

📥 Commits

Reviewing files that changed from the base of the PR and between ec20fb5 and 2f53d09.

📒 Files selected for processing (13)
  • app/http.php
  • app/views/install/installer.phtml
  • app/views/install/installer/css/styles.css
  • app/views/install/installer/js/installer.js
  • app/views/install/installer/js/modules/context.js
  • app/views/install/installer/js/modules/progress.js
  • app/views/install/installer/js/steps.js
  • app/views/install/installer/templates/steps/step-6.phtml
  • src/Appwrite/Platform/Installer/Http/Installer/Install.php
  • src/Appwrite/Platform/Installer/Http/Installer/View.php
  • src/Appwrite/Platform/Installer/Server.php
  • src/Appwrite/Platform/Tasks/Install.php
  • tests/unit/Platform/Modules/Installer/ModuleTest.php

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🧹 Nitpick comments (1)
app/views/install/installer/js/steps.js (1)

338-348: Dispatch the seeded migrate value once.

Other steps push their initial value through dispatchStateChange() immediately. Here the default checkbox state only mutates formState, so any listener that persists state or reacts to installer:state-change will miss migrate=true until the user toggles the control.

Suggested tweak
         if (checkbox) {
             if (formState.migrate !== undefined) {
                 checkbox.checked = formState.migrate;
             } else {
                 formState.migrate = checkbox.checked;
+                dispatchStateChange?.('migrate');
             }
             checkbox.addEventListener('change', () => {
                 formState.migrate = checkbox.checked;
                 dispatchStateChange?.('migrate');
             });
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@app/views/install/installer/js/steps.js` around lines 338 - 348, The code
sets formState.migrate from the checkbox default but doesn't notify listeners;
after the branch that assigns formState.migrate = checkbox.checked (the else
branch), invoke dispatchStateChange?.('migrate') so the initial seeded value is
dispatched once; keep the existing change listener on checkbox for subsequent
updates and only call dispatchStateChange when you set the seeded value (in the
block that runs when formState.migrate was undefined).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/Appwrite/Platform/Tasks/Install.php`:
- Around line 777-789: The exec() call running the migration ($command, $output,
$exit) must be bounded with the same proc_open() + procCloseWithTimeout()
pattern used elsewhere to avoid hangs; replace the raw exec() invocation with
proc_open to start the process, capture stdout/stderr, then call
procCloseWithTimeout() to enforce a timeout and obtain the exit code, and on
timeout or non-zero exit use updateProgress (InstallerServer::STEP_MIGRATION,
InstallerServer::STATUS_ERROR) with the captured output and throw the
RuntimeException as before; ensure you reuse the same timeout/closing helper and
preserve the existing messageOverride/details handling.
- Around line 773-779: In runDatabaseMigration(), replace the bare docker
compose exec invocation with the same explicit compose context used by
runDockerCompose(): add the -f <compose-file> flag, --project-name
<project-name>, and --project-directory <project-directory> arguments and
include -T to disable TTY allocation so the command runs non-interactively;
build the command using the same variables or helpers that runDockerCompose()
uses (e.g., the compose file path, project name, and project directory) and keep
the existing exec() call to capture output and exit code.

---

Nitpick comments:
In `@app/views/install/installer/js/steps.js`:
- Around line 338-348: The code sets formState.migrate from the checkbox default
but doesn't notify listeners; after the branch that assigns formState.migrate =
checkbox.checked (the else branch), invoke dispatchStateChange?.('migrate') so
the initial seeded value is dispatched once; keep the existing change listener
on checkbox for subsequent updates and only call dispatchStateChange when you
set the seeded value (in the block that runs when formState.migrate was
undefined).
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 9d26ce83-f0e3-48ac-a6ae-300af21b8d67

📥 Commits

Reviewing files that changed from the base of the PR and between 2f53d09 and b47ac00.

📒 Files selected for processing (7)
  • app/views/install/installer/js/modules/progress.js
  • app/views/install/installer/js/steps.js
  • app/views/install/installer/templates/steps/step-6.phtml
  • src/Appwrite/Platform/Installer/Http/Installer/Install.php
  • src/Appwrite/Platform/Tasks/Install.php
  • src/Appwrite/Platform/Tasks/Upgrade.php
  • tests/unit/Platform/Modules/Installer/ModuleTest.php
✅ Files skipped from review due to trivial changes (1)
  • tests/unit/Platform/Modules/Installer/ModuleTest.php
🚧 Files skipped from review as they are similar to previous changes (3)
  • app/views/install/installer/js/modules/progress.js
  • app/views/install/installer/templates/steps/step-6.phtml
  • src/Appwrite/Platform/Installer/Http/Installer/Install.php

@github-actions
Copy link
Copy Markdown

github-actions bot commented Mar 31, 2026

🔄 PHP-Retry Summary

Flaky tests detected across commits:

Commit b47ac00 - 2 flaky tests
Test Retries Total Time Details
FunctionsScheduleTest::testCreateScheduledAtExecution 1 128.00s Logs
UsageTest::testVectorsDBStats 1 10.17s Logs

@abnegate abnegate merged commit 416b26a into 1.9.x Mar 31, 2026
46 checks passed
@github-actions
Copy link
Copy Markdown

✨ Benchmark results

  • Requests per second: 1,567
  • Requests with 200 status code: 282,155
  • P99 latency: 0.114867094

⚡ Benchmark Comparison

Metric This PR Latest version
RPS 1,567 1,120
200 282,155 201,701
P99 0.114867094 0.20028422

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.

1 participant