Add `cacheControl` to control caching in CDN #252

brettwillis · 2023-05-18T18:52:15Z

Closes #251

Checklist

run npm run test and npm run benchmark
tests and/or benchmarks are included
documentation is changed or added
commit message and code follows the Developer's Certification of Origin
and the Code of conduct

mcollina

lgtm

jsumners · 2023-05-19T12:14:29Z

index.js

@@ -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') {


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).

How about patching normalizeCorsOptions?

Uzlopak

Some comments

types/index.test-d.ts

Uzlopak · 2023-05-19T14:34:55Z

index.js

@@ -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') {


How about patching normalizeCorsOptions?

Uzlopak · 2023-05-20T17:47:35Z

index.js

@@ -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')) {


@jsumners

do we still need a type check?

Suggested change

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

if (corsOptions.cacheControl) {

Yeah, the normalization has cleared this behaviour

cc @brettwillis

@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).

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.

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

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)?

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

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 · 2023-05-22T14:41:41Z

@jsumners

Did you see my comment?

jsumners · 2023-05-22T18:16:26Z

@jsumners

Did you see my comment?

I approved the changes as they are.

Uzlopak

LGTM

Fdawgs · 2023-05-24T19:00:21Z

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

Add cacheControl option with tests and docs

3f67601

mcollina approved these changes May 19, 2023

View reviewed changes

jsumners reviewed May 19, 2023

View reviewed changes

Uzlopak reviewed May 19, 2023

View reviewed changes

Normalise cacheControl option

8f08a44

Eomm approved these changes May 20, 2023

View reviewed changes

Uzlopak reviewed May 20, 2023

View reviewed changes

jsumners approved these changes May 22, 2023

View reviewed changes

Don't coerce cacheControl to number

59104e5

Uzlopak approved these changes May 23, 2023

View reviewed changes

jsumners approved these changes May 23, 2023

View reviewed changes

Uzlopak merged commit 8f219bc into fastify:master May 24, 2023

brettwillis deleted the cache-control branch May 25, 2023 23:54

Bad3r mentioned this pull request May 22, 2024

[Snyk] Upgrade @fastify/cors from 8.2.0 to 8.5.0 Bad3r/Logseq#668

Open

Bad3r mentioned this pull request Jun 12, 2024

[Snyk] Upgrade @fastify/cors from 8.2.0 to 8.5.0 Bad3r/Logseq#695

Open

Summonshr mentioned this pull request Jun 29, 2024

[Snyk] Upgrade @fastify/cors from 8.2.0 to 8.5.0 Summonshr/fastify-test#2

Open

Bad3r mentioned this pull request Sep 8, 2024

[Snyk] Upgrade: , , better-sqlite3, dugite, electron-dl, electron-log, electron-squirrel-startup, https-proxy-agent, node-fetch, open, posthog-js, semver, socks-proxy-agent Bad3r/Logseq#802

Open

gjovs mentioned this pull request Sep 10, 2024

[Snyk] Upgrade: , ws, , , axios, express, fastify, geolib, jsonwebtoken, prisma, socket.io, tsx, zod gjovs/backend-tcc-yvypora#68

Open

Bad3r mentioned this pull request Sep 11, 2024

[Snyk] Upgrade: , , better-sqlite3, dugite, electron-dl, electron-log, electron-squirrel-startup, https-proxy-agent, node-fetch, open, posthog-js, semver, socks-proxy-agent Bad3r/Logseq#806

Open

gjovs mentioned this pull request Sep 18, 2024

[Snyk] Upgrade: , ws, , , axios, express, fastify, geolib, jsonwebtoken, prisma, socket.io, tsx, zod gjovs/backend-tcc-yvypora#71

Open

Bad3r mentioned this pull request Sep 19, 2024

[Snyk] Upgrade: , , better-sqlite3, dugite, electron-dl, electron-log, electron-squirrel-startup, https-proxy-agent, node-fetch, open, posthog-js, semver, socks-proxy-agent Bad3r/Logseq#814

Open

Bad3r mentioned this pull request Sep 20, 2024

[Snyk] Upgrade: , , better-sqlite3, dugite, electron-dl, electron-log, electron-squirrel-startup, https-proxy-agent, node-fetch, open, posthog-js, semver, socks-proxy-agent Bad3r/Logseq#816

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `cacheControl` to control caching in CDN #252

Add `cacheControl` to control caching in CDN #252

brettwillis commented May 18, 2023

mcollina left a comment

jsumners May 19, 2023

Uzlopak May 19, 2023

Uzlopak left a comment

Uzlopak May 19, 2023

Uzlopak May 20, 2023

Eomm May 23, 2023

brettwillis May 23, 2023

jsumners May 23, 2023

Eomm May 23, 2023

brettwillis May 23, 2023

jsumners May 23, 2023

brettwillis May 23, 2023

Uzlopak commented May 22, 2023

jsumners commented May 22, 2023

Uzlopak left a comment

Fdawgs commented May 24, 2023

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

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

Add cacheControl to control caching in CDN #252

Add cacheControl to control caching in CDN #252

Conversation

brettwillis commented May 18, 2023

Checklist

mcollina left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Uzlopak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Uzlopak commented May 22, 2023

jsumners commented May 22, 2023

Uzlopak left a comment

Choose a reason for hiding this comment

Fdawgs commented May 24, 2023

Add `cacheControl` to control caching in CDN #252

Add `cacheControl` to control caching in CDN #252