Visitar URL original
fix: handle missing vary header values by gurgunday · Pull Request #4031 · nodejs/undici · GitHub
Skip to content

fix: handle missing vary header values - #4031

Merged
mcollina merged 2 commits into
nodejs:mainfrom
gurgunday:fix-vary
Feb 16, 2025
Merged

fix: handle missing vary header values#4031
mcollina merged 2 commits into
nodejs:mainfrom
gurgunday:fix-vary

Conversation

@gurgunday

@gurgunday gurgunday commented Jan 27, 2025

Copy link
Copy Markdown
Member

fixes: #3959

github-advanced-security[bot]
github-advanced-security AI found potential problems Jan 27, 2025
View reviewed changes
Comment thread test/types/cache-interceptor.test-d.ts Fixed
Comment thread test/types/cache-interceptor.test-d.ts Fixed
Comment thread test/utils/cache.js Fixed
Comment thread test/utils/cache.js Fixed
gurgunday
gurgunday commented Jan 27, 2025
View reviewed changes
Comment thread lib/util/cache.js Resolved
mcollina
mcollina reviewed Jan 28, 2025
View reviewed changes
Comment thread test/issue-3959.js Resolved
Comment thread test/utils/cache.js Resolved
Comment thread lib/util/cache.js Resolved
metcoder95
metcoder95 approved these changes Jan 28, 2025
View reviewed changes
Comment thread lib/util/cache.js Resolved
Comment thread test/issue-3959.js Resolved
@gurgunday
gurgunday requested a review from mcollina January 28, 2025 19:00
@gurgunday

gurgunday commented Jan 30, 2025

Copy link
Copy Markdown
Member Author

@KhafraDev can you take a look as well?

This issue recently bit me too

@gurgunday

gurgunday commented Feb 1, 2025 •
edited
Loading

Copy link
Copy Markdown
Member Author

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

@gurgunday
gurgunday requested a review from metcoder95 February 1, 2025 12:15
flakey5
flakey5 suggested changes Feb 2, 2025
View reviewed changes

@flakey5 flakey5 left a comment

Copy link
Copy Markdown
Member

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

Comment thread test/utils/cache.js Outdated
const assert = require('node:assert')
const { parseVaryHeader } = require('../../lib/util/cache.js')

describe('parseVaryHeader', () => {

@flakey5 flakey5 Feb 2, 2025

Copy link
Copy Markdown
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?

undici/test/cache-interceptor/utils.js

Line 179 in a9176c9

describe('parseVaryHeader', () => {

@gurgunday

gurgunday commented Feb 2, 2025 •
edited
Loading

Copy link
Copy Markdown
Member Author

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

@gurgunday
gurgunday requested a review from flakey5 February 2, 2025 19:44
@flakey5 flakey5 mentioned this pull request Feb 2, 2025
Closed
gurgunday and others added 2 commits February 9, 2025 17:32
@gurgunday @flakey5
fix: handle missing vary header values
5a94cf0 
Co-authored-by: flakey5 <73616808+flakey5@users.noreply.github.com>
@gurgunday
fix: test
d6fcb21
@gurgunday

gurgunday commented Feb 9, 2025

Copy link
Copy Markdown
Member Author

Rebased

mertcanaltin
mertcanaltin approved these changes Feb 10, 2025
View reviewed changes

@mertcanaltin mertcanaltin left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm

mcollina
mcollina approved these changes Feb 16, 2025
View reviewed changes

@mcollina mcollina left a comment

Copy link
Copy Markdown
Member

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
@gurgunday
gurgunday deleted the fix-vary branch February 16, 2025 11:33
@github-actions github-actions Bot mentioned this pull request Feb 28, 2025
Merged
@github-actions github-actions Bot mentioned this pull request Mar 12, 2025
Closed
ronag added a commit to nxtedition/undici that referenced this pull request Mar 14, 2025
@ronag
Merge remote-tracking branch 'nodejs/main'
cd214d4 
* nodejs/main: (23 commits)
  Bumped v7.5.0 (nodejs#4091)
  Removed clients with unrecoverable errors from the Pool (nodejs#4088)
  feat: Allow disabling autoSelectFamily in an Agent (nodejs#4070)
  chore: update cache tests (nodejs#4027)
  fix: Fix retry-handler.js when retry-after header is a Date (nodejs#4084)
  feat: add mock call history to access request configuration in test (nodejs#4029)
  feat(docs): button to switch dark and light mode (nodejs#4044)
  Bumped v7.4.0 (nodejs#4071)
  fix: fix EnvHttpProxyAgent for the Node.js bundle (nodejs#4064)
  chore: update WPT (nodejs#4062)
  chore: update WPT (nodejs#4028)
  fix: handle missing vary header values (nodejs#4031)
  fix: do not throw unhandled exception when data is undefined in interceptor.reply (nodejs#4036)
  test: fix windows wpt (nodejs#4050)
  feat: mark `EnvHttpProxyAgent` as stable (nodejs#4049)
  don't check AbortSignal maxListeners on some node versions (nodejs#4045)
  feat(docs): copy to clipboard button (nodejs#4037)
  docs: fix incorrect method signature of `onResponseError` (nodejs#4030)
  docs: document about global dispatcher and errors (nodejs#3987) (nodejs#4014)
  chore: update WPT (nodejs#4011)
  ...
@alxndrsn alxndrsn mentioned this pull request Mar 18, 2025
Closed
@github-actions github-actions Bot mentioned this pull request May 12, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcollina mcollina mcollina approved these changes
@metcoder95 metcoder95 metcoder95 approved these changes
@mertcanaltin mertcanaltin mertcanaltin approved these changes
+1 more reviewer
@flakey5 flakey5 flakey5 approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

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

6 participants

@gurgunday @mcollina @metcoder95 @mertcanaltin @github-advanced-security @flakey5