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: handle missing vary header values #4031

Merged
merged 2 commits into from
Feb 16, 2025
Merged

Conversation

gurgunday
Copy link
Contributor

fixes: #3959

@gurgunday gurgunday requested a review from mcollina January 28, 2025 19:00
@gurgunday
Copy link
Contributor Author

@KhafraDev can you take a look as well?

This issue recently bit me too

@gurgunday
Copy link
Contributor Author

gurgunday commented Feb 1, 2025

Cc @flakey5, just saw your PR but leaving it here if you want to merge this one

It fixes both the undercase problem and the cache issue

Copy link
Member

@flakey5 flakey5 left a comment

Choose a reason for hiding this comment

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

The issue looks to be still present when using the sqlite cache store?
image

const assert = require('node:assert')
const { parseVaryHeader } = require('../../lib/util/cache.js')

describe('parseVaryHeader', () => {
Copy link
Member

Choose a reason for hiding this comment

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

nit but could you move these to where the other tests are for this function?

describe('parseVaryHeader', () => {

@gurgunday
Copy link
Contributor Author

gurgunday commented Feb 2, 2025

Yeah had to go back to null, it works better with JSON 😁

gurgunday and others added 2 commits February 9, 2025 17:32
Co-authored-by: flakey5 <73616808+flakey5@users.noreply.github.com>
@gurgunday
Copy link
Contributor Author

Rebased

Copy link
Member

@mertcanaltin mertcanaltin 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
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

@mcollina mcollina merged commit 4269dab into nodejs:main Feb 16, 2025
30 of 31 checks passed
@gurgunday gurgunday deleted the fix-vary branch February 16, 2025 11:33
@github-actions github-actions bot mentioned this pull request Feb 28, 2025
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.

Response headers vary: Accept-Encoding is stopping undici from caching the http request
5 participants