Skip to content

Conversation

nikhilsinhaparseable
Copy link
Contributor

@nikhilsinhaparseable nikhilsinhaparseable commented Sep 24, 2025

Summary by CodeRabbit

  • Bug Fixes
    • Improved reliability of filter ID generation to prevent collisions by using a robust unique ID format.
    • Standardized and clarified error messaging to: “Filter does not exist or user is not authorized” across view, update, and delete actions.
    • Enhances consistency and user feedback without requiring changes from existing clients.

Copy link
Contributor

coderabbitai bot commented Sep 24, 2025

Walkthrough

Replaced timestamp-derived filter_id generation with Ulid::new() in user filter handler. Standardized missing/unauthorized error message across get, update, and delete flows. Added Ulid import. No public API signatures changed.

Changes

Cohort / File(s) Summary
User filter ID generation and error messaging
src/handlers/http/users/filters.rs
Generate filter_id via Ulid::new(); unify error text to "Filter does not exist or user is not authorized" for get/update/delete; add Ulid import.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant C as Client
  participant H as UsersHTTPHandler
  participant S as FilterService
  participant D as DB

  rect rgba(230,250,230,0.6)
  note over H: Create Filter
  C->>H: POST /users/{id}/filters
  H->>H: Ulid::new() -> filter_id
  H->>S: Create(filter_id, payload, user)
  S->>D: INSERT filter
  D-->>S: OK
  S-->>H: Created(filter_id)
  H-->>C: 201 Created (filter_id)
  end
Loading
sequenceDiagram
  autonumber
  participant C as Client
  participant H as UsersHTTPHandler
  participant S as FilterService
  participant D as DB

  note over H: Get/Update/Delete Filter
  C->>H: GET/PUT/DELETE /users/{id}/filters/{filter_id}
  H->>S: Fetch/Verify(filter_id, user)
  S->>D: SELECT/UPDATE/DELETE
  alt not found or unauthorized
    S-->>H: Err
    H-->>C: 404/403 "Filter does not exist or user is not authorized"
  else success
    S-->>H: OK
    H-->>C: 200/204
  end
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

I thump my paw: a new ID’s spun,
A ULID under autumn sun.
If filters hide or go astray,
One tidy message lights the way.
I hop through logs with quiet cheer—
Fewer squeaks, more signal here. 🐇✨

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings)
Check name Status Explanation Resolution
Description Check ⚠️ Warning No PR description was provided; the repository requires the provided template which includes a "Fixes #XXXX" line, a Description section describing the goal, chosen solution and rationale, key changes, and a checklist. The raw summary shows changes to filter error messages and replacing timestamp-derived IDs with Ulid::new(), but those details and the required template fields/checklist are missing from the PR description, so it does not meet the template requirements. Please populate the repository PR template: add "Fixes #XXXX" if applicable, provide a Description that states the goal, the chosen solution and rationale, list key changes (e.g., updated filter error messages across get/update/delete and switching to Ulid::new() for filter IDs), and complete the checklist with testing and documentation notes so reviewers have sufficient context.
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (1 passed)
Check name Status Explanation
Title Check ✅ Passed The title "chore: update error message on get filters" is concise and directly related to the changeset because the PR updates filter-related error messages; it accurately communicates the primary intent. It is slightly narrow since the changes also touch update/delete flows and switch to Ulid::new() for filter IDs, but it still conveys the main purpose of the change.
✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4f68965 and c8c7fc7.

📒 Files selected for processing (1)
  • src/handlers/http/users/filters.rs (5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
  • GitHub Check: Quest Smoke and Load Tests for Distributed deployments
  • GitHub Check: Quest Smoke and Load Tests for Standalone deployments
  • GitHub Check: coverage
  • GitHub Check: Build Default x86_64-unknown-linux-gnu
  • GitHub Check: Build Default x86_64-pc-windows-msvc
  • GitHub Check: Build Default aarch64-apple-darwin
  • GitHub Check: Build Default aarch64-unknown-linux-gnu
  • GitHub Check: Build Default x86_64-apple-darwin
  • GitHub Check: Build Kafka x86_64-unknown-linux-gnu
  • GitHub Check: Build Kafka aarch64-apple-darwin
🔇 Additional comments (3)
src/handlers/http/users/filters.rs (3)

54-56: Approve — standardized not-found/unauthorized message applied

Verified occurrences in src/handlers/http/users/filters.rs (lines 54–56, 85–87, 109–111).


34-34: No change needed — ulid is already declared (pinned to 1.0, serde feature).

Cargo.toml (root, line 139): ulid = { version = "1.0", features = ["serde"] }


65-65: ULID OK — no integer-only assumptions found

Searches show no parse:: usages tied to filter_id; route params and storage treat filter_id as strings. Key locations: src/handlers/http/users/filters.rs (Path, Ulid usage), src/handlers/http/modal/server.rs (web::resource("/{filter_id}")), src/metastore/metastores/object_store_metastore.rs (format!("{filter_id}.json")), src/users/filters.rs (filter_id: Option, string comparisons).


Comment @coderabbitai help to get the list of available commands and usage tips.

@nitisht nitisht merged commit f265751 into parseablehq:main Sep 25, 2025
13 checks passed
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