Skip to content
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

Add direction to ListDocuments API #312

Merged
merged 2 commits into from
Apr 27, 2022

Conversation

chacha912
Copy link
Contributor

@chacha912 chacha912 commented Apr 26, 2022

What this PR does / why we need it:

Add direction to ListDocuments API to navigate to the next and previous pages.

Which issue(s) this PR fixes:

Address yorkie-team/dashboard#4

Special notes for your reviewer:

Does this PR introduce a user-facing change?:


Additional documentation:

// If isForward is true, this request returns document lists with an id greater than previousID in ascending order.
// Otherwise, this request returns document lists with an id less than previousID in descending order.

listDocuments(previousID: string, pageSize: number, isForward: boolean)

Checklist:

  • Added relevant tests or not required
  • Didn't break anything

- To navigate to next and previous pages
@CLAassistant
Copy link

CLAassistant commented Apr 26, 2022

CLA assistant check
All committers have signed the CLA.

@chacha912 chacha912 marked this pull request as draft April 26, 2022 09:08
@hackerwins hackerwins marked this pull request as ready for review April 26, 2022 10:43
@hackerwins hackerwins requested a review from a team April 26, 2022 10:43
@codecov
Copy link

codecov bot commented Apr 26, 2022

Codecov Report

Merging #312 (430447b) into main (acdefd4) will increase coverage by 0.15%.
The diff coverage is 71.42%.

@@            Coverage Diff             @@
##             main     #312      +/-   ##
==========================================
+ Coverage   48.07%   48.23%   +0.15%     
==========================================
  Files          59       59              
  Lines        4903     4918      +15     
==========================================
+ Hits         2357     2372      +15     
  Misses       2285     2285              
  Partials      261      261              
Impacted Files Coverage Δ
yorkie/backend/db/mongo/client.go 0.00% <0.00%> (ø)
yorkie/backend/db/memory/db.go 52.10% <100.00%> (+2.10%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update acdefd4...430447b. Read the comment docs.

Copy link
Member

@hackerwins hackerwins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution. Overall it looks good.

I left some comments.

  1. To improve the readability of the test code, I clean up as follows:
	t.Run("docInfo pagination test", func(t *testing.T) {
		localDB, err := memory.New()
		assert.NoError(t, err)

		assertKeys := func (expectedKeys []string, infos []*db.DocInfo) {
			var keys []string
			for _, info := range infos {
				keys = append(keys, info.CombinedKey)
			}
			assert.EqualValues(t, expectedKeys, keys)
		}

		pageSize := 5
		totalSize := 9
		clientInfo, _ := localDB.ActivateClient(ctx, t.Name())
		for i := 0; i < totalSize; i++ {
			_, err := localDB.FindDocInfoByKey(ctx, clientInfo, fmt.Sprintf("%d", i), true)
			assert.NoError(t, err)
		}

		// initial page, previousID is empty
		infos, err := localDB.FindDocInfosByPreviousIDAndPageSize(ctx, "", pageSize, false)
		assert.NoError(t, err)
		assertKeys([]string{"8","7","6","5","4"}, infos)

		// backward
		infos, err = localDB.FindDocInfosByPreviousIDAndPageSize(ctx, infos[len(infos)-1].ID, pageSize, false)
		assert.NoError(t, err)
		assertKeys([]string{"3","2","1","0"}, infos)

		// backward again
		emptyInfos, err := localDB.FindDocInfosByPreviousIDAndPageSize(ctx, infos[len(infos)-1].ID, pageSize, false)
		assert.NoError(t, err)
		assertKeys(nil, emptyInfos)

		// forward
		infos, err = localDB.FindDocInfosByPreviousIDAndPageSize(ctx, infos[0].ID, pageSize, true)
		assert.NoError(t, err)
		assertKeys([]string{"4","5","6","7","8"}, infos)

		// forward again
		emptyInfos, err = localDB.FindDocInfosByPreviousIDAndPageSize(ctx, infos[len(infos)-1].ID, pageSize, true)
		assert.NoError(t, err)
		assertKeys(nil, emptyInfos)
	}
  1. Does MongoDB work in the same order(from the latest document) as MemoryDB?

- To improve the readability of the test code
@chacha912
Copy link
Contributor Author

chacha912 commented Apr 27, 2022

@hackerwins Thank you for your comment.

  1. The code is much easier to read. Thanks👍
  2. Yes. The FindDocInfosByPreviousIDAndPageSize function works the same way in MongoDB.

@hackerwins hackerwins self-requested a review April 27, 2022 05:18
@hackerwins hackerwins merged commit 83eca5b into yorkie-team:main Apr 27, 2022
jeonjonghyeok pushed a commit to jeonjonghyeok/yorkie that referenced this pull request Aug 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants