fix(export): include properties field and add property_keys filter to /export/events#389
Open
Shrotriya-lalit wants to merge 4 commits into
Open
Conversation
The export controller's `getEventList` call never enabled `properties` in its select object, so the column was silently omitted from SQL and every exported event had an empty properties field. Fixes Openpanel-dev#281 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
When exporting events with large property sets, callers can now pass `?property_keys=key1,key2` to receive only the specified keys in each event's properties map instead of the full payload. Uses ClickHouse's mapFilter function at query time so only the requested keys are transferred — no post-processing overhead. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
mapFilter(...) in a bare SELECT returns a column named after the full expression. transformEvent reads event.properties so the field was silently dropped. Adding 'AS properties' restores the expected column name. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe PR adds selective event properties export support to the ChangesEvent Properties Selective Export
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/db/src/services/event.service.ts (1)
553-559: 💤 Low valueConsider deduplicating
propertyKeysbefore constructing the filter.If the caller passes duplicate keys in
property_keys, the IN clause will contain duplicates. While this doesn't break functionality, deduplicating would make the query slightly more efficient.♻️ Proposed refactor to deduplicate keys
if (select.properties) { if (propertyKeys && propertyKeys.length > 0) { - const keys = propertyKeys.map((k) => sqlstring.escape(k)).join(', '); + const keys = [...new Set(propertyKeys)].map((k) => sqlstring.escape(k)).join(', '); sb.select.properties = `mapFilter((k, v) -> k IN (${keys}), properties) AS properties`; } else { sb.select.properties = 'properties'; } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/db/src/services/event.service.ts` around lines 553 - 559, Deduplicate the propertyKeys array before building the IN clause so duplicates aren't injected into sb.select.properties; e.g., within the block that checks propertyKeys and propertyKeys.length, derive a uniqueKeys set (e.g., via Array.from(new Set(propertyKeys)) or similar), then map and escape those uniqueKeys when creating keys and the mapFilter expression assigned to sb.select.properties (keep using sqlstring.escape, mapFilter and the same expression structure).apps/api/src/controllers/export.controller.ts (1)
86-109: ⚡ Quick winConsider extracting the duplicate preprocessing logic into a helper function.
The preprocessing logic for
includesandproperty_keysis identical. This duplication makes the code harder to maintain if the normalization logic needs to change.♻️ Proposed refactor to eliminate duplication
+const preprocessCommaSeparatedArray = (arg: unknown) => { + if (arg == null) return undefined; + if (Array.isArray(arg)) return arg; + if (typeof arg === 'string') + return arg.split(',').map((s) => s.trim()).filter(Boolean); + return arg; +}; + export const eventsScheme = z.object({ project_id: z.string().optional(), projectId: z.string().optional(), profileId: z.string().optional(), event: z.union([z.string(), z.array(z.string())]).optional(), start: z.coerce.string().optional(), end: z.coerce.string().optional(), page: z.coerce.number().optional().default(1), limit: z.coerce.number().optional().default(50), filters: z .preprocess((value) => { if (value == null || value === '') return undefined; if (Array.isArray(value)) return value; if (typeof value === 'string') { try { return JSON.parse(value); } catch { return undefined; } } return value; }, z.array(zChartEventFilter)) .optional(), - includes: z - .preprocess( - (arg) => { - if (arg == null) return undefined; - if (Array.isArray(arg)) return arg; - if (typeof arg === 'string') - return arg.split(',').map((s) => s.trim()).filter(Boolean); - return arg; - }, - z.array(z.string()), - ) - .optional(), - property_keys: z - .preprocess( - (arg) => { - if (arg == null) return undefined; - if (Array.isArray(arg)) return arg; - if (typeof arg === 'string') - return arg.split(',').map((s) => s.trim()).filter(Boolean); - return arg; - }, - z.array(z.string()), - ) - .optional(), + includes: z.preprocess(preprocessCommaSeparatedArray, z.array(z.string())).optional(), + property_keys: z.preprocess(preprocessCommaSeparatedArray, z.array(z.string())).optional(), });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/controllers/export.controller.ts` around lines 86 - 109, Extract the duplicated preprocessing function used for the includes and property_keys z.preprocess calls into a single helper (e.g., preprocessToStringArray or normalizeArrayInput) and replace both inline arrow functions with that helper; the helper should accept the raw arg, return undefined for null/undefined, return the arg if already an array, split a string by commas trimming/filtering empty entries, and otherwise return the arg so it can be validated by z.array(z.string()); define the helper near the schema (or export it if reused elsewhere) and then call z.preprocess(preprocessToStringArray, z.array(z.string())).optional() for both includes and property_keys.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@apps/api/src/controllers/export.controller.ts`:
- Around line 86-109: Extract the duplicated preprocessing function used for the
includes and property_keys z.preprocess calls into a single helper (e.g.,
preprocessToStringArray or normalizeArrayInput) and replace both inline arrow
functions with that helper; the helper should accept the raw arg, return
undefined for null/undefined, return the arg if already an array, split a string
by commas trimming/filtering empty entries, and otherwise return the arg so it
can be validated by z.array(z.string()); define the helper near the schema (or
export it if reused elsewhere) and then call
z.preprocess(preprocessToStringArray, z.array(z.string())).optional() for both
includes and property_keys.
In `@packages/db/src/services/event.service.ts`:
- Around line 553-559: Deduplicate the propertyKeys array before building the IN
clause so duplicates aren't injected into sb.select.properties; e.g., within the
block that checks propertyKeys and propertyKeys.length, derive a uniqueKeys set
(e.g., via Array.from(new Set(propertyKeys)) or similar), then map and escape
those uniqueKeys when creating keys and the mapFilter expression assigned to
sb.select.properties (keep using sqlstring.escape, mapFilter and the same
expression structure).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: adab4330-650e-4104-9d03-d45d046303df
📒 Files selected for processing (2)
apps/api/src/controllers/export.controller.tspackages/db/src/services/event.service.ts
…ess helper - Deduplicate propertyKeys before building ClickHouse IN clause to avoid redundant keys in the SQL query - Extract repeated comma-separated array preprocessing logic into a shared helper to eliminate duplication between includes and property_keys fields Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
GET /export/eventsnever returned thepropertiesfield — it was missing from the SQL SELECT, so every exported event had an empty properties object.Fixes #281
Changes
export.controller.ts: addproperties: trueto the select object so properties are always included in the export responseexport.controller.ts: add optionalproperty_keysquery param — accepts a comma-separated list of keys to return only specific properties, reducing payload size for events with large property mapsevent.service.ts: extendGetEventListOptionswithpropertyKeys?: string[]and use ClickHousemapFilterat query time when keys are specified; addAS propertiesalias so the filtered column name is preservedcorrectly
Usage