Skip to content
This repository

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Rack::Client::Parser's not working as expected #8

Closed
wants to merge 3 commits into from

3 participants

Ben Schwarz Ben Burkert Tim Carey-Smith
Ben Schwarz

I couldn't get it to work, so I thought I'd write a spec that validated that fact.

Hopefully this helps you either correct the operation or you can let me know if I've misinterpreted how this should exactly work.

Either way, I figure a pull request is a good start.

Ben Schwarz

Through reading all of the code and writing further tests, I discovered that the body is parsed, but it is kept on a header called rack-client.body_collection. Its stored in an array (I assume this is because responses could be chunked? Or Multiget?)

Either way, were you planning on writing this back to the body accessor? Or to a special parsed_body method?

Ben Burkert
Collaborator

The parser code was a spike that never really got finished, and probably shouldn't have been merged into master. It's no where near ready for use. I have been manually writing middleware's for dumping & loading json requests & responses.

The reason for the 'rack-client.body_collection' header was to keep the parser middleware compliant with the rack spec, which states that "The Body must respond to each and must only yield String values". The idea was for the Base adapter to follow the rack spec to the letter, but also allow for more adapters which implemented extensions to the rack spec. For example, the Simple adapter could be updated to look for the 'rack-client.body_collection' header when each is called on the body, similar to the way the collapsed body works.

Ben Schwarz

I'm cool to implement something because I actually want to use this functionality—

If you wrote some failing tests I'd be happy to implement it from there…

Ben Burkert
Collaborator

awesome.

I'll try adding some tests this weekend, although i don't usually write tests up front. I might end up doing a reference implementation for YAML or something.

Ben Schwarz

Thinking about this recently—

I think that the parsers (if included within your "stack") should alter the "body" of the response, here is the rationale:

  • These parsers are not themselves rack middlewares, they cannot be used outside the context of rack-client. (It simply does not make sense)
  • If a response yields a Content-Type of application/json, I'd expect it to become a native ruby hash after being run through the parser. It feels as a logical step from including it within my "http client stack".

Now—Having said that, I think that including it along side the Rack middlewares feels strange. I'm not sure if the syntax should be altered to include something like this, but I'd be interested to hear how halorgium feels about these thoughts.

Tim Carey-Smith
Owner

I believe this parser should be a middleware even though it is potentially meant to be returning something not like a Rack response.

The thing about a decorator is that the post-conditions must not be weakened.
So making the Parser.new(Client.new).call(request) now return a non-Rack response is not valid.
The reason for this is that it makes this client impossible to be extended further.
This does raise the question of whether this is part of the default client stack or as a final post-process.

Using the callbacks for capturing the output of the parser does seem to be sensible though.
I have been unsure how to sanely do things like have a middleware which converts the hash which is from the JSON into some Domain models.

Hope this provides some perspective of what I've been thinking.

Ben Schwarz

Thanks for your perspective Tim.

I think you're spot on with your comments about weakening the chain—This is why I think that a parser (which wouldn't/couldn't be used as a regular rack-middleware) should be a presenter that is either persistant in rack-client, or perhaps even explicitly required. (Change the way that we interface with it, as for a design, I'm not so sure…)

Being the way that this discussion has played out, I'm not certain who will be making the next strike here…

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
3  lib/rack/client/parser/json.rb
@@ -6,6 +6,7 @@ module Parser
6 6 class JSON < Parser::Base
7 7
8 8 content_type 'application', 'json'
  9 + content_type 'application', 'json', 'charset=utf-8'
9 10
10 11 def encode(input)
11 12 output = StringIO.new
@@ -48,7 +49,7 @@ def decode(body)
48 49 end
49 50 end
50 51
51   - Yaml = YAML
  52 + Json = JSON
52 53 end
53 54 end
54 55 end
18 spec/parser/json_spec.rb
... ... @@ -0,0 +1,18 @@
  1 +require 'spec_helper'
  2 +
  3 +describe Rack::Client::Parser::JSON do
  4 + it "should be returned for application/json" do
  5 + Rack::Client::Parser::Base.lookup("application/json").should == Rack::Client::Parser::JSON
  6 + end
  7 +
  8 + it "should be returned for application/json;charset=utf-8" do
  9 + Rack::Client::Parser::Base.lookup("application/json;charset=utf-8").should == Rack::Client::Parser::JSON
  10 + end
  11 +
  12 + sync_handler_contexts(Rack::Client::Parser) do
  13 + it 'parses json and makes it ruby' do
  14 + request { get('/json/json') }
  15 + response { body.should == {"key" => "value"} }
  16 + end
  17 + end
  18 +end
10 spec/spec_apps/json.ru
... ... @@ -0,0 +1,10 @@
  1 +require 'sinatra/base'
  2 +
  3 +class Json < Sinatra::Base
  4 + get '/json' do
  5 + content_type :json
  6 + '{"key":"value"}'
  7 + end
  8 +end
  9 +
  10 +run Json

Tip: You can add notes to lines in a file. Hover to the left of a line to make a note

Something went wrong with that request. Please try again.