GO-3870: HistoryGetVersions ignore limit param #1463
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: AnastasiaShemyakinskaya <shem98a@mail.ru>
WalkthroughThe recent changes improve the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant HistoryService
participant DataStore
Client->>HistoryService: Request versions
HistoryService->>DataStore: Fetch versions
alt Data is not empty
DataStore-->>HistoryService: Return versions
HistoryService->>HistoryService: Check limit
HistoryService->>Client: Return limited versions
else Data is empty
HistoryService-->>Client: Return empty response
end
Poem
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 Configuration File (
|
Coverage provided by /~https://github.com/seriousben/go-patch-cover-action |
![coderabbitai[bot]](/~https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- core/history/history.go (3 hunks)
- core/history/history_test.go (2 hunks)
Additional comments not posted (2)
core/history/history.go (1)
151-153: Improve handling of empty data slices.The added check for empty
dataslices prevents unnecessary operations. This is a good enhancement for robustness.core/history/history_test.go (1)
967-992: Test case aligns with PR objectives.The test case "no changes" correctly validates the behavior when there are no changes, aligning with the PR objectives.
Comment on lines
+180
to
+182
| if len(resp) > limit { | ||
| resp = resp[:limit] | ||
| } |
There was a problem hiding this comment.
Ensure limit parameter is correctly ignored.
The code truncates the resp slice if its length exceeds the specified limit. This contradicts the PR objective of ignoring the limit parameter. Consider removing this logic to align with the PR objectives.
- if len(resp) > limit {
- resp = resp[:limit]
- }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. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if len(resp) > limit { | |
| resp = resp[:limit] | |
| } |
Comment on lines
+861
to
+915
| t.Run("limit 10 - 10 changes", func(t *testing.T) { | ||
| // given | ||
| objectId := "objectId" | ||
| spaceID := "spaceID" | ||
| versionId := "versionId" | ||
|
|
||
| accountKeys, _ := accountdata.NewRandom() | ||
| account := accountKeys.SignKey.GetPublic() | ||
|
|
||
| blDataviewId := "blDataviewId" | ||
| relationKey := "key" | ||
|
|
||
| viewId := "viewId" | ||
| viewName := "view" | ||
|
|
||
| viewId1 := "viewId1" | ||
| view1Name := "view1" | ||
|
|
||
| viewId2 := "viewId2" | ||
| view2Name := "view2" | ||
|
|
||
| blDataview := provideDataviewBlock(viewId1, view1Name, relationKey, blDataviewId) | ||
|
|
||
| blSmartBlock := &model.Block{Id: objectId, Content: &model.BlockContentOfSmartblock{Smartblock: &model.BlockContentSmartblock{}}} | ||
|
|
||
| currChange := []*objecttree.Change{ | ||
| // create block changes | ||
| provideBlockEmptyChange(objectId, account), | ||
| provideBlockCreateChange(blSmartBlock, account), | ||
| provideBlockCreateChange(blDataview, account), | ||
|
|
||
| // dataview changes | ||
| provideBlockDataviewViewSetChange(blDataviewId, viewId1, view1Name, relationKey, account), | ||
| provideBlockDataviewViewSetChange(blDataviewId, viewId2, view2Name, relationKey, account), | ||
| provideBlockDataviewSourceSetChange(blDataviewId, account), | ||
| provideBlockDataviewRelationSetChange(blDataviewId, account), | ||
| provideBlockDataviewViewSetChange(blDataviewId, viewId, viewName, relationKey, account), | ||
| provideBlockDataviewViewOrderChange(blDataviewId, viewId, viewId1, account), | ||
| provideBlockDataviewViewDeleteChange(blDataviewId, viewId2, account), | ||
| provideBlockDataviewRelationDeleteChange(blDataviewId, relationKey, account), | ||
| provideBlockDataviewObjectOrderChange(blDataviewId, viewId, account), | ||
| provideBlockDataviewGroupOrderChange(blDataviewId, viewId, account), | ||
| provideBlockDataviewViewUpdateChange(blDataviewId, viewId, account), | ||
| provideBlockDataviewTargetObjectChange(blDataviewId, account), | ||
| } | ||
|
|
||
| history := newFixture(t, currChange, objectId, spaceID, versionId) | ||
|
|
||
| // when | ||
| resp, err := history.Versions(domain.FullID{ObjectID: objectId, SpaceID: spaceID}, versionId, 10) | ||
|
|
||
| // then | ||
| assert.Nil(t, err) | ||
| assert.Len(t, resp, 10) | ||
| }) |
There was a problem hiding this comment.
Test case contradicts PR objectives.
The test case "limit 10 - 10 changes" validates the behavior of the limit parameter, which contradicts the PR objective of ignoring it. Consider revising this test case to align with the PR objectives.
- t.Run("limit 10 - 10 changes", func(t *testing.T) {
+ t.Run("ignore limit parameter", func(t *testing.T) {Committable suggestion was skipped due to low confidence.
Comment on lines
+916
to
+966
| t.Run("number of changes equals limit", func(t *testing.T) { | ||
| // given | ||
| objectId := "objectId" | ||
| spaceID := "spaceID" | ||
| versionId := "versionId" | ||
|
|
||
| accountKeys, _ := accountdata.NewRandom() | ||
| account := accountKeys.SignKey.GetPublic() | ||
|
|
||
| blDataviewId := "blDataviewId" | ||
| relationKey := "key" | ||
|
|
||
| viewId := "viewId" | ||
| viewName := "view" | ||
|
|
||
| viewId1 := "viewId1" | ||
| view1Name := "view1" | ||
|
|
||
| viewId2 := "viewId2" | ||
| view2Name := "view2" | ||
|
|
||
| blDataview := provideDataviewBlock(viewId1, view1Name, relationKey, blDataviewId) | ||
|
|
||
| blSmartBlock := &model.Block{Id: objectId, Content: &model.BlockContentOfSmartblock{Smartblock: &model.BlockContentSmartblock{}}} | ||
|
|
||
| currChange := []*objecttree.Change{ | ||
| // create block changes | ||
| provideBlockEmptyChange(objectId, account), | ||
| provideBlockCreateChange(blSmartBlock, account), | ||
| provideBlockCreateChange(blDataview, account), | ||
|
|
||
| // dataview changes | ||
| provideBlockDataviewViewSetChange(blDataviewId, viewId1, view1Name, relationKey, account), | ||
| provideBlockDataviewViewSetChange(blDataviewId, viewId2, view2Name, relationKey, account), | ||
| provideBlockDataviewSourceSetChange(blDataviewId, account), | ||
| provideBlockDataviewRelationSetChange(blDataviewId, account), | ||
| provideBlockDataviewViewSetChange(blDataviewId, viewId, viewName, relationKey, account), | ||
| provideBlockDataviewViewOrderChange(blDataviewId, viewId, viewId1, account), | ||
| provideBlockDataviewViewDeleteChange(blDataviewId, viewId2, account), | ||
| provideBlockDataviewRelationDeleteChange(blDataviewId, relationKey, account), | ||
| } | ||
|
|
||
| history := newFixture(t, currChange, objectId, spaceID, versionId) | ||
|
|
||
| // when | ||
| resp, err := history.Versions(domain.FullID{ObjectID: objectId, SpaceID: spaceID}, versionId, 10) | ||
|
|
||
| // then | ||
| assert.Nil(t, err) | ||
| assert.Len(t, resp, 10) | ||
| }) |
There was a problem hiding this comment.
Test case contradicts PR objectives.
The test case "number of changes equals limit" validates the behavior of the limit parameter, which contradicts the PR objective of ignoring it. Consider revising this test case to align with the PR objectives.
- t.Run("number of changes equals limit", func(t *testing.T) {
+ t.Run("ignore limit parameter", func(t *testing.T) {Committable suggestion was skipped due to low confidence.
Comment on lines
+805
to
+860
| func TestHistory_Versions(t *testing.T) { | ||
| t.Run("limit 0 - 100 changes", func(t *testing.T) { | ||
| // given | ||
| objectId := "objectId" | ||
| spaceID := "spaceID" | ||
| versionId := "versionId" | ||
|
|
||
| accountKeys, _ := accountdata.NewRandom() | ||
| account := accountKeys.SignKey.GetPublic() | ||
|
|
||
| blDataviewId := "blDataviewId" | ||
| relationKey := "key" | ||
|
|
||
| viewId := "viewId" | ||
| viewName := "view" | ||
|
|
||
| viewId1 := "viewId1" | ||
| view1Name := "view1" | ||
|
|
||
| viewId2 := "viewId2" | ||
| view2Name := "view2" | ||
|
|
||
| blDataview := provideDataviewBlock(viewId1, view1Name, relationKey, blDataviewId) | ||
|
|
||
| blSmartBlock := &model.Block{Id: objectId, Content: &model.BlockContentOfSmartblock{Smartblock: &model.BlockContentSmartblock{}}} | ||
|
|
||
| currChange := []*objecttree.Change{ | ||
| // create block changes | ||
| provideBlockEmptyChange(objectId, account), | ||
| provideBlockCreateChange(blSmartBlock, account), | ||
| provideBlockCreateChange(blDataview, account), | ||
|
|
||
| // dataview changes | ||
| provideBlockDataviewViewSetChange(blDataviewId, viewId1, view1Name, relationKey, account), | ||
| provideBlockDataviewViewSetChange(blDataviewId, viewId2, view2Name, relationKey, account), | ||
| provideBlockDataviewSourceSetChange(blDataviewId, account), | ||
| provideBlockDataviewRelationSetChange(blDataviewId, account), | ||
| provideBlockDataviewViewSetChange(blDataviewId, viewId, viewName, relationKey, account), | ||
| provideBlockDataviewViewOrderChange(blDataviewId, viewId, viewId1, account), | ||
| provideBlockDataviewViewDeleteChange(blDataviewId, viewId2, account), | ||
| provideBlockDataviewRelationDeleteChange(blDataviewId, relationKey, account), | ||
| provideBlockDataviewObjectOrderChange(blDataviewId, viewId, account), | ||
| provideBlockDataviewGroupOrderChange(blDataviewId, viewId, account), | ||
| provideBlockDataviewViewUpdateChange(blDataviewId, viewId, account), | ||
| provideBlockDataviewTargetObjectChange(blDataviewId, account), | ||
| } | ||
|
|
||
| history := newFixture(t, currChange, objectId, spaceID, versionId) | ||
|
|
||
| // when | ||
| resp, err := history.Versions(domain.FullID{ObjectID: objectId, SpaceID: spaceID}, versionId, 0) | ||
|
|
||
| // then | ||
| assert.Nil(t, err) | ||
| assert.Len(t, resp, 14) | ||
| }) |
There was a problem hiding this comment.
Test case contradicts PR objectives.
The test case "limit 0 - 100 changes" validates the behavior of the limit parameter, which contradicts the PR objective of ignoring it. Consider revising this test case to align with the PR objectives.
- t.Run("limit 0 - 100 changes", func(t *testing.T) {
+ t.Run("ignore limit parameter", func(t *testing.T) {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. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func TestHistory_Versions(t *testing.T) { | |
| t.Run("limit 0 - 100 changes", func(t *testing.T) { | |
| // given | |
| objectId := "objectId" | |
| spaceID := "spaceID" | |
| versionId := "versionId" | |
| accountKeys, _ := accountdata.NewRandom() | |
| account := accountKeys.SignKey.GetPublic() | |
| blDataviewId := "blDataviewId" | |
| relationKey := "key" | |
| viewId := "viewId" | |
| viewName := "view" | |
| viewId1 := "viewId1" | |
| view1Name := "view1" | |
| viewId2 := "viewId2" | |
| view2Name := "view2" | |
| blDataview := provideDataviewBlock(viewId1, view1Name, relationKey, blDataviewId) | |
| blSmartBlock := &model.Block{Id: objectId, Content: &model.BlockContentOfSmartblock{Smartblock: &model.BlockContentSmartblock{}}} | |
| currChange := []*objecttree.Change{ | |
| // create block changes | |
| provideBlockEmptyChange(objectId, account), | |
| provideBlockCreateChange(blSmartBlock, account), | |
| provideBlockCreateChange(blDataview, account), | |
| // dataview changes | |
| provideBlockDataviewViewSetChange(blDataviewId, viewId1, view1Name, relationKey, account), | |
| provideBlockDataviewViewSetChange(blDataviewId, viewId2, view2Name, relationKey, account), | |
| provideBlockDataviewSourceSetChange(blDataviewId, account), | |
| provideBlockDataviewRelationSetChange(blDataviewId, account), | |
| provideBlockDataviewViewSetChange(blDataviewId, viewId, viewName, relationKey, account), | |
| provideBlockDataviewViewOrderChange(blDataviewId, viewId, viewId1, account), | |
| provideBlockDataviewViewDeleteChange(blDataviewId, viewId2, account), | |
| provideBlockDataviewRelationDeleteChange(blDataviewId, relationKey, account), | |
| provideBlockDataviewObjectOrderChange(blDataviewId, viewId, account), | |
| provideBlockDataviewGroupOrderChange(blDataviewId, viewId, account), | |
| provideBlockDataviewViewUpdateChange(blDataviewId, viewId, account), | |
| provideBlockDataviewTargetObjectChange(blDataviewId, account), | |
| } | |
| history := newFixture(t, currChange, objectId, spaceID, versionId) | |
| // when | |
| resp, err := history.Versions(domain.FullID{ObjectID: objectId, SpaceID: spaceID}, versionId, 0) | |
| // then | |
| assert.Nil(t, err) | |
| assert.Len(t, resp, 14) | |
| }) | |
| func TestHistory_Versions(t *testing.T) { | |
| t.Run("ignore limit parameter", func(t *testing.T) { | |
| // given | |
| objectId := "objectId" | |
| spaceID := "spaceID" | |
| versionId := "versionId" | |
| accountKeys, _ := accountdata.NewRandom() | |
| account := accountKeys.SignKey.GetPublic() | |
| blDataviewId := "blDataviewId" | |
| relationKey := "key" | |
| viewId := "viewId" | |
| viewName := "view" | |
| viewId1 := "viewId1" | |
| view1Name := "view1" | |
| viewId2 := "viewId2" | |
| view2Name := "view2" | |
| blDataview := provideDataviewBlock(viewId1, view1Name, relationKey, blDataviewId) | |
| blSmartBlock := &model.Block{Id: objectId, Content: &model.BlockContentOfSmartblock{Smartblock: &model.BlockContentSmartblock{}}} | |
| currChange := []*objecttree.Change{ | |
| // create block changes | |
| provideBlockEmptyChange(objectId, account), | |
| provideBlockCreateChange(blSmartBlock, account), | |
| provideBlockCreateChange(blDataview, account), | |
| // dataview changes | |
| provideBlockDataviewViewSetChange(blDataviewId, viewId1, view1Name, relationKey, account), | |
| provideBlockDataviewViewSetChange(blDataviewId, viewId2, view2Name, relationKey, account), | |
| provideBlockDataviewSourceSetChange(blDataviewId, account), | |
| provideBlockDataviewRelationSetChange(blDataviewId, account), | |
| provideBlockDataviewViewSetChange(blDataviewId, viewId, viewName, relationKey, account), | |
| provideBlockDataviewViewOrderChange(blDataviewId, viewId, viewId1, account), | |
| provideBlockDataviewViewDeleteChange(blDataviewId, viewId2, account), | |
| provideBlockDataviewRelationDeleteChange(blDataviewId, relationKey, account), | |
| provideBlockDataviewObjectOrderChange(blDataviewId, viewId, account), | |
| provideBlockDataviewGroupOrderChange(blDataviewId, viewId, account), | |
| provideBlockDataviewViewUpdateChange(blDataviewId, viewId, account), | |
| provideBlockDataviewTargetObjectChange(blDataviewId, account), | |
| } | |
| history := newFixture(t, currChange, objectId, spaceID, versionId) | |
| // when | |
| resp, err := history.Versions(domain.FullID{ObjectID: objectId, SpaceID: spaceID}, versionId, 0) | |
| // then | |
| assert.Nil(t, err) | |
| assert.Len(t, resp, 14) | |
| }) |
mcrakhman
reviewed
Aug 13, 2024
| @@ -148,6 +148,9 @@ func (h *history) Versions(id domain.FullID, lastVersionId string, limit int) (r | |||
| if e != nil { | |||
| return nil, e | |||
| } | |||
| if len(data) == 0 { | |||
There was a problem hiding this comment.
How is it possible that data == 0? Because I guess we will always call func on first element
mcrakhman
approved these changes
Aug 13, 2024
AnastasiaShemyakinskaya
deleted the
go-3870-historygetversions-ignore-limit-param
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
https://linear.app/anytype/issue/GO-3870/historygetversions-ignore-limit-param