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.

Sign up for GitHub

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

Jump to bottom

Add cacheControl to control caching in CDN #252

Merged
merged 3 commits into from
May 24, 2023
Merged

Add cacheControl to control caching in CDN #252

merged 3 commits into from
May 24, 2023

Conversation

brettwillis
Copy link
Contributor

Closes #251

Checklist

mcollina
mcollina approved these changes May 19, 2023
View reviewed changes
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

jsumners
jsumners reviewed May 19, 2023
View reviewed changes
index.js Outdated
@@ -235,6 +235,12 @@ function addPreflightHeaders (req, reply, corsOptions) {
if (corsOptions.maxAge !== null) {
reply.header('Access-Control-Max-Age', String(corsOptions.maxAge))
}

if (typeof corsOptions.cacheControl === 'number') {
Copy link
Member

Choose a reason for hiding this comment

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

Do you think this would be a better validation?

Suggested change
if (typeof corsOptions.cacheControl === 'number') {
if (Number.isInteger(Number(corsOptions.cacheControl)) === true) {

String numbers will be coerced, and all values will be verified to be integers (as per spec).

Copy link
Contributor

Choose a reason for hiding this comment

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

How about patching normalizeCorsOptions?

Uzlopak
Uzlopak reviewed May 19, 2023
View reviewed changes
Copy link
Contributor

@Uzlopak Uzlopak left a comment

Choose a reason for hiding this comment

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

Some comments

types/index.test-d.ts Show resolved Hide resolved
index.js Outdated
@@ -235,6 +235,12 @@ function addPreflightHeaders (req, reply, corsOptions) {
if (corsOptions.maxAge !== null) {
reply.header('Access-Control-Max-Age', String(corsOptions.maxAge))
}

if (typeof corsOptions.cacheControl === 'number') {
Copy link
Contributor

Choose a reason for hiding this comment

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

How about patching normalizeCorsOptions?

Uzlopak
Uzlopak reviewed May 20, 2023
View reviewed changes
index.js Outdated
@@ -235,6 +241,10 @@ function addPreflightHeaders (req, reply, corsOptions) {
if (corsOptions.maxAge !== null) {
reply.header('Access-Control-Max-Age', String(corsOptions.maxAge))
}

if (corsOptions.cacheControl && (typeof corsOptions.cacheControl === 'string')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@jsumners

do we still need a type check?

Suggested change
if (corsOptions.cacheControl && (typeof corsOptions.cacheControl === 'string')) {
if (corsOptions.cacheControl) {

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, the normalization has cleared this behaviour

cc @brettwillis

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Eomm are you agreeing to apply the suggested change? Initially we had cacheControl only applied if the value was string or number. If we remove the type check then we could end up applying any non-number value.

We should either (1) keep the check, (2) coerce the "any truthy value" to string e.g. String(cacheControl) and apply it, or (3) patch normalisation to also set cacheControl to null if it is not a string (and not a number).

Copy link
Member

Choose a reason for hiding this comment

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

This is-string check needs to happen in order to not add an invalid header value (still possible, but the module has done as much as it really can). Test should be added to show this if they do not already exist.

Copy link
Member

Choose a reason for hiding this comment

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

Test should be added to show this if they do not already exist.

This, or just the normalizeCorsOptions do the full normalization :D

This check protects us if the user provides a function or and object.
Right now - it is just ignored. Let's add a test and it is fine too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I discovered that this Number.isInteger(Number(corsOptions.cacheControl)) behaviour means that a boolean values are coerced to max-age=1 and max-age=0 respectively, which is probably not desired.

Should we add a check to exclude booleans e.g. Number.isInteger(Number(corsOptions.cacheControl)) && (typeof corsOptions.cacheControl !== 'boolean')?

Or only consider values that are strictly a number (don't coerce to number) e.g. Number.isInteger(corsOptions.cacheControl)?

Copy link
Member

Choose a reason for hiding this comment

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

Either accept a string or an integer. Do not accept anything else.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Either accept a string or an integer. Do not accept anything else.

Ok, done.

Also I moved the is-string check to the normalisation function, so there is one less conditional check being run per-request.

@Uzlopak
Copy link
Contributor

Uzlopak commented May 22, 2023

@jsumners

Did you see my comment?

@jsumners
Copy link
Member

@jsumners

Did you see my comment?

I approved the changes as they are.

Uzlopak
Uzlopak approved these changes May 23, 2023
View reviewed changes
Copy link
Contributor

@Uzlopak Uzlopak left a comment

Choose a reason for hiding this comment

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

LGTM

@Uzlopak Uzlopak merged commit 8f219bc into fastify:master May 24, 2023
@Fdawgs
Copy link
Member

Fdawgs commented May 24, 2023

How well does this play with the @fastify/caching plugin, or will it have no impact?

@brettwillis brettwillis deleted the cache-control branch May 25, 2023 23:54
bodinsamuel referenced this pull request in specfy/specfy Jul 17, 2023
@renovate
fix(deps): update dependency @fastify/cors to v8.3.0 (#45)
a5797b1 
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [@fastify/cors](https://togithub.com/fastify/fastify-cors) | [`8.2.0`
->
`8.3.0`](https://renovatebot.com/diffs/npm/@fastify%2fcors/8.2.0/8.3.0)
|
[![age](https://developer.mend.io/api/mc/badges/age/npm/@fastify%2fcors/8.3.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://developer.mend.io/api/mc/badges/adoption/npm/@fastify%2fcors/8.3.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://developer.mend.io/api/mc/badges/compatibility/npm/@fastify%2fcors/8.2.0/8.3.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://developer.mend.io/api/mc/badges/confidence/npm/@fastify%2fcors/8.2.0/8.3.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|

---

### Release Notes

<details>
<summary>fastify/fastify-cors (@&#8203;fastify/cors)</summary>

###
[`v8.3.0`](https://togithub.com/fastify/fastify-cors/releases/tag/v8.3.0)

[Compare
Source](https://togithub.com/fastify/fastify-cors/compare/v8.2.1...v8.3.0)

##### What's Changed

- chore(deps-dev): bump typescript from 4.9.5 to 5.0.2 by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[https://github.com/fastify/fastify-cors/pull/247](https://togithub.com/fastify/fastify-cors/pull/247)
- chore(deps-dev): bump tsd from 0.27.0 to 0.28.0 by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[https://github.com/fastify/fastify-cors/pull/248](https://togithub.com/fastify/fastify-cors/pull/248)
- ci: only trigger on pushes to main branches by
[@&#8203;Fdawgs](https://togithub.com/Fdawgs) in
[https://github.com/fastify/fastify-cors/pull/249](https://togithub.com/fastify/fastify-cors/pull/249)
- chore(deps-dev): bump
[@&#8203;types/node](https://togithub.com/types/node) from 18.16.5 to
20.1.0 by [@&#8203;dependabot](https://togithub.com/dependabot) in
[https://github.com/fastify/fastify-cors/pull/250](https://togithub.com/fastify/fastify-cors/pull/250)
- Add `cacheControl` to control caching in CDN by
[@&#8203;brettwillis](https://togithub.com/brettwillis) in
[https://github.com/fastify/fastify-cors/pull/252](https://togithub.com/fastify/fastify-cors/pull/252)

##### New Contributors

- [@&#8203;brettwillis](https://togithub.com/brettwillis) made their
first contribution in
[https://github.com/fastify/fastify-cors/pull/252](https://togithub.com/fastify/fastify-cors/pull/252)

**Full Changelog**:
fastify/fastify-cors@v8.2.1...v8.3.0

###
[`v8.2.1`](https://togithub.com/fastify/fastify-cors/releases/tag/v8.2.1)

[Compare
Source](https://togithub.com/fastify/fastify-cors/compare/v8.2.0...v8.2.1)

#### What's Changed

- chore(deps-dev): bump tsd from 0.24.1 to 0.25.0 by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[https://github.com/fastify/fastify-cors/pull/235](https://togithub.com/fastify/fastify-cors/pull/235)
- fix(types): Origin within OriginFunction can be undefined by
[@&#8203;joshmeads](https://togithub.com/joshmeads) in
[https://github.com/fastify/fastify-cors/pull/237](https://togithub.com/fastify/fastify-cors/pull/237)
- chore(.gitignore): add clinic by
[@&#8203;Fdawgs](https://togithub.com/Fdawgs) in
[https://github.com/fastify/fastify-cors/pull/239](https://togithub.com/fastify/fastify-cors/pull/239)
- chore(.gitignore): add bun lockfile by
[@&#8203;Fdawgs](https://togithub.com/Fdawgs) in
[https://github.com/fastify/fastify-cors/pull/241](https://togithub.com/fastify/fastify-cors/pull/241)
- chore(deps-dev): bump tsd from 0.25.0 to 0.26.0 by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[https://github.com/fastify/fastify-cors/pull/242](https://togithub.com/fastify/fastify-cors/pull/242)
- chore(deps-dev): bump tsd from 0.26.1 to 0.27.0 by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[https://github.com/fastify/fastify-cors/pull/245](https://togithub.com/fastify/fastify-cors/pull/245)
- add normalizeCorsOptions and handle wildcards in origin parameter by
[@&#8203;Uzlopak](https://togithub.com/Uzlopak) in
[https://github.com/fastify/fastify-cors/pull/244](https://togithub.com/fastify/fastify-cors/pull/244)

#### New Contributors

- [@&#8203;joshmeads](https://togithub.com/joshmeads) made their first
contribution in
[https://github.com/fastify/fastify-cors/pull/237](https://togithub.com/fastify/fastify-cors/pull/237)

**Full Changelog**:
fastify/fastify-cors@v8.2.0...v8.2.1

</details>

---

### Configuration

📅 **Schedule**: Branch creation - "after 4pm on friday,before 9am on
monday,every weekend" in timezone Europe/Paris, Automerge - At any time
(no schedule defined).

🚦 **Automerge**: Enabled.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View
repository job log
[here](https://developer.mend.io/github/specfy/specfy).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNi41LjMiLCJ1cGRhdGVkSW5WZXIiOiIzNi44LjExIiwidGFyZ2V0QnJhbmNoIjoiY2hvcmUvcmVub3ZhdGVCYXNlQnJhbmNoIn0=-->

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
bodinsamuel referenced this pull request in specfy/specfy Aug 8, 2023
@renovate
fix(deps): update dependency @fastify/cors to v8.3.0 (#79)
4d99293 
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [@fastify/cors](https://togithub.com/fastify/fastify-cors) | [`8.2.0`
->
`8.3.0`](https://renovatebot.com/diffs/npm/@fastify%2fcors/8.2.0/8.3.0)
|
[![age](https://developer.mend.io/api/mc/badges/age/npm/@fastify%2fcors/8.3.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://developer.mend.io/api/mc/badges/adoption/npm/@fastify%2fcors/8.3.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://developer.mend.io/api/mc/badges/compatibility/npm/@fastify%2fcors/8.2.0/8.3.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://developer.mend.io/api/mc/badges/confidence/npm/@fastify%2fcors/8.2.0/8.3.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|

---

### Release Notes

<details>
<summary>fastify/fastify-cors (@&#8203;fastify/cors)</summary>

###
[`v8.3.0`](https://togithub.com/fastify/fastify-cors/releases/tag/v8.3.0)

[Compare
Source](https://togithub.com/fastify/fastify-cors/compare/v8.2.1...v8.3.0)

##### What's Changed

- chore(deps-dev): bump typescript from 4.9.5 to 5.0.2 by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[https://github.com/fastify/fastify-cors/pull/247](https://togithub.com/fastify/fastify-cors/pull/247)
- chore(deps-dev): bump tsd from 0.27.0 to 0.28.0 by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[https://github.com/fastify/fastify-cors/pull/248](https://togithub.com/fastify/fastify-cors/pull/248)
- ci: only trigger on pushes to main branches by
[@&#8203;Fdawgs](https://togithub.com/Fdawgs) in
[https://github.com/fastify/fastify-cors/pull/249](https://togithub.com/fastify/fastify-cors/pull/249)
- chore(deps-dev): bump
[@&#8203;types/node](https://togithub.com/types/node) from 18.16.5 to
20.1.0 by [@&#8203;dependabot](https://togithub.com/dependabot) in
[https://github.com/fastify/fastify-cors/pull/250](https://togithub.com/fastify/fastify-cors/pull/250)
- Add `cacheControl` to control caching in CDN by
[@&#8203;brettwillis](https://togithub.com/brettwillis) in
[https://github.com/fastify/fastify-cors/pull/252](https://togithub.com/fastify/fastify-cors/pull/252)

##### New Contributors

- [@&#8203;brettwillis](https://togithub.com/brettwillis) made their
first contribution in
[https://github.com/fastify/fastify-cors/pull/252](https://togithub.com/fastify/fastify-cors/pull/252)

**Full Changelog**:
fastify/fastify-cors@v8.2.1...v8.3.0

###
[`v8.2.1`](https://togithub.com/fastify/fastify-cors/releases/tag/v8.2.1)

[Compare
Source](https://togithub.com/fastify/fastify-cors/compare/v8.2.0...v8.2.1)

##### What's Changed

- chore(deps-dev): bump tsd from 0.24.1 to 0.25.0 by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[https://github.com/fastify/fastify-cors/pull/235](https://togithub.com/fastify/fastify-cors/pull/235)
- fix(types): Origin within OriginFunction can be undefined by
[@&#8203;joshmeads](https://togithub.com/joshmeads) in
[https://github.com/fastify/fastify-cors/pull/237](https://togithub.com/fastify/fastify-cors/pull/237)
- chore(.gitignore): add clinic by
[@&#8203;Fdawgs](https://togithub.com/Fdawgs) in
[https://github.com/fastify/fastify-cors/pull/239](https://togithub.com/fastify/fastify-cors/pull/239)
- chore(.gitignore): add bun lockfile by
[@&#8203;Fdawgs](https://togithub.com/Fdawgs) in
[https://github.com/fastify/fastify-cors/pull/241](https://togithub.com/fastify/fastify-cors/pull/241)
- chore(deps-dev): bump tsd from 0.25.0 to 0.26.0 by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[https://github.com/fastify/fastify-cors/pull/242](https://togithub.com/fastify/fastify-cors/pull/242)
- chore(deps-dev): bump tsd from 0.26.1 to 0.27.0 by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[https://github.com/fastify/fastify-cors/pull/245](https://togithub.com/fastify/fastify-cors/pull/245)
- add normalizeCorsOptions and handle wildcards in origin parameter by
[@&#8203;Uzlopak](https://togithub.com/Uzlopak) in
[https://github.com/fastify/fastify-cors/pull/244](https://togithub.com/fastify/fastify-cors/pull/244)

##### New Contributors

- [@&#8203;joshmeads](https://togithub.com/joshmeads) made their first
contribution in
[https://github.com/fastify/fastify-cors/pull/237](https://togithub.com/fastify/fastify-cors/pull/237)

**Full Changelog**:
fastify/fastify-cors@v8.2.0...v8.2.1

</details>

---

### Configuration

📅 **Schedule**: Branch creation - "after 4pm on friday,before 9am on
monday,every weekend" in timezone Europe/Paris, Automerge - At any time
(no schedule defined).

🚦 **Automerge**: Disabled because a matching PR was automerged
previously.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View
repository job log
[here](https://developer.mend.io/github/specfy/specfy).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNi4xMS4wIiwidXBkYXRlZEluVmVyIjoiMzYuMjcuMSIsInRhcmdldEJyYW5jaCI6ImNob3JlL3Jlbm92YXRlQmFzZUJyYW5jaCJ9-->

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
@Bad3r Bad3r mentioned this pull request May 22, 2024
Open
@Bad3r Bad3r mentioned this pull request Jun 12, 2024
Open
@Summonshr Summonshr mentioned this pull request Jun 29, 2024
Open
This was referenced Sep 8, 2024
Open
Open
This was referenced Sep 17, 2024
Open
Open
Open
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Eomm Eomm Eomm approved these changes

@mcollina mcollina mcollina approved these changes

@jsumners jsumners jsumners approved these changes

@Uzlopak Uzlopak Uzlopak approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Setting for Cache-Control to allow preflight requests to be cached in CDNs
6 participants
@brettwillis @Uzlopak @jsumners @Fdawgs @mcollina @Eomm