Skip to content

Commit

Permalink
Throw explicit exception when group children is not found. (#1010)
Browse files Browse the repository at this point in the history
* Improve server error when group children is not found.

Previously this was a DictionaryKeyNotFoundException, but now I add the Id of the group component to make it easier to debug (especially if someone has emtpy string as id)

* Refactor and add tests

* Go for 100% coverage (and don't reference page before it is availible)
  • Loading branch information
ivarne authored Jan 10, 2025
1 parent 5af7bc5 commit 58f0eef
Show file tree
Hide file tree
Showing 4 changed files with 88 additions and 5 deletions.
18 changes: 13 additions & 5 deletions src/Altinn.App.Core/Models/Layout/PageComponentConverter.cs
Original file line number Diff line number Diff line change
Expand Up @@ -219,8 +219,17 @@ Dictionary<string, GroupComponent> childToGroupMapping
{
if (component is GroupComponent groupComponent)
{
var children = groupComponent.ChildIDs.Select(id => componentLookup[id]).ToList();
children.ForEach(c => groupComponent.AddChild(c));
foreach (var childID in groupComponent.ChildIDs)
{
if (!componentLookup.TryGetValue(childID, out var child))
{
throw new InvalidOperationException(
$"""Group "{component.Id}" references a child with id \"{childID}\" which was not found in layout"""
);
}

groupComponent.AddChild(child);
}
}

if (!childToGroupMapping.ContainsKey(component.Id))
Expand All @@ -233,11 +242,10 @@ Dictionary<string, GroupComponent> childToGroupMapping

private static void AddToComponentLookup(BaseComponent component, Dictionary<string, BaseComponent> componentLookup)
{
if (componentLookup.ContainsKey(component.Id))
if (!componentLookup.TryAdd(component.Id, component))
{
throw new JsonException($"Duplicate key \"{component.Id}\" detected on page \"{component.PageId}\"");
throw new JsonException($"Duplicate key \"{component.Id}\" detected");
}
componentLookup[component.Id] = component;
}

private static readonly Regex _multiPageIndexRegex = new Regex(
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
{
"name": "Group with missing child gives error",
"expectsFailure": "*missing-child*",
"expression": ["dataModel", "a"],
"layouts": {
"Page1": {
"$schema": "https://altinncdn.no/schemas/json/layout/layout.schema.v1.json",
"data": {
"layout": [
{
"id": "current-component",
"type": "group",
"children": [
"missing-child"
]
}
]
}
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
{
"name": "Two components with same id gives error",
"expectsFailure": "*duplicate-id*",
"expression": ["dataModel", "a"],
"layouts": {
"Page1": {
"$schema": "https://altinncdn.no/schemas/json/layout/layout.schema.v1.json",
"data": {
"layout": [
{
"id": "duplicate-id",
"type": "Input"
},
{
"id": "duplicate-id",
"type": "Input"
}
]
}
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
{
"name": "Two groups trying to reference the same child",
"expectsFailure": "*double-referenced-child*",
"expression": ["dataModel", "a"],
"layouts": {
"Page1": {
"$schema": "https://altinncdn.no/schemas/json/layout/layout.schema.v1.json",
"data": {
"layout": [
{
"id": "group1",
"type": "group",
"children": [
"double-referenced-child"
]
},
{
"id": "group2",
"type": "group",
"children": [
"double-referenced-child"
]
},
{
"id": "double-referenced-child",
"type": "Input"
}
]
}
}
}
}

0 comments on commit 58f0eef

Please sign in to comment.