fix(graphql): Add missing search parameters for paging and sorting #1671
Conversation
|
Warning Rate limit exceeded@oskogstad has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 7 minutes and 12 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. 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis pull request introduces enhancements to the GraphQL schema and related components for search functionality in dialogs, focusing on improved error handling, sorting, and pagination capabilities. Key changes include the addition of new types for error handling and sorting, modifications to existing payload and input types, and updates to the query handling logic to incorporate validation and parsing for continuation tokens and order criteria. Changes
Possibly related PRs
Suggested reviewers
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
|
|
oskogstad
force-pushed
the
fix/gql-add-missing-search-parameters-for-pagination-orderby-limit
branch
from
019ca1f to
ff335b7
Compare
src/Digdir.Domain.Dialogporten.GraphQL/EndUser/SearchDialogs/ObjectTypes.cs
Outdated
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
src/Digdir.Domain.Dialogporten.GraphQL/EndUser/DialogQueries.cs (1)
43-52: Consider localizing the error message for OrderBy parsing.The error handling for OrderBy parsing is robust, but the error message is hardcoded in
SearchDialogOrderByParsingError. Consider making error messages localizable for better internationalization support.src/Digdir.Domain.Dialogporten.GraphQL/EndUser/SearchDialogs/ObjectTypes.cs (3)
159-165: Consider throwing an exception for invalid OrderBy format.The current implementation silently ignores invalid parts in the OrderBy string. This could mask configuration errors and make debugging more difficult.
foreach (var orderByPart in orderBy.Split(',')) { var parts = orderByPart.Split('_'); if (parts.Length != 2) { - continue; + throw new FormatException($"Invalid OrderBy format: {orderByPart}. Expected format: field_direction"); }
154-157: Optimize string replacements using StringComparison.OrdinalIgnoreCase.The string replacements for 'id_desc' and 'id_asc' are using StringComparison.OrdinalIgnoreCase inconsistently. Consider using it for ToLower as well:
-orderBy = orderBy - .ToLower(CultureInfo.InvariantCulture) +orderBy = orderBy.ToLowerInvariant() .Replace("id_desc", "", StringComparison.OrdinalIgnoreCase) .Replace("id_asc", "", StringComparison.OrdinalIgnoreCase);
189-206: Optimize StringBuilder usage in TryToOrderSet.The current implementation appends to StringBuilder in a conditional manner, which could be simplified. Also, consider pre-allocating the StringBuilder capacity based on the expected size.
-var stringBuilder = new StringBuilder(); +var stringBuilder = new StringBuilder(searchDialogSortTypes.Count * 20); // Estimate 20 chars per sort type foreach (var orderBy in searchDialogSortTypes) { - if (orderBy.CreatedAt != null) - { - stringBuilder.Append(CultureInfo.InvariantCulture, $"createdAt_{orderBy.CreatedAt},"); - continue; - } - if (orderBy.UpdatedAt != null) - { - stringBuilder.Append(CultureInfo.InvariantCulture, $"updatedAt_{orderBy.UpdatedAt},"); - continue; - } - if (orderBy.DueAt != null) - { - stringBuilder.Append(CultureInfo.InvariantCulture, $"dueAt_{orderBy.DueAt},"); - } + var (field, direction) = orderBy switch + { + { CreatedAt: not null } => ("createdAt", orderBy.CreatedAt), + { UpdatedAt: not null } => ("updatedAt", orderBy.UpdatedAt), + { DueAt: not null } => ("dueAt", orderBy.DueAt), + _ => continue + }; + stringBuilder.Append(CultureInfo.InvariantCulture, $"{field}_{direction},"); }docs/schema/V1/schema.verified.graphql (4)
218-223: Enhance the description for better clarity.The description should be more explicit about the behavior when multiple sort fields are set. Consider clarifying whether fields are applied in order of appearance and what the default sort direction is when none is specified.
-"Set only one property per object. If more than one property is set, there is no guarantee which one will be used." +"Set only one property per object. Multiple properties will result in undefined sorting behavior. When specified, properties should use ASC or DESC direction, defaulting to ASC if not specified."
232-235: LGTM! Consider adding pagination examples to documentation.The implementation follows the cursor-based pagination pattern correctly. The non-nullable orderBy field ensures consistent pagination results.
Consider adding examples in the documentation showing how to use
continuationTokenwithorderByfor pagination, as this pattern might be new to some API consumers.
308-309: Add schema-level constraints for the limit field.While the description specifies the default and maximum values for
limit, consider adding schema-level constraints using@constraintdirective to enforce these limits."Limit the number of results returned" limit: Int @constraint(min: 1, max: 1000)
411-414: Add descriptions for the SortDirection enum.Consider adding descriptions to improve schema documentation:
+""" +Specifies the sort order for query results +""" enum SortDirection { + "Sort in ascending order" ASC + "Sort in descending order" DESC }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
docs/schema/V1/schema.verified.graphql(3 hunks)src/Digdir.Domain.Dialogporten.Application/Common/Pagination/IPaginationParameter.cs(2 hunks)src/Digdir.Domain.Dialogporten.GraphQL/EndUser/DialogQueries.cs(2 hunks)src/Digdir.Domain.Dialogporten.GraphQL/EndUser/SearchDialogs/MappingProfile.cs(1 hunks)src/Digdir.Domain.Dialogporten.GraphQL/EndUser/SearchDialogs/ObjectTypes.cs(4 hunks)
🔇 Additional comments (7)
src/Digdir.Domain.Dialogporten.GraphQL/EndUser/SearchDialogs/MappingProfile.cs (1)
12-13: LGTM! Appropriate mapping configuration for pagination and sorting.The explicit ignore configurations for
OrderByandContinuationTokenproperties are correct, as these properties require special handling in the query execution phase.Also applies to: 16-17
src/Digdir.Domain.Dialogporten.Application/Common/Pagination/IPaginationParameter.cs (1)
13-13: Consider the implications of mutable pagination properties.Changing
ContinuationTokenandOrderByfrominittosetmakes these properties mutable after object construction. While this enables the new pagination and sorting functionality, it could lead to unexpected behavior if these properties are modified during the object's lifecycle.Consider:
- Documenting the intended usage pattern
- Adding validation when these properties are modified
- Implementing immutable alternatives if possible
Also applies to: 25-25
✅ Verification successful
Property mutability is justified and well-controlled
The mutable properties are used appropriately within the pagination workflow, with modifications occurring only during query execution, token management, and result mapping. The implementation follows standard patterns for GraphQL pagination.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for any direct modifications to these properties outside of the initial setup rg -A 3 "ContinuationToken\s*=\s*" --type cs rg -A 3 "OrderBy\s*=\s*" --type csLength of output: 2778
src/Digdir.Domain.Dialogporten.GraphQL/EndUser/DialogQueries.cs (2)
37-41: LGTM! Robust continuation token handling.The implementation correctly parses and validates the continuation token before applying it to the query.
57-62: LGTM! Clean mapping with order preservation.The implementation correctly preserves the ordering information in the mapped result.
src/Digdir.Domain.Dialogporten.GraphQL/EndUser/SearchDialogs/ObjectTypes.cs (2)
45-51: LGTM! Well-structured sort type definition.The
SearchDialogSortTypeclass with its GraphQL description and nullable direction properties provides a clean and self-documenting API for sort operations.
137-144: LGTM! Clear pagination and sorting parameters.The input parameters for limit, continuation token, and ordering are well-documented with GraphQL descriptions and have appropriate nullable types.
docs/schema/V1/schema.verified.graphql (1)
316-320: LGTM! Input type correctly mirrors the sort type.The implementation follows GraphQL best practices by using a separate input type for mutations/queries.
…gination-orderby-limit
…gination-orderby-limit
Co-authored-by: Ole Jørgen Skogstad <skogstad@softis.net>
…gination-orderby-limit
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/Digdir.Domain.Dialogporten.GraphQL/EndUser/SearchDialogs/SearchDialogSortTypeExtensions.cs (3)
11-45: Consider using constants for field names.While the implementation is efficient using span operations, consider extracting the field names into constants to improve maintainability and reduce the risk of typos.
internal static class SearchDialogSortTypeExtensions { + private const string CreatedAtField = "createdat"; + private const string UpdatedAtField = "updatedat"; + private const string DueAtField = "dueat"; + public static List<SearchDialogSortType> ToSearchDialogSortTypeList(this ReadOnlySpan<char> orderSet)
47-80: Consider extracting a helper method to reduce duplication.The string building logic for each field follows the same pattern. Consider extracting it to a helper method to reduce duplication.
+ private static void AppendOrderBy(StringBuilder sb, string field, OrderDirection? direction) + { + if (direction.HasValue) + { + sb.Append(CultureInfo.InvariantCulture, $"{field}_{direction},"); + } + } + public static bool TryToOrderSet(this List<SearchDialogSortType> searchDialogSortTypes, out OrderSet<SearchDialogQueryOrderDefinition, IntermediateDialogDto>? orderSet) { var stringBuilder = new StringBuilder(); foreach (var orderBy in searchDialogSortTypes) { - if (orderBy.CreatedAt.HasValue) - { - stringBuilder.Append(CultureInfo.InvariantCulture, $"createdAt_{orderBy.CreatedAt},"); - continue; - } - - if (orderBy.UpdatedAt.HasValue) - { - stringBuilder.Append(CultureInfo.InvariantCulture, $"updatedAt_{orderBy.UpdatedAt},"); - continue; - } - - if (orderBy.DueAt.HasValue) - { - stringBuilder.Append(CultureInfo.InvariantCulture, $"dueAt_{orderBy.DueAt},"); - } + AppendOrderBy(stringBuilder, "createdAt", orderBy.CreatedAt); + AppendOrderBy(stringBuilder, "updatedAt", orderBy.UpdatedAt); + AppendOrderBy(stringBuilder, "dueAt", orderBy.DueAt); }
82-100: Consider using a switch expression for better readability.The if-else chain could be replaced with a more concise switch expression.
private static Func<OrderDirection, SearchDialogSortType> GetSearchDialogSortTypeFactory(ReadOnlySpan<char> field) - { - if (field.Equals("createdat", StringComparison.OrdinalIgnoreCase)) - { - return CreatedAtFactory; - } - - if (field.Equals("updatedat", StringComparison.OrdinalIgnoreCase)) - { - return UpdatedAtFactory; - } - - if (field.Equals("dueat", StringComparison.OrdinalIgnoreCase)) - { - return DuAtFactory; - } - - throw new InvalidOperationException("Invalid sort field"); - } + => field.ToString().ToLowerInvariant() switch + { + "createdat" => CreatedAtFactory, + "updatedat" => UpdatedAtFactory, + "dueat" => DuAtFactory, + _ => throw new InvalidOperationException("Invalid sort field") + };
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
docs/schema/V1/schema.verified.graphql(3 hunks)src/Digdir.Domain.Dialogporten.GraphQL/EndUser/DialogQueries.cs(2 hunks)src/Digdir.Domain.Dialogporten.GraphQL/EndUser/SearchDialogs/ObjectTypes.cs(4 hunks)src/Digdir.Domain.Dialogporten.GraphQL/EndUser/SearchDialogs/SearchDialogSortTypeExtensions.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/schema/V1/schema.verified.graphql
🔇 Additional comments (11)
src/Digdir.Domain.Dialogporten.GraphQL/EndUser/DialogQueries.cs (4)
33-33: LGTM! Good addition of input validation.Adding FluentValidation ensures that the input is validated before processing, which is a good practice for maintaining data integrity.
39-49: LGTM! Good error handling for continuation token.The code properly handles the case where continuation token parsing fails, returning a clear error message. The early return pattern makes the code flow easy to follow.
51-60: LGTM! Good error handling for order set.The code properly handles the case where order set parsing fails, returning a clear error message. The early return pattern makes the code flow easy to follow.
65-70: LGTM! Good use of Span for performance.The code efficiently maps the paginated list to the payload, and uses Span operations for better performance when mapping the order by field.
src/Digdir.Domain.Dialogporten.GraphQL/EndUser/SearchDialogs/SearchDialogSortTypeExtensions.cs (1)
102-104: LGTM! Clear and concise factory methods.The factory methods are well-named and follow a consistent pattern.
src/Digdir.Domain.Dialogporten.GraphQL/EndUser/SearchDialogs/ObjectTypes.cs (6)
13-21: LGTM! Clear error messages and consistent implementation.The new error classes follow the established pattern and provide clear error messages.
37-41: LGTM! Good documentation and null safety.The GraphQL description for ContinuationToken is clear and helpful. The OrderBy property is properly initialized to prevent null reference exceptions.
46-52: Add validation for sort type properties.While the GraphQL description indicates that only one property should be set per object, this isn't enforced by code.
129-129: LGTM! Accurate language code example.The description now uses the correct ISO 639 language code example ('nb' instead of 'no').
132-133: Add validation for the Limit property.While the GraphQL description specifies defaults and constraints for the Limit property, these aren't enforced by code.
135-139: LGTM! Clear documentation for pagination and sorting.The GraphQL descriptions for ContinuationToken and OrderBy are clear and helpful, properly documenting the pagination and sorting capabilities.
oskogstad
force-pushed
the
fix/gql-add-missing-search-parameters-for-pagination-orderby-limit
branch
from
1f79e87 to
8cb5a02
Compare
oskogstad
force-pushed
the
fix/gql-add-missing-search-parameters-for-pagination-orderby-limit
branch
from
8cb5a02 to
429d059
Compare
…gination-orderby-limit
oskogstad
deleted the
fix/gql-add-missing-search-parameters-for-pagination-orderby-limit
branch
🤖 I have created a release *beep* *boop* --- ## [1.45.1](v1.45.0...v1.45.1) (2025-01-18) ### Bug Fixes * **graphql:** Add missing search parameters for paging and sorting ([#1671](#1671)) ([02f2335](02f2335)) * Removed .AsSingleQuery from EndUser Search query ([#1707](#1707)) ([2a3153b](2a3153b)) * **webapi:** Use correct language code for norwegian in OpenApi description ([#1705](#1705)) ([ce0a07d](ce0a07d)) --- 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)