Feat/add query limit #597
Feat/add query limit #597
Conversation
Field Answer.Variables is a repeated non-nullable native type, so nullable=false has no effect.
WalkthroughThese updates introduce a new Changes
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to path filters (2)
x/logic/types/query.pb.gois excluded by:!**/*.pb.gox/logic/types/types.pb.gois excluded by:!**/*.pb.go
Files selected for processing (5)
- proto/logic/v1beta2/query.proto (1 hunks)
- proto/logic/v1beta2/types.proto (1 hunks)
- x/logic/keeper/grpc_query_ask.go (2 hunks)
- x/logic/keeper/grpc_query_ask_test.go (10 hunks)
- x/logic/keeper/interpreter.go (2 hunks)
Additional comments: 6
x/logic/keeper/grpc_query_ask.go (3)
- 7-7: The addition of
sdkmathandutilimports aligns with the usage of these packages in the modifiedAskfunction. Ensure that these packages are used effectively and that there are no unused imports.Also applies to: 14-14
- 17-17: The
defaultLimitsvariable is initialized withsdkmath.OneUint(), which sets a default limit of 1. This default value is consistent with the PR's objective to allow specifying a maximum number of responses. However, consider documenting the rationale behind choosing 1 as the default limit for better maintainability.- 50-51: The use of
util.DerefOrDefault(req.Limit, defaultLimits)in theAskfunction effectively applies the user-specified limit or the default limit when processing queries. This approach enhances flexibility and control over query results. Ensure that theDerefOrDefaultmethod is thoroughly tested, especially for edge cases such as null or extremely large values.proto/logic/v1beta2/query.proto (1)
- 47-54: The addition of the
limitfield to theQueryServiceAskRequestmessage, with a default value handling mechanism, is a crucial part of implementing the new functionality. It's important to ensure that thecosmossdk.io/math.Uintcustom type is correctly handled in all parts of the system that process this request. Additionally, consider adding validation to ensure that thelimitvalue provided by users is within acceptable bounds to prevent potential abuse or system overload.x/logic/keeper/interpreter.go (1)
- 43-43: The modification to the
executemethod to accept an additionallimitparameter and the use ofsdkmath.MinUint(limit, *limits.MaxResultCount)to determine the minimum value between the user-specified limit and the module's maximum result count limit are well-implemented. This ensures that the number of responses adheres to specified constraints. It's important to ensure that this logic is covered by unit tests to validate its correctness under various scenarios.Also applies to: 56-56
x/logic/keeper/grpc_query_ask_test.go (1)
- 37-37: The introduction of the
maxResultCountfield and its usage in test cases to control the maximum number of results returned in queries is a necessary adjustment to test the new limit functionality effectively. It's crucial to ensure that these test cases cover a wide range of scenarios, including edge cases such as limits higher than the number of available results and limits lower than the module's configured maximum result count. Additionally, consider adding tests to verify the behavior when invalid limit values are provided.Also applies to: 88-124, 235-292, 329-364
ccamel
force-pushed
the
feat/add-query-limit
branch
from
d8881f4
to
90c594e
Compare
ccamel
force-pushed
the
feat/add-query-limit
branch
from
90c594e
to
251068f
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to path filters (2)
x/logic/types/query.pb.gois excluded by:!**/*.pb.gox/logic/types/types.pb.gois excluded by:!**/*.pb.go
Files selected for processing (8)
- docs/proto/logic.md (1 hunks)
- proto/logic/v1beta2/query.proto (1 hunks)
- proto/logic/v1beta2/types.proto (1 hunks)
- x/logic/keeper/grpc_query_ask.go (2 hunks)
- x/logic/keeper/grpc_query_ask_test.go (10 hunks)
- x/logic/keeper/interpreter.go (2 hunks)
- x/logic/wasm/query.go (1 hunks)
- x/logic/wasm/types.go (1 hunks)
Files skipped from review as they are similar to previous changes (5)
- proto/logic/v1beta2/query.proto
- proto/logic/v1beta2/types.proto
- x/logic/keeper/grpc_query_ask.go
- x/logic/keeper/grpc_query_ask_test.go
- x/logic/keeper/interpreter.go
Additional comments: 4
x/logic/wasm/query.go (1)
- 30-30: The addition of the
Limitfield to theQueryServiceAskRequeststruct within theAskmethod is a direct implementation of the PR's objective to enhance theAskquery functionality by introducing an optionallimitparameter. This change allows users to specify the maximum number of responses they want, which is a valuable feature for controlling the size of query results. The implementation appears correct and aligns with the described functionality. However, it's important to ensure that theLimitfield is properly validated elsewhere in the code to handle cases where it might be set to an invalid value (e.g., negative numbers).Ensure that there is validation for the
Limitfield to handle invalid values gracefully. This might involve checking if theLimitis within an acceptable range and providing a default value if it's not specified.x/logic/wasm/types.go (2)
- 3-7: The import of the
cosmossdk.io/mathpackage is necessary for using thesdkmath.Uinttype for the newLimitfield in theAskQuerystruct. This change is appropriate and aligns with the objective of introducing a limit parameter for query functionality. It's important to ensure that this new dependency is properly managed and that the version of thecosmossdk.io/mathpackage is compatible with other dependencies in the project.Check the project's dependency management file (e.g.,
go.mod) to ensure that thecosmossdk.io/mathpackage is included with a compatible version.
- 15-15: Adding the
Limitfield of type*sdkmath.Uintto theAskQuerystruct is a crucial part of implementing the new limit functionality for queries. Using a pointer tosdkmath.Uintallows for distinguishing between a limit that is explicitly set to zero and an unset limit, which is a good practice for optional parameters. However, it's important to ensure that the handling of this field accounts for nil pointers to avoid potential runtime panics.Ensure that there is proper handling of the
Limitfield when it is nil, especially in the logic that processesAskQueryinstances. This might involve setting a default limit or using the module'smax_result_countconstraint when theLimitis nil.docs/proto/logic.md (1)
- 398-398: The addition of the
limitfield toQueryServiceAskRequestis well-documented, providing clear guidance on its behavior and default value. It's important to ensure that the documentation also includes examples or more detailed explanations on how thelimitinteracts withmax_result_countfor comprehensive understanding.Consider enhancing the documentation with examples or scenarios illustrating the use of the
limitfield, especially in cases wheremax_result_countmight also play a role.
|
🎉 This PR is included in version 7.1.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
This PR introduces an additional (optional) parameter
limitto set the desired limit on the number of responses for aAskquery. This parameter is quite common in data query APIs that can return a set of responses. Naturally, this limit is constrained by the value of themax_result_countparameter of the module.Summary by CodeRabbit
New Features
Bug Fixes
variablesfield in answers was incorrectly handled as non-nullable.Tests