Skip to content

Conversation

@sigridbra
Copy link
Contributor

@sigridbra sigridbra commented Nov 28, 2025

Description

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

  • Refactor

    • Consolidated cloud event deserialization into a single, simpler flow and removed the legacy conditional path.
  • Behavior Changes

    • Empty or null serialized input now fails fast with a clear argument error.
    • Deserialization errors are no longer silently swallowed and will surface to callers.
  • Tests

    • Updated tests to validate the wrapped-event flow and removed legacy direct-envelope scenarios.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 28, 2025

Warning

Rate limit exceeded

@sigridbra has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 1 minutes and 45 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Reviewing files that changed from the base of the PR and between 2488cd1 and bd991fc.

📒 Files selected for processing (1)
  • src/Events.Functions/Models/CloudEventEnvelope.cs (1 hunks)
📝 Walkthrough

Walkthrough

Replaces legacy and conditional deserialization paths with a single wrapped-envelope flow; deserialization now requires non-empty input and exceptions from wrapper parsing are allowed to propagate. Tests were adjusted to exercise the wrapped-envelope path only.

Changes

Cohort / File(s) Summary
Production code: deserialization & wrapper handling
src/Events.Functions/EventsOutbound.cs, src/Events.Functions/Extensions/RetryableEventWrapperExtensions.cs, src/Events.Functions/Models/CloudEventEnvelope.cs
Consolidates deserialization to always read the CloudEventEnvelope from wrapperCandidate?.Payload (removing legacy fallback). DeserializeToRetryableEventWrapper no longer catches JSON exceptions (exceptions now propagate). DeserializeToCloudEventEnvelope accepts string? and now throws on null/empty input via ArgumentException.ThrowIfNullOrEmpty.
Tests: adjust to wrapped-envelope scenarios
test/Altinn.Platform.Events.Functions.Tests/TestingFunctions/EventsOutboundTests.cs
Removed direct CloudEventEnvelope test, renamed/updated test to supply a serialized RetryableEventWrapper (with Payload) and verify wrapped-envelope processing; adjusted deserialization helper expectations (empty input now yields ArgumentException).

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

  • Verify null-safety where wrapperCandidate may be null and confirm callers guarantee wrapper presence or add checks.
  • Confirm new exception propagation from DeserializeToRetryableEventWrapper is handled by callers (requeueing/observability behavior).
  • Validate the change in DeserializeToCloudEventEnvelope signature and new ArgumentException behavior doesn't break consumers or existing test expectations.
  • Ensure removed legacy path is intentionally deprecated and external integrations relying on direct envelope payload are accounted for.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately reflects the main change: removing legacy cloudEventEnvelope deserialization support, which is the primary objective evident from the raw_summary and pr_objectives.

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.

@sonarqubecloud
Copy link

@sigridbra sigridbra requested a review from eskebab November 28, 2025 12:09
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.

1 participant