-
Notifications
You must be signed in to change notification settings - Fork 36
fix: GetStreamsToSnapshot crashing for results with more than 512 items #55
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes a crash in GetStreamsToSnapshot
when handling result sets with more than 512 items by removing an invalid parameter assignment that referenced a non-existent @StreamId
variable in the SQL query.
- Removes the callback that was attempting to set an invalid
@StreamId
parameter - Simplifies the paged query execution by using an empty callback since the
@Limit
parameter is handled automatically
@@ -212,7 +212,7 @@ public virtual IEnumerable<IStreamHead> GetStreamsToSnapshot(string bucketId, in | |||
query.AddParameter(_dialect.Threshold, maxThreshold); | |||
return | |||
query.ExecutePagedQuery(statement, | |||
(q, s) => q.SetParameter(_dialect.StreamId, _dialect.CoalesceParameterValue(s.StreamId()), DbType.AnsiString)) // todo: I'm not sure this is used, the query does not have a "StreamId" parameter | |||
(q, s) => { }) // There is no need for next page delegate in the Snapshot stream |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The comment should be more descriptive about why no next page delegate is needed. Consider: '// No next page delegate needed - @limit parameter is handled automatically by PagedEnumerationCollection'
(q, s) => { }) // There is no need for next page delegate in the Snapshot stream | |
(q, s) => { }) // No next page delegate needed - @Limit parameter is handled automatically by PagedEnumerationCollection |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AGiorgetti, sorry, my PR descriptions was wrong, its the @Skip
variable that is handled by the PagedEnumerationCollection
, but the behavior stills the same.
As you can see in the following code, the limit variable is set than _nextPage
(the delegate) is called.
Lines 148 to 158 in 437636b
private bool MoveToNextRecord() | |
{ | |
if (_pageSize > 0 && _position >= _pageSize) | |
{ | |
_command.SetParameter(_dialect.Skip, _position); | |
if (_current is null) | |
{ | |
throw new InvalidOperationException("Enumeration not started."); | |
} | |
_nextPage(_command, _current); | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hi @brustolin , I was testing copilot review ability :) Is it possible to also add a test case in order to avoid future regression? Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AGiorgetti I can try to add the tests, but I'm quite busy for a while, so it may take some time.
Co-authored-by: Copilot <[email protected]>
The previous code were trying to set the
@StreamId
variable that is not available in the query.The
@Limit
@Skip
variable is set inside thePagedEnumerationCollection
iterator, so there is no need for aNextPageDelegate
callback in here.Fixes #54