WIP: Feat/subform service task #1512
Conversation
…eration and eFormidling.
…ult enum and use result classes instead, for expandability.
…elds from result class and instead return generic failed ProcessChangeResult.
…served key word Next.
…iguration a bit more readable.
…ble the functionality for specific environments.
# Conflicts: # test/Altinn.App.Core.Tests/Internal/Process/ProcessEngineTest.cs
# Conflicts: # test/Altinn.App.Api.Tests/Program.cs
|
Important Review skippedDraft detected. 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 |
bjorntore
added
backport-ignore
|
/publish |
PR release:
|
|
/publish |
|
/publish |
PR release:
|
# Conflicts: # src/Altinn.App.Core/Internal/Pdf/IPdfService.cs # src/Altinn.App.Core/Internal/Pdf/PdfService.cs # test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| catch (Exception ex) | ||
| { | ||
| logger.LogWarning( | ||
| ex, | ||
| "Failed to delete existing subform PDF {DataElementId} from instance {InstanceId}", | ||
| pdf.Id, | ||
| instance.Id | ||
| ); | ||
| } |
Check notice
Code scanning / CodeQL
Generic catch clause Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 7 hours ago
To address the flagged issue, replace the generic catch (Exception ex) clause with catch clauses for more specific, expected exception types that can arise during the data deletion operation. In this context, exceptions such as IOException, OperationCanceledException, and possible lower-level service exceptions are likely relevant. For catch-all error logging (if truly necessary), use a separate clause but avoid catching System.Exception directly unless there's a strong justification.
Recommended changes:
- In
SubformPdfServiceTask.cs, in theforeachdeleting existing PDFs, replace thecatch (Exception ex)clause with specific exception types:- Consider
OperationCanceledExceptionfor cancellation scenarios. - Consider
IOExceptionorSystem.Net.Http.HttpRequestExceptionfor network/file issues. - If a custom exception is thrown by
dataClient.DeleteData, catch that.
- Consider
- Optionally, rethrow unexpected exceptions after logging or avoid catching them entirely.
No imports are necessary for these standard exception types as they are available in the .NET BCL.
| @@ -134,15 +134,34 @@ | ||
| instance.Id | ||
| ); | ||
| } | ||
| catch (Exception ex) | ||
| catch (OperationCanceledException ex) | ||
| { | ||
| logger.LogWarning( | ||
| ex, | ||
| "Failed to delete existing subform PDF {DataElementId} from instance {InstanceId}", | ||
| "Data deletion for subform PDF {DataElementId} in instance {InstanceId} was canceled", | ||
| pdf.Id, | ||
| instance.Id | ||
| ); | ||
| } | ||
| catch (System.IO.IOException ex) | ||
| { | ||
| logger.LogWarning( | ||
| ex, | ||
| "I/O error deleting subform PDF {DataElementId} from instance {InstanceId}", | ||
| pdf.Id, | ||
| instance.Id | ||
| ); | ||
| } | ||
| catch (System.Net.Http.HttpRequestException ex) | ||
| { | ||
| logger.LogWarning( | ||
| ex, | ||
| "HTTP request error while deleting subform PDF {DataElementId} from instance {InstanceId}", | ||
| pdf.Id, | ||
| instance.Id | ||
| ); | ||
| } | ||
| // Optionally add more specific exception handlers here as necessary | ||
| } | ||
| } | ||
| } |
| catch (Exception ex) | ||
| { | ||
| logger.LogWarning( | ||
| ex, | ||
| "Error during subform PDF cleanup in instance {InstanceId}", | ||
| context.InstanceDataMutator.Instance.Id | ||
| ); | ||
| } |
Check notice
Code scanning / CodeQL
Generic catch clause Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 7 hours ago
To fix this problem, replace the generic catch (Exception ex) clause with catch blocks targeting only the exception types we expect and want to handle—typically IOException, TaskCanceledException, or possibly FormatException due to Guid.Parse/data issues. Any unexpected exceptions (such as NullReferenceException, OutOfMemoryException, or other fatal errors) should be allowed to propagate.
How to fix:
- Replace the generic
catch (Exception ex)at line 149 with targetedcatchblocks for expected exception types. - Add one or more specific
catchblocks (for example,catch (TaskCanceledException),catch (FormatException),catch (IOException)if appropriate), each with appropriate logging. - If there is a truly compelling reason to suppress all exceptions, document the justification in a comment.
- Otherwise, add a final
catchthat rethrows unexpected exceptions, or remove the broad catch altogether.
What is needed:
- No new methods or definitions.
- No additional imports required.
- Only changes within the body of
CleanupExistingSubformPdfs.
| @@ -146,14 +146,24 @@ | ||
| } | ||
| } | ||
| } | ||
| catch (Exception ex) | ||
| catch (TaskCanceledException ex) | ||
| { | ||
| logger.LogWarning( | ||
| ex, | ||
| "Error during subform PDF cleanup in instance {InstanceId}", | ||
| "Subform PDF cleanup was canceled in instance {InstanceId}", | ||
| context.InstanceDataMutator.Instance.Id | ||
| ); | ||
| } | ||
| catch (FormatException ex) | ||
| { | ||
| logger.LogWarning( | ||
| ex, | ||
| "Subform PDF cleanup encountered invalid GUID format in instance {InstanceId}", | ||
| context.InstanceDataMutator.Instance.Id | ||
| ); | ||
| } | ||
| // Catch other anticipated exceptions as needed. | ||
| // If desired, add a comment explaining why not all exceptions are caught. | ||
| } | ||
|
|
||
| private async Task AddSubformPdfMetadata( |
|
|
/publish |
PR release:
|
1 participant
Description
Related Issue(s)
Verification
Documentation