Remove eventType variable from relational and clickhouse queries #3948
Conversation
|
@mod08 is attempting to deploy a commit to the umami-software Team on Vercel. A member of the Team first needs to authorize it. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This PR removes the eventType filter from both the relational and ClickHouse queries in getGoal.ts. This change introduces a critical bug that causes incorrect query results.
Problem
The goal query counts unique sessions that match a specific condition (path or event name) and calculates a conversion rate against total sessions. The eventType filter is essential because:
- Path-based goals (
type === 'path') should only count pageView events (eventType = 1) - Event-based goals (
type !== 'path') should only count customEvent events (eventType = 2)
According to the database schema (prisma/schema.prisma):
- All events have a
url_path(the URL where the event occurred) - Only custom events have an
event_namefield populated - The
eventTypefield distinguishes between pageViews (1) and customEvents (2)
Impact
Without the eventType filter:
- Path-based goals will incorrectly include custom events that occurred on the target path, inflating the goal count
- Event-based goals may incorrectly include page views (though less likely since pageViews typically have null
event_name) - The total count (denominator) will incorrectly mix both event types, making conversion rates meaningless
Required Fix
Restore the eventType calculation and pass it to parseFilters:
const eventType = type === 'path' ? EVENT_TYPE.pageView : EVENT_TYPE.customEvent;Then include it in the parseFilters call (as shown in similar queries like getAttribution.ts).
Confidence Score: 0/5
- UNSAFE TO MERGE - introduces critical data correctness bug
- This PR removes essential filtering that prevents mixing of different event types (pageViews vs customEvents). The bug will cause all goal metrics to return incorrect data, mixing event types that should be kept separate. This is a P0 blocking issue that will break the goal tracking feature.
- src/queries/sql/reports/getGoal.ts requires the eventType filter to be restored in both relationalQuery and clickhouseQuery functions
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| src/queries/sql/reports/getGoal.ts | 1/5 | Removed eventType filter from goal queries, causing incorrect mixing of pageView and customEvent data types |
Sequence Diagram
sequenceDiagram
participant Client
participant API as /api/reports/goal
participant getGoal
participant parseFilters
participant DB as Database
Client->>API: POST with goal parameters
Note over Client,API: {type: 'path', value: '/checkout'}
API->>getGoal: getGoal(websiteId, params, filters)
alt Before this PR (correct)
getGoal->>getGoal: eventType = EVENT_TYPE.pageView
getGoal->>parseFilters: parseFilters({...filters, eventType})
parseFilters->>parseFilters: Add "event_type = 1" to filterQuery
getGoal->>DB: Query with event_type filter
Note over DB: Only counts pageView events
DB-->>getGoal: {num: 50, total: 1000}
else After this PR (incorrect)
getGoal->>parseFilters: parseFilters({...filters})
Note over parseFilters: NO event_type filter added
getGoal->>DB: Query WITHOUT event_type filter
Note over DB: Counts ALL event types<br/>(pageViews + customEvents)
DB-->>getGoal: {num: 75, total: 1500}
Note over getGoal,DB: WRONG - includes custom events<br/>that occurred on /checkout
end
getGoal-->>API: Result
API-->>Client: Goal metrics
| const column = type === 'path' ? 'url_path' : 'event_name'; | ||
| const { filterQuery, dateQuery, joinSessionQuery, cohortQuery, queryParams } = parseFilters({ | ||
| ...filters, | ||
| websiteId, | ||
| value, | ||
| startDate, | ||
| endDate, | ||
| eventType, | ||
| }); |
There was a problem hiding this comment.
[P0] Removing the eventType filter causes incorrect query results. When type === 'path', the query should filter by eventType: EVENT_TYPE.pageView (1), and when type !== 'path', it should filter by eventType: EVENT_TYPE.customEvent (2). Without this filter, path-based goals will incorrectly include custom events that occurred on that path, and event-based goals will incorrectly include page views. The eventType parameter must be added back to parseFilters.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/queries/sql/reports/getGoal.ts
Line: 32:39
Comment:
[P0] Removing the `eventType` filter causes incorrect query results. When `type === 'path'`, the query should filter by `eventType: EVENT_TYPE.pageView` (1), and when `type !== 'path'`, it should filter by `eventType: EVENT_TYPE.customEvent` (2). Without this filter, path-based goals will incorrectly include custom events that occurred on that path, and event-based goals will incorrectly include page views. The eventType parameter must be added back to parseFilters.
How can I resolve this? If you propose a fix, please make it concise.| const column = type === 'path' ? 'url_path' : 'event_name'; | ||
| const { filterQuery, dateQuery, cohortQuery, queryParams } = parseFilters({ | ||
| ...filters, | ||
| websiteId, | ||
| value, | ||
| startDate, | ||
| endDate, | ||
| eventType, | ||
| }); |
There was a problem hiding this comment.
[P0] Same issue as in the relational query - the eventType filter must be added back to parseFilters to prevent incorrect mixing of pageView and customEvent types. Without it, both the numerator (goal count) and denominator (total count) will include events of the wrong type.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/queries/sql/reports/getGoal.ts
Line: 72:79
Comment:
[P0] Same issue as in the relational query - the `eventType` filter must be added back to parseFilters to prevent incorrect mixing of pageView and customEvent types. Without it, both the numerator (goal count) and denominator (total count) will include events of the wrong type.
How can I resolve this? If you propose a fix, please make it concise.…and goal-specific counts
Greptile OverviewGreptile SummaryThis PR addresses issue #3947 by separating the filter logic for goal queries. The implementation splits
The approach correctly applies the explicit However, there is a critical logic error: If users include The fix requires explicitly excluding Positive aspects:
Areas needing attention:
Confidence Score: 2/5
Important Files ChangedFile Analysis
Sequence DiagramsequenceDiagram
participant Client
participant API as /api/reports/goal
participant getGoal
participant parseFilters
participant DB as Database
Client->>API: POST request with filters & parameters
API->>API: getQueryFilters(filters)
Note over API: Extracts eventType from filters if present
API->>getGoal: getGoal(websiteId, parameters, filters)
Note over getGoal: Calculate explicit eventType from type
getGoal->>getGoal: eventType = type === 'path' ? 1 : 2
Note over getGoal: First parseFilters (base filters)
getGoal->>parseFilters: parseFilters({...filters, websiteId, startDate, endDate})
Note over parseFilters: BUG: filters may include eventType
parseFilters-->>getGoal: {baseFilterQuery, dateQuery, cohortQuery, baseQueryParams}
Note over getGoal: Second parseFilters (goal filters)
getGoal->>parseFilters: parseFilters({...filters, websiteId, value, startDate, endDate, eventType})
parseFilters-->>getGoal: {goalFilterQuery, goalQueryParams}
Note over getGoal: Merge parameters
getGoal->>getGoal: queryParams = {...baseQueryParams, ...goalQueryParams}
Note over getGoal: goalQueryParams.eventType overwrites baseQueryParams.eventType
Note over getGoal: Execute SQL query
getGoal->>DB: SELECT num, total FROM website_event
Note over DB: Subquery uses baseFilterQuery<br/>(may incorrectly filter by eventType)
Note over DB: Main query uses goalFilterQuery<br/>(correctly filters by eventType)
DB-->>getGoal: {num, total}
getGoal-->>API: Query result
API-->>Client: JSON response
|
| } = parseFilters({ | ||
| ...filters, | ||
| websiteId, | ||
| startDate, | ||
| endDate, | ||
| }); |
There was a problem hiding this comment.
The filters object is spread into the first parseFilters call, which means if the user includes eventType in their filters (e.g., via query parameters), it will still be applied to the total count calculation. This defeats the purpose of this fix, which is to exclude eventType filtering from the total count.
The getRequestFilters function in src/lib/request.ts (lines 70-81) extracts all filter parameters including eventType from the request, so users can pass eventType as a filter. When this happens:
baseFilterQuerywill includeand event_type = {{eventType}}queryParams(after merging on line 59) will contain the expliciteventTypevalue from line 56- The total count subquery will be incorrectly filtered by event_type
To fix this, eventType should be explicitly excluded from the first parseFilters call:
| } = parseFilters({ | |
| ...filters, | |
| websiteId, | |
| startDate, | |
| endDate, | |
| }); | |
| // Filters WITHOUT eventType for total count (all sessions) | |
| const { eventType: _, ...filtersWithoutEventType } = filters; | |
| const { | |
| filterQuery: baseFilterQuery, | |
| dateQuery, | |
| joinSessionQuery, | |
| cohortQuery, | |
| queryParams: baseQueryParams, | |
| } = parseFilters({ | |
| ...filtersWithoutEventType, | |
| websiteId, | |
| startDate, | |
| endDate, | |
| }); |
This ensures the total count is never filtered by eventType, regardless of whether it's provided in the user's filters.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/queries/sql/reports/getGoal.ts
Line: 42:47
Comment:
The `filters` object is spread into the first parseFilters call, which means if the user includes `eventType` in their filters (e.g., via query parameters), it will still be applied to the total count calculation. This defeats the purpose of this fix, which is to exclude eventType filtering from the total count.
The `getRequestFilters` function in `src/lib/request.ts` (lines 70-81) extracts all filter parameters including `eventType` from the request, so users can pass eventType as a filter. When this happens:
1. `baseFilterQuery` will include `and event_type = {{eventType}}`
2. `queryParams` (after merging on line 59) will contain the explicit `eventType` value from line 56
3. The total count subquery will be incorrectly filtered by event_type
To fix this, `eventType` should be explicitly excluded from the first parseFilters call:
```suggestion
// Filters WITHOUT eventType for total count (all sessions)
const { eventType: _, ...filtersWithoutEventType } = filters;
const {
filterQuery: baseFilterQuery,
dateQuery,
joinSessionQuery,
cohortQuery,
queryParams: baseQueryParams,
} = parseFilters({
...filtersWithoutEventType,
websiteId,
startDate,
endDate,
});
```
This ensures the total count is never filtered by eventType, regardless of whether it's provided in the user's filters.
How can I resolve this? If you propose a fix, please make it concise.| } = parseFilters({ | ||
| ...filters, | ||
| websiteId, | ||
| startDate, | ||
| endDate, | ||
| }); |
There was a problem hiding this comment.
Same issue as in the relational query: if eventType is included in the user's filters, it will be applied to the total count calculation, defeating the purpose of this fix.
The eventType should be explicitly excluded from the filters passed to this parseFilters call:
| } = parseFilters({ | |
| ...filters, | |
| websiteId, | |
| startDate, | |
| endDate, | |
| }); | |
| // Filters WITHOUT eventType for total count (all sessions) | |
| const { eventType: _, ...filtersWithoutEventType } = filters; | |
| const { | |
| filterQuery: baseFilterQuery, | |
| dateQuery, | |
| cohortQuery, | |
| queryParams: baseQueryParams, | |
| } = parseFilters({ | |
| ...filtersWithoutEventType, | |
| websiteId, | |
| startDate, | |
| endDate, | |
| }); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/queries/sql/reports/getGoal.ts
Line: 101:106
Comment:
Same issue as in the relational query: if `eventType` is included in the user's filters, it will be applied to the total count calculation, defeating the purpose of this fix.
The `eventType` should be explicitly excluded from the filters passed to this parseFilters call:
```suggestion
// Filters WITHOUT eventType for total count (all sessions)
const { eventType: _, ...filtersWithoutEventType } = filters;
const {
filterQuery: baseFilterQuery,
dateQuery,
cohortQuery,
queryParams: baseQueryParams,
} = parseFilters({
...filtersWithoutEventType,
websiteId,
startDate,
endDate,
});
```
How can I resolve this? If you propose a fix, please make it concise.1 participant
Fixes #3947
The total visitor count was being filtered by eventType, which I believe is incorrect. I’ve therefore removed the eventType parameter from the parseFilters function.