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.

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

[BREAKING]: Change internal layers stack to be value-based instead of type-based #789

Merged
merged 19 commits into from
Mar 22, 2022

Commits on Mar 18, 2022

  1. Change internal layers stack to be value-based instead of type-based

    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`
    quinnj committed Mar 18, 2022
    Configuration menu
    Copy the full SHA
    2f17156 View commit details
    Browse the repository at this point in the history
  2. Configuration menu
    Copy the full SHA
    747f725 View commit details
    Browse the repository at this point in the history
  3. fix tests

    quinnj committed Mar 18, 2022
    Configuration menu
    Copy the full SHA
    ee3aec8 View commit details
    Browse the repository at this point in the history
  4. Configuration menu
    Copy the full SHA
    ad7c1a7 View commit details
    Browse the repository at this point in the history
  5. more work

    quinnj committed Mar 18, 2022
    Configuration menu
    Copy the full SHA
    65188ed View commit details
    Browse the repository at this point in the history
  6. move stuff around

    quinnj committed Mar 18, 2022
    Configuration menu
    Copy the full SHA
    3e7677b View commit details
    Browse the repository at this point in the history
  7. Configuration menu
    Copy the full SHA
    206b847 View commit details
    Browse the repository at this point in the history
  8. work

    quinnj committed Mar 18, 2022
    Configuration menu
    Copy the full SHA
    ede2391 View commit details
    Browse the repository at this point in the history
  9. more work

    quinnj committed Mar 18, 2022
    Configuration menu
    Copy the full SHA
    4e9b8fe View commit details
    Browse the repository at this point in the history
  10. Another refactor to move layers to a more functional architecture and

    drastically reduce the interface requirements
    quinnj committed Mar 18, 2022
    Configuration menu
    Copy the full SHA
    60d4d73 View commit details
    Browse the repository at this point in the history
  11. more cleanup

    quinnj committed Mar 18, 2022
    Configuration menu
    Copy the full SHA
    a9473b4 View commit details
    Browse the repository at this point in the history
  12. fix failing tests

    quinnj committed Mar 18, 2022
    Configuration menu
    Copy the full SHA
    acc0727 View commit details
    Browse the repository at this point in the history
  13. Configuration menu
    Copy the full SHA
    a0e904c View commit details
    Browse the repository at this point in the history
  14. a little more cleanup

    quinnj committed Mar 18, 2022
    Configuration menu
    Copy the full SHA
    9aebfea View commit details
    Browse the repository at this point in the history

Commits on Mar 22, 2022

  1. Update test/loopback.jl

    Co-authored-by: mattBrzezinski <matt.brzezinski@invenia.ca>
    quinnj and mattBrzezinski committed Mar 22, 2022
    Configuration menu
    Copy the full SHA
    131e7e5 View commit details
    Browse the repository at this point in the history
  2. Update test/cookies.jl

    Co-authored-by: mattBrzezinski <matt.brzezinski@invenia.ca>
    quinnj and mattBrzezinski committed Mar 22, 2022
    Configuration menu
    Copy the full SHA
    08e8a6c View commit details
    Browse the repository at this point in the history
  3. Update test/client.jl

    Co-authored-by: mattBrzezinski <matt.brzezinski@invenia.ca>
    quinnj and mattBrzezinski committed Mar 22, 2022
    Configuration menu
    Copy the full SHA
    a317e43 View commit details
    Browse the repository at this point in the history
  4. Update test/chunking.jl

    Co-authored-by: mattBrzezinski <matt.brzezinski@invenia.ca>
    quinnj and mattBrzezinski committed Mar 22, 2022
    Configuration menu
    Copy the full SHA
    45e5a5a View commit details
    Browse the repository at this point in the history
  5. Update src/DefaultHeadersRequest.jl

    Co-authored-by: mattBrzezinski <matt.brzezinski@invenia.ca>
    quinnj and mattBrzezinski committed Mar 22, 2022
    Configuration menu
    Copy the full SHA
    3ea2c8b View commit details
    Browse the repository at this point in the history