Skip to content

Conversation

ivarne
Copy link
Member

@ivarne ivarne commented Sep 28, 2025

For historical reasons ["component"] lookups don't assume component ids are unique across pages, thus a component lookup wanted to know the current page and failed if no component is available. Frontend has since enforced unique component ids across all pages in a layout set, so this restriction no longer applies, but the lookup code is very simple to keep in backend.

TODO:

  • Copy shared tests from frontend (or write them)

Related issues:

Summary by CodeRabbit

  • Bug Fixes

    • Improved stability when evaluating expressions that reference missing components or pages.
    • More robust handling of optional page identifiers to avoid abrupt failures and inconsistent error paths.
    • Safer null handling to reduce edge-case crashes across layouts.
  • Tests

    • Added validation test covering lookups of hidden components.
    • Updated test fixtures and expectations to reflect nullable page handling and cleaned-up test data.

@coderabbitai
Copy link

coderabbitai bot commented Sep 28, 2025

📝 Walkthrough

Walkthrough

ExpressionEvaluator now checks the entire evaluation context for null and forwards a nullable PageId into GetComponentContext; LayoutEvaluatorState.GetComponentContext signature was changed to accept a nullable pageName and only applies the single-match shortcut when pageName is non-null. Tests updated and a new fixture added.

Changes

Cohort / File(s) Summary
Expression evaluator null-check
src/Altinn.App.Core/Internal/Expressions/ExpressionEvaluator.cs
Replaced context?.Component null-guard with a check for context itself; calls GetComponentContext with context.Component?.PageId, allowing a null PageId to be forwarded to the layout lookup.
Layout evaluator API and flow
src/Altinn.App.Core/Internal/Expressions/LayoutEvaluatorState.cs, test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt
Changed method signature to GetComponentContext(string? pageName, string componentId, int[]? rowIndexes = null). The single-match optimization is only used when pageName is non-null; null pageName falls through to multi-match/error handling. Public API verification updated to reflect nullable parameter.
Tests — model and fixture updates
test/Altinn.App.Core.Tests/Features/Validators/Default/ExpressionValidatorTests.cs, test/Altinn.App.Core.Tests/Features/Validators/expression-validation-tests/shared/component-lookup-hidden.json
Removed ComponentId property from nested ExpressionValidationTestModel.ExpectedObject. Added new test fixture component-lookup-hidden.json covering validation when a referenced component is hidden and expecting a null lookup path and a single validation error.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title is concise and specifically describes the main change in the PR by stating that ["component"] lookups will now work in expression validation, matching the PR objectives without extraneous detail.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch but/component-in-expression-validation

📜 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 4b2cd6f and b584358.

📒 Files selected for processing (1)
  • test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt (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). (5)
  • GitHub Check: Run dotnet build and test (ubuntu-latest)
  • GitHub Check: Run dotnet build and test (windows-latest)
  • GitHub Check: Static code analysis
  • GitHub Check: Analyze (csharp)
  • GitHub Check: Run dotnet build and test (macos-latest)

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.

@ivarne ivarne added bugfix Label Pull requests with bugfix. Used when generation releasenotes backport This PR should be cherry-picked onto older release branches labels Sep 28, 2025
@ivarne ivarne force-pushed the but/component-in-expression-validation branch from c7d5c96 to 4b2cd6f Compare October 1, 2025 21:01
Copy link
Contributor

@olemartinorg olemartinorg left a comment

Choose a reason for hiding this comment

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

🙌

@ivarne ivarne enabled auto-merge (squash) October 3, 2025 08:04
@ivarne ivarne merged commit 3ee9a9a into main Oct 3, 2025
12 checks passed
@ivarne ivarne deleted the but/component-in-expression-validation branch October 3, 2025 08:09
@github-actions
Copy link

github-actions bot commented Oct 3, 2025

⚠️ Automatic backport failed due to conflicts

The automatic backport to release/v8.8 failed because of merge conflicts.

The release branch release/v8.8 already existed and was updated.

Manual backport required:

# Checkout the release branch
git checkout release/v8.8
git pull origin release/v8.8

# Create backport branch
git checkout -b backport/1497

# Cherry-pick the merge commit
git cherry-pick 3ee9a9a93e98efce877d03edc831c923963f3500

# Resolve conflicts, then:
git add .
git cherry-pick --continue

# Push and create PR
git push origin backport/1497

@sonarqubecloud
Copy link

sonarqubecloud bot commented Oct 3, 2025

Quality Gate Failed Quality Gate failed

Failed conditions
38.5% Coverage on New Code (required ≥ 65%)
30.0% Condition Coverage on New Code (required ≥ 65%)
D Maintainability Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

ivarne added a commit that referenced this pull request Oct 14, 2025
* Make ["component"] lookups work in expression validation

* Fix snapshot

* Add shared test
ivarne added a commit that referenced this pull request Oct 14, 2025
* Make ["component"] lookups work in expression validation

* Fix snapshot

* Add shared test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport This PR should be cherry-picked onto older release branches bugfix Label Pull requests with bugfix. Used when generation releasenotes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants