feat(graphql): Create separate type for sub-parties #1510
Conversation
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughThe pull request introduces significant changes to the GraphQL schema, particularly enhancing the Changes
Possibly related PRs
Suggested reviewers
Warning Rate limit exceeded@oskogstad has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 19 minutes and 55 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![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
🧹 Outside diff range and nitpick comments (6)
src/Digdir.Domain.Dialogporten.GraphQL/EndUser/Parties/ObjectTypes.cs (2)
16-19: Consider using IReadOnlyList for better immutability.While the property is init-only, using
List<T>still allows modification of the collection after initialization. Consider usingIReadOnlyList<T>to ensure true immutability.- public List<AuthorizedSubParty>? SubParties { get; init; } + public IReadOnlyList<AuthorizedSubParty>? SubParties { get; init; }
Line range hint
3-22: Add XML documentation for GraphQL types.Consider adding XML documentation to describe the relationship between these types and their intended usage. This will improve both developer experience and GraphQL schema documentation.
Example:
/// <summary> /// Base class containing common properties for authorized parties. /// </summary> public class AuthorizedPartyBase { /// <summary> /// The unique identifier of the party. /// </summary> public string Party { get; init; } = null!; // ... document other properties } /// <summary> /// Represents an authorized party that may have sub-parties. /// </summary> public class AuthorizedParty : AuthorizedPartyBase { /// <summary> /// Optional list of sub-parties authorized under this party. /// </summary> public List<AuthorizedSubParty>? SubParties { get; init; } }src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/Authorization/LocalDevelopmentAltinnAuthorization.cs (1)
57-73: Consider using named constants for test dataThe hardcoded values like "Local Party" and "123456789" should be defined as named constants at the class level to make it clear they are test data and to facilitate updates.
internal sealed class LocalDevelopmentAltinnAuthorization : IAltinnAuthorization { + private const string TEST_PARTY_NAME = "Local Party"; + private const string TEST_SUB_PARTY_NAME = "Local Sub Party"; + private const string TEST_ORG_NUMBER = "123456789"; private readonly IDialogDbContext _db;docs/schema/V1/schema.verified.graphql (3)
69-79: Consider making subParties non-nullable for consistency.The
subPartiesfield is currently nullable while all other fields in the type are non-nullable. This inconsistency might lead to unnecessary null checks in clients. Consider making it non-nullable with an empty array as the default state:- subParties: [AuthorizedSubParty!] + subParties: [AuthorizedSubParty!]!
81-82: Add depth limit to prevent infinite nesting.The recursive structure of
AuthorizedSubPartywith its ownsubPartiesfield allows for infinite nesting, which could lead to performance issues or stack overflow errors if deeply nested data is queried. Consider:
- Adding a depth limit through a custom directive
- Documenting the maximum allowed depth
- Implementing server-side validation
Example directive definition to add:
directive @maxDepth(limit: Int!) on FIELD_DEFINITION type AuthorizedSubParty { subParties: [AuthorizedSubParty!] @maxDepth(limit: 5) # ... other fields }
69-82: Add documentation for type fields.Both
AuthorizedPartyandAuthorizedSubPartytypes lack field descriptions. Consider adding documentation comments to explain:
- The purpose and usage of each field
- The relationship between parties and sub-parties
- The meaning of boolean flags (e.g., hasKeyRole, isAccessManager)
- Any business rules or constraints
Example documentation:
type AuthorizedParty { """ List of sub-parties that this party has authority over. Empty array indicates no sub-parties. """ subParties: [AuthorizedSubParty!] """ Unique identifier of the party in the format specified by the system. """ party: String! # ... document other fields similarly }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
docs/schema/V1/schema.verified.graphql(1 hunks)src/Digdir.Domain.Dialogporten.GraphQL/EndUser/Parties/MappingProfile.cs(1 hunks)src/Digdir.Domain.Dialogporten.GraphQL/EndUser/Parties/ObjectTypes.cs(2 hunks)src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/Authorization/LocalDevelopmentAltinnAuthorization.cs(2 hunks)
🔇 Additional comments (6)
src/Digdir.Domain.Dialogporten.GraphQL/EndUser/Parties/MappingProfile.cs (1)
11-11: Verify the mapping configuration for AuthorizedSubParty
While the mapping is syntactically correct, we should verify:
- That all properties from AuthorizedPartyDto are appropriate to map to AuthorizedSubParty
- Whether any custom property mappings or transformations are needed
- That there's no risk of infinite recursion if AuthorizedParty contains SubParties
Let's analyze the type definitions and their usage:
✅ Verification successful
Based on the shell script results, I can now generate the final response:
The mapping configuration is correct and safe to use
The code analysis reveals that:
AuthorizedSubPartyis a sealed class that inherits fromAuthorizedParty, which in turn inherits fromAuthorizedPartyBase- All properties from
AuthorizedPartyDto(Party, Name, PartyType, IsDeleted, SubParties) align with the base properties inAuthorizedPartyBase - The inheritance hierarchy ensures that
AuthorizedSubPartyhas the same structure asAuthorizedParty, making the mapping compatible - No risk of infinite recursion as the SubParties property is properly typed as
List<AuthorizedSubParty>?in both DTO and model classes
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Analyze the structure of the involved types and their relationships
# Find the type definitions
echo "=== Type Definitions ==="
ast-grep --pattern 'class AuthorizedPartyDto {
$$$
}'
ast-grep --pattern 'class AuthorizedParty {
$$$
}'
ast-grep --pattern 'class AuthorizedSubParty {
$$$
}'
# Find usages of these types
echo "=== Type Usages ==="
rg -A 5 "AuthorizedPartyDto|AuthorizedParty|AuthorizedSubParty"
Length of output: 13897
src/Digdir.Domain.Dialogporten.GraphQL/EndUser/Parties/ObjectTypes.cs (1)
Line range hint 3-14: LGTM! Well-structured base class.
The base class effectively encapsulates common properties with appropriate immutability and null-checking.
src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/Authorization/LocalDevelopmentAltinnAuthorization.cs (3)
6-6: LGTM: Using directive properly added
The new using directive is correctly placed and necessary for the sub-party functionality.
59-72: LGTM: Sub-party structure implementation
The implementation correctly reflects the PR objective of supporting sub-parties with a proper hierarchical structure. The mock data provides a good example of the parent-child relationship between parties.
68-68: Verify the test organization number format
The organization number "123456789" is concatenated with NorwegianOrganizationIdentifier.PrefixWithSeparator. Ensure this generates a valid test identifier format.
✅ Verification successful
Organization identifier format is valid
The test organization number "123456789" is correctly formatted when concatenated with NorwegianOrganizationIdentifier.PrefixWithSeparator. From the codebase:
- The prefix is defined as
"urn:altinn:organization:identifier-no"inNorwegianOrganizationIdentifier.cs - The format is used consistently throughout the codebase, including in tests with valid organization numbers
- The validation logic is present in the
NorwegianOrganizationIdentifierclass - The format matches the error message in validation extensions:
"'{{PropertyName}}' must be on format '{NorwegianOrganizationIdentifier.PrefixWithSeparator}{{norwegian org-nr}}'"
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for NorwegianOrganizationIdentifier usage to verify format
ast-grep --pattern 'class NorwegianOrganizationIdentifier {
$$$
}'
Length of output: 70
Script:
#!/bin/bash
# Search for NorwegianOrganizationIdentifier class definition and usage
rg -l "NorwegianOrganizationIdentifier"
# Search for specific implementation details
rg "NorwegianOrganizationIdentifier" -A 5
Length of output: 15324
docs/schema/V1/schema.verified.graphql (1)
69-82: Verify the impact on existing queries.
The schema shows that parties query returns [AuthorizedParty!]!. With the new structure, ensure that:
- Existing clients can handle the new nested structure
- Performance remains acceptable when querying deeply nested parties
- GraphQL resolvers are properly implemented for the new fields
✅ Verification successful
The nested party structure is properly implemented with appropriate safeguards
The implementation shows that:
- The resolver and mapping are correctly implemented to handle the nested structure through AutoMapper
- Performance concerns are mitigated as the subparties are only populated when they exist (
SubParties = dto.Subunits.Count > 0 ? ... : null) - The mapping logic in
AuthorizedPartiesHelperproperly handles the recursive structure while maintaining all required fields - The GraphQL type definitions match the implementation with proper null handling (
SubPartiesis nullable in both schema and code)
The changes appear to be backward compatible since:
- All required fields from the original schema are preserved
- The nested structure is implemented as an optional field
- The mapping maintains data integrity through the object hierarchy
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for existing GraphQL queries that fetch parties
rg -l 'query.*\{.*parties.*\}' --type ts --type js --type graphql
# Search for resolver implementations
ast-grep --pattern 'class $_ : IResolver<$_, $_> {
$$$
parties($_, $_) {
$$$
}
$$$
}'
Length of output: 199
Script:
#!/bin/bash
# Search for GraphQL queries using parties, adjusting file type flags
rg -l 'query.*\{.*parties.*\}' --type typescript --type javascript
# Search for files containing 'parties' field or type definitions
rg -l 'type.*Authorized(Sub)?Party' --type typescript --type javascript
# Look for resolver implementations with different pattern
rg -A 5 'class.*Resolver.*implements.*' --type typescript --type javascript
# Search for files that might contain GraphQL queries
fd -e graphql -x rg -l 'query.*parties' {}
Length of output: 372
Script:
#!/bin/bash
# Search for GraphQL queries using parties without file type restrictions
rg -l 'query.*\{.*parties.*\}'
# Search for resolver implementations
rg -A 5 'class.*Resolver.*implements'
# Look for GraphQL operation files
fd -e gql -e graphql .
# Search for any files mentioning AuthorizedParty or AuthorizedSubParty
rg -l 'AuthorizedParty|AuthorizedSubParty'
Length of output: 991
Script:
#!/bin/bash
# Check the implementation of PartyQueries to understand the resolver
cat src/Digdir.Domain.Dialogporten.GraphQL/EndUser/PartyQueries.cs
# Look at the object types to understand the mapping
cat src/Digdir.Domain.Dialogporten.GraphQL/EndUser/Parties/ObjectTypes.cs
# Check the mapping profile for any transformation logic
cat src/Digdir.Domain.Dialogporten.GraphQL/EndUser/Parties/MappingProfile.cs
# Look at the helper class that handles authorization
cat src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/Authorization/AuthorizedPartiesHelper.cs
Length of output: 5282
src/Digdir.Domain.Dialogporten.GraphQL/EndUser/Parties/ObjectTypes.cs
Outdated
Show resolved
Hide resolved
...main.Dialogporten.Infrastructure/Altinn/Authorization/LocalDevelopmentAltinnAuthorization.cs
Show resolved
Hide resolved
oskogstad
dismissed stale reviews from arealmaas and MagnusSandgren
via
26e7c36
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
docs/schema/V1/schema.verified.graphql (1)
Line range hint
81-90: Consider introducing an interface for shared fieldsThere's significant field duplication between
AuthorizedPartyandAuthorizedSubParty. Consider introducing an interface to share common fields:+interface AuthorizedPartyBase { + party: String! + name: String! + partyType: String! + isDeleted: Boolean! + hasKeyRole: Boolean! + isCurrentEndUser: Boolean! + isMainAdministrator: Boolean! + isAccessManager: Boolean! + hasOnlyAccessToSubParties: Boolean! +} + -type AuthorizedParty { +type AuthorizedParty implements AuthorizedPartyBase { subParties: [AuthorizedSubParty!]! party: String! name: String! partyType: String! isDeleted: Boolean! hasKeyRole: Boolean! isCurrentEndUser: Boolean! isMainAdministrator: Boolean! isAccessManager: Boolean! hasOnlyAccessToSubParties: Boolean! } -type AuthorizedSubParty { +type AuthorizedSubParty implements AuthorizedPartyBase { party: String! name: String! partyType: String! isDeleted: Boolean! hasKeyRole: Boolean! isCurrentEndUser: Boolean! isMainAdministrator: Boolean! isAccessManager: Boolean! hasOnlyAccessToSubParties: Boolean! }This would:
- Reduce code duplication
- Make the relationship between the types explicit
- Simplify future maintenance
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
docs/schema/V1/schema.verified.graphql(1 hunks)src/Digdir.Domain.Dialogporten.GraphQL/EndUser/Parties/ObjectTypes.cs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Digdir.Domain.Dialogporten.GraphQL/EndUser/Parties/ObjectTypes.cs
🔇 Additional comments (1)
docs/schema/V1/schema.verified.graphql (1)
Line range hint 69-90: Overall schema changes look good
The introduction of the AuthorizedSubParty type and the updates to AuthorizedParty are well-structured and maintain schema consistency. The changes are properly contained and don't introduce breaking changes to existing queries.
|
🤖 I have created a release *beep* *boop* --- ## [1.39.0](v1.38.0...v1.39.0) (2024-11-22) ### Features * **azure:** adjust SKU and storage for yt01 and prod ([b7e4909](b7e4909)) * **azure:** adjust SKU and storage for yt01 and prod ([#1508](#1508)) ([5478275](5478275)) * **graphql:** Create separate type for sub-parties ([#1510](#1510)) ([9c75f11](9c75f11)) ### Bug Fixes * **azure:** ensure correct properties are used when adjusting SKU and storage for postgres ([#1514](#1514)) ([c51d2f5](c51d2f5)) * Reenable party list cache, log party name look failure with negative cache TTL ([#1395](#1395)) ([d18bb76](d18bb76)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Description
Related Issue(s)
Verification
Documentation
docs-directory, Altinnpedia or a separate linked PR in altinn-studio-docs., if applicable)Summary by CodeRabbit
New Features
AuthorizedPartyandAuthorizedSubPartytypes to enhance user representation and relationships.AuthorizedSubPartyto support nested party structures.Bug Fixes
GetAuthorizedPartiesmethod to return a more complex structure, including nested sub-party information.Refactor
AuthorizedPartyandAuthorizedSubPartyto facilitate better organization and relationships.