fix: meldinger client and make skipping av-scan opt in#80
Conversation
Co-authored-by: Copilot <copilot@github.com>
|
There was a problem hiding this comment.
Pull request overview
This PR updates the publish client to match the internal API’s GetMelding response shape and introduces configuration/compose changes intended to make virus scanning optional in local/docker environments.
Changes:
- Fix
MeldingerClient.GetMeldingto deserializeGetMeldingResponseand returnresponse.Melding. - Add
Infrastructure__SkipVirusScanconfiguration and compose changes to make ClamAV services profile-gated. - Bump package version to
1.2.8and update docs/changelog accordingly.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/compose.yaml | Enables Infrastructure__SkipVirusScan and removes ClamAV dependency from receiver service |
| src/compose.infra.yaml | Reformats command, adjusts curl payloads, and gates ClamAV services behind virus-scan profile |
| src/Publish/Receiver.Publish/Receiver.Publish.csproj | Version bump to 1.2.8 |
| src/Publish/Receiver.Publish/Implementation/MeldingerClient.cs | Fix GetMelding DTO deserialization |
| src/Infrastructure/src/VirusScan/DummyVirusScanService.cs | Introduces no-op virus scan service (currently not wired correctly) |
| src/Infrastructure/src/DependencyInjection.cs | Adds SkipVirusScan config and conditional virus scan registration (currently broken) |
| src/CHANGELOG.md | Adds 1.2.8 notes |
| src/App/src/appsettings.json | Adds Infrastructure:SkipVirusScan default |
| documentation/configuration.md | Documents Infrastructure__SkipVirusScan |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| command: | ||
| [ | ||
| "-scheme", | ||
| "http", | ||
| "-backend", | ||
| "filesystem", | ||
| "-external-url", | ||
| "http://localhost:4443", | ||
| ] |
There was a problem hiding this comment.
The flow-style command: [...] sequence has a trailing comma after the last element. YAML flow sequences don’t permit trailing commas in many parsers (and may either fail to parse or add an unintended empty argument), which can prevent docker compose from starting the container. Remove the trailing comma (and avoid trailing commas in the other flow sequences too).
| Infrastructure__AltinnAppConfiguration__AltinnOrgIdentifier: dat | ||
| Infrastructure__AltinnAppConfiguration__MainDocumentDataTypeName: structured-data | ||
| Infrastructure__AppDomain: http://receiver-api:8080 | ||
| Infrastructure__SkipVirusScan: true |
There was a problem hiding this comment.
The PR title says skipping AV scan should be opt-in, but the default docker-compose environment here sets Infrastructure__SkipVirusScan: true, which makes skipping the default for local runs. If the intention is truly opt-in skipping, consider defaulting this to false in compose and enabling skip via an override file / env var when needed.
|
|
||
| ### Fixed | ||
|
|
||
| - fix: The `MeldingerClient` implementation returned a mismatching dto from what we get in return from our internal API. Get melding detials via this client is working as it should now |
There was a problem hiding this comment.
Typo in changelog entry: “detials” should be “details”.
| - fix: The `MeldingerClient` implementation returned a mismatching dto from what we get in return from our internal API. Get melding detials via this client is working as it should now | |
| - fix: The `MeldingerClient` implementation returned a mismatching dto from what we get in return from our internal API. Get melding details via this client is working as it should now |
| { | ||
| services.AddScoped<IMeldingRepository, MeldingRepository>(); | ||
| services.AddScoped<IDocumentRepository, DocumentRepository>(); | ||
| services.AddScoped<IInternalDocumentRepository, DocumentRepository>(); | ||
| services.AddScoped<IDocumentStorage, DocumentStorage>(); | ||
| services.AddScoped<IVirusScanService, VirusScanService>(); | ||
| if (infrastructureConfiguration.SkipVirusScan) { } | ||
| else | ||
| { | ||
| services.AddScoped<IVirusScanService, VirusScanService>(); | ||
| } |
There was a problem hiding this comment.
New SkipVirusScan behavior in DI is not covered by tests. Consider adding a small DI registration test that asserts IVirusScanService resolves to DummyVirusScanService when SkipVirusScan=true, and to VirusScanService when false, to prevent regressions.
| if (infrastructureConfiguration.SkipVirusScan) { } | ||
| else | ||
| { | ||
| services.AddScoped<IVirusScanService, VirusScanService>(); | ||
| } |
There was a problem hiding this comment.
SkipVirusScan branch leaves IVirusScanService unregistered (empty if-block). With Infrastructure__SkipVirusScan=true (as in compose.yaml), resolving any component that depends on IVirusScanService will fail at runtime. Register a no-op implementation (e.g., DummyVirusScanService) when skipping, and keep VirusScanService registration for the non-skip path (or register one implementation via conditional factory).
|
|
||
| namespace Arbeidstilsynet.MeldingerReceiver.Infrastructure.VirusScan; | ||
|
|
||
| internal class DummyVirusScanService(ILogger logger) : IVirusScanService |
There was a problem hiding this comment.
ILogger (non-generic) is not registered by default in Microsoft.Extensions.DependencyInjection; only ILogger<T> is. If/when this service is registered, DI will likely fail to construct it. Change the constructor to take ILogger<DummyVirusScanService> (or ILoggerFactory) and store it as a field.
| internal class DummyVirusScanService(ILogger logger) : IVirusScanService | |
| internal class DummyVirusScanService(ILogger<DummyVirusScanService> logger) : IVirusScanService |
No description provided.