LNK-4714: Replicate MVP Report Output - #1379
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughIntroduces configuration flag Changes
Sequence Diagram(s)sequenceDiagram
actor Client as Submission<br/>Service
participant BS as BlobStorageService
participant IBS as Internal Blob<br/>Storage
participant EBS as External Blob<br/>Storage
Client->>BS: Upload(entry, settings)
alt useNdJson = false
BS->>IBS: Download manifest.ndjson
BS->>BS: Parse manifest & extract<br/>resources (Org, Device,<br/>List, MeasureReports)
BS->>IBS: Download patient files<br/>under root prefix
BS->>BS: Compute unique patient IDs<br/>from manifest & files
loop For each patient
BS->>IBS: Load patient NDJSON
BS->>BS: Parse resources &<br/>locate MeasureReport
BS->>BS: Build expanded Bundle<br/>(patient resources +<br/>related resources)
BS->>EBS: Upload patient bundle<br/>as JSON
end
BS->>EBS: Upload aggregate<br/>MeasureReports as JSON
BS->>EBS: Upload metadata files<br/>(census.json, org, device,<br/>validation, shared-resources)
else useNdJson = true
BS->>EBS: Upload using NDJSON<br/>with application/fhir+ndjson
end
BS-->>Client: Complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 2
🤖 Fix all issues with AI agents
In `@DotNet/Submission/Application/Services/BlobStorageService.cs`:
- Around line 388-404: In BlobStorageService.cs inside the block handling the
fallback aggregate (variable fallbackAgg) you currently serialize fallbackAgg
twice (once to aggregateJson/aggregateBytes and again in the stream.WriteAsync
call). Replace the second serialization by writing the already-created
aggregateBytes (or Encoding.UTF8.GetBytes(aggregateJson)) to the stream; remove
the duplicate JsonSerializer.Serialize(...) call so stream.WriteAsync uses
aggregateBytes and keep the existing jsonOptions and blobOptions/HttpHeaders
intact.
- Around line 228-232: The code uses value.PayloadUri without null-check in
BlobStorageService (BlobUriBuilder new Uri(value.PayloadUri!)), which can throw;
update the logic in the method that constructs BlobUriBuilder to first validate
value.PayloadUri (or ReportSchedule payload) is not null/empty, handle the null
case (return early, throw a clear ArgumentException, or skip processing) and
only call new Uri(...) when non-null, keeping references to BlobUriBuilder,
manifestBlobName and rootPrefix intact so the derived rootPrefix is only
computed from a valid PayloadUri.
🧹 Nitpick comments (5)
DotNet/Submission/Application/Services/BlobStorageService.cs (5)
57-59: Use appropriate log level for configuration info.
LogWarningis inappropriate for logging a configuration value at startup. This is informational, not a warning.♻️ Suggested fix
- _logger.LogWarning("_useNdJson is set to {}", _useNdJson); + _logger.LogInformation("useNdJson configuration is set to {UseNdJson}", _useNdJson);
165-170: Consider refactoring this large method.
ProcessAndUploadExpandedBundlesAsyncis ~360 lines long with multiple responsibilities:
- Parsing manifest NDJSON
- Extracting resources by type
- Downloading patient files
- Building and uploading patient bundles
- Uploading aggregate reports
- Uploading supporting files (census, org, device, validation)
Consider extracting logical sections into smaller private methods for improved maintainability and testability.
458-472: Hardcoded dummy address may need configuration or documentation.The fallback address ("1 Center Drive, Ann Arbor, MI 48109, USA") is hardcoded. If this is intentional for MVP/connectathon compliance, consider:
- Adding a code comment explaining why this specific address is used
- Making this configurable for different deployment scenarios
- Documenting this behavior in the configuration documentation
561-562: Improve placeholder comment.The "🔑 PLACEHOLDER" comment doesn't explain why "unknown" is used as the replacement value. Consider a more descriptive comment.
♻️ Suggested improvement
- // 🔑 PLACEHOLDER + // Replace empty/whitespace version value with "unknown" to pass FHIR validation writer.WriteString("value", "unknown");
604-623: Consider consolidating measure type mappings.Both
GetMeasureFolderPathandGetMeasureAcronymcontain similar pattern-matching logic for report types. Consider extracting the measure type detection into a shared helper or using a lookup dictionary to avoid duplication and improve maintainability.Also applies to: 725-751
|
|
||
| public override DomainResource Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options) | ||
| { | ||
| _fhirParser.Settings.PermissiveParsing = true; |
There was a problem hiding this comment.
Is this related to/required for JSON output?
There was a problem hiding this comment.
Permissive Parsing is to allow for invalid json objects to make it through as a C# object. Otherwise, if it's invalid FHIR, it will throw an error.
There was a problem hiding this comment.
Guess my question was more ... does MVP-style report output require Normalization to parse more leniently? Seems unrelated to the rest of the PR.
There was a problem hiding this comment.
Would it be possible to move the JSON translation logic into a service that BlobStorageService can take as a dependency?
| _externalSettings = externalSettings.Value; | ||
| _internalContainerClient = GetContainerClient(_internalSettings); | ||
| _externalContainerClient = GetContainerClient(_externalSettings); | ||
| _useNdJson = configuration.GetValue<bool>("useNdJson"); |
There was a problem hiding this comment.
Recommend moving this out of top-level configuration. Maybe under ExternalBlobStorage? Or else a new section dedicated to the JSON translation service (if you end up creating that).
|
|
||
| // Parse manifest NDJSON | ||
| string manifestContent = Encoding.UTF8.GetString(content); | ||
| var jsonOptions = new JsonSerializerOptions().ForFhir(new FhirJsonPocoDeserializerSettings { Validator = null }); |
There was a problem hiding this comment.
Firely recommends caching instances of JsonSerializerOptions statically for performance.
There was a problem hiding this comment.
Side note: there's now a lenient JsonSerializerOptions instance available for project-wide use in Shared's LinkFhirSerializerOptions.
| string reportName = ReportHelpers.GetReportName(key.ReportScheduleId, key.FacilityId, value.ReportTypes, value.StartDate); | ||
|
|
||
| string? nhsnOrgId = organization?.Identifier | ||
| .FirstOrDefault(i => i.System == "https://www.cdc.gov/nhsn/OrgID")?.Value; |
There was a problem hiding this comment.
The NHSN OrgID identifier system is already a constant in Report's ReportConstants.BundleSettings. Consolidate those with anything that should be a constant here, then move everything to Shared?
| _logger.LogInformation("Completed processing and uploading {PatientCount} expanded patient bundles", patientIds.Count); | ||
| } | ||
|
|
||
| private static string PatchEmptyDeviceVersionValue(string json) |
There was a problem hiding this comment.
Do we need this? If so, can we do it in ReportManifestProducer.CreateDevice? (And can we use Firely instead of direct JSON manipulation?)
| _externalSettings = externalSettings.Value; | ||
| _internalContainerClient = GetContainerClient(_internalSettings); | ||
| _externalContainerClient = GetContainerClient(_externalSettings); | ||
| _useNdJson = configuration.GetValue<bool>("useNdJson"); |
There was a problem hiding this comment.
Use UseNdJson casing instead of useNdJson
|
|
||
| public override DomainResource Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options) | ||
| { | ||
| _fhirParser.Settings.PermissiveParsing = true; |
There was a problem hiding this comment.
Guess my question was more ... does MVP-style report output require Normalization to parse more leniently? Seems unrelated to the rest of the PR.
|
|
||
| // Parse manifest NDJSON | ||
| string manifestContent = Encoding.UTF8.GetString(content); | ||
| var jsonOptions = new JsonSerializerOptions().ForFhir(new FhirJsonPocoDeserializerSettings { Validator = null }); |
There was a problem hiding this comment.
Side note: there's now a lenient JsonSerializerOptions instance available for project-wide use in Shared's LinkFhirSerializerOptions.
| if (censusList != null) | ||
| { | ||
| foreach (var entry in censusList.Entry ?? new List<Hl7.Fhir.Model.List.EntryComponent>()) | ||
| { | ||
| var refId = entry.Item?.Reference?.Split('/').Last(); | ||
| if (!string.IsNullOrEmpty(refId)) | ||
| { | ||
| patientIds.Add(refId); | ||
| } | ||
| } | ||
| } | ||
| else | ||
| { | ||
| _logger.LogWarning("No patient List found in manifest. Falling back to file names."); | ||
| } |
There was a problem hiding this comment.
Do we need to consult the census? We can only submit patients that we have files for, not to mention that we'll expect missing files in some cases (specifically, for non-reportable patients).
| return DownloadAsync(_externalContainerClient, prefix, cancellationToken); | ||
| } | ||
|
|
||
| private string GetMeasureAcronym(List<string> reportTypes) |
There was a problem hiding this comment.
Use Shared's MeasureNameShortener instead?
| var patientMR = patientResources.OfType<MeasureReport>() | ||
| .FirstOrDefault(mr => !IsAggregateMeasureReport(mr) && mr.Subject?.Reference?.Contains(patientId) == true); |
There was a problem hiding this comment.
This doesn't seem to handle the multi-measure use case (where there could be multiple individual measure reports in a patient file).
| var sharedBundle = new Bundle | ||
| { | ||
| Type = Bundle.BundleType.Collection, | ||
| Timestamp = DateTimeOffset.UtcNow | ||
| }; |
There was a problem hiding this comment.
Don't we need to add the shared resources here since we're not adding them to the patient bundles above?
| var bundleJson = JsonSerializer.Serialize(expandedBundle, jsonOptions); | ||
| byte[] bundleBytes = Encoding.UTF8.GetBytes(bundleJson); | ||
|
|
||
| string bundleName = $"{nhsnOrgId}_{measureAcronym}_{startDateStr}_patient-{patientId}.json"; | ||
| string blobName = GetBlobName(_externalSettings.BlobRoot, measureFolder, reportName, bundleName); | ||
| _logger.LogDebug("Uploading expanded patient bundle: {BlobName}", blobName); | ||
|
|
||
| BlockBlobClient blobClient = _externalContainerClient!.GetBlockBlobClient(blobName); | ||
| BlockBlobOpenWriteOptions blobOptions = new() | ||
| { | ||
| HttpHeaders = new BlobHttpHeaders { ContentType = "application/json" } | ||
| }; | ||
|
|
||
| await using var stream = await blobClient.OpenWriteAsync(true, blobOptions, cancellationToken); | ||
| await stream.WriteAsync(bundleBytes, cancellationToken); |
There was a problem hiding this comment.
Could benefit from a helper method that takes care of serialization and writing to blob storage ... then call into that method here and below, once per blob. And might be worth looking into whether we can JsonSerializer.Serialize directly to the blob stream, rather than first serializing to a string, then converting to a byte array, then writing to the blob stream.
| bool needsDummyAddress = organization.Address == null || | ||
| !organization.Address.Any() || | ||
| organization.Address.All(a => | ||
| string.IsNullOrEmpty(a.City) && | ||
| string.IsNullOrEmpty(a.State) && | ||
| (a.Line == null || !a.Line.Any())); |
There was a problem hiding this comment.
Is there a problem with submitting an Organization with no address? If so, can we skip the file entirely? Or if it's essential, throw an exception (probably earlier in this method)?
| if (reportTypesStr.Contains("ach") && reportTypesStr.Contains("hypo")) | ||
| { | ||
| return "NHSNdQMAcuteCareHospitalInitialPopulation_NHSNGlycemicControlHypoglycemicInitialPopulation"; | ||
| } | ||
| else if (reportTypesStr.Contains("rps")) | ||
| { | ||
| return "NHSNRespiratoryPathogensSurveillanceInitialPopulation"; | ||
| } |
There was a problem hiding this comment.
Should probably revise (even if this matches the existing upload_file.py) to use the "modern" dQM names.
- NHSNAcuteCareHospitalMonthlyInitialPopulation
- NHSNAcuteCareHospitalDailyInitialPopulation (instead of RPS)
| return reportTypes.FirstOrDefault() ?? string.Empty; | ||
| } | ||
|
|
||
| private HashSet<(string ResourceType, string ResourceId)> FindReferencesInResources(List<Resource> resources) |
There was a problem hiding this comment.
This method doesn't seem to be used. Nor does FindReferencesInJson, except by this method. Do we need them?
3 participants
🛠️ Description of Changes
Replcates MVP output in BOTW based on flag useNdJson (bool).
🧪 Testing Performed
Tested at connectathon
🧑🔬 Unit Testing
📓 Documentation Updated
Please update any relevant sections in the project documentation that were impacted by the changes in the PR.
Summary by CodeRabbit
Release Notes
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.