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

PB-1126: Removed the update_interval field from item and collections model #527

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

ltshb
Copy link
Contributor

@ltshb ltshb commented Mar 4, 2025

Due to performance issue we removed the update_interval field on the item and collection level and replace it by a single filed cache_control_header on the collection level.

Now use the previously new cache_control_header field on the collection model
to overwrite the default cache settings of assets, items and collections
endpoints. Note that endpoints that aggregate several collections like the
search endpoint and the collections list endpoint have now caching disabled
to avoid any caching issues and to keep the code simple.

⚠️ This should merge only when #528 has been released !

@ltshb ltshb force-pushed the feat-PB-1126-update-interval branch 3 times, most recently from caacb28 to cb15243 Compare March 4, 2025 17:05
@ltshb ltshb changed the base branch from develop to feat-PB-1126-cache-header-field March 4, 2025 17:06
@ltshb ltshb changed the title PB-1126: Replace the update_interval field by cache_control_header PB-1126: Removed the update_interval field from item and collections model Mar 4, 2025
@ltshb ltshb requested a review from boecklic March 4, 2025 17:08
Copy link
Contributor

@boecklic boecklic left a comment

Choose a reason for hiding this comment

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

Yay, code is getting simpler 👍 nice!

Comment on lines 201 to 203
parts = [i.strip() for i in cache_control_header.split(',')]
args = {i.split('=')[0].strip(): i.split('=')[-1].strip() for i in parts if i}
return {k: True if v == k else v for k, v in args.items()}
Copy link
Contributor

Choose a reason for hiding this comment

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

seeing this here I definitely think we should have some validation for that field, see comment here: #528 (comment)

@ltshb ltshb force-pushed the feat-PB-1126-update-interval branch 2 times, most recently from fc1942c to 48e3b44 Compare March 5, 2025 14:10
@ltshb ltshb force-pushed the feat-PB-1126-cache-header-field branch 3 times, most recently from 0a05bd8 to ace467f Compare March 6, 2025 09:36
Base automatically changed from feat-PB-1126-cache-header-field to develop March 6, 2025 09:59
ltshb added 3 commits March 6, 2025 14:05
…model

Now use the previously new cache_control_header field on the collection model
to overwrite the default cache settings of assets, items and collections
endpoints. Note that endpoints that aggregate several collections like the
search endpoint and the collections list endpoint have now caching disabled
to avoid any caching issues and to keep the code simple.
@ltshb ltshb force-pushed the feat-PB-1126-update-interval branch from 48e3b44 to 6914de5 Compare March 6, 2025 13:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants