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

refactor: optimize totalCount compute for X-Total-Count header #139

Merged
merged 8 commits into from Oct 10, 2022

Conversation

ricfio
Copy link
Contributor

@ricfio ricfio commented Oct 9, 2022

computes totalCount without extra query execution when possible

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Contributor

@marcopiraccini marcopiraccini left a comment

Choose a reason for hiding this comment

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

@ricfio LGTM, just please make sure that we have a test for this case

@ricfio
Copy link
Contributor Author

ricfio commented Oct 9, 2022

@ricfio LGTM, just please make sure that we have a test for this case

I tried adding some extra tests, but he was sure it was a good idea, so I preferred not to add them right now.

If you want I could add these:

  {
    const url = '/posts?offset=1&totalCount=true'
    const res = await app.inject({
      method: 'GET',
      url: url
    })
    equal(res.statusCode, 200, `${url} status code`)
    equal(res.headers['x-total-count'], posts.length, `${url} x-total-count`)
    same(res.json(), posts.map((p, i) => {
      return { ...p, id: i + 1 + '' }
    }).slice(1, 4), `${url} response`)
  }

  {
    const url = '/posts?limit=99&offset=0&totalCount=true'
    const res = await app.inject({
      method: 'GET',
      url: url
    })
    equal(res.statusCode, 200, `${url} status code`)
    equal(res.headers['x-total-count'], posts.length, `${url} x-total-count`)
    same(res.json(), posts.map((p, i) => {
      return { ...p, id: i + 1 + '' }
    }).slice(0, 4), `${url} response`)
  }

  {
    const url = '/posts?limit=99&offset=2&totalCount=true'
    const res = await app.inject({
      method: 'GET',
      url: url
    })
    equal(res.statusCode, 200, `${url} status code`)
    equal(res.headers['x-total-count'], posts.length, `${url} x-total-count`)
    same(res.json(), posts.map((p, i) => {
      return { ...p, id: i + 1 + '' }
    }).slice(2, 4), `${url} response`)
  }

  {
    const url = '/posts?limit=2&offset=99&totalCount=true'
    const res = await app.inject({
      method: 'GET',
      url: url
    })
    equal(res.statusCode, 200, `${url} status code`)
    equal(res.headers['x-total-count'], posts.length, `${url} x-total-count`)
    same(res.json(), [], `${url} response`)
  }

@mcollina
Copy link
Member

mcollina commented Oct 9, 2022

Add them ;)

@ricfio ricfio closed this Oct 9, 2022
@ricfio ricfio deleted the x-total-count-improve branch October 9, 2022 23:22
@ricfio ricfio restored the x-total-count-improve branch October 9, 2022 23:26
@ricfio ricfio reopened this Oct 9, 2022
@ricfio ricfio marked this pull request as draft October 9, 2022 23:26
@@ -311,13 +311,6 @@ test('list', async ({ pass, teardown, same, equal }) => {
}).slice(2), `${url} response`)
}

{
Copy link
Contributor Author

@ricfio ricfio Oct 9, 2022

Choose a reason for hiding this comment

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

I added after the test /posts?totalCount=true (temporarly removed) in other position with a later commit.

@ricfio
Copy link
Contributor Author

ricfio commented Oct 10, 2022

please see the changes history commit by commit because I have done some tests refactor before the last commit:

  • test: add the extra tests to cover totalCount compute optimization cases

@ricfio ricfio marked this pull request as ready for review October 10, 2022 00:15
Copy link
Contributor

@marcopiraccini marcopiraccini left a comment

Choose a reason for hiding this comment

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

OK!

Copy link
Contributor

@leorossi leorossi left a comment

Choose a reason for hiding this comment

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

LGTM

@mcollina mcollina merged commit f90cc28 into platformatic:main Oct 10, 2022
@ricfio ricfio deleted the x-total-count-improve branch October 10, 2022 08:57
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.

None yet

4 participants