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

Optimised queries made by get helper for posts #19859

Merged
merged 4 commits into from Mar 13, 2024

Conversation

allouis
Copy link
Contributor

@allouis allouis commented Mar 13, 2024

ref ENG-747
ref https://linear.app/tryghost/issue/ENG-747

H'okay - so what we're trying to do here is make get helper queries more cacheable. The way we're doing that is by modifying the filter used when we're trying to remove a single post from the query.

The idea is that we can remove that restriction on the filter, increase the number of posts fetched by 1 and then filter the fetched posts back down, this means that the same query, but filtering different posts, will be updated to make exactly the same query, and so share a cache!

We've been purposefully restrictive in the types of filters we manipulate, so that we only deal with the simplest cases and the code is easier to understand.

ref ENG-747
ref https://linear.app/tryghost/issue/ENG-747

H'okay - so what we're trying to do here is make get helper queries more
cacheable. The way we're doing that is by modifying the filter used when we're
trying to remove a single post from the query.

The idea is that we can remove that restriction on the filter, increase the
number of posts fetched by 1 and then filter the fetched posts back down, this
means that the same query, but filtering different posts, will be updated to
make _exactly_ the same query, and so share a cache!

We've been purposefully restrictive in the types of filters we manipulate, so
that we only deal with the simplest cases and the code is easier to understand.
Comment on lines 153 to 154
// The default limit is 15, the addition order is to cast apiOptions.limit to a number
const limit = apiOptions.limit ? 1 + apiOptions.limit : 16;
Copy link
Contributor

Choose a reason for hiding this comment

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

Where does that default limit come from?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a great question, I can't actually find where - but it is our default limit across our apis

Copy link
Contributor

Choose a reason for hiding this comment

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

OK. Found it in the Framework monorepo. No real issue, just always a bit averse to magic numbers 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup - get a big plus one from me on that :D

@allouis allouis enabled auto-merge (squash) March 13, 2024 19:20
@allouis allouis merged commit 52a28c0 into main Mar 13, 2024
19 checks passed
@allouis allouis deleted the egg-eng-747-make-get-helper-more-cacheable branch March 13, 2024 19:27
kevinansfield pushed a commit that referenced this pull request Mar 14, 2024
ref ENG-747
ref https://linear.app/tryghost/issue/ENG-747

H'okay - so what we're trying to do here is make get helper queries more
cacheable. The way we're doing that is by modifying the filter used when
we're trying to remove a single post from the query.

The idea is that we can remove that restriction on the filter, increase
the number of posts fetched by 1 and then filter the fetched posts back
down, this means that the same query, but filtering different posts,
will be updated to make _exactly_ the same query, and so share a cache!

We've been purposefully restrictive in the types of filters we
manipulate, so that we only deal with the simplest cases and the code is
easier to understand.
allouis added a commit that referenced this pull request Mar 14, 2024
ref ENG-747
ref https://linear.app/tryghost/issue/ENG-747
ref #19859

The original PR got merged prematurely, so here's some missing fixes for it.

- Ensures that we `parseInt` the limit
- Correctly passes down the `id:-<id>` filter
- Handles `limit=all` correctly
- Adds missing test cases
royalfig pushed a commit that referenced this pull request Mar 25, 2024
ref ENG-747
ref https://linear.app/tryghost/issue/ENG-747

H'okay - so what we're trying to do here is make get helper queries more
cacheable. The way we're doing that is by modifying the filter used when
we're trying to remove a single post from the query.

The idea is that we can remove that restriction on the filter, increase
the number of posts fetched by 1 and then filter the fetched posts back
down, this means that the same query, but filtering different posts,
will be updated to make _exactly_ the same query, and so share a cache!

We've been purposefully restrictive in the types of filters we
manipulate, so that we only deal with the simplest cases and the code is
easier to understand.
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

2 participants