fix: GetStreamsToSnapshot crashing for results with more than 512 items#55
fix: GetStreamsToSnapshot crashing for results with more than 512 items#55AGiorgetti merged 4 commits intoNEventStore:bugfix/54_GetStreamsToSnapshot_crashfrom brustolin:patch-1
Conversation
There was a problem hiding this comment.
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
@StreamIdparameter - Simplifies the paged query execution by using an empty callback since the
@Limitparameter is handled automatically
| 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.
[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 ne E880 ed for next page delegate in the Snapshot stream | |
| (q, s) => { }) // No next page delegate needed - @Limit parameter is handled automatically by PagedEnumerationCollection |
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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.
@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 <175728472+Copilot@users.noreply.github.com>
|
I've created a branch with tests that verify the issue, I'm merging the fix in that branch |
The previous code were trying to set the
@StreamIdvariable that is not available in the query.The
@Limit@Skipvariable is set inside thePagedEnumerationCollectioniterator, so there is no need for aNextPageDelegatecallback in here.Fixes #54