Add curated Rosetta domain clients and refresh API spec integration - #6
Open
sprucely wants to merge 3 commits into
Open
Add curated Rosetta domain clients and refresh API spec integration#6sprucely wants to merge 3 commits into
sprucely wants to merge 3 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe PR expands the Rosetta GraphQL schema with typed domain models, filters, result wrappers, and queries. It updates REST and GraphQL examples, integration tests, fixture caching, API-limit handling, documentation, and specification extraction. ChangesRosetta API contract and integration update
Estimated code review effort: 4 (Complex) | ~45 minutes Mergeability Score: ⚪ Minimal · up to This PR updates generated clients, API specifications, and integration examples; no actionable merge-blocking risk remains, so it is merge-ready after normal checks and review. Sequence Diagram(s)sequenceDiagram
participant RosettaApiTests
participant RosettaClientFixture
participant GeneratedRosettaClient
participant RosettaAPI
RosettaApiTests->>RosettaClientFixture: GetPeopleSampleAsync()
RosettaClientFixture->>GeneratedRosettaClient: PeopleGETAsync(limit 25)
GeneratedRosettaClient->>RosettaAPI: People request
RosettaAPI-->>GeneratedRosettaClient: People response
GeneratedRosettaClient-->>RosettaClientFixture: Cached people sample
RosettaApiTests->>GeneratedRosettaClient: Execute REST or GraphQL test request
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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: 2
🧹 Nitpick comments (5)
IntegrationTests/RosettaDomainClientTests.cs (1)
15-16: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove
asyncfrom the handler that has noawait.The lambda at line 15 is
asyncbut contains noawait, so the compiler reports CS1998. Return the response withTask.FromResult, as the handlers at lines 104 and 171 do.♻️ Proposed change
- var client = CreateGeneratedClient(async request => + var client = CreateGeneratedClient(request => {- return JsonResponse(""" + return Task.FromResult(JsonResponse(""" [{ ... - """); + """));Also applies to: 54-55
🤖 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/RosettaDomainClientTests.cs` around lines 15 - 16, Remove async from the request handler lambdas passed to CreateGeneratedClient at the affected locations, and return each response via Task.FromResult, matching the existing handlers around lines 104 and 171.IntegrationTests/RosettaApiTests.cs (1)
129-210: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider extracting the repeated discovery pattern.
The five helpers share one shape: probe the configured value with a search, then fall back to a sampled value, then skip. A single generic helper reduces duplication. Each probe also issues an extra
People.SearchAsynccall, which consumes quota in the same environment that the retry wrapper protects against.private async Task<string> ResolveFilterValueAsync( string? configured, Func<string, PeopleQuery> queryFactory, Func<ICollection<Person>, string?> fromSample, string skipReason) { if (!string.IsNullOrWhiteSpace(configured) && (await SkipEnvironmentLimitations(() => _fixture.Client.People.SearchAsync(queryFactory(configured)))).Count > 0) { return configured; } var value = fromSample(await SkipEnvironmentLimitations(() => _fixture.GetPeopleSampleAsync())); Skip.If(string.IsNullOrWhiteSpace(value), skipReason); return value!; }🤖 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 129 - 210, Extract the repeated configured-value, sample-fallback, and skip flow from GetIamIdForPeopleFilterAsync, GetIamIdsForPeopleFilterAsync, GetEmailForPeopleFilterAsync, GetLoginIdForPeopleFilterAsync, and GetManagerIamIdForPeopleFilterAsync into a generic ResolveFilterValueAsync helper. Pass each filter’s query factory and sample selector to preserve existing behavior and skip reasons, while keeping the People.SearchAsync calls wrapped by SkipEnvironmentLimitations.UCD.Rosetta.Client/Core/Domain/PeopleClient.cs (1)
16-59: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffConsider a single forwarding helper for the five people endpoints.
The five methods repeat the same 37-parameter mapping. Only the endpoint method and the generated enum family change. Each future spec change must be applied five times, and a single missed parameter is easy to overlook.
A generic helper keeps the per-endpoint enum types explicit and removes the repetition:
private Task<ICollection<Person>> InvokeAsync<TAffiliation, TAcademic, TClass>( PeopleQuery query, Func<PeopleQuery, TAffiliation?, TAcademic?, TClass?, CancellationToken, Task<ICollection<Person>>> invoke, CancellationToken cancellationToken) where TAffiliation : struct, Enum where TAcademic : struct, Enum where TClass : struct, Enum => invoke( query, RosettaDomainMapping.ParseEnumMember<TAffiliation>(query.AffiliationState), RosettaDomainMapping.ParseEnumMember<TAcademic>(query.AcademicLevel), RosettaDomainMapping.ParseEnumMember<TClass>(query.ClassLevel), cancellationToken);The per-endpoint methods then supply only the generated call. This is optional; the current code is correct.
Also applies to: 79-122, 124-167, 169-212, 214-257
🤖 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 `@UCD.Rosetta.Client/Core/Domain/PeopleClient.cs` around lines 16 - 59, Refactor the repeated parameter mapping in the five people endpoint methods, including SearchAsync, into a shared generic InvokeAsync helper. Keep the generated endpoint delegate and its explicit affiliation, academic, and class enum types supplied by each method, while centralizing the full PeopleQuery-to-endpoint argument mapping and cancellation handling.UCD.Rosetta.Client/Core/Domain/RosettaDomainModels.cs (1)
215-236: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winCache the enum-member lookup and report unmatched values clearly.
Two points apply to
ParseEnumMember:
- The method reflects over all fields of
TEnumon every call.PeopleClientcalls it three times per request, so each REST search pays reflection cost.- If no
EnumMemberAttributematches and the name does not match,Enum.ParsethrowsArgumentExceptionwithout naming the offending filter. Callers pass raw strings such asPeopleQuery.AffiliationState, so the message should identify the value and the target type.Also restrict the field scan to public static fields, so the scan does not inspect the compiler-generated
value__field.♻️ Proposed refactor
+ private static readonly System.Collections.Concurrent.ConcurrentDictionary<Type, Dictionary<string, object>> EnumMemberCache = new(); + public static TEnum? ParseEnumMember<TEnum>(string? value) where TEnum : struct, Enum { if (string.IsNullOrWhiteSpace(value)) return null; - foreach (var field in typeof(TEnum).GetFields()) - { - var enumMember = field.GetCustomAttributes(typeof(EnumMemberAttribute), false) - .OfType<EnumMemberAttribute>() - .FirstOrDefault(); - - if (string.Equals(enumMember?.Value, value, StringComparison.OrdinalIgnoreCase)) - return (TEnum)field.GetValue(null)!; - } - - return Enum.Parse<TEnum>(value, ignoreCase: true); + var map = EnumMemberCache.GetOrAdd(typeof(TEnum), static type => + { + var members = new Dictionary<string, object>(StringComparer.OrdinalIgnoreCase); + foreach (var field in type.GetFields(System.Reflection.BindingFlags.Public | System.Reflection.BindingFlags.Static)) + { + var enumMember = field.GetCustomAttributes(typeof(EnumMemberAttribute), false) + .OfType<EnumMemberAttribute>() + .FirstOrDefault(); + + if (!string.IsNullOrEmpty(enumMember?.Value)) + members[enumMember!.Value] = field.GetValue(null)!; + } + + return members; + }); + + if (map.TryGetValue(value, out var mapped)) + return (TEnum)mapped; + + if (Enum.TryParse<TEnum>(value, ignoreCase: true, out var parsed)) + return parsed; + + throw new ArgumentException($"'{value}' is not a valid {typeof(TEnum).Name} value.", nameof(value)); }🤖 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 `@UCD.Rosetta.Client/Core/Domain/RosettaDomainModels.cs` around lines 215 - 236, Update ParseEnumMember<TEnum> to use a cached lookup of public static enum fields and their EnumMemberAttribute values, avoiding reflection on every call and excluding the compiler-generated value__ field. Preserve case-insensitive matching for both attribute values and enum names, and when neither matches, throw an ArgumentException whose message includes the original value and target enum type instead of relying directly on Enum.Parse.UCD.Rosetta.Client/Core/Domain/DomainClients.cs (1)
22-26: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd an account-sources route assertion.
Assert
Sources2Asyncusesaccounts/sourcesandSourcesAsyncusesgroups/sources. A membership suffix swap already causes a compile-time type error because the methods returnRoleMembershipandGroupMembership, respectively.🤖 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 `@UCD.Rosetta.Client/Core/Domain/DomainClients.cs` around lines 22 - 26, Add route assertions covering the API client methods used by GetSourcesAsync: verify Sources2Async targets accounts/sources and SourcesAsync targets groups/sources. Keep the existing membership return-type assertions, which already detect a swapped suffix at compile time.
🤖 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 420-425: Update the groupId selection in the test to coalesce each
group's Groups collection to an empty collection before SelectMany, matching the
existing ?? [] pattern used nearby. Preserve the current filtering and
subsequent GetByIdAsync validation.
In `@README.md`:
- Line 81: Guard all nullable or potentially empty API collections in README.md:
at lines 81-81, replace direct Results[0] access with FirstOrDefault() or an
equivalent length check; at lines 184-184, ensure roles.First() is non-null and
available before calling GetByIdAsync; at lines 229-230, skip null Results
elements or use null-safe access before dereferencing them.
---
Nitpick comments:
In `@IntegrationTests/RosettaApiTests.cs`:
- Around line 129-210: Extract the repeated configured-value, sample-fallback,
and skip flow from GetIamIdForPeopleFilterAsync, GetIamIdsForPeopleFilterAsync,
GetEmailForPeopleFilterAsync, GetLoginIdForPeopleFilterAsync, and
GetManagerIamIdForPeopleFilterAsync into a generic ResolveFilterValueAsync
helper. Pass each filter’s query factory and sample selector to preserve
existing behavior and skip reasons, while keeping the People.SearchAsync calls
wrapped by SkipEnvironmentLimitations.
In `@IntegrationTests/RosettaDomainClientTests.cs`:
- Around line 15-16: Remove async from the request handler lambdas passed to
CreateGeneratedClient at the affected locations, and return each response via
Task.FromResult, matching the existing handlers around lines 104 and 171.
In `@UCD.Rosetta.Client/Core/Domain/DomainClients.cs`:
- Around line 22-26: Add route assertions covering the API client methods used
by GetSourcesAsync: verify Sources2Async targets accounts/sources and
SourcesAsync targets groups/sources. Keep the existing membership return-type
assertions, which already detect a swapped suffix at compile time.
In `@UCD.Rosetta.Client/Core/Domain/PeopleClient.cs`:
- Around line 16-59: Refactor the repeated parameter mapping in the five people
endpoint methods, including SearchAsync, into a shared generic InvokeAsync
helper. Keep the generated endpoint delegate and its explicit affiliation,
academic, and class enum types supplied by each method, while centralizing the
full PeopleQuery-to-endpoint argument mapping and cancellation handling.
In `@UCD.Rosetta.Client/Core/Domain/RosettaDomainModels.cs`:
- Around line 215-236: Update ParseEnumMember<TEnum> to use a cached lookup of
public static enum fields and their EnumMemberAttribute values, avoiding
reflection on every call and excluding the compiler-generated value__ field.
Preserve case-insensitive matching for both attribute values and enum names, and
when neither matches, throw an ArgumentException whose message includes the
original value and target enum type instead of relying directly on Enum.Parse.
🪄 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: 3b85f7d4-85d7-4155-bc08-77e4a583e2fe
⛔ Files ignored due to path filters (1)
UCD.Rosetta.Client/Generated/RosettaApiClient.g.csis excluded by!**/generated/**
📒 Files selected for processing (12)
Example/Program.csIntegrationTests/RosettaApiTests.csIntegrationTests/RosettaClientFixture.csIntegrationTests/RosettaDomainClientTests.csREADME.mdUCD.Rosetta.Client/Core/Domain/DomainClients.csUCD.Rosetta.Client/Core/Domain/PeopleClient.csUCD.Rosetta.Client/Core/Domain/RosettaDomainModels.csUCD.Rosetta.Client/Core/RosettaClient.csspecs/rosetta-api.graphqlspecs/rosetta-api.jsonupdate-spec.sh
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
IntegrationTests/RosettaApiTests.cs (2)
144-160: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRequire two distinct IAM IDs for this test.
Both paths can return one IAM ID. The test then does not exercise the comma-separated
iamidsfilter behavior. Require at least two distinct IDs, or skip the test.🤖 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 144 - 160, Update GetIamIdsForPeopleFilterAsync to require at least two distinct IAM IDs in both the configured TestData.IamIds path and the sampled people path. Only return the comma-separated IDs when two or more are available; otherwise skip the test with an appropriate message.
221-231: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert the GraphQL response contents.
GraphqlAsyncdeserializes HTTP 200 bodies asobjectand does not inspect GraphQL errors. CastresulttoJsonElement, then assert thaterrorsis absent or empty and thatdata.people.resultsexists and contains data.🤖 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 221 - 231, Strengthen the assertions in GraphqlAsync_WithPeopleFilter_ReturnsResult by casting the response to JsonElement and verifying the GraphQL errors field is absent or empty. Also validate that data.people.results exists and contains at least one result instead of only asserting the response is non-null.README.md (1)
280-280: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winDispose the
FileResponsein the CSV example.Use
using var csvFile = await client.Api.CampaignContactsAsync(limit: 1000, save: true);.🤖 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 `@README.md` at line 280, Update the CSV example’s CampaignContactsAsync call to declare the returned FileResponse with using var, ensuring csvFile is disposed automatically while preserving the existing limit and save arguments.Source: MCP tools
🤖 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 `@README.md`:
- Line 112: Add the UCD.Rosetta.Client.Generated namespace import to both REST
examples in README.md so Person and PeoplePostRequest resolve, or fully qualify
both model types consistently in those snippets.
---
Outside diff comments:
In `@IntegrationTests/RosettaApiTests.cs`:
- Around line 144-160: Update GetIamIdsForPeopleFilterAsync to require at least
two distinct IAM IDs in both the configured TestData.IamIds path and the sampled
people path. Only return the comma-separated IDs when two or more are available;
otherwise skip the test with an appropriate message.
- Around line 221-231: Strengthen the assertions in
GraphqlAsync_WithPeopleFilter_ReturnsResult by casting the response to
JsonElement and verifying the GraphQL errors field is absent or empty. Also
validate that data.people.results exists and contains at least one result
instead of only asserting the response is non-null.
In `@README.md`:
- Line 280: Update the CSV example’s CampaignContactsAsync call to declare the
returned FileResponse with using var, ensuring csvFile is disposed automatically
while preserving the existing limit and save arguments.
🪄 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: d89baca6-2f79-4d5d-988f-3f63f2639348
⛔ Files ignored due to path filters (1)
UCD.Rosetta.Client/Generated/RosettaApiClient.g.csis excluded by!**/generated/**
📒 Files selected for processing (5)
Example/Program.csIntegrationTests/RosettaApiTests.csIntegrationTests/RosettaClientFixture.csREADME.mdUCD.Rosetta.Client/Core/RosettaClient.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- IntegrationTests/RosettaClientFixture.cs
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
1 participant
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
New Features
Documentation
Bug Fixes