Skip to content

Conversation

@uddhavdave
Copy link
Contributor

@uddhavdave uddhavdave commented Dec 2, 2025

fixes #9443

@github-actions
Copy link
Contributor

github-actions bot commented Dec 2, 2025

Failed to generate code suggestions for PR

@github-actions github-actions bot added the ☢️ Bug Something isn't working label Dec 2, 2025
@greptile-apps
Copy link
Contributor

greptile-apps bot commented Dec 2, 2025

Greptile Overview

Greptile Summary

Fixed null value handling in alert condition evaluation and string comparison logic:

  • Added support for evaluating Value::Null in condition checks - now correctly returns true when operator is EqualTo and condition value is the string "null"
  • Fixed string comparison by trimming quotes from condition values using trim_matches('"') to handle JSON-serialized string values properly

Both changes improve the robustness of alert condition evaluation when dealing with null values and quoted strings in JSON data.

Confidence Score: 4/5

  • This PR is safe to merge with minimal risk - it fixes specific edge cases in alert evaluation logic
  • The changes are focused and defensive, improving null handling and string comparison. However, the lack of test coverage and potential edge cases around the trim_matches('"') logic prevent a perfect score
  • No files require special attention, but verify alert behavior with null values and quoted strings in production

Important Files Changed

File Analysis

Filename Score Overview
src/service/alerts/mod.rs 4/5 Added null value handling in condition evaluation and fixed string comparison by trimming quotes from condition values

Sequence Diagram

sequenceDiagram
    participant User
    participant Alert System
    participant ConditionExt
    participant Value Handler
    
    User->>Alert System: Trigger alert evaluation
    Alert System->>ConditionExt: evaluate(row)
    ConditionExt->>ConditionExt: Get column value from row
    
    alt Column not found
        ConditionExt-->>Alert System: return false
    else Column found
        ConditionExt->>Value Handler: Match on value type
        
        alt Value::String
            Value Handler->>Value Handler: trim_matches('"') on condition value
            Value Handler->>Value Handler: Compare strings with operator
            Value Handler-->>ConditionExt: Return comparison result
        else Value::Number
            Value Handler->>Value Handler: Parse and compare numbers
            Value Handler-->>ConditionExt: Return comparison result
        else Value::Bool
            Value Handler->>Value Handler: Parse and compare booleans
            Value Handler-->>ConditionExt: Return comparison result
        else Value::Null (NEW)
            Value Handler->>Value Handler: Check if operator is EqualTo
            Value Handler->>Value Handler: Check if condition value is "null" string
            Value Handler-->>ConditionExt: Return true if both match
        else Other types
            Value Handler-->>ConditionExt: return false
        end
        
        ConditionExt-->>Alert System: Return evaluation result
    end
    Alert System-->>User: Alert triggered or not
Loading

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 file reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

@uddhavdave uddhavdave merged commit 25c3b1d into main Dec 2, 2025
42 of 47 checks passed
@uddhavdave uddhavdave deleted the ud/fix-conditions-pipeline-main branch December 2, 2025 19:04
@uddhavdave uddhavdave added this to the v0.30.0 milestone Dec 3, 2025
@Shrinath-O2 Shrinath-O2 added Needs-Testing Needs-Testing Needs-Automation Needs-Automation labels Dec 3, 2025
@Shrinath-O2 Shrinath-O2 added Testing-Completed Testing-Completed and removed Needs-Testing Needs-Testing labels Dec 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

☢️ Bug Something isn't working Needs-Automation Needs-Automation Testing-Completed Testing-Completed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Null check in pipeline condition is broken

3 participants