Update Rosetta API spec and align GraphQL examples with v1.0.40 - #5
Update Rosetta API spec and align GraphQL examples with v1.0.40#5sprucely wants to merge 2 commits into
Conversation
|
Warning Review limit reached
Next review available in: 40 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThe GraphQL schema now uses filter inputs and result wrappers, and adds group and association queries. Examples and integration tests use the new API shape. JSON collection conversion accepts broader scalar and primitive values. Specification extraction supports flexible GraphQL fences. ChangesGraphQL client contracts and validation
Serialization and specification support
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Example
participant RosettaAPI
participant Results
Example->>RosettaAPI: send query with PeopleFilterInput
RosettaAPI-->>Results: return PeopleResult
Example->>Results: select Results and person fields
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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: 4
🧹 Nitpick comments (3)
specs/rosetta-api.graphql (1)
310-348: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueResult wrapper naming is inconsistent.
CollegeResultis singular whileMajorsResultis plural, andGroupSourceResultsuses a pluralResultssuffix.GroupByIdResult.resultsreturns a single object while every other wrapper returns a list. These names become generated C# types, so the inconsistency is visible to consumers. If the schema is generated from the upstream service, keep it as is and ignore this note. If the schema is authored here, align the suffixes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@specs/rosetta-api.graphql` around lines 310 - 348, If this schema is authored locally, align the result wrapper naming in PeopleResult and related types: use consistent singular Result suffixes and make GroupByIdResult.results return a list like the other wrappers, updating affected references accordingly. If the schema is generated from the upstream service, leave these definitions unchanged.IntegrationTests/RosettaApiTests.cs (1)
354-359: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winDetect quota errors from the errors collection, not from the serialized payload.
IsQuotaExceeded<TResponse>serializes the entire response to JSON on every typed query, including successful ones. This has two drawbacks. It allocates and copies the full result set on the success path. It also matches the quota strings against response data, so a person record or a college title that contains the text triggers a false skip.Inspect the GraphQL errors instead of the whole response.
♻️ Suggested direction
- private static bool IsQuotaExceeded<TResponse>(TResponse response) - { - var responseJson = JsonSerializer.Serialize(response); - return responseJson.Contains("status code 429", StringComparison.OrdinalIgnoreCase) - || responseJson.Contains("Quota has been exceeded", StringComparison.OrdinalIgnoreCase); - } + private static bool IsQuotaExceeded<TResponse>(TResponse response) + { + // Serialize only the errors, not the full data payload. + var errors = (response as dynamic)?.Errors; + if (errors is null) + return false; + + var errorsJson = JsonSerializer.Serialize(errors); + return errorsJson.Contains("status code 429", StringComparison.OrdinalIgnoreCase) + || errorsJson.Contains("Quota has been exceeded", StringComparison.OrdinalIgnoreCase); + }A stronger option is to constrain
TResponseto the concrete ZeroQL result type so theErrorsproperty is accessed withoutdynamic.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@IntegrationTests/RosettaApiTests.cs` around lines 354 - 359, Update IsQuotaExceeded<TResponse> to inspect the concrete ZeroQL response’s Errors collection rather than serializing the entire response. Constrain TResponse to the appropriate result type so Errors can be accessed statically, and detect the existing quota indicators only within error messages while preserving the boolean behavior for quota and non-quota responses.IntegrationTests/RosettaClientFixture.cs (1)
18-18: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winThe cached sample also caches failures.
Lazy<Task<T>>stores the returnedTask. If the firstPeopleAsync(limit: 25)call fails, for example with HTTP 429, the faulted task stays cached. Every later call toGetPeopleSampleAsyncthen rethrows the same exception, so all discovery-based tests in the class fail instead of skipping or retrying. This PR adds quota retry handling for GraphQL, so the same transient condition is expected on the REST path.Consider returning an empty collection on failure, or resetting the
Lazywhen the task faults.♻️ Option: do not cache a faulted result
- private readonly Lazy<Task<ICollection<Person>>> _peopleSample; + private Lazy<Task<ICollection<Person>>> _peopleSample = null!;- _peopleSample = new Lazy<Task<ICollection<Person>>>(() => Client.Api.PeopleAsync(limit: 25)); + _peopleSample = new Lazy<Task<ICollection<Person>>>(CreatePeopleSampleTask);- public Task<ICollection<Person>> GetPeopleSampleAsync() => _peopleSample.Value; + public Task<ICollection<Person>> GetPeopleSampleAsync() => _peopleSample.Value; + + private async Task<ICollection<Person>> CreatePeopleSampleTask() + { + try + { + return await Client.Api.PeopleAsync(limit: 25); + } + catch + { + // Allow a later call to retry instead of replaying the cached failure. + _peopleSample = new Lazy<Task<ICollection<Person>>>(CreatePeopleSampleTask); + throw; + } + }Also applies to: 42-42, 52-52
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@IntegrationTests/RosettaClientFixture.cs` at line 18, Update the cached people-sample flow around _peopleSample and GetPeopleSampleAsync so transient failures from PeopleAsync(limit: 25) are not retained by Lazy<Task<ICollection<Person>>>. On a fault, reset the lazy before retrying or return an empty collection, while preserving successful-result caching and the existing discovery-test behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@IntegrationTests/RosettaApiTests.cs`:
- Around line 232-239: Replace the [Fact] attributes with [SkippableFact] on
GraphqlAsync_WithPeopleQuery_ReturnsResult,
GraphQL_TypedPeopleQuery_ReturnsResults, and
GraphQL_TypedCollegesQuery_ReturnsAllColleges so Skip.If(true) from their
quota-skipping helpers is reported as a skipped test rather than a failure.
In `@README.md`:
- Around line 216-224: Update the second example’s response variable to a
distinct name so it does not redeclare response, and revise the ❌ comment to
identify the captured _options.LoginId property access as the ZeroQL compilation
error; do not imply that inline PeopleFilterInput construction is itself
invalid.
In `@UCD.Rosetta.Client/Core/Converters/LenientTypedCollectionConverter.cs`:
- Line 24: Remove the JsonTokenType-based prefilter from
LenientTypedCollectionConverter.CanReadCurrentTokenAsElement. In the collection
parsing flow, copy the Utf8JsonReader and attempt JsonSerializer.Deserialize<T>
on the copy; on failure, skip the original reader and continue, and on success
assign the value and advance the original reader from the copy. Preserve support
for nullable primitives, object, Guid, DateTime, arrays, custom converters, and
scalar ICollection<T> values.
In `@update-spec.sh`:
- Around line 47-48: Update the fence-matching expressions in the script’s
extraction logic to allow leading blank-space before both opening and closing
Markdown fences, while preserving the existing optional graphql label and
termination behavior.
---
Nitpick comments:
In `@IntegrationTests/RosettaApiTests.cs`:
- Around line 354-359: Update IsQuotaExceeded<TResponse> to inspect the concrete
ZeroQL response’s Errors collection rather than serializing the entire response.
Constrain TResponse to the appropriate result type so Errors can be accessed
statically, and detect the existing quota indicators only within error messages
while preserving the boolean behavior for quota and non-quota responses.
In `@IntegrationTests/RosettaClientFixture.cs`:
- Line 18: Update the cached people-sample flow around _peopleSample and
GetPeopleSampleAsync so transient failures from PeopleAsync(limit: 25) are not
retained by Lazy<Task<ICollection<Person>>>. On a fault, reset the lazy before
retrying or return an empty collection, while preserving successful-result
caching and the existing discovery-test behavior.
In `@specs/rosetta-api.graphql`:
- Around line 310-348: If this schema is authored locally, align the result
wrapper naming in PeopleResult and related types: use consistent singular Result
suffixes and make GroupByIdResult.results return a list like the other wrappers,
updating affected references accordingly. If the schema is generated from the
upstream service, leave these definitions unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ff9804ca-28ac-4c0a-8081-b09e1cfe03e6
⛔ Files ignored due to path filters (1)
UCD.Rosetta.Client/Generated/RosettaApiClient.g.csis excluded by!**/generated/**
📒 Files selected for processing (8)
Example/Program.csIntegrationTests/RosettaApiTests.csIntegrationTests/RosettaClientFixture.csREADME.mdUCD.Rosetta.Client/Core/Converters/LenientTypedCollectionConverter.csspecs/rosetta-api.graphqlspecs/rosetta-api.jsonupdate-spec.sh
2 participants
Summary by CodeRabbit
New Features
Improvements