app/controllers/api/messaging.php (3)

2543-2545: Same geoRecord concerns as provider logs

This topic-logs endpoint uses geoRecord in the same way as the provider logs (countryCode/countryName taken from a single per-request geoRecord and applied to all log rows, with direct array indexing). The same semantic and null-safety issues apply here as in the /providers/:providerId/logs comment.

Also applies to: 2601-2602

---

2959-2961: Same geoRecord concerns as provider logs

Subscriber logs also inject a single geoRecord and reuse it for all log entries, with direct array indexing. This has the same behavior and robustness issues described in the provider logs comment.

Also applies to: 3017-3018

---

3752-3754: Same geoRecord concerns as provider logs

Message logs likewise apply one per-request geoRecord to every log entry and access countryCode/countryName without null-coalescing. Same semantic and null-safety issues as noted for provider logs.

Also applies to: 3810-3811

docker-compose.yml (1)

965-971: Consider production-readiness configurations for the geo service.

The new appwrite-geo service is using version 0.1.0 and lacks several configurations present in other services:

1. No restart policy: Other services use restart: unless-stopped for resilience
2. No health checks: Unable to detect if the service is functioning properly
3. No resource limits: Could consume unlimited resources
4. Version 0.1.0: Very early version - verify this is production-ready
5. Missing dependency: The main appwrite service doesn't list appwrite-geo in depends_on

Consider applying this configuration pattern consistent with other services:

   appwrite-geo:
     container_name: appwrite-geo
     image: appwrite/geo:0.1.0
+    restart: unless-stopped
+    <<: *x-logging
     networks:
       - appwrite
     environment:
       - GEO_SECRET=$_APP_GEO_SECRET

Also, add the geo service as a dependency where needed:

# In the appwrite service
depends_on:
  - mariadb
  - redis
  - appwrite-geo

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

77-77: Remove unused $locale parameter.

The $locale parameter is injected but never used in the action method. This adds unnecessary overhead to the dependency injection system.

Remove the unused injection:

             ->param('queries', [], new Queries([new Limit(), new Offset()]), 'Array of query strings generated using the Query class provided by the SDK. [Learn more about queries](https://appwrite.io/docs/queries). Only supported methods are limit and offset', true)
             ->inject('response')
             ->inject('dbForProject')
-            ->inject('locale')
             ->inject('geoRecord')
             ->callback($this->action(...));

And update the method signature:

-    public function action(string $databaseId, string $collectionId, array $queries, UtopiaResponse $response, Database $dbForProject, Locale $locale, array $geoRecord): void
+    public function action(string $databaseId, string $collectionId, array $queries, UtopiaResponse $response, Database $dbForProject, array $geoRecord): void

app/init/resources.php (1)

717-741: Back-compat: expose countryName and safe defaults.

Downstream code expects countryName. Add alias and guard absent fields.

     $output = [];
     $output['ip'] = $ip;
@@
-    if (!empty($record)) {
-        $output['countryCode'] = $record['countryCode'];
+    if (!empty($record)) {
+        $output['countryCode'] = $record['countryCode'] ?? '--';
         $output['country'] = $locale->getText('countries.' . strtolower($record['countryCode']), $locale->getText('locale.country.unknown'));
         $output['continent'] = $locale->getText('continents.' . strtolower($record['continentCode']), $locale->getText('locale.country.unknown'));
-        $output['continentCode'] = $record['continentCode'];
-        $output['eu'] = (\in_array($record['countryCode'], $eu)) ? true : false;
+        $output['continentCode'] = $record['continentCode'] ?? '--';
+        $output['eu'] = (\in_array($record['countryCode'] ?? '', $eu)) ? true : false;
@@
-        $output['currency'] = $currency;
+        $output['currency'] = $currency;
+        // Alias for existing consumers
+        $output['countryName'] = $output['country'];
     } else {
         $output['countryCode'] = '--';
         $output['country'] = $locale->getText('locale.country.unknown');
         $output['continent'] = $locale->getText('locale.country.unknown');
         $output['continentCode'] = '--';
         $output['eu'] = false;
-        $output['currency'] = $currency;
+        $output['currency'] = $currency;
+        $output['countryName'] = $output['country'];
     }

app/controllers/shared/api/auth.php (1)

39-45: Clarify denylist condition and normalize country codes.

Use explicit checks and trim codes. Also guard missing geoRecord key.

-        $denylist = System::getEnv('_APP_CONSOLE_COUNTRIES_DENYLIST', '');
-        if (!empty($denylist && $project->getId() === 'console')) {
-            $countries = explode(',', $denylist);
-            $country = $geoRecord['countryCode'] ?? '';
+        $denylist = System::getEnv('_APP_CONSOLE_COUNTRIES_DENYLIST', '');
+        if ($project->getId() === 'console' && $denylist !== '') {
+            $countries = array_map(static fn($c) => strtoupper(trim($c)), explode(',', $denylist));
+            $country = strtoupper($geoRecord['countryCode'] ?? '');
             if (in_array($country, $countries)) {
                 throw new Exception(Exception::GENERAL_REGION_ACCESS_DENIED);
             }
         }

app/controllers/api/users.php (1)

2212-2215: Guard session countryCode assignment.

Minor: add default to avoid notices if geoRecord is missing fields (defensive coding).

-                'countryCode' => $geoRecord['countryCode'],
+                'countryCode' => $geoRecord['countryCode'] ?? '--',

app/controllers/api/locale.php (1)

34-36: Expose countryName for callers relying on it.

Returning geoRecord is fine, but many call sites expect countryName. Consider adding it before responding.

-        $output = $geoRecord;
+        $output = $geoRecord;
+        $output['countryName'] = $output['country'] ?? $output['countryName'] ?? null;

app/controllers/general.php (3)

59-60: Remove unused $utopia param from router signature.

Not used inside router(). Drop it or prefix with _ to satisfy linters.

-function router(App $utopia, Database $dbForPlatform, callable $getProjectDB, SwooleRequest $swooleRequest, Request $request, Response $response, Log $log, Event $queueForEvents, StatsUsage $queueForStatsUsage, Func $queueForFunctions, Executor $executor, array $geoRecord, callable $isResourceBlocked, string $previewHostname, ?Key $apiKey)
+function router(Database $dbForPlatform, callable $getProjectDB, SwooleRequest $swooleRequest, Request $request, Response $response, Log $log, Event $queueForEvents, StatsUsage $queueForStatsUsage, Func $queueForFunctions, Executor $executor, array $geoRecord, callable $isResourceBlocked, string $previewHostname, ?Key $apiKey)

Note: Update all call sites accordingly.

---

380-387: Use geoRecord['eu'] directly.

Slight simplification; avoids re-checking against config.

-            if ($geoRecord) {
-                $eu = Config::getParam('locale-eu');
-
-                $headers['x-appwrite-country-code'] = $geoRecord['countryCode'] ?? '';
-                $headers['x-appwrite-continent-code'] = $geoRecord['continentCode'] ?? '';
-                $headers['x-appwrite-continent-eu'] = (\in_array($geoRecord['countryCode'], $eu)) ? 'true' : 'false';
-            }
+            if ($geoRecord) {
+                $headers['x-appwrite-country-code'] = $geoRecord['countryCode'] ?? '';
+                $headers['x-appwrite-continent-code'] = $geoRecord['continentCode'] ?? '';
+                $headers['x-appwrite-continent-eu'] = ($geoRecord['eu'] ?? false) ? 'true' : 'false';
+            }

---

868-875: Lazy-inject geoRecord to avoid external calls on init/OPTIONS/robots/humans requests.

Verification confirms geoRecord is injected in broad paths but underutilized:

• Line 852 (App::init() for api/web groups): Passed to all downstream handlers regardless of need
• Line 1123 (App::options()): Only used conditionally in router() calls
• Line 1441 (/robots.txt): Only needed in else branch for router(); main path renders template without it
• Line 1475 (/humans.txt): Only needed in else branch for router(); main path renders template without it

Move geoRecord injection into the router path only, or refactor to resolve it lazily within router() instead of at the entry points.

app/controllers/api/teams.php (1)

1512-1514: Per-log geo should not use the viewer’s geoRecord; also add null-coalescing.

• Use stored per-log geo (if available) or derive from each log’s IP; geoRecord reflects the current caller.
• Add safe defaults to avoid notices.

-            $output[$i]['countryCode'] = $geoRecord['countryCode'];
-            $output[$i]['countryName'] = $geoRecord['countryName'];
+            $output[$i]['countryCode'] = $geoRecord['countryCode'] ?? '';
+            $output[$i]['countryName'] = $geoRecord['countryName'] ?? '';

If per-log geo is desired, confirm product intent; we can wire a lookup using $log['ip'] against the geo service.

app/controllers/api/account.php (1)

2937-2939: Per-log geo should not use the viewer’s geoRecord; also add null-coalescing.

Use stored per-log geo or derive from each log’s IP; add safe defaults meanwhile.

-            $output[$i]['countryCode'] = $geoRecord['countryCode'];
-            $output[$i]['countryName'] = $geoRecord['countryName'];
+            $output[$i]['countryCode'] = $geoRecord['countryCode'] ?? '';
+            $output[$i]['countryName'] = $geoRecord['countryName'] ?? '',

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Learnt from: stnguyen90
Repo: appwrite/appwrite PR: 10119
File: app/controllers/api/account.php:1226-1232
Timestamp: 2025-07-08T01:20:14.364Z
Learning: In Appwrite, `_APP_DOMAIN` is a required environment variable that must always be set for the system to function properly.

Learnt from: abnegate
Repo: appwrite/appwrite PR: 10546
File: src/Appwrite/Platform/Workers/Migrations.php:144-148
Timestamp: 2025-10-23T08:06:38.889Z
Learning: In the Appwrite codebase, migration workers receive already-validated data from queued jobs. Query validation using Query::parseQueries() happens at the API endpoint level (with try-catch for QueryException) before jobs are queued, so workers in src/Appwrite/Platform/Workers/Migrations.php don't need redundant validation.

docker-compose.yml (1)

966-972: Add restart policy and logging configuration for consistency.

The appwrite-geo service is missing a restart policy and logging configuration that other services in the stack use. For consistency and reliability, consider adding:

1. restart: unless-stopped (similar to other services like appwrite-console at line 224)
2. <<: *x-logging (for consistent log rotation/management like other services)

Apply this diff:

   appwrite-geo:
+    <<: *x-logging
     container_name: appwrite-geo
     image: appwrite/geo:0.1.0
+    restart: unless-stopped
     networks:
       - appwrite
     environment:
       - GEO_SECRET=$_APP_GEO_SECRET

app/init/resources.php (1)

686-694: Add timeout configuration and consider making the base URL configurable.

The HTTP client lacks timeout configuration, which could cause requests to hang indefinitely if the geo service is slow or unresponsive. Additionally, the base URL is hardcoded to http://appwrite-geo/v1, which works for Docker Compose but reduces flexibility.

Consider these improvements:

1. Add timeout configuration to the Client
2. Make the base URL configurable via environment variable

Example:

$client = new Client();
$client->addHeader('Authorization', 'Bearer ' . System::getEnv('_APP_GEO_SECRET'));
$client->setTimeout(5); // 5 second timeout
$client->setBaseUrl(System::getEnv('_APP_GEO_URL', 'http://appwrite-geo/v1'));

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Learnt from: stnguyen90
Repo: appwrite/appwrite PR: 10119
File: app/controllers/api/account.php:1226-1232
Timestamp: 2025-07-08T01:20:14.364Z
Learning: In Appwrite, `_APP_DOMAIN` is a required environment variable that must always be set for the system to function properly.

app/init/resources.php (1)

710-720: Add safety checks for array access in geodb fallback.

Lines 715-718 access nested array keys from $dbRecord without verifying they exist. If the geodb returns data in an unexpected format, this will cause undefined index errors. This issue was previously flagged and remains unaddressed.

Apply this diff to add safety checks:

     if (empty($record)) {
         // Fallback
         $dbRecord = $geodb->get($ip);
         if ($dbRecord) {
             $record = [];
-            $record['countryCode'] = $dbRecord['country']['iso_code'];
-            $record['country'] = $dbRecord['country']['names'];
-            $record['continent'] = $dbRecord['continent']['names'];
-            $record['continentCode'] = $dbRecord['continent']['code'];
+            $record['countryCode'] = $dbRecord['country']['iso_code'] ?? '--';
+            $record['country'] = $dbRecord['country']['names'] ?? [];
+            $record['continent'] = $dbRecord['continent']['names'] ?? [];
+            $record['continentCode'] = $dbRecord['continent']['code'] ?? '--';
         }
     }

app/init/resources.php (1)

697-702: Simplify error handling—avoid throw-catch for expected fallback.

Throwing Exception::GENERAL_SERVER_ERROR on non-200 responses only to catch it immediately and set $record = null is unnecessarily complex. Since fallback to geodb is the intended behavior, simply check the status and skip the exception.

Apply this diff:

     $record = null;
     try {
         $record = $client->fetch("/ips/{$ip}", Client::METHOD_GET);
-        if ($record->getStatusCode() !== 200) {
-            $error = $record->json();
-            throw new Exception(Exception::GENERAL_SERVER_ERROR, $error['message'] ?? 'Failed to fetch geo data');
-        }
-        $record = $record->json();
+        $record = ($record->getStatusCode() === 200) ? $record->json() : null;
     } catch (Throwable $th) {
         Console::error($th->getMessage());
         Console::error($th->getTraceAsString());
         $record = null;
     }

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

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

144-145: Defensively access countryCode / countryName from $geoRecord.

Directly indexing $geoRecord['countryCode'] / ['countryName'] will emit notices if the geo service returns incomplete data. Mirror the defensive style used for OS/client/device fields and fall back to a safe default.

-            $output[$i]['countryCode'] = $geoRecord['countryCode'];
-            $output[$i]['countryName'] = $geoRecord['countryName'];
+            $output[$i]['countryCode'] = $geoRecord['countryCode'] ?? '';
+            $output[$i]['countryName'] = $geoRecord['countryName'] ?? '';

app/config/collections/common.php (1)

870-1001: Previous review concerns still apply.

The comprehensive issues raised in the earlier review remain unaddressed in the current code:

1. Latitude/longitude precision: size: 2 for float fields provides only ~1.1km accuracy; standard geolocation requires 6-8 decimal places for precision.
2. Autonomous System Number data type: Stored as Database::VAR_STRING but represents a numeric value (0–4,294,967,295); should use Database::VAR_INTEGER.
3. Missing indexes: None of the 12 new attributes have database indexes, which will cause slow queries if these fields are used for filtering or searching.
4. Migration strategy: The deployment path for adding these attributes to existing session documents remains unclear.

Since you're in "Chill" mode, consider prioritizing items 1 and 2 for immediate correction, while items 3 and 4 can be evaluated based on actual query patterns and deployment procedures.

src/Appwrite/Platform/Modules/Functions/Http/Executions/Create.php (1)

95-99: Harden geoRecord access when computing EU header

The new geoRecord injection and parameter wiring look fine, but this block is still brittle:

if (!empty($ip)) {
    if ($geoRecord) {
        $eu = Config::getParam('locale-eu');

        $headers['x-appwrite-country-code'] = $geoRecord['countryCode'] ?? '';
        $headers['x-appwrite-continent-code'] = $geoRecord['continentCode'] ?? '';
        $headers['x-appwrite-continent-eu'] = (\in_array($geoRecord['countryCode'], $eu)) ? 'true' : 'false';
    }
}

If countryCode is missing, the in_array call can trigger an undefined index notice. Prefer deriving locals with null‑coalescing and then using them:

-        $headers['x-appwrite-country-code'] = $geoRecord['countryCode'] ?? '';
-        $headers['x-appwrite-continent-code'] = $geoRecord['continentCode'] ?? '';
-        $headers['x-appwrite-continent-eu'] = (\in_array($geoRecord['countryCode'], $eu)) ? 'true' : 'false';
+        $countryCode = $geoRecord['countryCode'] ?? '';
+        $continentCode = $geoRecord['continentCode'] ?? '';
+
+        $headers['x-appwrite-country-code'] = $countryCode;
+        $headers['x-appwrite-continent-code'] = $continentCode;
+        $headers['x-appwrite-continent-eu'] = (!empty($countryCode) && \in_array($countryCode, $eu)) ? 'true' : 'false';

This avoids undefined index notices and keeps behavior unchanged when geo data is present.

Also applies to: 118-121, 236-243

app/controllers/api/users.php (1)

908-916: Make user log geo fields defensive and use locale fallback

The user‑logs route now injects geoRecord and maps it into each log entry:

$output[$i]['countryCode'] = $geoRecord['countryCode'];
$output[$i]['countryName'] = $geoRecord['countryName'];

If the geo service omits one of these keys you’ll get undefined index notices, and $locale is currently unused.

Align this with other endpoints by guarding the keys and using a sensible default:

-            $output[$i]['countryCode'] = $geoRecord['countryCode'];
-            $output[$i]['countryName'] = $geoRecord['countryName'];
+            $countryCode = $geoRecord['countryCode'] ?? '--';
+            $countryName = $geoRecord['countryName']
+                ?? $locale->getText('locale.country.unknown');
+
+            $output[$i]['countryCode'] = $countryCode;
+            $output[$i]['countryName'] = $countryName;

This eliminates undefined index notices and provides a clear “unknown country” label when geo data is missing.

Also applies to: 941-974

app/controllers/api/account.php (1)

194-195: Guard $geoRecord access for countryCode / countryName across sessions and logs

Several places assume $geoRecord['countryCode'] / $geoRecord['countryName'] always exist when creating sessions or enriching logs (token/magic/phone sessions via $createSession, email-password and anonymous sessions, OAuth2 sessions, and /v1/account/logs). Given geoRecord is populated from an external geo service and other code already treats its keys as optional, this can easily produce undefined-index notices when geo lookup fails or the payload shape changes.

Use a null‑coalescing fallback (or isset) for these fields so session creation and log rendering remain robust even when geo data is missing.

Example adjustments:

@@ function $createSession(...)
-            'countryCode' => $geoRecord['countryCode'],
+            'countryCode' => $geoRecord['countryCode'] ?? '',
@@ createEmailPasswordSession
-                'countryCode' => $geoRecord['countryCode'],
+                'countryCode' => $geoRecord['countryCode'] ?? '',
@@ createAnonymousSession
-                'countryCode' => $geoRecord['countryCode'],
+                'countryCode' => $geoRecord['countryCode'] ?? '',
@@ OAuth2 redirect session creation
-                'countryCode' => $geoRecord['countryCode'],
+                'countryCode' => $geoRecord['countryCode'] ?? '',
@@ /v1/account/logs action
-            $output[$i]['countryCode'] = $geoRecord['countryCode'];
-            $output[$i]['countryName'] = $geoRecord['countryName'];
+            $output[$i]['countryCode'] = $geoRecord['countryCode'] ?? '';
+            $output[$i]['countryName'] = $geoRecord['countryName'] ?? '';

Also applies to: 234-235, 927-933, 966-967, 1064-1068, 1129-1130, 1207-1211, 1401-1404, 1806-1807, 2912-2915, 2947-2948

app/controllers/api/teams.php (1)

1206-1208: Guard $geoRecord when setting countryCode on membership-confirmation session

In the membership status update flow, the auto-created session writes 'countryCode' => $geoRecord['countryCode'] without checking the key. If the geo service fails or returns a partial record, this will trigger undefined-index notices during a user-accept-invite flow.

Use a safe fallback here as well.

@@ ->inject('project')
-    ->inject('geoRecord')
+    ->inject('geoRecord')
@@ session payload
-                'factors' => ['email'],
-                'countryCode' => $geoRecord['countryCode'],
+                'factors' => ['email'],
+                'countryCode' => $geoRecord['countryCode'] ?? '',

Also applies to: 1278-1279

app/controllers/api/messaging.php (1)

1145-1146: geoRecord in log endpoints: add null-safety and confirm per-request vs per-log geo semantics

Across provider/topic/subscriber/message log endpoints you inject array $geoRecord and then index ['countryCode'] / ['countryName'] directly for every log row. Two points:

1. Null-safety
   If the geo service fails or returns a partial record, those keys may be absent, leading to PHP notices. Other code (e.g. Functions/Http/Executions/Create) guards with ??. Suggest at least:

-            $output[$i]['countryCode'] = $geoRecord['countryCode'];
-            $output[$i]['countryName'] = $geoRecord['countryName'];
+            $output[$i]['countryCode'] = $geoRecord['countryCode'] ?? null;
+            $output[$i]['countryName'] = $geoRecord['countryName'] ?? null;

and apply the same pattern in the topic, subscriber, and message log handlers.

2. Semantics of geo on logs
   geoRecord appears to represent the current viewer’s IP, while each log entry has its own $log['ip']. With this change, all rows will show the same country for a given response, which is different from resolving each log’s IP (if that was the previous behavior). This is a potential audit/logging semantics regression; if logs are meant to reflect the geo of the event rather than the viewer, these endpoints probably need per-log geo (or persisted geo fields) instead of geoRecord.

Can you confirm what the intended contract is for countryCode/countryName on these log APIs and whether the per-request geo is acceptable here?

Also applies to: 1202-1203, 2546-2547, 2604-2605, 2961-2962, 3019-3020, 3757-3758, 3815-3816

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

70-76: Geo DI wiring looks consistent; consider documenting geoRecord shape.

->inject('geoRecord') aligns with the updated action() signature and matches the pattern used in Documents\Logs\XList, so wiring looks correct. To help future maintainers (and DI providers), consider adding a short docblock on action() describing the expected keys in $geoRecord (e.g., countryCode, countryName).

---

78-78: Signature matches injection order; unused $locale is benign.

The updated action() signature correctly mirrors the parameter/injection order (response, dbForProject, locale, geoRecord, authorization), so DI should remain stable. The $locale parameter is currently unused; given it’s part of the injection contract, it’s reasonable to leave it as-is rather than removing it and risking a mismatch. If PHPMD noise becomes an issue, you can suppress this specific rule at the method level instead of altering the signature.

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Learnt from: abnegate
Repo: appwrite/appwrite PR: 10546
File: src/Appwrite/Platform/Workers/Migrations.php:144-148
Timestamp: 2025-10-23T08:06:38.889Z
Learning: In the Appwrite codebase, migration workers receive already-validated data from queued jobs. Query validation using Query::parseQueries() happens at the API endpoint level (with try-catch for QueryException) before jobs are queued, so workers in src/Appwrite/Platform/Workers/Migrations.php don't need redundant validation.

app/controllers/api/account.php (1)

1401-1404: Fix TYPE::EMAIL typo and guard geoRecord in OAuth2 session creation

Two issues in the OAuth2 redirect session block:

• TYPE::EMAIL (Line 1805) is almost certainly a typo for Type::EMAIL (matching the imported Appwrite\Auth\MFA\Type used elsewhere). As written, this will cause an undefined-constant error at runtime when this path is hit.
• countryCode is read as $geoRecord['countryCode'] without a fallback, unlike other flows that use ?? ''. If the geo service omits this key, you’ll get undefined-index notices. This was previously flagged and fixed, so this looks like a regression.

Consider applying:

-                'factors' => [TYPE::EMAIL, 'oauth2'], // include a special oauth2 factor to bypass MFA checks
-                'countryCode' => $geoRecord['countryCode'],
+                'factors' => [Type::EMAIL, 'oauth2'], // include a special oauth2 factor to bypass MFA checks
+                'countryCode' => $geoRecord['countryCode'] ?? '',

Also applies to: 1793-1808

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro