Add utilites for reading using xsd files from the models#1615
Add utilites for reading using xsd files from the models#1615
Conversation
📝 WalkthroughWalkthroughAdds GET /{org}/{app}/api/xsdschema/{id} to return model XSDs, implements IAppResources.GetXsdSchema and AppResourcesSI.GetXsdSchema, marks existing GetLayoutSettings(org, app) obsolete, and adds tests, test fixtures, and OpenAPI/public-API updates. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @src/Altinn.App.Api/Controllers/ResourceController.cs:
- Around line 38-53: Add XML documentation for the GetModelXsdSchema method:
include a <param name="id"> describing the id route parameter (e.g., "The
resource id for which to retrieve the XSD schema") and a <returns> tag that
documents the possible responses (200 OK returning "application/xml" with the
schema, and 404 NotFound when the schema is missing). Update the triple-slash
comment immediately above the GetModelXsdSchema method (in
ResourceController.GetModelXsdSchema) to include these tags and brief
descriptions; no code changes required.
In @src/Altinn.App.Core/Implementation/AppResourcesSI.cs:
- Around line 527-541: GetXsdSchema lacks telemetry; wrap its body with the same
pattern used in GetValidationConfiguration by adding a using var activity =
_telemetry?.StartGetXsdSchemaActivity(); at the start of GetXsdSchema (before
Path.Join calls) and ensure the telemetry StartGetXsdSchemaActivity method is
implemented on the Telemetry class (matching naming/pattern of
StartGetValidationConfigurationActivity) so XSD retrievals are tracked.
In @test/Altinn.App.Api.Tests/Controllers/ResourceControllerTests.cs:
- Around line 26-37: The test method
GetModelJsonSchema_InvalidDataTypeId_ReturnsNotFound in
ResourceControllerTests.cs is misnamed because it asserts a
FileNotFoundException rather than a 404; rename the test to reflect the actual
behavior (e.g.,
GetModelJsonSchema_InvalidDataTypeId_ThrowsFileNotFoundException) and update any
references/usages of that method, leaving the test body unchanged (or
alternatively change the assertion to check response.StatusCode == NotFound if
you intend to change behavior instead of the name).
🧹 Nitpick comments (2)
test/Altinn.App.Api.Tests/Data/apps/tdd/contributer-restriction/models/Skjema.xsd (1)
1-4: Consider completing the test XSD schema.The TODO comment indicates this schema should be kept in sync with the actual model. For robust testing of the XSD retrieval functionality, consider adding actual type definitions and elements that match the data model, rather than using a minimal placeholder schema.
test/Altinn.App.Api.Tests/Data/apps/tdd/contributer-restriction/models/Skjema.schema.json (1)
1-3: Consider completing the test JSON schema.The TODO comment suggests uncertainty about whether complete content is needed. For comprehensive testing of the schema retrieval endpoints, consider adding actual schema definitions that represent a realistic data model structure.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/Altinn.App.Api/Controllers/ResourceController.cssrc/Altinn.App.Core/Implementation/AppResourcesSI.cssrc/Altinn.App.Core/Internal/App/IAppResources.cstest/Altinn.App.Api.Tests/Controllers/ResourceControllerTests.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/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.txt
🧰 Additional context used
📓 Path-based instructions (2)
**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.cs: Use CSharpier for code formatting (required before commits). Formatting happens automatically when building due to CSharpier.MSBuild
Use internal accessibility on types by default
Use sealed for classes unless inheritance is considered a valid use-case
Use Nullable Reference Types in C#
Remember to dispose IDisposable/IAsyncDisposable instances
For HTTP APIs, define ...Request and ...Response DTOs (e.g., LookupPersonRequest.cs and corresponding response)
Types meant to be implemented by apps should be marked with the ImplementableByApps attribute
Don't use .GetAwaiter().GetResult(), .Result(), .Wait() or other blocking APIs on Task
Don't use Async suffix for async methods
Write efficient code: Don't allocate unnecessarily (e.g., avoid calling ToString twice in a row, store in a variable; sometimes a for loop is better than LINQ)
Don't invoke the same async operation multiple times in the same codepath unless necessary
Don't await async operations in a loop (prefer batching, but maintain an upper bound on parallelism that makes sense)
Use strongly-typed configuration classes instead of untyped configuration
Register services in DI container properly following existing patterns
Files:
src/Altinn.App.Core/Internal/App/IAppResources.cssrc/Altinn.App.Api/Controllers/ResourceController.cstest/Altinn.App.Api.Tests/Controllers/ResourceControllerTests.cstest/Altinn.App.Core.Tests/Implementation/AppResourcesSITests.cssrc/Altinn.App.Core/Implementation/AppResourcesSI.cs
**/test/**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
**/test/**/*.cs: Prefer xUnit asserts over FluentAssertions in tests
Mock external dependencies with Moq in tests
Files:
test/Altinn.App.Api.Tests/Controllers/ResourceControllerTests.cstest/Altinn.App.Core.Tests/Implementation/AppResourcesSITests.cs
🧠 Learnings (15)
📓 Common learnings
Learnt from: CR
Repo: Altinn/app-lib-dotnet PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T08:07:52.106Z
Learning: New features should follow the established pattern in /src/Altinn.App.Core/Features/ with feature-specific folders, DI registration, telemetry, and test coverage
📚 Learning: 2025-09-21T08:19:43.290Z
Learnt from: ivarne
Repo: Altinn/app-lib-dotnet PR: 1473
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:1414-1414
Timestamp: 2025-09-21T08:19:43.290Z
Learning: In Altinn/app-lib-dotnet, *.verified.txt files (e.g., test/**/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt) are verification snapshots for guarding public API. Do not generate review suggestions based on these files; ignore them when assessing code changes.
Applied to files:
test/Altinn.App.Api.Tests/Data/apps/tdd/contributer-restriction/models/Skjema.schema.jsontest/Altinn.App.Api.Tests/Controllers/ResourceControllerTests.cstest/Altinn.App.Api.Tests/OpenApi/OpenApiSpecChangeDetection.SaveJsonSwagger.verified.jsontest/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txttest/Altinn.App.Api.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt
📚 Learning: 2025-08-22T13:46:43.017Z
Learnt from: martinothamar
Repo: Altinn/app-lib-dotnet PR: 1446
File: test/Altinn.App.Integration.Tests/Basic/_snapshots/BasicAppTests.Full_auth=ServiceOwner_testCase=MultipartXmlPrefill_7_Logs.verified.txt:41-42
Timestamp: 2025-08-22T13:46:43.017Z
Learning: In Altinn App Integration Tests, OldUser and OldServiceOwner test snapshots intentionally preserve the legacy "AuthMethod: localtest" authentication method as part of a migration strategy, while new User and ServiceOwner tests use updated authentication methods like BankID and maskinporten. The "Old*" variants should not be updated to remove localtest references.
Applied to files:
test/Altinn.App.Api.Tests/Data/apps/tdd/contributer-restriction/models/Skjema.schema.jsontest/Altinn.App.Core.Tests/Implementation/AppResourcesSITests.cstest/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt
📚 Learning: 2025-08-22T13:35:09.565Z
Learnt from: martinothamar
Repo: Altinn/app-lib-dotnet PR: 1446
File: test/Altinn.App.Integration.Tests/Basic/_snapshots/BasicAppTests.Authentication_auth=OldUser.verified.txt:16-38
Timestamp: 2025-08-22T13:35:09.565Z
Learning: Test data in Altinn App Integration Tests snapshots (like SSNs, addresses, phone numbers in BasicAppTests.Authentication snapshots) are synthetic/fake values created for testing purposes, not real PII that needs to be scrubbed from version control.
Applied to files:
test/Altinn.App.Api.Tests/Data/apps/tdd/contributer-restriction/models/Skjema.schema.jsontest/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt
📚 Learning: 2025-09-07T20:03:48.030Z
Learnt from: ivarne
Repo: Altinn/app-lib-dotnet PR: 1463
File: test/Altinn.App.SourceGenerator.Tests/DiagnosticTests.RunJsonError.verified.txt:1-21
Timestamp: 2025-09-07T20:03:48.030Z
Learning: In Altinn.App.SourceGenerator.Tests, the path C:\temp\config\applicationmetadata.json is a hardcoded test fixture path required by Roslyn and is used consistently across all operating systems, not an actual filesystem path that varies by OS.
Applied to files:
test/Altinn.App.Api.Tests/Data/apps/tdd/contributer-restriction/models/Skjema.schema.jsontest/Altinn.App.Api.Tests/Controllers/ResourceControllerTests.cstest/Altinn.App.Core.Tests/Implementation/AppResourcesSITests.cs
📚 Learning: 2025-08-29T10:45:57.158Z
Learnt from: martinothamar
Repo: Altinn/app-lib-dotnet PR: 1456
File: test/Altinn.App.Integration.Tests/_fixture/Tests.cs:3-4
Timestamp: 2025-08-29T10:45:57.158Z
Learning: In Altinn.App.Integration.Tests project, xUnit attributes like [Fact] are available without explicit "using Xunit;" directives, likely through global usings or implicit usings configuration. The code compiles and works as-is.
Applied to files:
test/Altinn.App.Api.Tests/Controllers/ResourceControllerTests.cstest/Altinn.App.Core.Tests/Implementation/AppResourcesSITests.cs
📚 Learning: 2025-11-28T08:07:52.106Z
Learnt from: CR
Repo: Altinn/app-lib-dotnet PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T08:07:52.106Z
Learning: New features should follow the established pattern in /src/Altinn.App.Core/Features/ with feature-specific folders, DI registration, telemetry, and test coverage
Applied to files:
test/Altinn.App.Api.Tests/Controllers/ResourceControllerTests.cstest/Altinn.App.Core.Tests/Implementation/AppResourcesSITests.cs
📚 Learning: 2025-08-29T10:45:57.158Z
Learnt from: martinothamar
Repo: Altinn/app-lib-dotnet PR: 1456
File: test/Altinn.App.Integration.Tests/_fixture/Tests.cs:3-4
Timestamp: 2025-08-29T10:45:57.158Z
Learning: In Altinn app-lib-dotnet projects, ImplicitUsings is enabled in Directory.Build.props, which automatically includes common using statements including Xunit namespace for test projects. This eliminates the need for explicit "using Xunit;" statements in test files.
Applied to files:
test/Altinn.App.Api.Tests/Controllers/ResourceControllerTests.cstest/Altinn.App.Core.Tests/Implementation/AppResourcesSITests.cs
📚 Learning: 2025-11-28T08:07:52.106Z
Learnt from: CR
Repo: Altinn/app-lib-dotnet PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T08:07:52.106Z
Learning: Applies to **/*.cs : Use Nullable Reference Types in C#
Applied to files:
test/Altinn.App.Core.Tests/Implementation/AppResourcesSITests.cs
📚 Learning: 2025-10-17T07:45:15.474Z
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 1474
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:233-237
Timestamp: 2025-10-17T07:45:15.474Z
Learning: In Altinn/app-lib-dotnet, maintainers accept source compatibility without guaranteeing binary compatibility across versions because consumers upgrade via NuGet and rebuild. Adding optional parameters to public extension methods (e.g., CancellationToken) is acceptable provided release notes document the change.
Applied to files:
test/Altinn.App.Core.Tests/Implementation/AppResourcesSITests.cstest/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt
📚 Learning: 2025-08-27T06:58:28.126Z
Learnt from: cammiida
Repo: Altinn/app-lib-dotnet PR: 1451
File: src/Altinn.App.Api/Controllers/DataTagsController.cs:0-0
Timestamp: 2025-08-27T06:58:28.126Z
Learning: In the Altinn app-lib-dotnet codebase, validation endpoints follow a pattern where validation only runs when the ignoredValidators query parameter is provided (not null). This is a conscious design choice for consistency across validation endpoints.
Applied to files:
test/Altinn.App.Core.Tests/Implementation/AppResourcesSITests.cs
📚 Learning: 2025-09-21T09:04:43.977Z
Learnt from: ivarne
Repo: Altinn/app-lib-dotnet PR: 1473
File: src/Altinn.App.Core/Features/IInstanceDataAccessor.cs:36-41
Timestamp: 2025-09-21T09:04:43.977Z
Learning: The IInstanceDataAccessor interface in Altinn.App.Core is designed for consumption only - users are not expected to implement this interface themselves, only use framework-provided implementations.
Applied to files:
test/Altinn.App.Core.Tests/Implementation/AppResourcesSITests.cs
📚 Learning: 2025-09-29T08:34:53.864Z
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 745
File: src/Altinn.App.Core/EFormidling/Interface/IEFormidlingService.cs:18-30
Timestamp: 2025-09-29T08:34:53.864Z
Learning: Altinn.App.Core project targets net8.0, not netstandard2.0, which supports default interface methods.
Applied to files:
test/Altinn.App.Core.Tests/Implementation/AppResourcesSITests.cs
📚 Learning: 2025-09-29T08:34:53.864Z
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 745
File: src/Altinn.App.Core/EFormidling/Interface/IEFormidlingService.cs:18-30
Timestamp: 2025-09-29T08:34:53.864Z
Learning: Altinn.App.Core project targets net8.0, not netstandard2.0, which fully supports default interface methods.
Applied to files:
test/Altinn.App.Core.Tests/Implementation/AppResourcesSITests.cs
📚 Learning: 2025-08-29T05:32:47.222Z
Learnt from: martinothamar
Repo: Altinn/app-lib-dotnet PR: 1456
File: test/Altinn.App.Integration.Tests/_fixture/AppFixture.cs:958-958
Timestamp: 2025-08-29T05:32:47.222Z
Learning: In Altinn.App.Integration.Tests, the maintainer prefers fail-fast behavior (Environment.FailFast) when critical test infrastructure components like log reading fail, rather than graceful error handling, because diagnostics are essential for debugging test failures.
Applied to files:
test/Altinn.App.Core.Tests/Implementation/AppResourcesSITests.cs
🧬 Code graph analysis (3)
src/Altinn.App.Core/Internal/App/IAppResources.cs (1)
src/Altinn.App.Core/Implementation/AppResourcesSI.cs (1)
GetXsdSchema(528-541)
src/Altinn.App.Api/Controllers/ResourceController.cs (2)
src/Altinn.App.Core/Implementation/AppResourcesSI.cs (3)
GetXsdSchema(528-541)Obsolete(221-250)Obsolete(313-329)src/Altinn.App.Core/Internal/App/IAppResources.cs (6)
GetXsdSchema(173-173)Obsolete(41-45)Obsolete(51-55)Obsolete(61-65)Obsolete(85-86)Obsolete(137-138)
src/Altinn.App.Core/Implementation/AppResourcesSI.cs (2)
src/Altinn.App.Core/Internal/App/IAppResources.cs (1)
GetXsdSchema(173-173)src/Altinn.App.Core/Helpers/PathHelper.cs (2)
PathHelper(6-34)EnsureLegalPath(27-33)
⏰ 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 (windows-latest)
- GitHub Check: Run dotnet build and test (macos-latest)
- GitHub Check: Static code analysis
- GitHub Check: Run dotnet build and test (ubuntu-latest)
- GitHub Check: Analyze (csharp)
🔇 Additional comments (10)
test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt (2)
2350-2360: Public API snapshot updated forAppResourcesSI.GetXsdSchemaThe snapshot correctly includes the new
GetXsdSchema(string dataTypeId)method onAppResourcesSI; no issues from a public‑API perspective.
2928-2935: Public API snapshot updated forIAppResources.GetXsdSchemaThe internal
IAppResourcesinterface now exposesGetXsdSchema(string dataTypeId), matching the implementation and intended new functionality.test/Altinn.App.Core.Tests/Implementation/AppResourcesSITests.cs (2)
32-32: LGTM! Appropriate null-forgiving operators for test code.The use of
null!to suppress nullable warnings when passing null to non-nullable parameters is appropriate for test code where null values are intentionally used. This aligns with enabling Nullable Reference Types across the codebase.Also applies to: 83-83, 137-137, 192-192, 208-208, 224-224, 242-242, 259-259, 277-277
307-307: LGTM! Proper nullable parameter declaration.The parameter type change from
IFrontendFeaturestoIFrontendFeatures?with a nullable default correctly expresses the method's intent and supports the nullable reference types pattern.src/Altinn.App.Core/Internal/App/IAppResources.cs (1)
170-173: New GetXsdSchema interface member looks correctSignature and nullability align with the AppResourcesSI implementation and expected 404 behavior in the controller; no issues from an API-contract perspective.
test/Altinn.App.Api.Tests/OpenApi/OpenApiSpecChangeDetection.SaveJsonSwagger.verified.json (1)
5088-5163: xsdschema OpenAPI path matches the new endpoint behaviorThe new
/api/xsdschema/{id}path mirrors the existing jsonschema endpoint with appropriate path params, XML response type, and ProblemDetails 404 responses; looks consistent with the controller and resource conventions.src/Altinn.App.Api/Controllers/ResourceController.cs (1)
100-102: LGTM! Clear deprecation guidance.The
Obsoleteattribute correctly guides users to the layout set-specific endpoint while maintaining backwards compatibility.test/Altinn.App.Api.Tests/Controllers/ResourceControllerTests.cs (3)
1-14: LGTM! Well-structured test class.The test class follows established patterns for API testing with proper use of
WebApplicationFactoryandApiTestBase.
15-24: LGTM! Adequate test coverage for JSON schema endpoint.The test correctly validates the happy path with appropriate assertions. The TODO comment appropriately acknowledges the test data limitation.
39-56: LGTM! Comprehensive test coverage for XSD schema endpoint.Both tests correctly validate the new endpoint:
- The success case properly checks for XML content
- The error case correctly expects 404 NotFound, which demonstrates proper HTTP semantics compared to the existing JSON schema endpoint
The TODO comment at Line 47 appropriately acknowledges the test data limitation.
| /// <summary> | ||
| /// Get the xsd schema for the model | ||
| /// </summary> | ||
| [HttpGet] | ||
| [ProducesResponseType(typeof(string), StatusCodes.Status200OK, "application/xml")] | ||
| [ProducesResponseType(StatusCodes.Status404NotFound)] | ||
| [Route("{org}/{app}/api/xsdschema/{id}")] | ||
| public ActionResult GetModelXsdSchema([FromRoute] string id) | ||
| { | ||
| string? schema = _appResourceService.GetXsdSchema(id); | ||
| if (schema == null) | ||
| { | ||
| return NotFound(); | ||
| } | ||
| return Content(schema, "application/xml", System.Text.Encoding.UTF8); | ||
| } |
There was a problem hiding this comment.
Add missing parameter documentation.
The XML documentation is missing the <param name="id"> tag and could benefit from a <returns> tag explaining the 200 vs 404 responses.
📝 Suggested documentation improvement
/// <summary>
/// Get the xsd schema for the model
/// </summary>
+/// <param name="id">Unique identifier of the model to fetch XSD schema for.</param>
+/// <returns>The XSD schema as XML if found; otherwise, 404 NotFound.</returns>
[HttpGet]
[ProducesResponseType(typeof(string), StatusCodes.Status200OK, "application/xml")]
[ProducesResponseType(StatusCodes.Status404NotFound)]Note: This endpoint properly returns 404 for missing schemas, which is an improvement over GetModelJsonSchema (lines 24-36) that throws FileNotFoundException. Consider aligning the error handling of GetModelJsonSchema with this approach in a future update.
🤖 Prompt for AI Agents
In @src/Altinn.App.Api/Controllers/ResourceController.cs around lines 38 - 53,
Add XML documentation for the GetModelXsdSchema method: include a <param
name="id"> describing the id route parameter (e.g., "The resource id for which
to retrieve the XSD schema") and a <returns> tag that documents the possible
responses (200 OK returning "application/xml" with the schema, and 404 NotFound
when the schema is missing). Update the triple-slash comment immediately above
the GetModelXsdSchema method (in ResourceController.GetModelXsdSchema) to
include these tags and brief descriptions; no code changes required.
| /// <inheritdoc /> | ||
| public string? GetXsdSchema(string dataTypeId) | ||
| { | ||
| string legalPath = Path.Join(_settings.AppBasePath, _settings.ModelsFolder); | ||
| string filename = Path.Join(legalPath, $"{dataTypeId}.xsd"); | ||
| PathHelper.EnsureLegalPath(legalPath, filename); | ||
|
|
||
| string? filedata = null; | ||
| if (File.Exists(filename)) | ||
| { | ||
| filedata = File.ReadAllText(filename, Encoding.UTF8); | ||
| } | ||
|
|
||
| return filedata; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add telemetry to match the established pattern.
The GetXsdSchema method is missing telemetry instrumentation, which is inconsistent with other resource-loading methods in this class. For example, GetValidationConfiguration (line 513) includes:
using var activity = _telemetry?.StartGetValidationConfigurationActivity();Consider adding similar telemetry to track XSD schema retrieval operations.
📊 Proposed addition
/// <inheritdoc />
public string? GetXsdSchema(string dataTypeId)
{
+ using var activity = _telemetry?.StartGetXsdSchemaActivity();
string legalPath = Path.Join(_settings.AppBasePath, _settings.ModelsFolder);
string filename = Path.Join(legalPath, $"{dataTypeId}.xsd");
PathHelper.EnsureLegalPath(legalPath, filename);
string? filedata = null;
if (File.Exists(filename))
{
filedata = File.ReadAllText(filename, Encoding.UTF8);
}
return filedata;
}Note: You'll also need to add the corresponding telemetry activity method to the Telemetry class.
🤖 Prompt for AI Agents
In @src/Altinn.App.Core/Implementation/AppResourcesSI.cs around lines 527 - 541,
GetXsdSchema lacks telemetry; wrap its body with the same pattern used in
GetValidationConfiguration by adding a using var activity =
_telemetry?.StartGetXsdSchemaActivity(); at the start of GetXsdSchema (before
Path.Join calls) and ensure the telemetry StartGetXsdSchemaActivity method is
implemented on the Telemetry class (matching naming/pattern of
StartGetValidationConfigurationActivity) so XSD retrievals are tracked.
| [Fact] | ||
| public async Task GetModelJsonSchema_InvalidDataTypeId_ReturnsNotFound() | ||
| { | ||
| var client = GetRootedClient(Org, App); | ||
| await Assert.ThrowsAsync<FileNotFoundException>(async () => | ||
| { | ||
| using var response = await client.GetAsync($"/{Org}/{App}/api/jsonschema/InvalidDataTypeId"); | ||
| // TODO: Should probably return 404 NotFound instead of throwing FileNotFoundException, | ||
| // but adding test for current behaviour. | ||
| // Assert.Equal(System.Net.HttpStatusCode.NotFound, response.StatusCode); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Test name doesn't match the behavior being tested.
The test is named GetModelJsonSchema_InvalidDataTypeId_ReturnsNotFound but it actually expects a FileNotFoundException to be thrown, not a 404 status code. While the TODO comment (lines 33-35) acknowledges this inconsistency, the test name should reflect the actual behavior being verified.
📝 Suggested test name update
-public async Task GetModelJsonSchema_InvalidDataTypeId_ReturnsNotFound()
+public async Task GetModelJsonSchema_InvalidDataTypeId_ThrowsFileNotFoundException()Note: The TODO comment correctly identifies that the endpoint should return 404 NotFound instead of throwing an exception. This could be addressed in a follow-up improvement to align with the GetModelXsdSchema endpoint behavior.
🤖 Prompt for AI Agents
In @test/Altinn.App.Api.Tests/Controllers/ResourceControllerTests.cs around
lines 26 - 37, The test method
GetModelJsonSchema_InvalidDataTypeId_ReturnsNotFound in
ResourceControllerTests.cs is misnamed because it asserts a
FileNotFoundException rather than a 404; rename the test to reflect the actual
behavior (e.g.,
GetModelJsonSchema_InvalidDataTypeId_ThrowsFileNotFoundException) and update any
references/usages of that method, leaving the test body unchanged (or
alternatively change the assertion to check response.StatusCode == NotFound if
you intend to change behavior instead of the name).
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
ivarne
force-pushed
the
feat/methods-to-access-xsd
branch
from
f01d20c to
4efd20d
Compare
ivarne
added
feature
|
/publish |
PR release:
|
ivarne
force-pushed
the
feat/methods-to-access-xsd
branch
from
4efd20d to
f1c9038
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/Altinn.App.Core/Implementation/AppResourcesSI.cs (1)
527-541: Add telemetry activity for consistency with similar methods.The implementation is correct, but it's missing telemetry instrumentation that's present in all comparable methods in this class (e.g.,
GetValidationConfiguration,GetModelJsonSchema,GetPrefillJson). Adding telemetry maintains observability consistency across file access operations.📊 Proposed fix to add telemetry
/// <inheritdoc /> public string? GetXsdSchema(string dataTypeId) { + using var activity = _telemetry?.StartGetXsdSchemaActivity(); string legalPath = Path.Join(_settings.AppBasePath, _settings.ModelsFolder); string filename = Path.Join(legalPath, $"{dataTypeId}.xsd"); PathHelper.EnsureLegalPath(legalPath, filename);Note: You'll also need to add the corresponding
StartGetXsdSchemaActivity()method to theTelemetryclass (likely in a separate file) following the existing pattern for telemetry activities.test/Altinn.App.Api.Tests/OpenApi/OpenApiSpecChangeDetection.SaveJsonSwagger.verified.json (1)
5088-5162: New/api/xsdschema/{id}endpoint looks consistent with existing patternsThe XSD schema endpoint is wired up consistently with the existing
/api/jsonschema/{id}route and with other XML-returning resource endpoints (application/xml 200 payload + rich ProblemDetails 404). Only very minor nit: to keep the OpenAPI docs aligned, you could consider adding adescriptionfor theidparameter in the controller (matching the jsonschema endpoint), which will then flow into this snapshot.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/Altinn.App.Api/Controllers/ResourceController.cssrc/Altinn.App.Core/Implementation/AppResourcesSI.cssrc/Altinn.App.Core/Internal/App/IAppResources.cstest/Altinn.App.Api.Tests/Controllers/ResourceControllerTests.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/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
✅ Files skipped from review due to trivial changes (1)
- test/Altinn.App.Api.Tests/Data/apps/tdd/contributer-restriction/models/Skjema.schema.json
🚧 Files skipped from review as they are similar to previous changes (4)
- src/Altinn.App.Core/Internal/App/IAppResources.cs
- test/Altinn.App.Core.Tests/Implementation/AppResourcesSITests.cs
- src/Altinn.App.Api/Controllers/ResourceController.cs
- test/Altinn.App.Api.Tests/Data/apps/tdd/contributer-restriction/models/Skjema.xsd
🧰 Additional context used
📓 Path-based instructions (2)
**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.cs: Use CSharpier for code formatting (required before commits). Formatting happens automatically when building due to CSharpier.MSBuild
Use internal accessibility on types by default
Use sealed for classes unless inheritance is considered a valid use-case
Use Nullable Reference Types in C#
Remember to dispose IDisposable/IAsyncDisposable instances
For HTTP APIs, define ...Request and ...Response DTOs (e.g., LookupPersonRequest.cs and corresponding response)
Types meant to be implemented by apps should be marked with the ImplementableByApps attribute
Don't use .GetAwaiter().GetResult(), .Result(), .Wait() or other blocking APIs on Task
Don't use Async suffix for async methods
Write efficient code: Don't allocate unnecessarily (e.g., avoid calling ToString twice in a row, store in a variable; sometimes a for loop is better than LINQ)
Don't invoke the same async operation multiple times in the same codepath unless necessary
Don't await async operations in a loop (prefer batching, but maintain an upper bound on parallelism that makes sense)
Use strongly-typed configuration classes instead of untyped configuration
Register services in DI container properly following existing patterns
Files:
src/Altinn.App.Core/Implementation/AppResourcesSI.cstest/Altinn.App.Api.Tests/Controllers/ResourceControllerTests.cs
**/test/**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
**/test/**/*.cs: Prefer xUnit asserts over FluentAssertions in tests
Mock external dependencies with Moq in tests
Files:
test/Altinn.App.Api.Tests/Controllers/ResourceControllerTests.cs
🧠 Learnings (11)
📓 Common learnings
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 745
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:3656-3671
Timestamp: 2025-09-25T08:15:25.624Z
Learning: In PR Altinn/app-lib-dotnet#745, the team decided to defer adding idempotency documentation, enriched ServiceTaskFailedResult (code/title/message), and additional OTel spans for service tasks. Do not re-suggest these changes within this PR; propose a follow-up issue instead if needed.
Learnt from: CR
Repo: Altinn/app-lib-dotnet PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T08:07:52.106Z
Learning: New features should follow the established pattern in /src/Altinn.App.Core/Features/ with feature-specific folders, DI registration, telemetry, and test coverage
📚 Learning: 2025-08-22T13:46:43.017Z
Learnt from: martinothamar
Repo: Altinn/app-lib-dotnet PR: 1446
File: test/Altinn.App.Integration.Tests/Basic/_snapshots/BasicAppTests.Full_auth=ServiceOwner_testCase=MultipartXmlPrefill_7_Logs.verified.txt:41-42
Timestamp: 2025-08-22T13:46:43.017Z
Learning: In Altinn App Integration Tests, OldUser and OldServiceOwner test snapshots intentionally preserve the legacy "AuthMethod: localtest" authentication method as part of a migration strategy, while new User and ServiceOwner tests use updated authentication methods like BankID and maskinporten. The "Old*" variants should not be updated to remove localtest references.
Applied to files:
test/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.txttest/Altinn.App.Api.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txttest/Altinn.App.Api.Tests/OpenApi/OpenApiSpecChangeDetection.SaveJsonSwagger.verified.jsontest/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt
📚 Learning: 2025-08-22T13:35:09.565Z
Learnt from: martinothamar
Repo: Altinn/app-lib-dotnet PR: 1446
File: test/Altinn.App.Integration.Tests/Basic/_snapshots/BasicAppTests.Authentication_auth=OldUser.verified.txt:16-38
Timestamp: 2025-08-22T13:35:09.565Z
Learning: Test data in Altinn App Integration Tests snapshots (like SSNs, addresses, phone numbers in BasicAppTests.Authentication snapshots) are synthetic/fake values created for testing purposes, not real PII that needs to be scrubbed from version control.
Applied to files:
test/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.txttest/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt
📚 Learning: 2025-09-21T08:19:43.290Z
Learnt from: ivarne
Repo: Altinn/app-lib-dotnet PR: 1473
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:1414-1414
Timestamp: 2025-09-21T08:19:43.290Z
Learning: In Altinn/app-lib-dotnet, *.verified.txt files (e.g., test/**/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt) are verification snapshots for guarding public API. Do not generate review suggestions based on these files; ignore them when assessing code changes.
Applied to files:
test/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.txttest/Altinn.App.Api.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txttest/Altinn.App.Api.Tests/OpenApi/OpenApiSpecChangeDetection.SaveJsonSwagger.verified.jsontest/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt
📚 Learning: 2025-09-18T10:23:05.995Z
Learnt from: martinothamar
Repo: Altinn/app-lib-dotnet PR: 1357
File: test/Altinn.App.Api.Tests/OpenApi/OpenApiSpecChangeDetection.SaveJsonSwagger.verified.json:7263-0
Timestamp: 2025-09-18T10:23:05.995Z
Learning: In PR #1357, ApplicationMetadata.apiScopes (now ApiScopesConfiguration) is currently unused at runtime; the schema change is acceptable without a transition mechanism for now.
Applied to files:
test/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
📚 Learning: 2025-09-07T20:03:48.030Z
Learnt from: ivarne
Repo: Altinn/app-lib-dotnet PR: 1463
File: test/Altinn.App.SourceGenerator.Tests/DiagnosticTests.RunJsonError.verified.txt:1-21
Timestamp: 2025-09-07T20:03:48.030Z
Learning: In Altinn.App.SourceGenerator.Tests, the path C:\temp\config\applicationmetadata.json is a hardcoded test fixture path required by Roslyn and is used consistently across all operating systems, not an actual filesystem path that varies by OS.
Applied to files:
test/Altinn.App.Integration.Tests/CustomScopes/_snapshots/CustomScopesTests.Metadata_Custom_0_Metadata.verified.txtsrc/Altinn.App.Core/Implementation/AppResourcesSI.cstest/Altinn.App.Api.Tests/Controllers/ResourceControllerTests.cs
📚 Learning: 2025-09-18T10:12:21.841Z
Learnt from: martinothamar
Repo: Altinn/app-lib-dotnet PR: 1357
File: src/Altinn.App.Api/Infrastructure/Middleware/ScopeAuthorizationMiddleware.cs:289-293
Timestamp: 2025-09-18T10:12:21.841Z
Learning: In Altinn App security middleware like ScopeAuthorizationMiddleware, prefer explicit failures (throwing exceptions) over silently skipping unexpected endpoint types during discovery to prevent potential authorization gaps and ensure all endpoints are properly handled according to security configuration.
Applied to files:
test/Altinn.App.Integration.Tests/CustomScopes/_snapshots/CustomScopesTests.Metadata_Standard_0_Metadata.verified.txt
📚 Learning: 2025-08-29T10:45:57.158Z
Learnt from: martinothamar
Repo: Altinn/app-lib-dotnet PR: 1456
File: test/Altinn.App.Integration.Tests/_fixture/Tests.cs:3-4
Timestamp: 2025-08-29T10:45:57.158Z
Learning: In Altinn.App.Integration.Tests project, xUnit attributes like [Fact] are available without explicit "using Xunit;" directives, likely through global usings or implicit usings configuration. The code compiles and works as-is.
Applied to files:
test/Altinn.App.Api.Tests/Controllers/ResourceControllerTests.cs
📚 Learning: 2025-11-28T08:07:52.106Z
Learnt from: CR
Repo: Altinn/app-lib-dotnet PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T08:07:52.106Z
Learning: New features should follow the established pattern in /src/Altinn.App.Core/Features/ with feature-specific folders, DI registration, telemetry, and test coverage
Applied to files:
test/Altinn.App.Api.Tests/Controllers/ResourceControllerTests.cs
📚 Learning: 2025-08-29T10:45:57.158Z
Learnt from: martinothamar
Repo: Altinn/app-lib-dotnet PR: 1456
File: test/Altinn.App.Integration.Tests/_fixture/Tests.cs:3-4
Timestamp: 2025-08-29T10:45:57.158Z
Learning: In Altinn app-lib-dotnet projects, ImplicitUsings is enabled in Directory.Build.props, which automatically includes common using statements including Xunit namespace for test projects. This eliminates the need for explicit "using Xunit;" statements in test files.
Applied to files:
test/Altinn.App.Api.Tests/Controllers/ResourceControllerTests.cs
📚 Learning: 2025-10-17T07:45:15.474Z
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 1474
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:233-237
Timestamp: 2025-10-17T07:45:15.474Z
Learning: In Altinn/app-lib-dotnet, maintainers accept source compatibility without guaranteeing binary compatibility across versions because consumers upgrade via NuGet and rebuild. Adding optional parameters to public extension methods (e.g., CancellationToken) is acceptable provided release notes document the change.
Applied to files:
test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt
🧬 Code graph analysis (1)
src/Altinn.App.Core/Implementation/AppResourcesSI.cs (2)
src/Altinn.App.Core/Internal/App/IAppResources.cs (1)
GetXsdSchema(173-173)src/Altinn.App.Core/Helpers/PathHelper.cs (2)
PathHelper(6-34)EnsureLegalPath(27-33)
⏰ 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: Static code analysis
- GitHub Check: Analyze (csharp)
- GitHub Check: Run dotnet build and test (macos-latest)
- GitHub Check: Run dotnet build and test (ubuntu-latest)
- GitHub Check: Run dotnet build and test (windows-latest)
🔇 Additional comments (5)
test/Altinn.App.Api.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt (1)
531-557: Verification snapshot update—no action needed.This verified snapshot file documents the public API surface changes. The updates reflect the expected additions (new
GetModelXsdSchemaendpoint at lines 554-557) and deprecation (obsolete marker onGetLayoutSettingsat lines 531-533) introduced in this PR. These changes align with the feature to add utilities for reading XSD files from models.test/Altinn.App.Api.Tests/OpenApi/OpenApiSpecChangeDetection.SaveJsonSwagger.verified.json (1)
5255-5294: Deprecation of/api/layoutsettingsis correctly surfacedMarking the legacy
/api/layoutsettingsoperation as"deprecated": trueis in line with how other deprecated endpoints are represented and clearly nudges consumers towards/api/layoutsettings/{id}without altering behavior.test/Altinn.App.Api.Tests/Controllers/ResourceControllerTests.cs (3)
1-13: LGTM! Test class setup follows established patterns.The test class structure, dependency injection via IClassFixture, and test data constants are well-organized and follow the project's testing conventions.
39-48: LGTM! XSD schema success test is well-structured.The test validates the new XSD endpoint correctly, checking for successful response and expected XSD content structure.
50-56: LGTM! Proper error handling for invalid resource.The new XSD endpoint correctly returns 404 NotFound for invalid data type IDs, which is proper HTTP semantics and an improvement over the existing JSON schema endpoint behavior (which throws FileNotFoundException).
|
1 participant
Brreg wants to write a validator that uses the xsd file instead of data annotations. This PR adds tooling for reading the xsd files for models
Summary by CodeRabbit
New Features
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.