Allow fetching the count from GetPage #9
Conversation
This change adds an optional count argument to GetPage. To make it optional it's implemented as a variadic function but only one count pointer is accepted if set.
WalkthroughThe recent update enhances the 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: 1
Configuration used: CodeRabbit UI
Files ignored due to path filters (1)
testproto/test.pb.gois excluded by:!**/*.pb.go
Files selected for processing (5)
- pika.go (1 hunks)
- pika_page_token.go (1 hunks)
- pika_psql.go (2 hunks)
- pika_psql_test.go (2 hunks)
- testproto/test.proto (1 hunks)
Additional comments: 5
testproto/test.proto (2)
- 36-40: The
TestRequest1message is well-structured and follows protobuf best practices. It appropriately supports pagination and filtering functionalities.- 43-46: The
SimpleModel1message is appropriately designed to represent simple model data with fields forid,title, anddescription.pika_page_token.go (1)
- 60-60: While the
Decodemethod is correctly implemented, it's important to ensure that user-provided data is validated before deserialization to prevent potential security vulnerabilities.pika.go (1)
- 158-160: The addition of the variadic
countparameter to theGetPagemethod is a thoughtful way to enhance functionality while maintaining backward compatibility. It would be beneficial to clarify in the documentation the expected behavior when multiplecountarguments are passed, as the current design strictly allows only one count pointer to be set.pika_psql.go (1)
- 542-552: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [545-589]
The implementation of the
GetPagemethod with the optionalcountPointerparameter is well thought out, ensuring backward compatibility and preventing misuse by limiting the count pointer to at most one. However, there are a few areas for improvement and clarification:
Error Handling and Messages: The error message "too many arguments (count should be one pointer or none)" is clear, but consider including the method name in the error message for easier debugging, e.g., "GetPage: too many arguments (count should be one pointer or none)".
Performance Consideration: The method performs a count operation regardless of whether a count pointer is provided. This could lead to unnecessary performance overhead in cases where the count is not needed. Consider optimizing this by only performing the count operation if a count pointer is provided.
Documentation: While the code changes are straightforward, adding documentation for the
GetPagemethod, especially regarding the newcountPointerparameter, would be beneficial for future maintainability and clarity for other developers.func (b *basePsql[T]) GetPage(paginatable Paginatable, options AIPFilterOptions, countPointer ...*int) ([]*T, string, error) { if len(countPointer) > 1 { - return nil, "", fmt.Errorf("too many arguments (count should be one pointer or none)") + return nil, "", fmt.Errorf("GetPage: too many arguments (count should be one pointer or none)") } + // Perform count operation only if countPointer is provided to optimize performance + var count int + var err error + if len(countPointer) > 0 { + count, err = b.Count() + if err != nil { + return nil, "", fmt.Errorf("GetPage: getting count: %w", err) + } + } + if b.err != nil { return nil, "", b.err } // Only decode if token is not empty if paginatable.GetPageToken() != "" { err := b.PageToken.Decode(paginatable.GetPageToken()) if err != nil { return nil, "", err } } else { // Otherwise use initial filter b.PageToken.Offset = 0 b.PageToken.Filter = paginatable.GetFilter() b.PageToken.OrderBy = paginatable.GetOrderBy() b.PageToken.PageSize = uint(paginatable.GetPageSize()) } qs, err := b.pageToken(b, options) if err != nil { return nil, "", err } result, err := qs.All() if err != nil { return nil, "", err } b.PageToken.Offset += uint(len(result)) - // Get count and check if there are more results - count, err := b.Count() - if err != nil { - return nil, "", fmt.Errorf("getting count: %w", err) - } - if len(countPointer) > 0 { *countPointer[0] = count } // If no more results after this page, return empty page token if b.PageToken.Offset >= uint(count) { return result, "", nil } tk, err := b.PageToken.Encode() if err != nil { return nil, "", err } return result, tk, nil }Consider optimizing the performance by only performing the count operation if a count pointer is provided. Additionally, enhance the error message for clarity and add documentation for the
GetPagemethod to explain the new functionality.
| func TestGetPageCount(t *testing.T) { | ||
| psql := newPsql(t) | ||
| createTestEntries(t, psql) | ||
| qs := Q[simpleModel1](psql) | ||
|
|
||
| aipOptions := ProtoReflect(&pikatestpb.SimpleModel1{}) | ||
| req := &pikatestpb.TestRequest1{ | ||
| PageSize: int32(1), | ||
| } | ||
| var count int | ||
| page, nt, err := qs.GetPage(req, aipOptions, &count) | ||
| require.Nil(t, err) | ||
|
|
||
| require.Equal(t, 1, len(page)) | ||
| require.NotEmpty(t, nt) | ||
| require.Equal(t, 3, count) | ||
| } |
There was a problem hiding this comment.
The test function TestGetPageCount correctly sets up a test environment, creates test entries, and calls the GetPage method with a count pointer to verify the functionality of retrieving a page count along with the page data. The assertions check for the expected length of the returned page, the presence of a next token, and the correct count of entries.
However, there are a few improvements and considerations:
- Error Handling: It's good practice to include error handling for the setup functions like
createTestEntries. While the current test assumes these functions always succeed, explicitly checking for errors can prevent false negatives in test results if setup fails. - Test Isolation: Consider using table-driven tests or subtests (
t.Run()) for different scenarios, such as varying page sizes or testing boundary conditions. This enhances test readability and maintainability. - Magic Numbers: The test uses magic numbers, such as the expected count of
3. It would be more maintainable to derive these values programmatically or define them as constants, especially if the setup data changes in the future.
func TestGetPageCount(t *testing.T) {
psql := newPsql(t)
- createTestEntries(t, psql)
+ err := createTestEntries(t, psql)
+ require.Nil(t, err, "setup: creating test entries failed")
qs := Q[simpleModel1](psql)
aipOptions := ProtoReflect(&pikatestpb.SimpleModel1{})
req := &pikatestpb.TestRequest1{
PageSize: int32(1),
}
var count int
page, nt, err := qs.GetPage(req, aipOptions, &count)
require.Nil(t, err)
require.Equal(t, 1, len(page))
require.NotEmpty(t, nt)
- require.Equal(t, 3, count)
+ const expectedCount = 3 // Consider deriving this value programmatically if possible
+ require.Equal(t, expectedCount, count)
}Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| func TestGetPageCount(t *testing.T) { | |
| psql := newPsql(t) | |
| createTestEntries(t, psql) | |
| qs := Q[simpleModel1](psql) | |
| aipOptions := ProtoReflect(&pikatestpb.SimpleModel1{}) | |
| req := &pikatestpb.TestRequest1{ | |
| PageSize: int32(1), | |
| } | |
| var count int | |
| page, nt, err := qs.GetPage(req, aipOptions, &count) | |
| require.Nil(t, err) | |
| require.Equal(t, 1, len(page)) | |
| require.NotEmpty(t, nt) | |
| require.Equal(t, 3, count) | |
| } | |
| func TestGetPageCount(t *testing.T) { | |
| psql := newPsql(t) | |
| err := createTestEntries(t, psql) | |
| require.Nil(t, err, "setup: creating test entries failed") | |
| qs := Q[simpleModel1](psql) | |
| aipOptions := ProtoReflect(&pikatestpb.SimpleModel1{}) | |
| req := &pikatestpb.TestRequest1{ | |
| PageSize: int32(1), | |
| } | |
| var count int | |
| page, nt, err := qs.GetPage(req, aipOptions, &count) | |
| require.Nil(t, err) | |
| require.Equal(t, 1, len(page)) | |
| require.NotEmpty(t, nt) | |
| const expectedCount = 3 // Consider deriving this value programmatically if possible | |
| require.Equal(t, expectedCount, count) | |
| } |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (12)
- parser/gen.go (1 hunks)
- pika.go (2 hunks)
- pika_aip_filter.go (1 hunks)
- pika_aip_filter_proto.go (1 hunks)
- pika_aip_filter_proto_test.go (1 hunks)
- pika_aip_filter_psql_test.go (1 hunks)
- pika_page_token.go (2 hunks)
- pika_psql.go (3 hunks)
- pika_psql_experimental.go (1 hunks)
- pika_psql_test.go (3 hunks)
- testproto/gen.go (1 hunks)
- utils.go (1 hunks)
Files skipped from review due to trivial changes (8)
- parser/gen.go
- pika_aip_filter.go
- pika_aip_filter_proto.go
- pika_aip_filter_proto_test.go
- pika_aip_filter_psql_test.go
- pika_psql_experimental.go
- testproto/gen.go
- utils.go
Files skipped from review as they are similar to previous changes (4)
- pika.go
- pika_page_token.go
- pika_psql.go
- pika_psql_test.go
This change adds an optional count argument to GetPage. To make it optional it's implemented as a variadic function but only one count pointer is accepted if set.
Summary by CodeRabbit
countparameter in theGetPagemethod for fetching page counts alongside query results.TestRequest1andSimpleModel1to enhance request parameter and model data handling.ParsePageTokenmethod toDecodefor more accurate base64 string handling.TestGetPageCountto verify correct page count retrieval functionality.