Open
Conversation
📝 WalkthroughWalkthroughThis PR introduces GraphQL support to the Rosetta API client using ZeroQL for compile-time-checked queries. It adds a GraphQL schema definition, integrates a RosettaGraphQLClient accessible via the main client, updates configuration and OAuth handling, refactors test infrastructure, and adds debug logging capabilities. Changes
Sequence DiagramsequenceDiagram
participant Client
participant RosettaClient
participant RosettaGraphQLClient
participant OAuthHandler
participant GraphQL Server as GraphQL API Server
participant JsonConverter
Client->>RosettaClient: request Person data via GraphQL
RosettaClient->>RosettaGraphQLClient: GraphQL.QueryAsync()
RosettaGraphQLClient->>OAuthHandler: request OAuth token
OAuthHandler->>GraphQL Server: POST /token (with optional scope)
GraphQL Server-->>OAuthHandler: access_token
RosettaGraphQLClient->>GraphQL Server: GraphQL query (with Bearer token)
GraphQL Server-->>RosettaGraphQLClient: JSON response (potentially malformed)
RosettaGraphQLClient->>JsonConverter: deserialize [Person]
JsonConverter-->>RosettaGraphQLClient: [Person] (lenient, skipping invalid entries)
RosettaGraphQLClient-->>RosettaClient: typed result
RosettaClient-->>Client: [Person] data
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
UCD.Rosetta.Client/Core/Extensions/ClientExtensions.cs (1)
24-24:⚠️ Potential issue | 🟠 MajorSync-over-async call may cause deadlocks.
ReadAsStringAsync().GetAwaiter().GetResult()blocks synchronously on an async operation. In UI or ASP.NET synchronization contexts, this can cause deadlocks. SinceProcessResponseis a synchronous partial method from NSwag, consider usingReadAsString()if available, or document this limitation.Alternative using synchronous read
-var responseBody = response.Content.ReadAsStringAsync().GetAwaiter().GetResult(); +var responseBody = response.Content.ReadAsStream().ReadToEnd(); // Or buffer manuallyHowever, the simplest fix may be to use
.ConfigureAwait(false)if sticking with async:-var responseBody = response.Content.ReadAsStringAsync().GetAwaiter().GetResult(); +var responseBody = response.Content.ReadAsStringAsync().ConfigureAwait(false).GetAwaiter().GetResult();This won't fully prevent deadlocks but reduces risk in some contexts.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@UCD.Rosetta.Client/Core/Extensions/ClientExtensions.cs` at line 24, The synchronous blocking call that assigns responseBody (response.Content.ReadAsStringAsync().GetAwaiter().GetResult()) can deadlock; inside the ProcessResponse partial method replace the sync-over-async pattern by either using a true synchronous reader if available (e.g., a hypothetical response.Content.ReadAsString()) or, if you must keep it synchronous, call the task with ConfigureAwait(false) before blocking (response.Content.ReadAsStringAsync().ConfigureAwait(false).GetAwaiter().GetResult()) to reduce deadlock risk; update the assignment to responseBody and keep the change scoped to the ProcessResponse handling code.
🧹 Nitpick comments (5)
UCD.Rosetta.Client/Core/Converters/LenientTypedCollectionConverter.cs (1)
64-80: Consider wrapping item deserialization in a try-catch for resilience.The current implementation skips malformed tokens (nulls, primitives) but will throw if
JsonSerializer.Deserialize<T>fails on a valid-looking JSON object with an unexpected structure. For maximum resilience against unpredictable API responses, consider catching deserialization errors per-item.♻️ Optional: Add per-item exception handling
if (reader.TokenType == JsonTokenType.Null || reader.TokenType != JsonTokenType.StartObject) { // Skip unexpected tokens (null elements, strings, numbers, booleans) reader.Skip(); continue; } - var item = JsonSerializer.Deserialize<T>(ref reader, options); - if (item != null) - list.Add(item); + try + { + var item = JsonSerializer.Deserialize<T>(ref reader, options); + if (item != null) + list.Add(item); + } + catch (JsonException) + { + // Skip items that fail to deserialize + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@UCD.Rosetta.Client/Core/Converters/LenientTypedCollectionConverter.cs` around lines 64 - 80, In LenientTypedCollectionConverter's read loop (the method that calls JsonSerializer.Deserialize<T>(ref reader, options)), wrap the per-item deserialization call in a try-catch that catches JsonException (and optionally Exception) so a single bad object doesn't fail the whole array; on catch, advance past the problematic value (e.g., call reader.Skip() or otherwise consume the current token) and continue the loop without adding an item, and optionally emit a debug/log entry; this change ensures JsonSerializer.Deserialize<T>(ref reader, options) failures are handled per-item and the method (Read) returns the successfully parsed list.update-spec.sh (1)
38-48: Consider validating extraction result.If the
externalDocs.descriptionfield is missing or doesn't contain a GraphQL code block, the extraction will produce an empty file without warning, potentially causing ZeroQL codegen to fail later with a confusing error.Proposed validation
jq -r '.externalDocs.description' "${SPEC_FILE}" \ | awk '/```graphql/{found=1; next} found && /```/{exit} found{print}' \ | sed 's/^ //' \ > "${GRAPHQL_FILE}" + if [ ! -s "${GRAPHQL_FILE}" ]; then + echo "❌ GraphQL schema extraction produced empty file — check externalDocs.description in ${SPEC_FILE}" + exit 1 + fi # Append schema root declaration required by ZeroQL codegen printf '\nschema {\n query: Query\n}\n' >> "${GRAPHQL_FILE}" echo "✅ GraphQL schema extracted: ${GRAPHQL_FILE}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@update-spec.sh` around lines 38 - 48, The extraction pipeline that reads .externalDocs.description into ${GRAPHQL_FILE} can produce an empty file if the field or a GraphQL code block is missing; after the jq/awk/sed pipeline that writes to ${GRAPHQL_FILE}, add a check (e.g., test -s "${GRAPHQL_FILE}") and if the file is empty print a clear error referencing ${SPEC_FILE} and ${GRAPHQL_FILE} and exit non‑zero, and only append the schema root declaration to ${GRAPHQL_FILE} after this validation passes.UCD.Rosetta.Client/Core/Extensions/ClientExtensions.cs (2)
43-43: File path may not exist or be writable in production.
FindSolutionRoot()returnsnullwhen no.slnfile is found (common in deployed environments), falling back toPath.GetTempPath(). However:
- Each response overwrites the previous file (line 63), so concurrent requests lose debug data.
- Writing to solution root during development could inadvertently commit debug files.
Suggestions
- Consider appending a timestamp or request ID to the filename for concurrent request debugging.
- Always use temp path to avoid writing to source directories:
-var logPath = System.IO.Path.Combine(FindSolutionRoot() ?? System.IO.Path.GetTempPath(), "rosetta-debug.json"); +var logPath = System.IO.Path.Combine(System.IO.Path.GetTempPath(), $"rosetta-debug-{DateTime.UtcNow:yyyyMMddHHmmss}.json");Also applies to: 62-63
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@UCD.Rosetta.Client/Core/Extensions/ClientExtensions.cs` at line 43, The current construction of logPath using FindSolutionRoot() can point at the repo and causes overwrites; change ClientExtensions.cs to always use System.IO.Path.GetTempPath() (not FindSolutionRoot()) for the base directory and make the filename unique (append a timestamp or GUID / request ID) so each invocation writes to a distinct file; ensure the code that writes the file (the logic around the logPath variable and the write at the code that previously overwrote on lines referencing the write) uses this new unique temp-path filename and verifies the target directory exists and is writable before writing.
67-77: Minor: Optimize solution root search.
GetFiles("*.sln").Length > 0allocates an array just to check existence. Consider usingEnumerateFileswithAny()for efficiency.Proposed fix
-if (dir.GetFiles("*.sln").Length > 0) +if (dir.EnumerateFiles("*.sln").Any()) return dir.FullName;This requires adding
using System.Linq;if not already present.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@UCD.Rosetta.Client/Core/Extensions/ClientExtensions.cs` around lines 67 - 77, In FindSolutionRoot(), avoid allocating an array by changing the existence check from dir.GetFiles("*.sln").Length > 0 to using dir.EnumerateFiles("*.sln").Any(), and ensure System.Linq is imported (add using System.Linq; if absent) so Any() is available; keep the rest of the method logic unchanged.UCD.Rosetta.Client/Core/RosettaClient.cs (1)
82-90: Consider sharing OAuth handler between REST and GraphQL clients.Both constructors create a separate
OAuthHandlerfor the GraphQL client. This results in two independent token caches, potentially causing duplicate token acquisition requests when both APIs are called before the first token is cached.Additionally, in the DI constructor (line 99), the REST
HttpClientis externally managed while the GraphQL client always creates its own. This asymmetry may surprise users expecting consistent lifecycle management.One approach: share the OAuth handler
If the OAuth handler were shared, both clients would use the same token cache. However, this requires ensuring
OAuthHandleris thread-safe for concurrent use (it appears to be based on the locking inGetAccessTokenAsync).Alternatively, document this behavior so users understand token acquisition may occur separately for each surface.
Also applies to: 115-123
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@UCD.Rosetta.Client/Core/RosettaClient.cs` around lines 82 - 90, The code creates a separate OAuthHandler for the GraphQL client causing two independent token caches and asymmetric lifecycle handling; fix by creating a single OAuthHandler instance and reusing it for both HttpClient instances (the one used for REST and the one assigned to _graphqlHttpClient) so they share the same token cache, and update both constructors (the DI constructor and the other constructor where graphqlOAuthHandler is created) to accept or construct a single OAuthHandler to pass into new HttpClient(...) and into RosettaGraphQLClient initialization (refer to OAuthHandler, _graphqlHttpClient, GraphQL, RosettaGraphQLClient and the class constructors) ensuring the externally-managed REST HttpClient scenario either shares its OAuthHandler or documents/handles lifecycle consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@IntegrationTests/RosettaApiTests.cs`:
- Around line 87-100: Update the three tests
(PeopleAsync_WithLoginId_ReturnsResults,
PeopleAsync_WithManagerIamId_ReturnsResults,
GraphQL_TypedPeopleQuery_ByLoginId_ReturnsMatchingPerson) to assert that the
returned records actually match the requested filter values: use the existing
inputs (_fixture.TestData.LoginId and _fixture.TestData.ManagerIamId) and verify
that each returned result (or at minimum that at least one returned result) has
its Loginid property equal to the requested login id (case-insensitive) for the
loginid tests and that the Manager_iam_id property equals the requested manager
id for the manager test; replace or augment the current non-empty assertions
(result.Count/ElementAt checks) with these equality checks so the tests fail if
the API ignores the filters.
- Around line 210-223: The test
CollegesAsync_WithCollegeCode_ReturnsMatchingCollege uses
all.First().College_code without validating it; update the seed selection before
calling _fixture.Client.Api.CollegesAsync(college_code: code) by finding a
non-null/non-empty College_code (e.g., use all.FirstOrDefault(c =>
!string.IsNullOrWhiteSpace(c.College_code))) and if none found call Skip or
Assert.Skip so the test doesn't run with an empty filter; ensure you reference
the local variable code and the calls to _fixture.Client.Api.CollegesAsync when
implementing the guard.
In `@UCD.Rosetta.Client/Core/Configuration/RosettaClientOptions.cs`:
- Around line 29-34: The Validate() method in RosettaClientOptions currently
throws if Scope is null/whitespace which conflicts with the property being
nullable and the OAuthHandler conditionally adding scope; update
RosettaClientOptions.Validate() to remove the null/whitespace check for the
Scope property so Scope remains optional, leaving the Scope string? declaration
and documentation unchanged and ensuring OAuthHandler's conditional logic (the
code that checks Scope before including it) continues to work.
---
Outside diff comments:
In `@UCD.Rosetta.Client/Core/Extensions/ClientExtensions.cs`:
- Line 24: The synchronous blocking call that assigns responseBody
(response.Content.ReadAsStringAsync().GetAwaiter().GetResult()) can deadlock;
inside the ProcessResponse partial method replace the sync-over-async pattern by
either using a true synchronous reader if available (e.g., a hypothetical
response.Content.ReadAsString()) or, if you must keep it synchronous, call the
task with ConfigureAwait(false) before blocking
(response.Content.ReadAsStringAsync().ConfigureAwait(false).GetAwaiter().GetResult())
to reduce deadlock risk; update the assignment to responseBody and keep the
change scoped to the ProcessResponse handling code.
---
Nitpick comments:
In `@UCD.Rosetta.Client/Core/Converters/LenientTypedCollectionConverter.cs`:
- Around line 64-80: In LenientTypedCollectionConverter's read loop (the method
that calls JsonSerializer.Deserialize<T>(ref reader, options)), wrap the
per-item deserialization call in a try-catch that catches JsonException (and
optionally Exception) so a single bad object doesn't fail the whole array; on
catch, advance past the problematic value (e.g., call reader.Skip() or otherwise
consume the current token) and continue the loop without adding an item, and
optionally emit a debug/log entry; this change ensures
JsonSerializer.Deserialize<T>(ref reader, options) failures are handled per-item
and the method (Read) returns the successfully parsed list.
In `@UCD.Rosetta.Client/Core/Extensions/ClientExtensions.cs`:
- Line 43: The current construction of logPath using FindSolutionRoot() can
point at the repo and causes overwrites; change ClientExtensions.cs to always
use System.IO.Path.GetTempPath() (not FindSolutionRoot()) for the base directory
and make the filename unique (append a timestamp or GUID / request ID) so each
invocation writes to a distinct file; ensure the code that writes the file (the
logic around the logPath variable and the write at the code that previously
overwrote on lines referencing the write) uses this new unique temp-path
filename and verifies the target directory exists and is writable before
writing.
- Around line 67-77: In FindSolutionRoot(), avoid allocating an array by
changing the existence check from dir.GetFiles("*.sln").Length > 0 to using
dir.EnumerateFiles("*.sln").Any(), and ensure System.Linq is imported (add using
System.Linq; if absent) so Any() is available; keep the rest of the method logic
unchanged.
In `@UCD.Rosetta.Client/Core/RosettaClient.cs`:
- Around line 82-90: The code creates a separate OAuthHandler for the GraphQL
client causing two independent token caches and asymmetric lifecycle handling;
fix by creating a single OAuthHandler instance and reusing it for both
HttpClient instances (the one used for REST and the one assigned to
_graphqlHttpClient) so they share the same token cache, and update both
constructors (the DI constructor and the other constructor where
graphqlOAuthHandler is created) to accept or construct a single OAuthHandler to
pass into new HttpClient(...) and into RosettaGraphQLClient initialization
(refer to OAuthHandler, _graphqlHttpClient, GraphQL, RosettaGraphQLClient and
the class constructors) ensuring the externally-managed REST HttpClient scenario
either shares its OAuthHandler or documents/handles lifecycle consistently.
In `@update-spec.sh`:
- Around line 38-48: The extraction pipeline that reads
.externalDocs.description into ${GRAPHQL_FILE} can produce an empty file if the
field or a GraphQL code block is missing; after the jq/awk/sed pipeline that
writes to ${GRAPHQL_FILE}, add a check (e.g., test -s "${GRAPHQL_FILE}") and if
the file is empty print a clear error referencing ${SPEC_FILE} and
${GRAPHQL_FILE} and exit non‑zero, and only append the schema root declaration
to ${GRAPHQL_FILE} after this validation passes.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
.config/dotnet-tools.json.gitignoreExample/Program.csExample/appsettings.jsonIntegrationTests/RosettaApiTests.csIntegrationTests/TestDataOptions.csIntegrationTests/appsettings.jsonREADME.mdUCD.Rosetta.Client/.editorconfigUCD.Rosetta.Client/Core/Authentication/OAuthHandler.csUCD.Rosetta.Client/Core/Configuration/RosettaClientOptions.csUCD.Rosetta.Client/Core/Converters/LenientTypedCollectionConverter.csUCD.Rosetta.Client/Core/Extensions/ClientExtensions.csUCD.Rosetta.Client/Core/RosettaClient.csUCD.Rosetta.Client/UCD.Rosetta.Client.csprojUCD.Rosetta.Client/rosetta.zeroql.jsonspecs/rosetta-api.graphqlupdate-spec.sh
Comment on lines
+87
to
+100
| public async Task PeopleAsync_WithLoginId_ReturnsResults() | ||
| { | ||
| // Skip if no test data configured | ||
| Skip.IfNot(!string.IsNullOrWhiteSpace(_fixture.TestData.LoginId), | ||
| "TestData:LoginId not configured in user secrets or environment variables"); | ||
|
|
||
| #region Accounts | ||
|
|
||
| //[SkippableFact] | ||
| //public async Task AccountsAllAsync_WithIamId_ReturnsResults() | ||
| //{ | ||
| // // Skip if no test data configured | ||
| // Skip.IfNot(!string.IsNullOrWhiteSpace(_fixture.TestData.IamId), | ||
| // "TestData:IamId not configured in user secrets or environment variables"); | ||
|
|
||
| // // Act | ||
| // var result = await _fixture.Client.Api.AccountsAllAsync(iamid: _fixture.TestData.IamId); | ||
|
|
||
| // // Assert | ||
| // Assert.NotNull(result); | ||
| // var first = result.FirstOrDefault(); | ||
| // Assert.Fail($"AccountsAllAsync returns untyped 'ICollection<object>'. API spec incomplete. First result: {first}"); | ||
| //} | ||
|
|
||
| //[SkippableFact] | ||
| //public async Task AccountsAllAsync_WithIamIds_ReturnsResults() | ||
| //{ | ||
| // // Skip if no test data configured | ||
| // Skip.IfNot(!string.IsNullOrWhiteSpace(_fixture.TestData.IamIds), | ||
| // "TestData:IamIds not configured in user secrets or environment variables"); | ||
|
|
||
| // // Act | ||
| // var result = await _fixture.Client.Api.AccountsAllAsync(iamids: _fixture.TestData.IamIds); | ||
|
|
||
| // // Assert | ||
| // Assert.NotNull(result); | ||
| // var first = result.FirstOrDefault(); | ||
| // Assert.Fail($"AccountsAllAsync returns untyped 'ICollection<object>'. API spec incomplete. First result: {first}"); | ||
| //} | ||
|
|
||
| //[Fact] | ||
| //public async Task AccountsAllAsync_WithLimit_ReturnsResults() | ||
| //{ | ||
| // // Arrange | ||
| // var limit = 10; | ||
|
|
||
| // // Act | ||
| // var result = await _fixture.Client.Api.AccountsAllAsync(limit: limit); | ||
|
|
||
| // // Assert | ||
| // Assert.NotNull(result); | ||
| // Assert.True(result.Count <= limit, | ||
| // $"Expected at most {limit} results, got {result.Count}"); | ||
| // var first = result.FirstOrDefault(); | ||
| // Assert.Fail($"AccountsAllAsync returns untyped 'ICollection<object>'. API spec incomplete. First result: {first}"); | ||
| //} | ||
|
|
||
| //[SkippableFact] | ||
| //public async Task AccountsAsync_WithId_ReturnsResult() | ||
| //{ | ||
| // // Skip if no test data configured | ||
| // Skip.IfNot(!string.IsNullOrWhiteSpace(_fixture.TestData.AccountId), | ||
| // "TestData:AccountId not configured in user secrets or environment variables"); | ||
|
|
||
| // // Act | ||
| // var result = await _fixture.Client.Api.AccountsAsync(_fixture.TestData.AccountId!); | ||
|
|
||
| // // Assert | ||
| // Assert.NotNull(result); | ||
| // Assert.Fail($"AccountsAsync returns untyped 'object'. API spec incomplete. Response: {result}"); | ||
| //} | ||
| // Act | ||
| var result = await _fixture.Client.Api.PeopleAsync(loginid: _fixture.TestData.LoginId); | ||
|
|
||
| #endregion | ||
| // Assert | ||
| result.ShouldNotBeNull(); | ||
| result.Count.ShouldBeGreaterThan(0); | ||
| result.ElementAt(0).Iam_id.ShouldNotBeNullOrEmpty(); | ||
| } |
There was a problem hiding this comment.
Filter-focused tests should assert filter correctness, not just non-empty results.
These tests currently pass even if the API ignores loginid / manager_iam_id. Please assert that returned rows actually satisfy the requested filter values, especially in PeopleAsync_WithLoginId_ReturnsResults, PeopleAsync_WithManagerIamId_ReturnsResults, and GraphQL_TypedPeopleQuery_ByLoginId_ReturnsMatchingPerson.
Also applies to: 103-115, 153-177
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@IntegrationTests/RosettaApiTests.cs` around lines 87 - 100, Update the three
tests (PeopleAsync_WithLoginId_ReturnsResults,
PeopleAsync_WithManagerIamId_ReturnsResults,
GraphQL_TypedPeopleQuery_ByLoginId_ReturnsMatchingPerson) to assert that the
returned records actually match the requested filter values: use the existing
inputs (_fixture.TestData.LoginId and _fixture.TestData.ManagerIamId) and verify
that each returned result (or at minimum that at least one returned result) has
its Loginid property equal to the requested login id (case-insensitive) for the
loginid tests and that the Manager_iam_id property equals the requested manager
id for the manager test; replace or augment the current non-empty assertions
(result.Count/ElementAt checks) with these equality checks so the tests fail if
the API ignores the filters.
Comment on lines
+210
to
+223
| public async Task CollegesAsync_WithCollegeCode_ReturnsMatchingCollege() | ||
| { | ||
| // First get all colleges to find a valid code | ||
| var all = await _fixture.Client.Api.CollegesAsync(); | ||
| Skip.If(all.Count == 0, "No colleges returned from API"); | ||
| var code = all.First().College_code; | ||
|
|
||
| // Act | ||
| var result = await _fixture.Client.Api.CollegesAsync(college_code: code); | ||
|
|
||
| // Assert | ||
| result.ShouldNotBeNull(); | ||
| result.Count.ShouldBeGreaterThan(0); | ||
| result.All(c => c.College_code == code).ShouldBeTrue(); |
There was a problem hiding this comment.
Guard the seed college_code before using it as a filter.
all.First().College_code can be null/empty, which can turn the filtered call into an unfiltered one and make this test flaky.
Proposed fix
var all = await _fixture.Client.Api.CollegesAsync();
+ all.ShouldNotBeNull();
Skip.If(all.Count == 0, "No colleges returned from API");
var code = all.First().College_code;
+ Skip.If(string.IsNullOrWhiteSpace(code), "Seed college_code was empty");
// Act
var result = await _fixture.Client.Api.CollegesAsync(college_code: code);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@IntegrationTests/RosettaApiTests.cs` around lines 210 - 223, The test
CollegesAsync_WithCollegeCode_ReturnsMatchingCollege uses
all.First().College_code without validating it; update the seed selection before
calling _fixture.Client.Api.CollegesAsync(college_code: code) by finding a
non-null/non-empty College_code (e.g., use all.FirstOrDefault(c =>
!string.IsNullOrWhiteSpace(c.College_code))) and if none found call Skip or
Assert.Skip so the test doesn't run with an empty filter; ensure you reference
the local variable code and the calls to _fixture.Client.Api.CollegesAsync when
implementing the guard.
Comment on lines
29
to
+34
| /// <summary> | ||
| /// The OAuth 2.0 scope for token requests. | ||
| /// Optional: Space-separated list of OAuth 2.0 scopes to request. | ||
| /// Available scopes: admin:all, read:public, read:sensitive, read:campaign, write:public, write:sensitive. | ||
| /// Example: "read:public read:sensitive" | ||
| /// </summary> | ||
| public string Scope { get; set; } = string.Empty; | ||
| public string? Scope { get; set; } |
There was a problem hiding this comment.
Inconsistency between nullable declaration and validation.
The Scope property is declared as nullable (string?) and documented as "Optional", but Validate() at lines 65-66 throws if it's null or whitespace. This creates conflicting expectations.
Either:
- Make validation consistent with the optional declaration by removing the Scope check from
Validate(), or - Keep Scope required and revert the property to non-nullable with appropriate documentation.
Given that OAuthHandler already conditionally includes the scope (lines 92-93), option 1 seems aligned with the intended design.
🔧 Proposed fix to align validation with optional declaration
public void Validate()
{
if (string.IsNullOrWhiteSpace(BaseUrl))
throw new InvalidOperationException("RosettaClientOptions.BaseUrl is required.");
if (string.IsNullOrWhiteSpace(TokenUrl))
throw new InvalidOperationException("RosettaClientOptions.TokenUrl is required.");
if (string.IsNullOrWhiteSpace(ClientId))
throw new InvalidOperationException("RosettaClientOptions.ClientId is required.");
if (string.IsNullOrWhiteSpace(ClientSecret))
throw new InvalidOperationException("RosettaClientOptions.ClientSecret is required.");
-
- if (string.IsNullOrWhiteSpace(Scope))
- throw new InvalidOperationException("RosettaClientOptions.Scope is required.");
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@UCD.Rosetta.Client/Core/Configuration/RosettaClientOptions.cs` around lines
29 - 34, The Validate() method in RosettaClientOptions currently throws if Scope
is null/whitespace which conflicts with the property being nullable and the
OAuthHandler conditionally adding scope; update RosettaClientOptions.Validate()
to remove the null/whitespace check for the Scope property so Scope remains
optional, leaving the Scope string? declaration and documentation unchanged and
ensuring OAuthHandler's conditional logic (the code that checks Scope before
including it) continues to work.
There was a problem hiding this comment.
Pull request overview
Adds a strongly-typed GraphQL client surface to the existing Rosetta .NET client by introducing ZeroQL codegen and extracting a GraphQL SDL schema from the OpenAPI spec.
Changes:
- Extract GraphQL SDL from
specs/rosetta-api.jsonintospecs/rosetta-api.graphqland add ZeroQL config/tooling. - Extend
RosettaClientwith aGraphQLproperty backed by a dedicated HttpClient. - Improve debug logging output and add a lenient JSON converter to tolerate irregular array payloads; update docs/examples/tests accordingly.
Reviewed changes
Copilot reviewed 17 out of 18 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| update-spec.sh | Extracts embedded GraphQL schema into a standalone SDL file during spec updates. |
| specs/rosetta-api.graphql | Adds the extracted GraphQL SDL schema used for ZeroQL generation. |
| UCD.Rosetta.Client/rosetta.zeroql.json | Configures ZeroQL code generation (schema path/namespace/client name). |
| UCD.Rosetta.Client/UCD.Rosetta.Client.csproj | Adds ZeroQL package dependency. |
| UCD.Rosetta.Client/Core/RosettaClient.cs | Introduces GraphQL client surface and GraphQL-specific HttpClient lifecycle. |
| UCD.Rosetta.Client/Core/Extensions/ClientExtensions.cs | Changes debug logging to Trace + file output; registers new converter. |
| UCD.Rosetta.Client/Core/Converters/LenientTypedCollectionConverter.cs | Adds converter factory to skip invalid elements in typed collections. |
| UCD.Rosetta.Client/Core/Configuration/RosettaClientOptions.cs | Updates OAuth scope option docs/type. |
| UCD.Rosetta.Client/Core/Authentication/OAuthHandler.cs | Makes OAuth scope parameter conditional in token requests. |
| UCD.Rosetta.Client/.editorconfig | Attempts to suppress CS1591 for ZeroQL-generated code. |
| README.md | Documents the new GraphQL client surface and updates examples/endpoint docs. |
| IntegrationTests/appsettings.json | Updates token URL and test data keys; sets a default scope. |
| IntegrationTests/TestDataOptions.cs | Updates test data options to align with revised integration tests. |
| IntegrationTests/RosettaApiTests.cs | Adds GraphQL raw + typed query integration tests and improves assertions. |
| Example/appsettings.json | Sets an explicit example scope. |
| Example/Program.cs | Adds GraphQL examples (raw + strongly typed) and expands REST examples. |
| .gitignore | Ignores the new debug log file output. |
| .config/dotnet-tools.json | Adds zeroql CLI as a local dotnet tool. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
29
to
35
| /// <summary> | ||
| /// The OAuth 2.0 scope for token requests. | ||
| /// Optional: Space-separated list of OAuth 2.0 scopes to request. | ||
| /// Available scopes: admin:all, read:public, read:sensitive, read:campaign, write:public, write:sensitive. | ||
| /// Example: "read:public read:sensitive" | ||
| /// </summary> | ||
| public string Scope { get; set; } = string.Empty; | ||
| public string? Scope { get; set; } | ||
|
|
There was a problem hiding this comment.
Scope is now documented/typed as optional (string?), and OAuthHandler conditionally adds the scope parameter, but Validate() still throws when Scope is null/empty. This makes the option effectively required at runtime and contradicts the new behavior/docs. Either make Scope required again (non-null + default) or update Validate() to not require it.
Comment on lines
+62
to
+75
| // Write to a temp file — always accessible regardless of test runner output capture | ||
| System.IO.File.WriteAllText(logPath, string.Join(System.Environment.NewLine, lines)); | ||
| } | ||
| } | ||
|
|
||
| private static string? FindSolutionRoot() | ||
| { | ||
| var dir = new System.IO.DirectoryInfo(AppContext.BaseDirectory); | ||
| while (dir != null) | ||
| { | ||
| if (dir.GetFiles("*.sln").Length > 0) | ||
| return dir.FullName; | ||
| dir = dir.Parent; | ||
| } |
There was a problem hiding this comment.
ProcessResponse writes the debug log to disk unconditionally when enabled. If FindSolutionRoot() hits an unreadable directory (e.g., restricted parent folders) or the log path is not writable, GetFiles/WriteAllText can throw and break otherwise-successful API calls while debugging. Consider wrapping the path discovery and file write in a try/catch (and fall back to Trace only) so debug logging can’t fail requests.
Suggested change
| // Write to a temp file — always accessible regardless of test runner output capture | |
| System.IO.File.WriteAllText(logPath, string.Join(System.Environment.NewLine, lines)); | |
| } | |
| } | |
| private static string? FindSolutionRoot() | |
| { | |
| var dir = new System.IO.DirectoryInfo(AppContext.BaseDirectory); | |
| while (dir != null) | |
| { | |
| if (dir.GetFiles("*.sln").Length > 0) | |
| return dir.FullName; | |
| dir = dir.Parent; | |
| } | |
| // Write to a file — swallow any IO errors so logging cannot break requests | |
| try | |
| { | |
| System.IO.File.WriteAllText(logPath, string.Join(System.Environment.NewLine, lines)); | |
| } | |
| catch (System.Exception ex) | |
| { | |
| Trace.WriteLine($"Failed to write debug log to '{logPath}': {ex}"); | |
| } | |
| } | |
| } | |
| private static string? FindSolutionRoot() | |
| { | |
| try | |
| { | |
| var dir = new System.IO.DirectoryInfo(AppContext.BaseDirectory); | |
| while (dir != null) | |
| { | |
| if (dir.GetFiles("*.sln").Length > 0) | |
| return dir.FullName; | |
| dir = dir.Parent; | |
| } | |
| } | |
| catch (System.Exception ex) | |
| { | |
| // Swallow any exceptions and fall back to null so logging cannot break requests | |
| Trace.WriteLine($"Error while trying to find solution root for debug logging: {ex}"); | |
| } |
Comment on lines
112
to
124
| BaseUrl = baseUrl | ||
| }; | ||
| _disposeHttpClient = false; // Don't dispose HttpClient when using IHttpClientFactory | ||
|
|
||
| // Create a separate HttpClient for the GraphQL endpoint (always owned by RosettaClient) | ||
| var graphqlOAuthHandler = new OAuthHandler(options) { InnerHandler = new HttpClientHandler() }; | ||
| _graphqlHttpClient = new HttpClient(graphqlOAuthHandler) | ||
| { | ||
| BaseAddress = new Uri(baseUrl.TrimEnd('/') + "/graphql"), | ||
| Timeout = TimeSpan.FromSeconds(options.TimeoutSeconds) | ||
| }; | ||
| GraphQL = new RosettaGraphQLClient(_graphqlHttpClient); | ||
| } |
There was a problem hiding this comment.
When using the HttpClient + IHttpClientFactory constructor, the REST client benefits from pooled handlers, but the GraphQL client still creates its own HttpClient/HttpClientHandler. In scoped/transient lifetimes this can lead to unnecessary handler churn and potential socket exhaustion, undermining the factory-based setup. Consider creating the GraphQL HttpClient via IHttpClientFactory as well (e.g., a named client) or reusing the provided httpClient with a relative /graphql request path.
Comment on lines
+39
to
+45
| jq -r '.externalDocs.description' "${SPEC_FILE}" \ | ||
| | awk '/```graphql/{found=1; next} found && /```/{exit} found{print}' \ | ||
| | sed 's/^ //' \ | ||
| > "${GRAPHQL_FILE}" | ||
| # Append schema root declaration required by ZeroQL codegen | ||
| printf '\nschema {\n query: Query\n}\n' >> "${GRAPHQL_FILE}" | ||
| echo "✅ GraphQL schema extracted: ${GRAPHQL_FILE}" |
There was a problem hiding this comment.
The GraphQL schema extraction always appends the schema { query: Query } block and prints success even if no schema was actually extracted (e.g., missing/changed externalDocs.description format). This can leave rosetta-api.graphql empty/invalid and still proceed. Consider validating that extraction produced non-empty output (or contains expected markers like type Query) before appending/overwriting, and emit a clear warning/error if not.
Suggested change
| jq -r '.externalDocs.description' "${SPEC_FILE}" \ | |
| | awk '/```graphql/{found=1; next} found && /```/{exit} found{print}' \ | |
| | sed 's/^ //' \ | |
| > "${GRAPHQL_FILE}" | |
| # Append schema root declaration required by ZeroQL codegen | |
| printf '\nschema {\n query: Query\n}\n' >> "${GRAPHQL_FILE}" | |
| echo "✅ GraphQL schema extracted: ${GRAPHQL_FILE}" | |
| GRAPHQL_TEMP="$(mktemp)" | |
| jq -r '.externalDocs.description' "${SPEC_FILE}" \ | |
| | awk '/```graphql/{found=1; next} found && /```/{exit} found{print}' \ | |
| | sed 's/^ //' \ | |
| > "${GRAPHQL_TEMP}" | |
| # Validate that we actually extracted a GraphQL schema before overwriting the target file | |
| if [ -s "${GRAPHQL_TEMP}" ] && grep -q 'type[[:space:]]\+Query' "${GRAPHQL_TEMP}"; then | |
| mv "${GRAPHQL_TEMP}" "${GRAPHQL_FILE}" | |
| # Append schema root declaration required by ZeroQL codegen | |
| printf '\nschema {\n query: Query\n}\n' >> "${GRAPHQL_FILE}" | |
| echo "✅ GraphQL schema extracted: ${GRAPHQL_FILE}" | |
| else | |
| rm -f "${GRAPHQL_TEMP}" | |
| echo "⚠️ No GraphQL schema could be extracted from externalDocs.description; leaving existing ${GRAPHQL_FILE} unchanged." | |
| fi |
|
|
||
| # Suppress missing XML comment warnings (CS1591) for ZeroQL auto-generated code. | ||
| # ZeroQL v8 does not emit XML docs on its generated types; this is a known limitation. | ||
| [obj/ZeroQL/**.g.cs] |
There was a problem hiding this comment.
The EditorConfig section header [obj/ZeroQL/**.g.cs] is unlikely to match files in nested folders as intended (typical patterns use **/*.g.cs). If this glob doesn’t match, CS1591 warnings from ZeroQL-generated code won’t be suppressed and builds may start failing when warnings are treated as errors. Consider changing the pattern to something like obj/ZeroQL/**/*.g.cs (or **/obj/ZeroQL/**/*.g.cs).
Suggested change
| [obj/ZeroQL/**.g.cs] | |
| [obj/ZeroQL/**/*.g.cs] |
Comment on lines
+36
to
+45
| var elementType = typeToConvert.GetGenericArguments()[0]; | ||
| var converterType = typeof(LenientTypedCollectionConverter<>).MakeGenericType(elementType); | ||
| return (JsonConverter)Activator.CreateInstance(converterType)!; | ||
| } | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Deserializes ICollection<T> from a JSON array, skipping any elements that are not | ||
| /// JSON objects (e.g. null tokens, strings, or numbers) instead of throwing. | ||
| /// </summary> |
There was a problem hiding this comment.
CanConvert claims to support both ICollection<T> and List<T>, but CreateConverter always returns LenientTypedCollectionConverter<T> which is a JsonConverter<ICollection<T>>. If the serializer ever hits a List<T> property/value, this factory will be selected and then fail because the returned converter isn’t compatible with List<T>. Either remove typeof(List<>) from CanConvert, or add a dedicated JsonConverter<List<T>> implementation for the List<> case.
Suggested change
| var elementType = typeToConvert.GetGenericArguments()[0]; | |
| var converterType = typeof(LenientTypedCollectionConverter<>).MakeGenericType(elementType); | |
| return (JsonConverter)Activator.CreateInstance(converterType)!; | |
| } | |
| } | |
| /// <summary> | |
| /// Deserializes ICollection<T> from a JSON array, skipping any elements that are not | |
| /// JSON objects (e.g. null tokens, strings, or numbers) instead of throwing. | |
| /// </summary> | |
| var genericDef = typeToConvert.GetGenericTypeDefinition(); | |
| var elementType = typeToConvert.GetGenericArguments()[0]; | |
| Type converterType; | |
| if (genericDef == typeof(List<>)) | |
| { | |
| converterType = typeof(LenientTypedListConverter<>).MakeGenericType(elementType); | |
| } | |
| else | |
| { | |
| // Default to ICollection<T> handling | |
| converterType = typeof(LenientTypedCollectionConverter<>).MakeGenericType(elementType); | |
| } | |
| return (JsonConverter)Activator.CreateInstance(converterType)!; | |
| } | |
| } | |
| /// <summary> | |
| /// Deserializes ICollection<T> from a JSON array, skipping any elements that are not | |
| /// Deserializes List<T> from a JSON array, skipping any elements that are not | |
| /// JSON objects (e.g. null tokens, strings, or numbers) instead of throwing. | |
| /// </summary> | |
| internal class LenientTypedListConverter<T> : JsonConverter<List<T>> where T : class | |
| { | |
| public override List<T>? Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options) | |
| { | |
| if (reader.TokenType == JsonTokenType.Null) | |
| return null; | |
| if (reader.TokenType != JsonTokenType.StartArray) | |
| { | |
| // Non-array value where an array was expected; skip and return empty | |
| reader.Skip(); | |
| return new List<T>(); | |
| } | |
| // CanConvert only matches List<T> / ICollection<T>, never T itself, so no recursion risk | |
| // when we call Deserialize<T> below. | |
| var list = new List<T>(); | |
| while (reader.Read()) | |
| { | |
| if (reader.TokenType == JsonTokenType.EndArray) | |
| return list; | |
| if (reader.TokenType == JsonTokenType.Null | |
| || reader.TokenType != JsonTokenType.StartObject) | |
| { | |
| // Skip unexpected tokens (null elements, strings, numbers, booleans) | |
| reader.Skip(); | |
| continue; | |
| } | |
| var item = JsonSerializer.Deserialize<T>(ref reader, options); | |
| if (item != null) | |
| list.Add(item); | |
| } | |
| return list; | |
| } | |
| public override void Write(Utf8JsonWriter writer, List<T> value, JsonSerializerOptions options) | |
| { | |
| writer.WriteStartArray(); | |
| foreach (var item in value) | |
| JsonSerializer.Serialize(writer, item, options); | |
| writer.WriteEndArray(); | |
| } | |
| } | |
| /// <summary> | |
| /// Deserializes ICollection<T> from a JSON array, skipping any elements that are not | |
| /// JSON objects (e.g. null tokens, strings, or numbers) instead of throwing. | |
| /// </summary> |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Release Notes
New Features
Documentation
Configuration