[BREAKING]: Change internal layers stack to be value-based instead of type-based #789
Conversation
src/HTTP.jl
Outdated
|
|
||
| function stacklayertypes(::Type{T}, layers; kw...) where {T} | ||
| for (k, _) in pairs(kw) | ||
| layer = Layers.keywordforlayer(Val(k)) |
There was a problem hiding this comment.
I am not sure if I understood the design here, but in order to insert a new layer you need to 1) overload this method, and 2) pass the matching kwarg?
There was a problem hiding this comment.
Yes, that's the idea. The only "baked in" layers then are those where the keyword args included in this stack function call. I've also updated the mechanism (see below) so that if the layer constructor returns nothing it isn't included in the stack.
There was a problem hiding this comment.
I find it slightly strange that you need to do both of these things. If you need to modify the call to HTTP.request anyway (to add the kwarg) why not pass the layer directly?
There was a problem hiding this comment.
That would be pretty inconvenient I think; it would be the difference between:
HTTP.get(...; retry=true) # includes the RetryLayer based on retry keyword "hook"vs.
HTTP.get(...; layer=HTTP.RetryLayer(; retry=true))There's awkwardness there because how do we "pass" the layer? We can't do retry=HTTP.RetryLayer() unless we loop through all keyword arguments and check if the value is a Layer or not. And then there's awkwardness because what if the layer author wants to use a keyword argument as an argument to their layer constructor? Then users have to be burdened with the responsibility of knowing whether to pass args/keyword args to the layer constructor or as a keyword arg to HTTP.request.
In my proposal, everything is keyword args and users don't have to be aware of layers at all.
There was a problem hiding this comment.
Right, but it becomes a bit of "keyword argument piracy". Right now AWS calls HTTP.request with their own stack I think? So in that case, even if I have loaded the aws package I won't accidentally use that layer. BrokenRecords.jl already does "layer piracy" I think?
There was a problem hiding this comment.
I think cases like AWS though shouldn't need to provide an entire custom stack. They should be able to just define their own custom layer or two and then it all composes with the "base" stack in HTTP.jl. The problem with everyone providing their own stacks is there isn't any composability. There isn't a nice way to use AWS custom layer along with my own custom layer in my company's auth system.
I'm going to take some more time to study the AWS and BrokenRecord setups now that this PR is "done", because I want to make sure I understand those use-cases thoroughly and how they fit in this potential new paradigm.
There was a problem hiding this comment.
Right, the current solution isn't ideal either, just not sure the kwargs based one is better.
There was a problem hiding this comment.
Looking back over the previous layer-manipulation code vs. the proposed here and there is a key important difference: the previous code led to the scenario where you were modifying a single, global, and I'll add, non-threadsafe, client request stack by inserting/removing a custom layer for a request. Ok, you weren't modifying the whole stack, but it was still a single global "extra custom layers" stack. That doesn't lend itself to concurrent code needs at all!
In my proposal there is no single global stack; but rather on a per request basis, and with the user-supplied keyword arguments, the stack is dynamically generated and used for the duration of that request, in isolation from any other callers.
I partly discovered this by coming back to this PR and wondering if we shouldn't just use a single global Dict to "register" keyword arguments instead of overloading the Layers.keywordforlayer with a Val, but then again, we could get into thread-safety issues if that keyword registry is being modified concurrently.
So while I still agree the Layers.keywordforlayer mechanism is somewhat ugly, it does have several advantages now that make it simpler in terms of implmentation.
I think it is nice that the order is deterministic at least. |
|
@fredrikekre, the other consideration is I haven't looked into how exactly this interacts with #469; I admit to not fully remembering the motivation for that issue or if that can be folded into the changes here somehow. |
|
The motivation was that #463 where calls to |
|
Ok, sounds good. Do you mind if I hijack those changes and fold them into my PR here? It's pretty deeply tangled in the stuff I'm doing, so might as well? |
|
Go ahead. |
Codecov Report
@@ Coverage Diff @@
## master #789 +/- ##
==========================================
- Coverage 78.12% 78.10% -0.02%
==========================================
Files 39 36 -3
Lines 2482 2430 -52
==========================================
- Hits 1939 1898 -41
+ Misses 543 532 -11
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
|
Ok, quick update. This PR now includes the main changes from #469 since the 2 PRs basically touch all the same files/lines. I also went ahead and removed the AWSLayer (#486, cc: @mattBrzezinski ) since it was also intertwined with all the layer code updates. I like how the code has shaped up; all the test suite passes locally for me, so let's see how CI does. My plan from here is to request feedback/review from folks (@christopher-dG, @mattBrzezinski, and @c42f), I'm going to take a deeper look at BrokenRecord.jl, which seems to be the biggest public package that heavily uses the current-master stack machinery, and then I also want to do a thorough update of the examples code and README where the custom layer/stack functionality are mentioned, so update all the docs around this proposed new stack architecture. |
|
Ok, I've been mulling the entire client request mechanisms again and came up w/ a modified proposal from the one initially posted in this PR. I spent some time detailing everything in the new
resp = HTTP.request(HTTP.stack(CustomLayer), "GET", "https://google.com")But it obviously would get annoying having to include the
module CustomClient
using HTTP
include("customlayer_definitions.jl")
HTTP.@client CustomLayer
endand users could then easily tap into the new using CustomClient
resp = CustomClient.get("https://google.com")Basically, So notably this removes the So for AWS.jl, for example, they would, in theory, be able to define their own Similarly for BrokenRecord.jl, we can define the record/playback functionality in new custom Layers, and then make our own little client users could call to get the record/playback functionality, like The main downside I see of this approach is the single direction in which custom layers can compose w/ each other; i.e. you can't define CustomLayerA in PackageA.jl and CustomLayerB in PackageB.jl, load them both, and just "get" both custom layers automatically (like which was possible with the |
|
So if I'm reading this right, anyone wanting to use BrokenRecord would need to update their source code to use BrokenRecord.get instead of HTTP.get? My ideal case is that the test dependency doesn't leak into the package the way Mocking.jl does. |
Can you expound on what you mean by "doesn't leak into the package"? Perhaps share a small example of code and where it would live (either Part of the motivation in this latest proposal is to make it very explicit when custom layers are being integrated into the request stack. You can imagine this being a security issue in the future if someone were able to "inject" a malicious layer. In the latest proposal, layers must be explicitly included either via manual call to |
I believe this kind of injection would be what @christopher-dG wants to do with BrokenRecord.jl, where it would be possible to monitor a package using HTTP.get without that package having to depend on BrokenRecord.jl. |
I just don't follow what this is supposed to mean. Monitoring a package without that package taking a dependency sounds......malicious? Undesirable? Wrong? What am I missing here. |
Difference of philosophy? This is very common in dynamic languages, it's just monkey patching. But this BrokenRecord package would likely not be part of a typical dependency tree, it would sit as a test dependency. Compare https://github.com/vcr/vcr and (coincidentally?) Cassette.jl. AWS also provides a Python tool that patches requests and httplib to add X-Ray support, which is pretty similar to this. An actually malicious Julia package could pretty easily replace all code in another package anyway. |
|
In this proposal would "body" be an IOStream? In this way, a client or server could process data incrementally. |
|
I'd shy away from naming filters in each layer by keyword and just have the programmer manually specify the stack. If one goes with kw based naming of filters within a given layer, one probably has to specify a way for one filter to mark that it comes before/after another. For example, server side, some filters in the response layer may require that the user is already authenticated, and hence, those layers may want to have a "after=auth" mark, so that they are invoked after authorization. |
This is very proof-of-concept currently, but wanted to put the code up
to get initial feedback/ideas. Here's the gist:
* I define 3 abstract subtypes of `Layer` in the Layers module:
`InitialLayer`, `RequestLayer`, and `ConnectionLayer`; these
correspond to the types of arguments the layer would receive when
overloading `request`; `InitialLayer` => `request(method, url,
headers, body), `RequestLayer` => `request(url, req, body)`, and
`ConnectionLayer` => `request(io, req, body)`. I feel like these are
useful abstract types to help distinguish the different _kinds_ of
layers based on the arguments they receive in different parts of the
overall stack. I feel like it hopefully also solves the "how do I
insert my layer in the appropriate level of the stack" because you
just pick which abstract layer to subtype
* Custom layers are then required to:
* be concrete types
* must subtype one of the 3 abstract layer types mentioned
* must have a constructor of the form: `Layer(next::Layer; kw...)
* must have a field to store the `next::Layer` argument
* must overload: `request(x::MyLayer, args...; kw...)` where `args`
will depend on which abstract layer `MyLayer` subtypes
* in the overloaded `request` method, it must, at some point, call
`request(layer.next, args...; kw...)` to move to the next layer in
the stack
* the final requirement is the custom layer must overload
`Layers.keywordforlayer(::Val{:kw}) = MyLayer` where `:kw` is a
keyword argument "hook" or trigger that will result in `MyLayer`
being inserted into the stack
What I like about this approach so far:
* It feels more straightforward to define my own custom layer: pick
which level of the stack I want it in, subtype that abstract layer,
register a keyword argument that will include my layer, then define my
own `request` overload; we can define all the current HTTP layers in
terms of this machinery, custom packages could define their own layers
and they all are treated just like any HTTP.jl-baked layer
* It also feels like it would be easier to create my own custom "http
client"; what I mean by this is that I feel like it's common for API
packages to basically have their own `AWS.get`/`AWS.post`/`AWS.put`
wrapper methods that eventually call `HTTP.get`/`HTTP.post`/etc., but
in between, they're adding custom headers, validating targets, and
whatever. A lot of that is _like_ a custom layer, but by having your
own `AWS.get`/`AWS.post` wrapper methods, you can _ensure_ certain
layers are included; and with this approach that's straightforward
because they can indeed make their own custom layer as described
above, and then just ensure every `HTTP.request` call includes the new
keyword argument to "hook in" their custom layer
Potential downsides:
* Relying on a global keyword registry; i.e. there's potential for
clashes if two different packages try to overload
`Layers.keywordforlayer(::Val{:readtimeout}) = ...`. Whichever package
is loaded last will "pirate" the keyword arg and overwrite the
previous method. We could potentially add some extra safety around
this by having another `clientenv=:default` keyword and then devs
could overload `Layers.keywordforlayer(::Val{:default},
::Val{:readtimeout}) = ...` but that feels a tad icky. I also don't
expect there to be even dozens of total keywords/layers to process, so
with that in mind, it also lowers the risk of clash.
* The previous layer functionality allowed specifying the _exact_
layer you wanted your custom layer to be inserted before; do we think
that level of specificity/granularity is really necessary? I'm kind of
going on a gut feel here that what you really want control over is
_what kind of args_ your overloaded `request` method will receive and
that's what my 3 new abstract layer subtypes aim to solve. So you can
pick 1 of 3 different _general_ layers of where your custom layer gets
inserted, but don't have control over anything more specific in terms
of what layer comes before/after
* One thing I noticed is that the current `stack` function actually
has some hard-coded logic that checks keyword arguments and
_conditionally_ will include layers based on the _value_ of the
keyword arg. We could potentially handle that in the proposed scheme
by saying that your custom `Layer(next; kw...)` constructor can
return the `nothing` value, in which case, no layer will be inserted.
That would give, for example, the `TimeoutLayer` the chance to check
if the `readtimeout > 0` before returning a `TimeoutLayer` or
`nothing`
drastically reduce the interface requirements
It's about composability. Suppose you're using The proposal with The bigger problem is capturing HTTP requests from If we removed the default global HTTP client entirely and encouraged all libraries to accept a HTTP client context as a parameter this would "fix" the problem. But obviously that's intrusive on APIs using HTTP anywhere. An analogy for the dependency injection which is desired here is logging: it's nice to have a logging frontend which is independent of the logging backends. Authors of various modules can independently use |
| end | ||
| end | ||
| if !isempty(cookiestosend) | ||
| existingcookie = header(req.headers, "Cookie") |
There was a problem hiding this comment.
This is net-new code; I realized our test here was wrong because by default cookies=false and so that request wasn't actually deleting any cookies, but just didn't include them at all. This is apparent because it should only be deleting the single hey cookie, yet the test was testing that there were no cookies.
The solution here is that we need to track when we automatically include cookies on a request, because in the case of a redirect, the initial response may return a set-cookie that expires or otherwise modifies a cookie we've already added to the request. When redirecting, we re-use the request headers that were set, but for cookies, we want to filter out any cookies that were automatically included on the 1st request and just let the "auto" logic do its thing by including them again if they're applicable. This lets cookies that have expired by removed and not sent in subsequent redirect requests.
It's unfortunate it's kind of lost in such a big refactor PR, but oh well.
| @@ -0,0 +1,62 @@ | |||
| module DefaultHeadersRequest | |||
There was a problem hiding this comment.
This is a new "layer" that splits some of the logic out from MessageRequest.jl layer to a separate layer since the MessageRequest.jl layer is now purely for constructing the Request object at the start of the stack
| const STREAM_LAYERS = [timeoutlayer, exceptionlayer, debuglayer] | ||
| const REQUEST_LAYERS = [messagelayer, redirectlayer, defaultheaderslayer, basicauthlayer, contenttypedetectionlayer, cookielayer, retrylayer, canonicalizelayer] | ||
|
|
||
| pushlayer!(layer; request::Bool=true) = push!(request ? REQUEST_LAYERS : STREAM_LAYERS, layer) |
There was a problem hiding this comment.
@christopher-dG, @c42f, here's the code for how I'm imagining we can do global stack modifications in BrokenRecord and elsewhere. Basically we have our 2 arrays of layer types above and we can document these pushlayer!/poplayer! functions as the way to add/remove layers globally.
| end | ||
|
|
||
| request(::Type{Union{}}, resp::Response) = resp | ||
| macro client(requestlayers, streamlayers=[]) |
There was a problem hiding this comment.
New lexically-scoped client macro generator that provides the most common/convenient user-facing functions that can be defined in your own module.
| @@ -10,25 +10,37 @@ module IOExtras | |||
| using ..Sockets | |||
| using MbedTLS: MbedException | |||
|
|
|||
| export bytes, ByteView, nobytes, CodeUnits, IOError, isioerror, | |||
| export bytes, isbytes, nbytes, ByteView, nobytes, IOError, isioerror, | |||
There was a problem hiding this comment.
The changes here are a consolidation of the previous bodybytes/bytes and bodylength functions into a single place. There was some unfortunate mixups of when to use bodybytes vs bytes and we don't really need both.
|
|
||
| """ | ||
| request(MessageLayer, method, ::URI, headers, body) -> HTTP.Response | ||
| messagelayer(method, ::URI, headers, body) -> HTTP.Response |
There was a problem hiding this comment.
We now require messagelayer to be the 1st/outermost layer in our stack as it translates the method, url, headers, and body into the Request object that all other layers consume.
| @@ -103,26 +105,25 @@ Represents a HTTP Response Message. | |||
| You can get each data with [`HTTP.status`](@ref), [`HTTP.headers`](@ref), and [`HTTP.body`](@ref). | |||
|
|
|||
| """ | |||
| mutable struct Response <: Message | |||
| mutable struct Response{T} <: Message | |||
There was a problem hiding this comment.
The Response and Request objects are now both parameterized on their body fields. We previously had some slight awkwardness of having body be hard-coded as Vector{UInt8}, but then needing to pass a separate body argument to all layer functions in case the body was an IO or some other supported format. We now just parameterize body on whatever the user passes and can remove the body_was_streamed business and extra argument passing.
| @@ -140,19 +141,19 @@ HTTP.Response(200, headers; body = "Hello") | |||
| Response() = Request().response | |||
|
|
|||
| Response(s::Int, body::AbstractVector{UInt8}) = Response(s; body=body) | |||
| Response(s::Int, body::AbstractString) = Response(s, bytes(body)) | |||
| Response(s::Int, body::AbstractString) = Response(s; body=bytes(body)) | |||
There was a problem hiding this comment.
I'm pretty sure this and the method just below were just wrong by not passing body as a keyword argument
| response::Response | ||
| txcount::Int | ||
| parent | ||
| url::URI |
There was a problem hiding this comment.
We add a new url field because there are many times throughout the client layers where we want access to the original url provided by the user and not just the resource(url) that we store as the target field.
| parent | ||
| url::URI | ||
| parent::Union{Response, Nothing} | ||
| context::Context |
There was a problem hiding this comment.
We add a new context field that is a Dict{Symbol, Any} that allows some state sharing between layers. It's not used a ton, but it makes a more streamlined/formal way layers can share state if needed instead of relying on some of the other hacks we were doing before (passing state via keyword arguments, or having the txcount field on the Request object, requiring extra positional args to layers, etc.).
| verbose::Int=0, | ||
| kw...)::Response where Next | ||
|
|
||
| function streamlayer(stream::Stream; iofunction=nothing, verbose=0, redirect_limit::Int=3, kw...)::Response |
There was a problem hiding this comment.
This is our only client-side "handler" function that is not a middleware; i.e. it actually takes the stream, sends the request and reads the response. So all other client layers can be seen as "wrapping" this layer that does the actual send/receive operation.
|
Cool I haven't yet dived into the details here. But the layers you're suggesting remind me of Mux.jl's middleware. I think this can be powerful (IIUC express.js uses a similar design too?). So this is interesting and I'll try to look a bit closer at it soon. I do think closures can be a nice way to define these things, but I'd say that in practice I've found Mux pretty annoying to use! Some issues I've observed:
I'm not saying these things apply to the design here which I think is a little different from Mux (?) But they're aspects to be aware of: do we want to optimize so that it's easiest to define new middleware vs reuse existing middleware? |
|
Ok, I'd like to merge this soon, so I'm going to throw out a: merge-in-24-hours call to any potential reviewers. I think I've addressed everything from previous reviews, and provided a necessary "escape hatch" for the global stack manipulation if needed. I'm punting on the docs for now, because I'm already sketching out a bigger docs overhaul to do before the next release. |
| import ..@debug, ..DEBUG_LEVEL | ||
|
|
||
| abstract type ContentTypeDetectionLayer{Next <: Layer} <: Layer{Next} end | ||
| export ContentTypeDetectionLayer | ||
| export contenttypedetectionlayer |
There was a problem hiding this comment.
I feel like these layer names are getting a bit lengthy to read without some way of breaking apart words such as,
content_type_detection_layer
| l = nbytes(req.body) | ||
| if l !== nothing |
There was a problem hiding this comment.
| l = nbytes(req.body) | |
| if l !== nothing | |
| if nbytes(req.body) !== nothing |
There was a problem hiding this comment.
We use the l variable in the next line; but this is probably due to being a really poor variable name more than anything.
| end | ||
| end | ||
|
|
||
| const USER_AGENT = Ref{Union{String, Nothing}}("HTTP.jl/$VERSION") |
There was a problem hiding this comment.
Const definitions under using/import?
| @@ -74,6 +75,7 @@ import ..bytes | |||
|
|
|||
| include("ascii.jl") | |||
|
|
|||
| const nobody = UInt8[] | |||
There was a problem hiding this comment.
Uppercase to match style in other files?
There was a problem hiding this comment.
this is how its been defined for ages; but yeah, we should probably go through and update it.
| @debug 1 "Adding Authorization: Basic header." | ||
| setkv(headers, "Authorization", "Basic $(base64encode(userinfo))") | ||
| function basicauthlayer(handler) | ||
| return function(req; basicauth::Bool=true, kw...) |
There was a problem hiding this comment.
What does the stack traces look like now when everything are anonymous functions?
There was a problem hiding this comment.
also would it improve stacktraces if these were named functions?
e.g.
| return function(req; basicauth::Bool=true, kw...) | |
| return function basicauthhandler(req; basicauth::Bool=true, kw...) |
so they have names like (::var"#basicauthhandler#5") (generic function with 1 method)
There was a problem hiding this comment.
I think the backtraces are as legible as they used to be: the use of closures means we still see a nested list of the layers, and the use of a module per layer means its pretty clear where the layer is coming from.
Co-authored-by: mattBrzezinski <matt.brzezinski@invenia.ca>
Co-authored-by: mattBrzezinski <matt.brzezinski@invenia.ca>
Co-authored-by: mattBrzezinski <matt.brzezinski@invenia.ca>
Co-authored-by: mattBrzezinski <matt.brzezinski@invenia.ca>
Co-authored-by: mattBrzezinski <matt.brzezinski@invenia.ca>
|
Hmmm, this docs deploy error seems to be some kind of unrelated errro? |
Seems to have been removed in: JuliaWeb#789 and is no longer needed.
Seems to have been removed in: #789 and is no longer needed.
This is very proof-of-concept currently, but wanted to put the code up
to get initial feedback/ideas. Here's the gist:
Layerin the Layers module:InitialLayer,RequestLayer, andConnectionLayer; thesecorrespond to the types of arguments the layer would receive when
overloading
request;InitialLayer=>request(method, url, headers, body),RequestLayer=>request(url, req, body), andConnectionLayer=>request(io, req, body). I feel like these areuseful abstract types to help distinguish the different kinds of
layers based on the arguments they receive in different parts of the
overall stack. I feel like it hopefully also solves the "how do I
insert my layer in the appropriate level of the stack" because you
just pick which abstract layer to subtype
next::Layerargumentrequest(x::MyLayer, args...; kw...)whereargswill depend on which abstract layer
MyLayersubtypesrequestmethod, it must, at some point, callrequest(layer.next, args...; kw...)to move to the next layer inthe stack
Layers.keywordforlayer(::Val{:kw}) = MyLayerwhere:kwis akeyword argument "hook" or trigger that will result in
MyLayerbeing inserted into the stack
What I like about this approach so far:
which level of the stack I want it in, subtype that abstract layer,
register a keyword argument that will include my layer, then define my
own
requestoverload; we can define all the current HTTP layers interms of this machinery, custom packages could define their own layers
and they all are treated just like any HTTP.jl-baked layer
client"; what I mean by this is that I feel like it's common for API
packages to basically have their own
AWS.get/AWS.post/AWS.putwrapper methods that eventually call
HTTP.get/HTTP.post/etc., butin between, they're adding custom headers, validating targets, and
whatever. A lot of that is like a custom layer, but by having your
own
AWS.get/AWS.postwrapper methods, you can ensure certainlayers are included; and with this approach that's straightforward
because they can indeed make their own custom layer as described
above, and then just ensure every
HTTP.requestcall includes the newkeyword argument to "hook in" their custom layer
Potential downsides:
clashes if two different packages try to overload
Layers.keywordforlayer(::Val{:readtimeout}) = .... Whichever packageis loaded last will "pirate" the keyword arg and overwrite the
previous method. We could potentially add some extra safety around
this by having another
clientenv=:defaultkeyword and then devscould overload
Layers.keywordforlayer(::Val{:default}, ::Val{:readtimeout}) = ...but that feels a tad icky. I also don'texpect there to be even dozens of total keywords/layers to process, so
with that in mind, it also lowers the risk of clash.
layer you wanted your custom layer to be inserted before; do we think
that level of specificity/granularity is really necessary? I'm kind of
going on a gut feel here that what you really want control over is
what kind of args your overloaded
requestmethod will receive andthat's what my 3 new abstract layer subtypes aim to solve. So you can
pick 1 of 3 different general layers of where your custom layer gets
inserted, but don't have control over anything more specific in terms
of what layer comes before/after
stackfunction actuallyhas some hard-coded logic that checks keyword arguments and
conditionally will include layers based on the value of the
keyword arg. We could potentially handle that in the proposed scheme
by saying that your custom
Layer(next; kw...)constructor canreturn the
nothingvalue, in which case, no layer will be inserted.That would give, for example, the
TimeoutLayerthe chance to checkif the
readtimeout > 0before returning aTimeoutLayerornothing