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

Fix caching issues for remote content API calls #11464

Conversation

rtibbles
Copy link
Member

Summary

  • Fixes issue whereby non-200 responses would get cached
  • Fixes content API caching to bypass Django caching middleware, as bespoke caching mechanisms are being used instead
  • Fixes public content API endpoints to use matching cache headers to the same endpoints on Studio
  • Fixes Etag checking on access to remote libraries by keeping a cache of Etags by baseurl to allow quick 304 responses without having to go to the remote

References

Fixes #11457

Reviewer guidance

Load e.g. http://127.0.0.1:8000/api/public/v2/contentnode_tree/d6e3b856125f5e6aa5fb40c8b112d5e9/?format=json and check that refreshes result in a 304 immediately

Likewise for access to remote URLs, such as http://localhost:8000/api/content/contentnode_tree/d6e3b856125f5e6aa5fb40c8b112d5e9/?include_coach_content=true&baseurl=https%3A%2F%2Fstudio.learningequality.org&format=json

Also follow the steps outlined in the issue to ensure that 410s are not being cached.


Testing checklist

  • Contributor has fully tested the PR manually
  • If there are any front-end changes, before/after screenshots are included
  • Critical user journeys are covered by Gherkin stories
  • Critical and brittle code paths are covered by unit tests

PR process

  • PR has the correct target branch and milestone
  • PR has 'needs review' or 'work-in-progress' label
  • If PR is ready for review, a reviewer has been added. (Don't use 'Assignees')
  • If this is an important user-facing change, PR or related issue has a 'changelog' label
  • If this includes an internal dependency change, a link to the diff is provided

Reviewer checklist

  • Automated test coverage is satisfactory
  • PR is fully functional
  • PR has been tested for accessibility regressions
  • External dependency files were updated if necessary (yarn and pip)
  • Documentation is updated
  • Contributor is in AUTHORS.md

@rtibbles rtibbles added the TODO: needs review Waiting for review label Oct 26, 2023
@rtibbles rtibbles requested review from jredrejo and akolson October 26, 2023 19:53
@github-actions github-actions bot added the DEV: backend Python, databases, networking, filesystem... label Oct 26, 2023
@rtibbles rtibbles force-pushed the naming_things_is_hard_but_so_is_trying_to_keep_a_quickly_accessible_record_of_things_seen_previously branch from 8347565 to d80a2a8 Compare October 26, 2023 19:53
@akolson
Copy link
Member

akolson commented Oct 26, 2023

It looks like I am still able to replicate the 410s by following #11457 replication steps. Maybe rebasing to include #11459 (review) changes could prove beneficial?

410s.mov

@rtibbles
Copy link
Member Author

Ugh - no, this seems to be that in spite of refusing to use disk cache for other requests, browsers are very happy to use the disk cache for the 410 response! I'll patch the response headers in this case to prevent caching.

@rtibbles
Copy link
Member Author

@akolson I've made one more update to ensure that we add never cache headers to non-200 responses as well, so that browsers are never tempted to cache them!

Copy link
Member

@akolson akolson left a comment

Choose a reason for hiding this comment

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

Changes look correct and also seem to fix the issue reported. Tagging @radinamatic @pcenov for more manual QA. Thanks @rtibbles

@pcenov pcenov self-requested a review October 27, 2023 14:05
Copy link
Member

@pcenov pcenov left a comment

Choose a reason for hiding this comment

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

LGTM as well, no caching issues observed while manually testing.

@marcellamaki marcellamaki merged commit 848873a into learningequality:release-v0.16.x Oct 27, 2023
34 checks passed
@rtibbles rtibbles deleted the naming_things_is_hard_but_so_is_trying_to_keep_a_quickly_accessible_record_of_things_seen_previously branch October 27, 2023 22:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DEV: backend Python, databases, networking, filesystem... SIZE: medium SIZE: small TODO: needs review Waiting for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Remote content browsing] Accessing previously failed remote resources returns 410s
4 participants