Skip to content

chore: drop location column and parse N-part resource paths#118

Merged
lohanidamodar merged 3 commits into
mainfrom
chore/drop-location-and-parse-six-part-resource
May 18, 2026
Merged

chore: drop location column and parse N-part resource paths#118
lohanidamodar merged 3 commits into
mainfrom
chore/drop-location-and-parse-six-part-resource

Conversation

@lohanidamodar
Copy link
Copy Markdown
Contributor

Summary

  • Drops the location column from the audit schema. Country is now resolved at write time in the consuming worker (cloud's Activity audit override populates country via geodb), and no remaining consumer needs location. Breaking change for callers that still pass it.
  • Generalizes SQL::parseResource() to handle any even number of segments shaped as alternating <type>/<id>. Previously only 2-part and 4-part paths were parsed; 6-part paths like database/<dbId>/collection/<colId>/document/<docId> fell through to the default fallback, leaving resourceType and resourceParent empty in stored audit rows.

Resource parsing rule

For an input split on / into N segments:

  • N == 0 or N is odd → resourceId = $resource (whole string), resourceType = '', resourceParent = ''.
  • N == 2resourceType = parts[0], resourceId = parts[1], resourceParent = ''.
  • N == 4, 6, 8, ... (even) → resourceId = parts[N-1], resourceType = parts[N-2], resourceParent = implode('/', parts[0..N-3]).

Breaking changes

  • Audit::log() signature loses the string $location positional parameter. Move callers from log(..., \$ip, \$location, \$data) to log(..., \$ip, \$data).
  • Adapter::create() / Adapter::createBatch() array shapes no longer accept a location key.
  • Log::getLocation() is removed.
  • location column is dropped from SQL::getAttributes() (downstream ClickHouse / Database schemas should run a migration to drop the column).

Test plan

  • composer lint (Pint, pass)
  • composer check (PHPStan level max, pass)
  • Run the PHPUnit suite against a real ClickHouse — confirm testParseResourceMethod covers the new 6-part / 2-part / odd-part cases.
  • Downstream: update appwrite/server-ce and any other consumer that still passes \$location to Audit::log().

🤖 Generated with Claude Code

Removes the unused `location` column from the audit schema. The cloud
audit pipeline resolves country at write time instead, and no consumer
relies on the legacy `location` field.

Generalizes `SQL::parseResource()` to handle any even number of segments
shaped as alternating `<type>/<id>`. Previously only 2-part and 4-part
paths were parsed; 6-part paths like
`database/<id>/collection/<id>/document/<id>` fell through to the
default fallback, leaving `resourceType` and `resourceParent` empty.

Breaking change: `Audit::log()` no longer accepts the `$location`
positional parameter; the `location` key is removed from the create /
createBatch log shape and from `Log::getLocation()`.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@greptile-apps
Copy link
Copy Markdown

greptile-apps Bot commented May 17, 2026

Greptile Summary

This PR removes the location column from the audit schema (country resolution now happens at write time in the consuming worker) and generalises SQL::parseResource() to handle any even-number-of-segment resource paths, fixing the bug where 6-part paths like database/<id>/collection/<id>/document/<id> were silently falling through to an empty-string fallback.

  • Schema change: location attribute dropped from SQL::getAttributes(), Audit::log() signature, batch array shapes, Log::getLocation(), and all test call sites. This is a documented breaking change requiring downstream callers and a DB migration.
  • parseResource() generalisation: replaces hardcoded count == 2 / count == 4 branches with a single count >= 2 && count % 2 == 0 guard; new testParseResourceMethod assertions cover the 6-part, 2-part, and odd-segment cases via reflection.

Confidence Score: 5/5

Safe to merge; the logic changes are well-scoped and consistent across the codebase.

The parseResource() rewrite is correct for all segment counts, the location removal is applied uniformly across source and tests, and the new test cases validate the key new behaviour. The only gap is that round-trip DB assertions for 6-part resource paths are missing from AuditBase, but this does not indicate a defect in the production code.

No files require special attention; downstream callers and DB migration noted in the PR description are outside this repository's scope.

Important Files Changed

Filename Overview
src/Audit/Adapter/SQL.php Drops the location attribute from the schema and generalises parseResource() to handle any even number of /-separated segments; logic is correct for all expected inputs.
src/Audit/Audit.php Removes the $location parameter from log() and logBatch() signatures; types and PHPDoc updated consistently.
src/Audit/Log.php Removes getLocation() accessor; straightforward deletion with no remaining callers in the repository.
src/Audit/Adapter.php Removes location from PHPDoc array shapes for create() and createBatch(); consistent with the schema change.
tests/Audit/Adapter/ClickHouseTest.php Updates call sites to remove $location, adds country to required attributes, and extends testParseResourceMethod with 6-part, 2-part, and odd-segment assertions.
tests/Audit/AuditBase.php All $location references removed throughout the trait; baseline test resources remain 3-part odd paths so derived parsed fields are untested in round-trip scenarios.

Reviews (3): Last reviewed commit: "test: cover country via getRequiredAttri..." | Re-trigger Greptile

lohanidamodar and others added 2 commits May 17, 2026 03:29
Country is now resolved at write time by callers (the cloud audit
worker resolves the lowercase ISO 3166-1 alpha-2 code from IP via
geodb) and supplied through the log data array. Adds a round-trip
test covering Audit::log(), Audit::logBatch(), and the
no-country fallback.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…test

Country is a ClickHouse-only schema column (the SQL/Database adapter
has no country column — country lives inside the JSON data column
there). Adding `country` to ClickHouseTest::getRequiredAttributes()
makes every AuditBase test that creates logs against ClickHouse pass
the value through the data array, where the adapter then extracts it
to the schema column. Same pattern already in use for resourceType,
resourceId, projectId, and friends.

Drops the dedicated testCountryRoundTrip in favor of this implicit
coverage.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@lohanidamodar lohanidamodar merged commit 9e730da into main May 18, 2026
4 checks passed
@lohanidamodar lohanidamodar deleted the chore/drop-location-and-parse-six-part-resource branch May 18, 2026 04:04
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