Conversation
ivarne
added
the
feature
Contributor
|
Warning Review limit reached
More reviews will be available in 15 minutes and 12 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR introduces XSD validation support across the Altinn app framework. It adds a new ChangesXSD Schema Validation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
Contributor
There was a problem hiding this comment.
Pull request overview
Adds opt-in XSD-based validation of XML form data, and exposes XSD schemas via a new resource endpoint so apps (and tooling) can retrieve schemas by model id.
Changes:
- Introduces
XsdValidatorand registers it behindAppSettings.XsdValidation. - Extends
IAppResources/AppResourcesSIandResourceControllerwithGetXsdSchema+GET /{org}/{app}/api/xsdschema/{id}. - Updates translations, public API baselines, OpenAPI snapshots, and adds/adjusts tests + test app artifacts to cover the new behavior.
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| test/Altinn.App.Integration.Tests/CustomScopes/_snapshots/CustomScopesTests.Metadata_Standard_0_Metadata.verified.txt | Snapshot update to include new XSD schema endpoint metadata. |
| test/Altinn.App.Integration.Tests/CustomScopes/_snapshots/CustomScopesTests.Metadata_Custom_0_Metadata.verified.txt | Snapshot update to include new XSD schema endpoint metadata. |
| test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt | Public API baseline updated for new setting, validator, and IAppResources.GetXsdSchema. |
| test/Altinn.App.Core.Tests/Implementation/AppResourcesSITests.cs | Test nullability updates for AppResourcesSI construction. |
| test/Altinn.App.Api.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt | Public API baseline updated for new XSD endpoint + deprecation attribute. |
| test/Altinn.App.Api.Tests/OpenApi/OpenApiSpecChangeDetection.SaveJsonSwagger.verified.json | Swagger snapshot updated for new endpoint + deprecated layoutsettings. |
| test/Altinn.App.Api.Tests/OpenApi/OpenApiSpecChangeDetection.SaveCustomOpenApiSpec.verified.json | OpenAPI snapshot updated for schema content changes in test app model. |
| test/Altinn.App.Api.Tests/Data/TestData.cs | Adds helper to rewrite XML data elements for tests. |
| test/Altinn.App.Api.Tests/Data/apps/tdd/permissive-app/models/Skjema.cs | Adjusts XML serialization patterns for AltinnRowId emission. |
| test/Altinn.App.Api.Tests/Data/apps/tdd/contributer-restriction/models/Skjema.xsd | Adds XSD used by integration tests for validation. |
| test/Altinn.App.Api.Tests/Data/apps/tdd/contributer-restriction/models/Skjema.schema.json | Adds minimal JSON schema placeholder for resource tests. |
| test/Altinn.App.Api.Tests/Data/apps/tdd/contributer-restriction/models/Skjema.cs | Updates XML array serialization and adds a field used to trigger XSD validation errors. |
| test/Altinn.App.Api.Tests/Controllers/ValidateController_ValidateInstanceTests.cs | Adds test coverage for XSD validation via XsdValidator. |
| test/Altinn.App.Api.Tests/Controllers/ResourceControllerTests.cs | Adds tests for new XSD schema endpoint (and existing JSON schema behavior). |
| src/Altinn.App.Core/Internal/Texts/TranslationService.cs | Adds default text resource for XSD validation issues. |
| src/Altinn.App.Core/Internal/Data/IDataElementAccessChecker.cs | Makes interface public and documents it (public API surface change). |
| src/Altinn.App.Core/Internal/App/IAppResources.cs | Adds GetXsdSchema(modelId) to app resource abstraction. |
| src/Altinn.App.Core/Implementation/AppResourcesSI.cs | Implements GetXsdSchema by reading {modelId}.xsd from models folder. |
| src/Altinn.App.Core/Features/Validation/Default/XsdValidator.cs | New validator that validates serialized XML against XSD schema if present. |
| src/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cs | Registers XsdValidator when AppSettings.XsdValidation is enabled. |
| src/Altinn.App.Core/Configuration/AppSettings.cs | Adds XsdValidation setting. |
| src/Altinn.App.Api/Controllers/ResourceController.cs | Adds XSD schema endpoint and marks legacy layoutsettings endpoint as obsolete. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
test/Altinn.App.Core.Tests/Implementation/AppResourcesSITests.cs (1)
32-32: ⚡ Quick winReplace repeated
null!constructor injections with an explicit test double.Lines [32], [83], [137], [192], [208], [224], [242], [259], and [277] bypass nullable safety and make these tests fragile if
AppResourcesSIstarts using that dependency on covered paths. Prefer a shared Moq/fake dependency instead.As per coding guidelines: "Use Nullable Reference Types in C# code".
Also applies to: 83-83, 137-137, 192-192, 208-208, 224-224, 242-242, 259-259, 277-277
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/Altinn.App.Core.Tests/Implementation/AppResourcesSITests.cs` at line 32, Replace the repeated null! constructor arguments in AppResourcesSITests with a shared explicit test double (e.g., a Moq mock or simple fake) and inject that into the AppResourcesSI instances used by the tests; specifically, create a single mocked dependency (using Moq or a lightweight fake) in the AppResourcesSITests setup and use it instead of null! for every constructor parameter currently passed as null! (the instances of AppResourcesSI constructed across the test methods), so the tests no longer bypass nullable safety and will be resilient if AppResourcesSI starts using that dependency.test/Altinn.App.Api.Tests/Data/apps/tdd/contributer-restriction/models/Skjema.schema.json (1)
1-3: ⚡ Quick winReplace the placeholder schema with a minimal concrete test schema.
Keeping only a TODO payload makes the fixture ambiguous and reduces confidence in schema-related tests. A minimal explicit JSON schema would make intent and failures clearer.
If you want, I can draft a minimal schema content for this fixture and open a follow-up issue text.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/Altinn.App.Api.Tests/Data/apps/tdd/contributer-restriction/models/Skjema.schema.json` around lines 1 - 3, The current Skjema.schema.json contains only a "$comment" placeholder; replace it with a minimal concrete JSON Schema (e.g., a top-level "type": "object" with a small "properties" map and a "required" array) so tests have an explicit schema to validate against; update the file Skjema.schema.json accordingly and ensure the schema is valid JSON and matches any expectations in tests that reference this fixture (look for usages of Skjema.schema.json or the "$comment" placeholder to confirm compatibility).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Altinn.App.Core/Features/Validation/Default/XsdValidator.cs`:
- Line 15: Change the XsdValidator class declaration from public to internal
sealed to narrow its visibility and prevent inheritance: update the declaration
for the class named XsdValidator that implements IValidator from "public class
XsdValidator" to "internal sealed class XsdValidator" (ensure any tests or
callers in other assemblies are adjusted if they require access).
- Around line 46-49: ShouldRunForTask synchronously blocks on
_appMetadata.GetApplicationMetadata().Result — change design to consult an
asynchronously populated cache (preload metadata during startup and expose it
for synchronous reads) so ShouldRunForTask can read the cached
ApplicationMetadata.DataTypes without awaiting; also change the XsdValidator
class declaration to internal sealed (if no inheritance intended). For schema
parsing, wrap the parsedSchema.Add(null, xsdReader) call in a try/catch that
catches XmlSchemaException (optionally XmlException) and convert parse failures
into a ValidationIssue (do not throw), ensuring validation continues gracefully;
refer to ShouldRunForTask, _appMetadata.GetApplicationMetadata(),
parsedSchema.Add(null, xsdReader), and class XsdValidator when making these
edits.
- Around line 102-105: Wrap the parsedSchema.Add(null, xsdReader) call in a
try/catch that specifically catches XmlSchemaException and converts the failure
into the existing validation error handling used by the XsdValidator class
instead of letting the exception bubble (i.e., catch XmlSchemaException around
the parsedSchema.Add call where the XmlReader is created and do the same for the
second parsedSchema.Add block later). Ensure you reference and use the
validator's existing mechanism for reporting schema errors (add the error to the
validation result/collection or call the class's error reporting helper) so
malformed XSDs surface as validation issues rather than causing a 500.
In
`@test/Altinn.App.Api.Tests/Controllers/ValidateController_ValidateInstanceTests.cs`:
- Around line 168-170: Replace FluentAssertions usages in the
ValidateController_ValidateInstanceTests method with xUnit Assert calls: instead
of response.Should().HaveStatusCode(HttpStatusCode.OK) use
Assert.Equal(HttpStatusCode.OK, response.StatusCode); instead of
parsedResponse.Should().HaveCount(1) use Assert.Single(parsedResponse) (or
Assert.Equal(1, parsedResponse.Count)); keep the call to
ParseResponse<List<ValidationIssue>>(responseString) as-is and add any necessary
Assert.NotNull(parsedResponse) if desired. This targets the assertions around
response and parsedResponse in the test method.
- Around line 174-177: Replace the brittle exact string equality assertion in
ValidateController_ValidateInstanceTests (the Assert.Equal comparing the full
XSD parser message for issue.Description) with stable substring checks: assert
that issue.Description contains the invariant user-facing prefix (e.g., "Et felt
bryter reglene satt av XSD") and the relevant element name (e.g.,
"missing-from-xsd") or use a regex match for those stable parts; update the test
where the Assert.Equal appears to use Assert.Contains/Assert.Matches against
issue.Description instead.
---
Nitpick comments:
In
`@test/Altinn.App.Api.Tests/Data/apps/tdd/contributer-restriction/models/Skjema.schema.json`:
- Around line 1-3: The current Skjema.schema.json contains only a "$comment"
placeholder; replace it with a minimal concrete JSON Schema (e.g., a top-level
"type": "object" with a small "properties" map and a "required" array) so tests
have an explicit schema to validate against; update the file Skjema.schema.json
accordingly and ensure the schema is valid JSON and matches any expectations in
tests that reference this fixture (look for usages of Skjema.schema.json or the
"$comment" placeholder to confirm compatibility).
In `@test/Altinn.App.Core.Tests/Implementation/AppResourcesSITests.cs`:
- Line 32: Replace the repeated null! constructor arguments in
AppResourcesSITests with a shared explicit test double (e.g., a Moq mock or
simple fake) and inject that into the AppResourcesSI instances used by the
tests; specifically, create a single mocked dependency (using Moq or a
lightweight fake) in the AppResourcesSITests setup and use it instead of null!
for every constructor parameter currently passed as null! (the instances of
AppResourcesSI constructed across the test methods), so the tests no longer
bypass nullable safety and will be resilient if AppResourcesSI starts using that
dependency.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1395d518-ae45-4438-a5a7-f51cc33be1f4
📒 Files selected for processing (22)
src/Altinn.App.Api/Controllers/ResourceController.cssrc/Altinn.App.Core/Configuration/AppSettings.cssrc/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cssrc/Altinn.App.Core/Features/Validation/Default/XsdValidator.cssrc/Altinn.App.Core/Implementation/AppResourcesSI.cssrc/Altinn.App.Core/Internal/App/IAppResources.cssrc/Altinn.App.Core/Internal/Data/IDataElementAccessChecker.cssrc/Altinn.App.Core/Internal/Texts/TranslationService.cstest/Altinn.App.Api.Tests/Controllers/ResourceControllerTests.cstest/Altinn.App.Api.Tests/Controllers/ValidateController_ValidateInstanceTests.cstest/Altinn.App.Api.Tests/Data/TestData.cstest/Altinn.App.Api.Tests/Data/apps/tdd/contributer-restriction/models/Skjema.cstest/Altinn.App.Api.Tests/Data/apps/tdd/contributer-restriction/models/Skjema.schema.jsontest/Altinn.App.Api.Tests/Data/apps/tdd/contributer-restriction/models/Skjema.xsdtest/Altinn.App.Api.Tests/Data/apps/tdd/permissive-app/models/Skjema.cstest/Altinn.App.Api.Tests/OpenApi/OpenApiSpecChangeDetection.SaveCustomOpenApiSpec.verified.jsontest/Altinn.App.Api.Tests/OpenApi/OpenApiSpecChangeDetection.SaveJsonSwagger.verified.jsontest/Altinn.App.Api.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txttest/Altinn.App.Core.Tests/Implementation/AppResourcesSITests.cstest/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txttest/Altinn.App.Integration.Tests/CustomScopes/_snapshots/CustomScopesTests.Metadata_Custom_0_Metadata.verified.txttest/Altinn.App.Integration.Tests/CustomScopes/_snapshots/CustomScopesTests.Metadata_Standard_0_Metadata.verified.txt
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
AppSettings.XsdValidationto enable validation form data using xsd schema with the newThanks to @MagnusNordboe for the initial work in #1668
Summary by CodeRabbit
Release Notes
New Features
Tests