refactor(cloud): Changes a few things - #51
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR refactors the storage adapter call chain to propagate ChangesAuthentication Context Propagation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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 (2)
src/storage/adapter/clickhouse/handlers/queryEvents.ts (1)
199-213:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftEnforce
auth.modein all query paths.
authis accepted but never applied to query predicates, so list/aggregation results can mix production/test data. Please threadauthinto query builders and add a mandatory mode filter for each table query (including total-count paths).🤖 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 `@src/storage/adapter/clickhouse/handlers/queryEvents.ts` around lines 199 - 213, The code accepts AuthContext but never applies it to queries; update handleQueryEvents to thread auth (AuthContext.auth.mode) into all query-building paths by passing auth into handleAggregationQuery and handleListQuery and into any total/count helpers (e.g., functions that compute totals for tables), and enforce a mandatory mode predicate per-table (e.g., add WHERE mode = auth.mode or equivalent) so every table query and total-count path filters by auth.mode; locate getTablesForRequest, handleAggregationQuery, handleListQuery and any total-count builders and modify their signatures to accept auth and apply the mode filter when composing predicates.src/storage/adapter/postgres/handlers/queryEvents.ts (1)
189-203:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftApply
auth.modeas a required SQL filter in event queries.The new
authparameter is unused, so query/list/aggregation/count operations are not scoped by mode. Add an enforced mode predicate to every generated subquery to keep production/test data isolated.🤖 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 `@src/storage/adapter/postgres/handlers/queryEvents.ts` around lines 189 - 203, handleQueryEvents currently ignores the auth parameter so queries aren’t scoped by mode; update it to enforce auth.mode as a required SQL filter by adding the mode predicate into every generated subquery (either by injecting it into the QueryRequest.where before calling or by passing auth down). Specifically, ensure handleQueryEvents appends "mode = auth.mode" (or equivalent predicate) to the request filter and propagate auth or the mode value into handleAggregationQuery, handleListQuery and any count/subquery builders so every generated SQL includes the mode constraint; also validate auth.mode is present/non-empty before running queries to avoid accidental unscoped access.
🤖 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.
Inline comments:
In `@src/routes/gRPC/query/queryEvents.ts`:
- Around line 36-39: The handler currently uses auth! when calling adapter.query
which can pass undefined; add a null check similar to createCheckoutLink.ts:
retrieve auth from call[apiKeyContextKey], if (!auth) throw or return an
AuthError (ensure you import AuthError) and terminate the request before calling
StorageAdapterFactory.getEventStorageAdapter or adapter.query; specifically,
update the logic around auth, reference auth and call[apiKeyContextKey], and
ensure adapter.query(queryRequest, auth) is only called with a defined auth
after importing AuthError from the errors module.
---
Outside diff comments:
In `@src/storage/adapter/clickhouse/handlers/queryEvents.ts`:
- Around line 199-213: The code accepts AuthContext but never applies it to
queries; update handleQueryEvents to thread auth (AuthContext.auth.mode) into
all query-building paths by passing auth into handleAggregationQuery and
handleListQuery and into any total/count helpers (e.g., functions that compute
totals for tables), and enforce a mandatory mode predicate per-table (e.g., add
WHERE mode = auth.mode or equivalent) so every table query and total-count path
filters by auth.mode; locate getTablesForRequest, handleAggregationQuery,
handleListQuery and any total-count builders and modify their signatures to
accept auth and apply the mode filter when composing predicates.
In `@src/storage/adapter/postgres/handlers/queryEvents.ts`:
- Around line 189-203: handleQueryEvents currently ignores the auth parameter so
queries aren’t scoped by mode; update it to enforce auth.mode as a required SQL
filter by adding the mode predicate into every generated subquery (either by
injecting it into the QueryRequest.where before calling or by passing auth
down). Specifically, ensure handleQueryEvents appends "mode = auth.mode" (or
equivalent predicate) to the request filter and propagate auth or the mode value
into handleAggregationQuery, handleListQuery and any count/subquery builders so
every generated SQL includes the mode constraint; also validate auth.mode is
present/non-empty before running queries to avoid accidental unscoped access.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c15f319d-b209-42de-80b0-b1c1ad6ec426
📒 Files selected for processing (13)
src/interface/storage/Storage.tssrc/routes/gRPC/payment/createCheckoutLink.tssrc/routes/gRPC/query/queryEvents.tssrc/services/pricingService.tssrc/storage/adapter/clickhouse/ClickHouseAdapter.tssrc/storage/adapter/clickhouse/handlers/priceRequestAiTokenUsage.tssrc/storage/adapter/clickhouse/handlers/priceRequestBasicUsage.tssrc/storage/adapter/clickhouse/handlers/queryEvents.tssrc/storage/adapter/postgres/handlers/priceRequest.tssrc/storage/adapter/postgres/handlers/priceRequestAiTokenUsage.tssrc/storage/adapter/postgres/handlers/priceRequestBasicUsage.tssrc/storage/adapter/postgres/handlers/queryEvents.tssrc/storage/adapter/postgres/postgres.ts
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
1 participant
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.
Summary by CodeRabbit