-
Notifications
You must be signed in to change notification settings - Fork 7
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
src: implement OriginProvider and S3Provider #119
Conversation
96a6790
to
e1a15d2
Compare
src/providers/r2Provider.ts
Outdated
this.ctx.sentry | ||
); | ||
} catch (err) { | ||
if (this.fallbackProvider !== undefined) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shoult this fallback for all errors or 404 only?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any error that reaches here would be regarding the request to R2 failing, if a file doesn't exist in the bucket the binding returns null which is handled a few lines further down.
So only if the initial request to R2 + all of the retries fail with a 5xx, then fallback. Notable that an error can be thrown for 4xx response codes (re #106), but I'll be opening a pr soonish to validate any client-provided variables to prevent a 4xx from ever being the cause here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd say this logic should not reside within the R2Provider, but on whatever middleware that manages the providers.
Like, since the responses of these methods are always a Promise with either an object or undefined, we could say, well if we got undefined, go to the next middleware, until we get an object.
We could also have booleans like if a next provider should be attempted by the middleware of just skipped.
So the method types could be Promise<HeadFileResult | boolean>
so when false
it means, simply did not handle due to X reasons, and if true
provider got skipped try next one. So if false
was returned it won't do anything anymore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now there isn't any middleware that manages the providers. The middleware that is planned however would be something like
const provider = getPrimaryProvider();
const result = await provider.headFile(...)
//...
(impl ref here)
A response of undefined
means that the file/directory doesn't exist, if an error occurs it's thrown. But we might be able to do something like this, where it's a Provider
that will loop through all of the providers that
class ParentProvider implements Provider {
providers: Provider[]
headFile(path) {
let response
for (const provider of this.providers) {
try {
response = await provider.headFile(path)
} catch (err) {
// continue, do nothing
}
}
return response
}
// ...getFile, headDirectory, ...
}
// using it would be:
const provider = new ParentProvider()
const result = await provider.headFile('...')
//...
Imo the former way would be a bit nicer to implement and use however
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel that having something like a routing middleware makes more sense, and then you have these "s3, origin, etc" middlewares.
I'm really fond of the idea, as it allows us to conditionally handle things on different middlewares on-demand, rather than everything goes in one middleware and if it fails it tries another one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but even then I think it'd lead to a lot of code duplication with parsing the request since each middleware needs to do it.
Not sure what you're talking about, could you give an example?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what you're talking about, could you give an example?
From my understanding, an api like get('/dist/*', [r2Middleware, originMiddleware])
means each middleware is a request handler, so we'd need to have the same validation logic in each of the middlewares. Although I think we might be able to have a pass-through middleware specifically for validating the request, get('/dist/*', [validationMiddleware, r2Middleware, originMiddleware])
but there will be cases in which you don't want to use R2 and go straight to the origin
Could you give an example?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
means each middleware is a request handler, so we'd need to have the same validation logic in each of the middlewares. Although I think we might be able to have a pass-through
Im pretty sure we can have a base class that handles such things or having a router middleware that handles the basic common validations for each route and respective http method and after it all got validated it calls next() or simply aborts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll remove the retry stuff from this pr in a bit and will open another pr to explore the middleware approach
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The retry logic is removed now in the provider impls
e1a15d2
to
1f709ed
Compare
): Promise<GetFileResult | undefined> { | ||
const res = await fetch(this.ctx.env.ORIGIN_HOST + path, { | ||
headers: { | ||
'user-agent': 'release-cloudflare-worker', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting, love the approach on the cache headers by using content matches, so that the browser can "proactively" fetch and replace on successive calls to this endpoint if an update was done 🆒
37215f4
to
b2c3e11
Compare
Implements the `OriginProvider` and `S3Provider` classes as described in #111. Allows `R2Provider` to take in a fallback provider that will be used if R2 fails.
b2c3e11
to
6bf5d2a
Compare
options?: GetFileOptions | undefined | ||
): Promise<GetFileResult | undefined> { | ||
const res = await fetch(this.ctx.env.ORIGIN_HOST + path, { | ||
headers: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO: For future, having a function to build these headers extracted from this function would be nice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Implements the
OriginProvider
class as described in #111 and allowsR2Provider
to take in a fallback provider that will be used if R2 fails.