refactor: review backend data loading #17053
Conversation
github-actions
bot
added
skip-releasenotes
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ 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 |
cammiida
changed the title
cammiida
force-pushed
the
review/backend-data-loading
branch
from
a213ac8 to
5678a5c
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| catch | ||
| { | ||
| // TODO: Log error | ||
| return null; | ||
| } |
Check notice
Code scanning / CodeQL
Generic catch clause Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 3 days ago
To fix the problem, the generic catch clause in the TryDeserialize<T> method should be replaced by specific exception handlers. For JSON deserialization, the correct exception is JsonException. Sometimes, deserialization might throw ArgumentNullException or ArgumentException (if the JSON is malformed or parameters are incorrect). We should catch those and return null, consistent with the intended behavior. Any other exceptions (which may signal internal problems, bugs, or environmental failures) should be rethrown or logged, not suppressed.
Changes need to be made only in the body of the TryDeserialize<T> method in the file src/App/backend/src/Altinn.App.Core/Features/Bootstrap/InitialDataService.cs, replacing the generic catch with targeted exception handling. No new imports are needed because JsonException is already available from System.Text.Json.
| @@ -397,11 +397,21 @@ | ||
| { | ||
| return JsonSerializer.Deserialize<T>(json, _jsonSerializerOptions); | ||
| } | ||
| catch | ||
| catch (JsonException) | ||
| { | ||
| // TODO: Log error | ||
| return null; | ||
| } | ||
| catch (ArgumentException) | ||
| { | ||
| // TODO: Log error | ||
| return null; | ||
| } | ||
| catch (Exception) | ||
| { | ||
| // TODO: Log unexpected error | ||
| throw; | ||
| } | ||
| } | ||
|
|
||
| private async Task<bool> CanPartyInstantiate(int? partyId) |
| private Task GetMockParty(int partyId, InitialDataResponse response) | ||
| private Party? GetMockParty(int? partyId) | ||
| { | ||
| if (partyId.HasValue == false) |
Check notice
Code scanning / CodeQL
Unnecessarily complex Boolean expression Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 3 days ago
To fix the unnecessarily complex boolean expression, simply change partyId.HasValue == false to !partyId.HasValue. This is a direct, well-understood way to check that the nullable int does not have a value, is more concise, and improves readability. The change should be made at line 473 in src/App/backend/src/Altinn.App.Core/Features/Bootstrap/InitialDataService.cs. No additional imports, definitions, or special handling is needed, as this expression is well-supported in C# and does not modify any program logic or behavior outside the specific negation.
| @@ -470,7 +470,7 @@ | ||
|
|
||
| private Party? GetMockParty(int? partyId) | ||
| { | ||
| if (partyId.HasValue == false) | ||
| if (!partyId.HasValue) | ||
| { | ||
| return null; | ||
| } |
2 participants
Description
Verification