Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Adds a new C#/.NET implementation of the Orchestrate SDK alongside existing TypeScript and Python clients, including CI integration and a comprehensive test suite.
Changes:
- Introduces
CareEvolution.Orchestrate.NET SDK (Terminology, Convert, Insight APIs) plus Identity/Local Hashing clients. - Adds .NET unit tests, optional live tests, and supporting test fixtures/helpers.
- Updates repo docs and GitHub Actions workflows to build/test/pack/publish the NuGet package.
Reviewed changes
Copilot reviewed 108 out of 110 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| README.md | Document C# install/usage/config examples |
| dotnet/tests/CareEvolution.Orchestrate.Tests/LiveLocalHashingApiTests.cs | Live tests for Local Hashing API |
| dotnet/tests/CareEvolution.Orchestrate.Tests/LiveIdentityApiTests.cs | Live tests for Identity API |
| dotnet/tests/CareEvolution.Orchestrate.Tests/LiveData/x12.txt | X12 fixture for live convert tests |
| dotnet/tests/CareEvolution.Orchestrate.Tests/LiveData/risk_profile_bundle.json | FHIR bundle fixture for risk profile tests |
| dotnet/tests/CareEvolution.Orchestrate.Tests/LiveData/r4_bundle.json | R4 bundle fixture for convert/serialize tests |
| dotnet/tests/CareEvolution.Orchestrate.Tests/LiveData/hl7.txt | HL7 fixture for live convert tests |
| dotnet/tests/CareEvolution.Orchestrate.Tests/LiveData/encoding_cda.xml | CDA fixture with alternative encoding |
| dotnet/tests/CareEvolution.Orchestrate.Tests/LiveData/cda.xml | CDA fixture for live convert tests |
| dotnet/tests/CareEvolution.Orchestrate.Tests/LiveApiTests.cs | Live integration tests across Terminology/Convert/Insight |
| dotnet/tests/CareEvolution.Orchestrate.Tests/IdentityApiTests.cs | Unit tests for Identity + Local Hashing routing/payload behavior |
| dotnet/tests/CareEvolution.Orchestrate.Tests/Helpers/LiveTestData.cs | Fixture loader/parsers for live tests |
| dotnet/tests/CareEvolution.Orchestrate.Tests/Helpers/LiveTestAttributes.cs | Live test skip logic + .env loading |
| dotnet/tests/CareEvolution.Orchestrate.Tests/Helpers/LiveClients.cs | Simple factory for live API clients |
| dotnet/tests/CareEvolution.Orchestrate.Tests/Helpers/FakeHttpMessageHandler.cs | HTTP test harness for request/response assertions |
| dotnet/tests/CareEvolution.Orchestrate.Tests/Helpers/EnvironmentVariableScope.cs | Scoped env var helper for tests |
| dotnet/tests/CareEvolution.Orchestrate.Tests/GlobalUsings.cs | Test project global using aliases |
| dotnet/tests/CareEvolution.Orchestrate.Tests/ConfigurationTests.cs | Tests for env/config precedence and error handling |
| dotnet/tests/CareEvolution.Orchestrate.Tests/CareEvolution.Orchestrate.Tests.csproj | Test project config + dependencies + fixtures copy |
| dotnet/tests/CareEvolution.Orchestrate.Tests/AssemblyInfo.cs | Disables test parallelization (live tests) |
| dotnet/tests/CareEvolution.Orchestrate.Tests/ApiSurfaceTests.cs | Ensures expected routes/headers/serialization and DI registration |
| dotnet/src/CareEvolution.Orchestrate/TranslateFhirR4ConceptMapRequest.cs | Model for concept map translate request |
| dotnet/src/CareEvolution.Orchestrate/TerminologyApi.cs | Terminology API implementation + routing |
| dotnet/src/CareEvolution.Orchestrate/SummarizeFhirR4ValueSetScopeRequest.cs | Model for value set scope summary request |
| dotnet/src/CareEvolution.Orchestrate/SummarizeFhirR4ValueSetRequest.cs | Model for value set summary request |
| dotnet/src/CareEvolution.Orchestrate/SummarizeFhirR4CodeSystemRequest.cs | Model for code system summary request |
| dotnet/src/CareEvolution.Orchestrate/StandardizeResponseCoding.cs | Standardize response coding model |
| dotnet/src/CareEvolution.Orchestrate/StandardizeResponse.cs | Standardize response model |
| dotnet/src/CareEvolution.Orchestrate/StandardizeRequest.cs | Standardize request model |
| dotnet/src/CareEvolution.Orchestrate/RouteBuilder.cs | Shared querystring builder/escaping helpers |
| dotnet/src/CareEvolution.Orchestrate/ResponseKind.cs | Internal discriminator for response parsing |
| dotnet/src/CareEvolution.Orchestrate/ResolvedConfiguration.cs | Internal resolved configuration record |
| dotnet/src/CareEvolution.Orchestrate/Registration.cs | DI registration for IOrchestrateApi |
| dotnet/src/CareEvolution.Orchestrate/OrchestrateHttpClient.cs | Core HTTP transport + auth headers + JSON/FHIR serialization |
| dotnet/src/CareEvolution.Orchestrate/OrchestrateApi.cs | Public SDK entrypoint wiring APIs/transport |
| dotnet/src/CareEvolution.Orchestrate/Options.cs | Options types for Orchestrate/Identity/LocalHashing |
| dotnet/src/CareEvolution.Orchestrate/IOrchestrateHttpClient.cs | Advanced/escape hatch transport interface |
| dotnet/src/CareEvolution.Orchestrate/InsightRiskProfileRequest.cs | Insight risk profile request model |
| dotnet/src/CareEvolution.Orchestrate/InsightApi.cs | Insight API implementation |
| dotnet/src/CareEvolution.Orchestrate/Identity/SourceTotal.cs | Identity metrics model |
| dotnet/src/CareEvolution.Orchestrate/Identity/RemoveMatchGuidanceResponse.cs | Identity guidance response model |
| dotnet/src/CareEvolution.Orchestrate/Identity/Record.cs | Identity record model |
| dotnet/src/CareEvolution.Orchestrate/Identity/PersonStatus.cs | Identity person status model |
| dotnet/src/CareEvolution.Orchestrate/Identity/Person.cs | Identity person model |
| dotnet/src/CareEvolution.Orchestrate/Identity/OverlapMetricsResponse.cs | Identity overlap metrics response |
| dotnet/src/CareEvolution.Orchestrate/Identity/MatchGuidanceRequest.cs | Identity guidance request base |
| dotnet/src/CareEvolution.Orchestrate/Identity/MatchedPersonReference.cs | Base matched-person response shape |
| dotnet/src/CareEvolution.Orchestrate/Identity/MatchDemographicsResponse.cs | Identity match response model |
| dotnet/src/CareEvolution.Orchestrate/Identity/MatchBlindedDemographicsResponse.cs | Identity blinded match response model |
| dotnet/src/CareEvolution.Orchestrate/Identity/LocalHashingApi.cs | Local hashing client implementation |
| dotnet/src/CareEvolution.Orchestrate/Identity/IdentityMonitoringApi.cs | Identity monitoring sub-client |
| dotnet/src/CareEvolution.Orchestrate/Identity/IdentityApi.cs | Identity client implementation |
| dotnet/src/CareEvolution.Orchestrate/Identity/IdentifierMetricsResponse.cs | Identity identifier metrics response |
| dotnet/src/CareEvolution.Orchestrate/Identity/IdentifierMetricsRecord.cs | Identity identifier metrics record model |
| dotnet/src/CareEvolution.Orchestrate/Identity/HashDemographicResponse.cs | Local hashing response model |
| dotnet/src/CareEvolution.Orchestrate/Identity/GetPersonByIdRequest.cs | Request model for get-person-by-id |
| dotnet/src/CareEvolution.Orchestrate/Identity/Demographic.cs | Demographic payload model + JSON naming |
| dotnet/src/CareEvolution.Orchestrate/Identity/DeleteRecordResponse.cs | Delete record response model |
| dotnet/src/CareEvolution.Orchestrate/Identity/DatasourceOverlapRecord.cs | Overlap record model |
| dotnet/src/CareEvolution.Orchestrate/Identity/BlindedDemographic.cs | Blinded demographic payload model |
| dotnet/src/CareEvolution.Orchestrate/Identity/Advisories.cs | Advisories model |
| dotnet/src/CareEvolution.Orchestrate/Identity/AddOrUpdateRecordResponse.cs | Add/update record response model |
| dotnet/src/CareEvolution.Orchestrate/Identity/AddOrUpdateRecordRequest.cs | Add/update record request model |
| dotnet/src/CareEvolution.Orchestrate/Identity/AddOrUpdateBlindedRecordRequest.cs | Add/update blinded record request model |
| dotnet/src/CareEvolution.Orchestrate/Identity/AddMatchGuidanceResponse.cs | Add match guidance response model |
| dotnet/src/CareEvolution.Orchestrate/Identity/AddMatchGuidanceRequest.cs | Add match guidance request model |
| dotnet/src/CareEvolution.Orchestrate/GlobalUsings.cs | SDK-wide global usings + HL7 type aliases |
| dotnet/src/CareEvolution.Orchestrate/GetFhirR4ValueSetsByScopeRequest.cs | Request model for value set search |
| dotnet/src/CareEvolution.Orchestrate/GetFhirR4ValueSetRequest.cs | Request model for value set retrieval |
| dotnet/src/CareEvolution.Orchestrate/GetFhirR4CodeSystemRequest.cs | Request model for code system retrieval |
| dotnet/src/CareEvolution.Orchestrate/Exceptions/OrchestrateHttpError.cs | Base HTTP error type with status code |
| dotnet/src/CareEvolution.Orchestrate/Exceptions/OrchestrateError.cs | Base SDK exception type |
| dotnet/src/CareEvolution.Orchestrate/Exceptions/OrchestrateClientError.cs | 4xx/5xx error wrapper with parsed issues |
| dotnet/src/CareEvolution.Orchestrate/EnvironmentConfiguration.cs | Env var + constructor option resolution |
| dotnet/src/CareEvolution.Orchestrate/ConvertX12ToFhirR4Request.cs | Convert X12 request model |
| dotnet/src/CareEvolution.Orchestrate/ConvertRequestFactory.cs | Helper to generate NDJSON combined-bundle requests |
| dotnet/src/CareEvolution.Orchestrate/ConvertHl7ToFhirR4Request.cs | Convert HL7 request model |
| dotnet/src/CareEvolution.Orchestrate/ConvertFhirStu3ToFhirR4Request.cs | Convert STU3 request model |
| dotnet/src/CareEvolution.Orchestrate/ConvertFhirR4ToOmopRequest.cs | Convert OMOP request model |
| dotnet/src/CareEvolution.Orchestrate/ConvertFhirR4ToNemsisV35Request.cs | Convert NEMSIS v3.5 request model |
| dotnet/src/CareEvolution.Orchestrate/ConvertFhirR4ToNemsisV34Request.cs | Convert NEMSIS v3.4 request model |
| dotnet/src/CareEvolution.Orchestrate/ConvertFhirR4ToManifestRequest.cs | Convert manifest request model |
| dotnet/src/CareEvolution.Orchestrate/ConvertFhirR4ToHealthLakeRequest.cs | Convert HealthLake request model |
| dotnet/src/CareEvolution.Orchestrate/ConvertFhirR4ToCdaRequest.cs | Convert CDA request model |
| dotnet/src/CareEvolution.Orchestrate/ConvertFhirDstu2ToFhirR4Request.cs | Convert DSTU2 request model |
| dotnet/src/CareEvolution.Orchestrate/ConvertCombineFhirR4BundlesRequest.cs | Combine bundles request model |
| dotnet/src/CareEvolution.Orchestrate/ConvertCdaToPdfRequest.cs | CDA->PDF request model |
| dotnet/src/CareEvolution.Orchestrate/ConvertCdaToHtmlRequest.cs | CDA->HTML request model |
| dotnet/src/CareEvolution.Orchestrate/ConvertCdaToFhirR4Request.cs | CDA->FHIR R4 request model |
| dotnet/src/CareEvolution.Orchestrate/ConvertApi.cs | Convert API implementation (routes + content types) |
| dotnet/src/CareEvolution.Orchestrate/ClassifyObservationResponse.cs | Observation classify response model |
| dotnet/src/CareEvolution.Orchestrate/ClassifyObservationRequest.cs | Observation classify request model |
| dotnet/src/CareEvolution.Orchestrate/ClassifyMedicationResponse.cs | Medication classify response model |
| dotnet/src/CareEvolution.Orchestrate/ClassifyMedicationRequest.cs | Medication classify request model |
| dotnet/src/CareEvolution.Orchestrate/ClassifyConditionResponse.cs | Condition classify response model |
| dotnet/src/CareEvolution.Orchestrate/ClassifyConditionRequest.cs | Condition classify request model |
| dotnet/src/CareEvolution.Orchestrate/CareEvolution.Orchestrate.csproj | SDK project metadata + package references |
| dotnet/src/CareEvolution.Orchestrate/BatchResponse.cs | Internal batch response wrapper |
| dotnet/src/CareEvolution.Orchestrate/BatchRequest.cs | Internal batch request wrapper |
| dotnet/OrchestrateSDK.DotNet.sln | .NET solution definition |
| dotnet/.gitignore | Ignore bin/obj/test results within dotnet/ |
| dotnet/.csharpierignore | Excludes generated/fixture data from formatting checks |
| dotnet/.config/dotnet-tools.json | Pins csharpier tool for formatting |
| CONTRIBUTING.md | Adds C# bootstrap/test instructions |
| .gitignore | Adds .codex ignore entry |
| .github/workflows/test.yml | Adds C# build/test/format job matrix |
| .github/workflows/deploy.yml | Adds NuGet pack/publish job |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| using var httpContent = new StringContent(content); | ||
| httpContent.Headers.ContentType = new System.Net.Http.Headers.MediaTypeHeaderValue( | ||
| contentType | ||
| ); | ||
| return _http.PostAsync<T>(route, httpContent, accept, responseKind, cancellationToken); | ||
| } |
There was a problem hiding this comment.
PostTextAsync disposes httpContent before the asynchronous request completes (using var httpContent + returning the Task). This can lead to ObjectDisposedException or empty request bodies. Avoid disposing the content here (let HttpRequestMessage dispose it), or make the method async and await the send before leaving the using scope.
| var metricsKey = GetPriority(options?.MetricsKey, IdentityMetricsKeyEnvironmentVariable); | ||
| return new ResolvedConfiguration( | ||
| BaseUrl: url, | ||
| TimeoutMs: GetTimeout(options?.TimeoutMs), | ||
| AdditionalHeaders: GetAdditionalHeaders(), | ||
| ApiKey: GetPriority(options?.ApiKey, IdentityApiKeyEnvironmentVariable), | ||
| Authorization: string.IsNullOrWhiteSpace(metricsKey) | ||
| ? null | ||
| : $"Basic {metricsKey.Replace("Basic ", string.Empty, StringComparison.Ordinal)}" | ||
| ); |
There was a problem hiding this comment.
Metrics key normalization is case-sensitive and only strips the exact substring "Basic " using StringComparison.Ordinal. If a user provides a common variant like "basic " (or different casing/extra whitespace), the resulting header becomes Basic basic <key>. Consider trimming and doing a case-insensitive prefix check/removal (only at the start) before prepending Basic .
| public Task<CodeSystem> GetFhirR4CodeSystemAsync( | ||
| GetFhirR4CodeSystemRequest request, | ||
| CancellationToken cancellationToken = default | ||
| ) | ||
| { | ||
| var route = RouteBuilder.Build( | ||
| $"/terminology/v1/fhir/r4/codesystem/{request.CodeSystem}", | ||
| [ | ||
| new KeyValuePair<string, string?>("page.num", request.PageNumber?.ToString()), | ||
| new KeyValuePair<string, string?>("_count", request.PageSize?.ToString()), | ||
| new KeyValuePair<string, string?>("concept:contains", request.ConceptContains), | ||
| ] | ||
| ); | ||
| return _http.GetJsonAsync<CodeSystem>(route, cancellationToken); |
There was a problem hiding this comment.
GetFhirR4CodeSystemAsync interpolates request.CodeSystem directly into the URL path without URL-escaping. If CodeSystem contains reserved characters (e.g., spaces, slashes), the request route can break. Use RouteBuilder.Escape(request.CodeSystem) (as is done in other endpoints) when building this path segment.
| public static ResolvedConfiguration Resolve(OrchestrateClientOptions? options) | ||
| { | ||
| return new ResolvedConfiguration( | ||
| BaseUrl: GetPriority(options?.BaseUrl, BaseUrlEnvironmentVariable) ?? DefaultBaseUrl, | ||
| TimeoutMs: GetTimeout(options?.TimeoutMs), | ||
| AdditionalHeaders: GetAdditionalHeaders(), | ||
| ApiKey: GetPriority(options?.ApiKey, ApiKeyEnvironmentVariable) | ||
| ); | ||
| } |
There was a problem hiding this comment.
Resolve(OrchestrateClientOptions) will accept an explicit BaseUrl of ""/whitespace (since GetPriority only checks for null) and propagate it into ResolvedConfiguration, which later causes a UriFormatException when building requests. Consider treating whitespace-only values as missing (fall back to env/default) or throwing an ArgumentException with a clearer message when BaseUrl is not a valid absolute URI.
| ```csharp | ||
| using CareEvolution.Orchestrate; | ||
|
|
||
| var services = new ServiceCollection(); | ||
| services.AddOrchestrateApi(); | ||
| ``` |
There was a problem hiding this comment.
The DI example uses ServiceCollection but doesn’t show the required using Microsoft.Extensions.DependencyInjection; (and readers may not realize the package/reference needed). Consider adding the missing using in the snippet (or a short note) so the example compiles as-is.
|
|
||
| namespace CareEvolution.Orchestrate.Exceptions; | ||
|
|
||
| public class OrchestrateHttpError(string message, HttpStatusCode? statusCode = null) |
There was a problem hiding this comment.
I think the .NET convention is to name all exceptions XyzException
There was a problem hiding this comment.
Yeah, you're right. I was just keeping the error name the same from other languages.
|
|
||
| namespace CareEvolution.Orchestrate; | ||
|
|
||
| internal static class RouteBuilder |
There was a problem hiding this comment.
why are we building our own URL building logic instead of using a library like Flurl?
There was a problem hiding this comment.
To keep dependencies down, I guess?
There was a problem hiding this comment.
That's a good reason. Can we get unit tests for RouteBuilder?
| { | ||
| if (body is null) | ||
| { | ||
| return "null"; |
There was a problem hiding this comment.
why are we returning a string that says "null"?
There was a problem hiding this comment.
TS does this already: https://github.com/CareEvolution/OrchestrateSDK/blob/main/typescript/src/httpHandler.ts#L125. Python is definitely in the wrong here, but that out of scope for this particular PR.
|
|
||
| internal sealed class OrchestrateHttpClient( | ||
| ResolvedConfiguration configuration, | ||
| HttpClient? httpClient = null |
There was a problem hiding this comment.
httpClient should not be optional here.
Allowing a naive user to let private readonly HttpClient _httpClient = httpClient ?? new HttpClient(); happen runs the risk of weirdness like socket exhaustion happening under load.
| public async Task<T> PostAsync<T>( | ||
| string path, | ||
| HttpContent content, | ||
| string accept, |
There was a problem hiding this comment.
why are we giving the user the option for what format to use?
There was a problem hiding this comment.
This class isn't really public. So this is here to help us switch between application/zip, pdf, etc. without requiring that we doubly-map all request routes to the expected Accept header. The other languages do this by passing in a header dictionary. It's a little more explicit here.
| ? CancellationTokenSource.CreateLinkedTokenSource(cancellationToken) | ||
| : null; | ||
| cts?.CancelAfter(_configuration.TimeoutMs); | ||
| var effectiveCancellationToken = cts?.Token ?? cancellationToken; |
There was a problem hiding this comment.
timeout should be set on the HttpClient, we shouldn't need a custom CTS
There was a problem hiding this comment.
This has the same kind of schism between the passed-in client again. In the other languages, we allowed timeouts to be controlled by ORCHESTRATE_TIMEOUT_MS. So with the injected client, we're left with:
- Disregard the env var, making this language a little different than everyone else
- Add a per-request cts.
- Mutating the HttpClient we were given
I'm inclined to keep the current behavior, but I'm open to whatever.
| return new OrchestrateHttpError(responseText, response.StatusCode); | ||
| } | ||
|
|
||
| private static IReadOnlyList<string> ReadOperationalOutcomes(string responseText) |
There was a problem hiding this comment.
instead of parsing the OO for errors here we should just deserialize to an OperationOutcome object and attach that to the exception?
how to render the error to the client or in logs is a job for another piece of code.
There was a problem hiding this comment.
I've addressed this and the other OperationalOutcome concern below by attaching some information on the exceptions. But we also parse out RFC 9110 issues here, so there's some reason to continue parsing to look for other issues--not every response is FHIR.
| { | ||
| var severity = issue["severity"]?.GetValue<string>() ?? string.Empty; | ||
| var code = issue["code"]?.GetValue<string>() ?? string.Empty; | ||
| var diagnostics = issue["diagnostics"]?.GetValue<string>() ?? string.Empty; |
There was a problem hiding this comment.
we should not be hand parsing FHIR JSON when we have a very nice OperationOutcome object and deserializer from the FHIR library.
| } | ||
| catch | ||
| { | ||
| return []; |
There was a problem hiding this comment.
we should not silently discard error responses that we cannot parse as json
…n permissions Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
|
|
||
| public sealed class OrchestrateClientError( | ||
| /// <summary> | ||
| /// Raised when the Orchestrate API returns a 4xx or 5xx status code. |
There was a problem hiding this comment.
If OrchestrateClientException is for client errors, wouldn't that just be 4XX errors? 5XX is usually should not be something the client can fix other than retrying later.
There was a problem hiding this comment.
I don't know, but these classes exist in the other languages, too, and do exactly this in those cases. I think if we want to work around these changes,we'll need to major bump and have a discussion around behavior.
Description of Changes
This adds a C# variant of this SDK. The structure of the SDK should be fairly expected compared to others in this same repository: Terminology, Convert, and Identity APIs. The same environment variables are used for configuration, and the HTTP handler is somewhat exposed (albiet with an Advanced option) to allow users to use undocumented routes.
Notable deviances from other languages:
Security
This change follows the same pattern as in other languages, though some of the examples are now serialized files rather than just being in-line. Even so, the content is the same as it was before. API authentication is handled using environment variables.
Change Control Board (CCB) Approval
@careevolution/ccb.Testing/Validation
Tested C# live tests, though Identity will need to be handled separately.
Backout Plan
Revert.
Implementation Notes
Merge.
I suspect that deployment does not yet work. I'll update this as I can.
Documentation Updates
@careevolution/mdhd-docsor@careevolution/api-docs.@careevolution/api-docs
Reviewers