Skip to content

Revert "Fix allow date filter comparison on time aspect (#682)"#829

Merged
HenningNormann merged 2 commits intomainfrom
revert-get-app-events-cast
Nov 3, 2025
Merged

Revert "Fix allow date filter comparison on time aspect (#682)"#829
HenningNormann merged 2 commits intomainfrom
revert-get-app-events-cast

Conversation

@eskebab
Copy link
Contributor

@eskebab eskebab commented Nov 3, 2025

This reverts commit 226722c.

Description

Reverts changes to get app events function introduced to fix the issue below. Not able to recreate the original issue with the previous version that properly used the time index.

Related Issue(s)

Verification

  • Your code builds clean without any errors or warnings
  • Manual testing done (required)
  • Relevant automated test added (if you find this hard, leave it and we'll help out)
  • All tests run green

Documentation

  • User documentation is updated with a separate linked PR in altinn-studio-docs. (if applicable)

Summary by CodeRabbit

  • Bug Fixes

    • Corrected time filtering logic in event queries to properly compare timestamps.
  • New Features

    • Added subscription management capabilities including creation, deletion, and validation.
    • Enhanced event retrieval with improved filtering options by subject, time range, type, and source.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 3, 2025

📝 Walkthrough

Walkthrough

Two files modified: one alters time filtering logic in getappevents.sql from timestamptz casting to text comparison, and another adds ten new PL/pgSQL functions and procedures in v0.48 migration for subscription and event management operations including creation, querying, insertion, and validation.

Changes

Cohort / File(s) Summary
Time filtering logic modification
src/Events/Migration/FunctionsAndProcedures/getappevents.sql
Changed time comparison from cloudevent->>'time' cast as timestamptz to cast as text, with corresponding parameter conversions (_from::text, _to::text). Predicate structure and WHERE clause logic preserved.
New subscription and event management PL/pgSQL objects
src/Events/Migration/v0.48/01-functions-and-procedures.sql
Added 10 database objects: create_logs_partition(), deletesubscription(), find_subscription(), getappevents_v2(), getevents(), getsubscription_v2(), getsubscriptions(), getsubscriptionsbyconsumer_v2(), insert_subscription(), and setvalidsubscription(). Provide subscription CRUD operations, event filtering by multiple dimensions (subject, time range, type, source, resource), and partition management.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

  • Attention areas:
    • Verify correctness of text-based time comparison in modified getappevents.sql—confirm that string comparison semantics match intended behavior versus timestamptz comparison
    • Review complex filtering logic in getappevents_v2() and getevents() for boundary conditions (sequence bounds, time ranges, array type handling)
    • Validate subscription insertion in insert_subscription() for UTC timestamp handling and field mapping
    • Confirm partition creation logic in create_logs_partition() for monthly trace_log partitioning correctness

Pre-merge checks and finishing touches

✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The pull request title accurately describes the main change: reverting a previous fix to the date filter comparison logic in the getappevents function. The title directly relates to the modifications shown in the raw_summary, where the getappevents.sql file is altered to change the time filtering logic. The revert reference to PR #682 provides additional context about which specific changes are being undone.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch revert-get-app-events-cast

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

@eskebab eskebab marked this pull request as draft November 3, 2025 11:14
@eskebab eskebab force-pushed the revert-get-app-events-cast branch from 287023c to 6f12c59 Compare November 3, 2025 11:40
@eskebab eskebab marked this pull request as ready for review November 3, 2025 11:41
@sonarqubecloud
Copy link

sonarqubecloud bot commented Nov 3, 2025

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e0439a4 and 6f12c59.

📒 Files selected for processing (1)
  • src/Events/Migration/v0.48/01-functions-and-procedures.sql (1 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). (1)
  • GitHub Check: Analyze (csharp)
🔇 Additional comments (2)
src/Events/Migration/v0.48/01-functions-and-procedures.sql (2)

1-279: Auto-generated migration requires verification of temporal and type semantics.

This file is marked as auto-generated from DbTools (line 1). Ensure the code generation tool is handling:

  • Proper timezone handling in all timestamp-related operations (particularly in insert_subscription, which sets currentTime := NOW())
  • Consistent parameter types across function signatures
  • Correct casting of JSON-extracted values to appropriate SQL types

Additionally, confirm that reverting the timestamptz casting in getappevents_v2 (the core of PR #829) actually resolves the original issue #673 without introducing new correctness problems.


120-121: Text-based timestamp comparison in v0.48 revert requires verification of format enforcement and impact.

This PR explicitly reverts commit #682 ("Fix allow date filter comparison on time aspect"), switching back from proper timestamptz casting to text comparison in the getappevents_v2 function (lines 120–121):

AND (_from IS NULL OR cloudevent->>'time' >= _from::text)
AND (_to IS NULL OR cloudevent->>'time' <= _to::text)

The revert commit provides no explanation for why the prior fix was reverted. Verify:

  • ISO8601 format is consistently enforced in the cloudevent->>'time' field (text comparison only works correctly if format is uniform)
  • Timezone information handling is correct—timestamptz comparison understands time zones; text comparison does not
  • Edge cases (fractional seconds, timezone offsets, format variations) do not introduce ordering or filtering discrepancies
  • Index utilization and query performance are not degraded

If #682's timestamptz approach caused issues, debug and address those issues rather than reverting to the less robust text-comparison approach.

@HenningNormann HenningNormann merged commit 64350fc into main Nov 3, 2025
13 checks passed
@HenningNormann HenningNormann deleted the revert-get-app-events-cast branch November 3, 2025 19:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants