sanitise and remove reserved fields from alert other_fields

[nikhilsinhaparseable]

Summary by CodeRabbit

• Bug Fixes

  • Alert submissions no longer error when reserved keys are present; reserved keys are automatically removed (with a warning) so alerts can be created from legacy or custom payloads.

• Changes

  • The alert "created" timestamp is captured at submission and uses standard date parsing (no special empty-string fallback).

• Chores

  • No breaking changes to external APIs.

[coderabbitai]

Walkthrough

Removed the custom DateTime deserializer/default and adjusted reserved-field handling: AlertRequest conversion now captures a creation timestamp, drops reserved keys from other_fields (with a warning) instead of erroring, and AlertConfig gained a sanitize_other_fields helper; related public re-exports were removed.

Changes

Cohort / File(s)	Summary
Alert structs — sanitization & created handling src/alerts/alert_structs.rs	Removed custom DateTime deserializer import and default provider; introduced capture of created_timestamp during AlertRequest::into; removed serde attributes for created fields; added sanitize_other_fields(&mut self) to remove reserved keys and drop empty other_fields; adjusted RESERVED_FIELDS contents/order and changed AlertRequest::into to drop reserved keys with a warning instead of returning an error.
Alert types & public re-exports src/alerts/alert_types.rs, src/alerts/mod.rs	Removed serde attribute usage on ThresholdAlert.created (no custom default/deserializer applied); removed public re-exports of default_created_time and deserialize_datetime_with_empty_string_fallback from the alerts module export list; imports restructured accordingly.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant Client
  participant AlertRequest
  participant Into as "AlertRequest::into"
  participant DB

  Note over AlertRequest,Into: New conversion flow (sanitization + timestamp capture)
  Client->>AlertRequest: submit payload with other_fields
  AlertRequest->>Into: start conversion (capture created_timestamp)
  Into->>Into: drop reserved keys from other_fields (log warning if dropped)
  Into->>Into: enforce 10-field limit
  alt over limit
    Into-->>Client: return validation error
  else
    Into->>DB: persist AlertConfig (uses captured created_timestamp)
    DB-->>Client: success
  end

Loading

sequenceDiagram
  autonumber
  participant Deserializer
  participant ThresholdAlert

  Note over Deserializer,ThresholdAlert: Deserialization behavior changed
  Deserializer->>ThresholdAlert: parse `created` using standard serde rules
  ThresholdAlert->>ThresholdAlert: no custom empty-string fallback or default applied

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

• Pay attention to:
  • Places relying on prior reserved-field rejection behavior.
  • Tests or callers expecting empty-string fallback/default for created.
  • Consumers importing the removed re-exports.

Possibly related PRs

• update: add other fields to alert request, config and response #1459 — introduced RESERVED_FIELDS and the stricter reserved-field validation that this change now relaxes.
• update: add tags, created to alerts #1389 — modifies created timestamp handling and related alert struct behavior.
• Alerts updates #1398 — touches serde handling and re-exports for created on alert types.

Suggested labels

for next release

Poem

I’m a rabbit in the code, I pruned a guarded field,
I let stray keys hop home again and caught the time they yield.
No mystic deserializer, just simpler skies—
I thumped my foot and gave a hop; the timestamps now arise. 🐇✨

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	The pull request description is missing entirely. No context, rationale, or testing information is provided per the repository template.	Add a complete PR description following the template: include the issue being fixed, description of changes, key modifications, and testing/documentation checkboxes.
Docstring Coverage	⚠️ Warning	Docstring coverage is 66.67% 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 accurately describes the main change: removing the 'created' field from reserved fields handling in alert structures.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

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

[coderabbitai]

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/alerts/alert_structs.rs (1)

293-315: Address misleading comment, inconsistent error handling, and missing test coverage.

The code silently removes reserved fields from other_fields with only a server-side warning instead of returning a validation error to the client. This is problematic for several reasons:

1. Inconsistent error handling (lines 295-298 vs 302-305): Field count violations error out, but reserved field conflicts silently remove with only a warning. Why the different treatment?
2. Misleading comment (line 292): Says "Validate" but actually performs silent removal/sanitization.
3. Unused sanitization method: sanitize_other_fields() method exists in AlertConfig (lines 435-442) but has no callers—appears to be duplicate/dead code.
4. No test coverage: Zero tests found for this critical validation logic.

Consider:

• Either return ValidationFailure for reserved fields (consistent with field count validation)
• Or clearly document that silent removal is intentional API behavior (not a validation error)
• Remove or utilize the unused sanitize_other_fields() method
• Add tests for reserved field handling

🧹 Nitpick comments (1)

src/alerts/alert_structs.rs (1)

433-450: Clarify when sanitize_other_fields should be invoked.

The implementation correctly removes reserved fields and cleans up empty other_fields, but as a public method it's unclear when callers should invoke it. Consider documenting:

• Is this for deserializing legacy data that may contain reserved fields?
• Should it be called after modifying AlertConfig programmatically?
• Is it a migration helper for existing persisted alerts?

Apply this diff to add usage documentation:

-    /// Filters out reserved field names from other_fields
-    /// This prevents conflicts when flattening other_fields during serialization
+    /// Filters out reserved field names from other_fields
+    /// This prevents conflicts when flattening other_fields during serialization
+    /// 
+    /// Call this method when:
+    /// - Loading AlertConfig from untrusted/legacy storage
+    /// - After programmatically modifying other_fields
+    /// - During migration to ensure clean data
     pub fn sanitize_other_fields(&mut self) {

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between dc796e5 and 920be48.

📒 Files selected for processing (1)

• src/alerts/alert_structs.rs (6 hunks)

🧰 Additional context used

🧠 Learnings (1)

📚 Learning: 2025-07-24T11:09:21.781Z

Learnt from: nikhilsinhaparseable
Repo: parseablehq/parseable PR: 1388
File: src/alerts/mod.rs:88-104
Timestamp: 2025-07-24T11:09:21.781Z
Learning: In the Parseable alert system (src/alerts/mod.rs), alert versions are server-generated and controlled via CURRENT_ALERTS_VERSION constant, not user input. The AlertVerison enum's From<&str> implementation correctly defaults unknown versions to V2 since the server only generates known versions (v1, v2). Unknown versions would only occur in exceptional cases like file corruption, making the current fallback approach appropriate.

Applied to files:

• src/alerts/alert_structs.rs

🧬 Code graph analysis (1)

src/alerts/alert_structs.rs (3)

src/alerts/mod.rs (2)

• serde_json (1034-1034)
• alerts (1329-1333)

src/storage/object_storage.rs (7)

• serde_json (540-540)
• serde_json (584-584)
• serde_json (628-628)
• serde_json (654-654)
• serde_json (701-701)
• alert_json_path (1140-1142)
• alert_state_json_path (1157-1162)

src/query/mod.rs (2)

• serde_json (556-556)
• resolve_stream_names (519-531)

⏰ 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: Build Default x86_64-apple-darwin
• 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 Kafka aarch64-apple-darwin
• GitHub Check: Build Default x86_64-unknown-linux-gnu
• GitHub Check: Build Kafka x86_64-unknown-linux-gnu
• GitHub Check: coverage
• GitHub Check: Quest Smoke and Load Tests for Standalone deployments
• GitHub Check: Quest Smoke and Load Tests for Distributed deployments

🔇 Additional comments (4)

src/alerts/alert_structs.rs (4)

19-40: LGTM! Import reorganization aligns with removed custom deserializer.

The removal of Deserializer from line 22 and the expanded internal crate imports support the transition from custom datetime deserialization to standard handling. All new imports are utilized in the updated logic.

---

42-69: LGTM! Reserved fields now cover both naming conventions.

The expansion correctly includes both camelCase and snake_case variants for all alert configuration fields, preventing collisions when other_fields is flattened during serialization. This aligns with serde's flexible field naming support.

---

329-329: LGTM! Consistent timestamp capture for alert creation.

Capturing created_timestamp at the start of the conversion (line 329) and reusing it (line 368) ensures a consistent creation time throughout the AlertConfig construction, avoiding potential timestamp skew from multiple Utc::now() calls.

Also applies to: 368-368

---

397-397: Verify backward compatibility for v2 alerts in storage without created field.

The removal of serde attributes from the created field at line 397 (and 425 for AlertConfigResponse) is confirmed—it now requires this field during JSON deserialization. While all new alerts created via the build() method (line 328-375) set this field, and all migrated v1 alerts receive it (line 120), any existing v2 alert JSON in storage that lacks the created field will fail to deserialize at line 1034 and be silently skipped.

This is a breaking change if older v2 alert formats were stored without the created field. Verify:

• Whether the created field was always part of the v2 alert schema in your deployment
• If not, either restore #[serde(default)] for backward compatibility or implement a migration for existing alerts