On 5/07/09 1:57, Jordan Curzon wrote:
> On Jul 4, 2:52 pm, Brice Figureau <[email protected]>
> wrote:
>> The way the format was given to the server was kind of broken when
>> we were saving to the server.
>> It was using the first member of the Accept header.
>> The Accept header is usually reserved for the other side, ie what
>> the client will accept when the server will respond.
>>
>> This patch adds a Content-Type header containing the mime-type of
>> the object sent by the client when saving.
>>
>> Signed-off-by: Brice Figureau <[email protected]>
>> ---
>> lib/puppet/indirector/rest.rb | 2 +-
>> lib/puppet/network/format_handler.rb | 8 +++++++-
>> lib/puppet/network/http/handler.rb | 17 ++++++++++++++---
>> lib/puppet/network/http/mongrel/rest.rb | 4 ++++
>> lib/puppet/network/http/rack/rest.rb | 5 +++++
>> lib/puppet/network/http/webrick/rest.rb | 4 ++++
>> spec/unit/indirector/rest.rb | 9 ++++++++-
>> spec/unit/network/format_handler.rb | 26 ++++++++++++++++++++++++++
>> spec/unit/network/http/handler.rb | 26 ++++++++++++++++++--------
>> spec/unit/network/http/mongrel/rest.rb | 5 +++++
>> spec/unit/network/http/rack/rest.rb | 5 +++++
>> spec/unit/network/http/webrick/rest.rb | 5 +++++
>> 12 files changed, 102 insertions(+), 14 deletions(-)
>>
>> diff --git a/lib/puppet/indirector/rest.rb b/lib/puppet/indirector/rest.rb
>> index 6903f9a..909be9a 100644
>> --- a/lib/puppet/indirector/rest.rb
>> +++ b/lib/puppet/indirector/rest.rb
>> @@ -81,7 +81,7 @@ class Puppet::Indirector::REST <
>> Puppet::Indirector::Terminus
>>
>> def save(request)
>> raise ArgumentError, "PUT does not accept options" unless
>> request.options.empty?
>> - deserialize network(request).put(indirection2uri(request),
>> request.instance.render, headers)
>> + deserialize network(request).put(indirection2uri(request),
>> request.instance.render, headers.merge({ "Content-Type" =>
>> request.instance.mime }))
>> end
>
> I like the idea of using proper mime types in the Content-Type
> headers, Hopefully we can get the server responses to send proper mime
> types too.
It already does, AFAIK.
>> private
>> diff --git a/lib/puppet/network/format_handler.rb
>> b/lib/puppet/network/format_handler.rb
>> index 0a7e9dc..839fe66 100644
>> --- a/lib/puppet/network/format_handler.rb
>> +++ b/lib/puppet/network/format_handler.rb
>> @@ -23,7 +23,7 @@ module Puppet::Network::FormatHandler
>> @format = format
>> end
>>
>> - [:intern, :intern_multiple, :render, :render_multiple].each do
>> |method|
>> + [:intern, :intern_multiple, :render, :render_multiple, :mime].each
>> do |method|
>> define_method(method) do |*args|
>> protect(method, args)
>> end
>> @@ -123,6 +123,12 @@ module Puppet::Network::FormatHandler
>>
>> Puppet::Network::FormatHandler.protected_format(format).render(self)
>> end
>>
>> + def mime(format = nil)
>> + format ||= self.class.default_format
>> +
>> + Puppet::Network::FormatHandler.protected_format(format).mime
>> + end
>> +
>> def support_format?(name)
>> self.class.support_format?(name)
>> end
>> diff --git a/lib/puppet/network/http/handler.rb
>> b/lib/puppet/network/http/handler.rb
>> index c6b809d..0383713 100644
>> --- a/lib/puppet/network/http/handler.rb
>> +++ b/lib/puppet/network/http/handler.rb
>> @@ -17,9 +17,20 @@ module Puppet::Network::HTTP::Handler
>> raise NotImplementedError
>> end
>>
>> - # Which format to use when serializing our response. Just picks
>> - # the first value in the accept header, at this point.
>> + # Retrieve the Content-Type header from the http request.
>> + def content_type_header(request)
>> + raise NotImplementedError
>> + end
>> +
>> + # Which format to use when serializing our response or interpreting the
>> request.
>> + # IF the client provided a Content-Type use this, otherwise use the
>> Accept header
>> + # and just pick the first value.
>> def format_to_use(request)
>> + if header = content_type_header(request)
>> + format = Puppet::Network::FormatHandler.mime(header)
>> + return format.name.to_s if format.suitable?
>> + end
>> +
>
> I'm not sure you should use format_to_use for this purpose because it
> is used to determine what format to use for the response. If a client
> (or a 3rd party app) sends an Accepts header different than the
> Content-Type header and both are suitable?, the client will receive
> content that they didn't say they can accept. I think it should be a
> parallel method to determine the format to use to decode request
> bodies. format_to_use should look at the accept header and the other
> method should look at the Content-Type header.
Yes, you're completely right. I'm cooking a new patch.
The question is what to do when we don't receive a Content-Type header?
For the moment I'm raising an exception, but there might be some other
possibilities:
* use the preferred format for the sent model instance
* try to render with all format and choose the one not giving an
exception?
>> unless header = accept_header(request)
>> raise ArgumentError, "An Accept header must be provided to pick
>> the right format"
>> end
>> @@ -118,7 +129,7 @@ module Puppet::Network::HTTP::Handler
>>
>> format = format_to_use(request)
>>
>> - obj =
>> indirection_request.model.convert_from(format_to_use(request), data)
>> + obj = indirection_request.model.convert_from(format, data)
>> result = save_object(indirection_request, obj)
>>
>> set_content_type(response, "yaml")
>> diff --git a/lib/puppet/network/http/mongrel/rest.rb
>> b/lib/puppet/network/http/mongrel/rest.rb
>> index 369d4c6..6b1fafb 100644
>> --- a/lib/puppet/network/http/mongrel/rest.rb
>> +++ b/lib/puppet/network/http/mongrel/rest.rb
>> @@ -15,6 +15,10 @@ class Puppet::Network::HTTP::MongrelREST <
>> Mongrel::HttpHandler
>> request.params[ACCEPT_HEADER]
>> end
>>
>> + def content_type_header(request)
>> + request.params[Mongrel::Const::CONTENT_TYPE]
>> + end
>> +
>> # which HTTP verb was used in this request
>> def http_method(request)
>> request.params[Mongrel::Const::REQUEST_METHOD]
>> diff --git a/lib/puppet/network/http/rack/rest.rb
>> b/lib/puppet/network/http/rack/rest.rb
>> index e98bffc..a4d475d 100644
>> --- a/lib/puppet/network/http/rack/rest.rb
>> +++ b/lib/puppet/network/http/rack/rest.rb
>> @@ -28,6 +28,11 @@ class Puppet::Network::HTTP::RackREST <
>> Puppet::Network::HTTP::RackHttpHandler
>> request.env[HEADER_ACCEPT]
>> end
>>
>> + # Retrieve the accept header from the http request.
>> + def content_type_header(request)
>> + request.env['HTTP_CONTENT_TYPE']
>> + end
>> +
>> # Return which HTTP verb was used in this request.
>> def http_method(request)
>> request.request_method
>> diff --git a/lib/puppet/network/http/webrick/rest.rb
>> b/lib/puppet/network/http/webrick/rest.rb
>> index 5f77da8..b27b304 100644
>> --- a/lib/puppet/network/http/webrick/rest.rb
>> +++ b/lib/puppet/network/http/webrick/rest.rb
>> @@ -27,6 +27,10 @@ class Puppet::Network::HTTP::WEBrickREST <
>> WEBrick::HTTPServlet::AbstractServlet
>> request["accept"]
>> end
>>
>> + def content_type_header(request)
>> + request["content-type"]
>> + end
>> +
>> def http_method(request)
>> request.request_method
>> end
>> diff --git a/spec/unit/indirector/rest.rb b/spec/unit/indirector/rest.rb
>> index 9f77deb..696af9a 100755
>> --- a/spec/unit/indirector/rest.rb
>> +++ b/spec/unit/indirector/rest.rb
>> @@ -306,7 +306,7 @@ describe Puppet::Indirector::REST do
>> @connection = stub('mock http connection', :put => @response)
>> @searcher.stubs(:network).returns(@connection) # neuter the
>> network connection
>>
>> - @instance = stub 'instance', :render => "mydata"
>> + @instance = stub 'instance', :render => "mydata", :mime =>
>> "mime"
>> @request = Puppet::Indirector::Request.new(:foo, :save, "foo
>> bar")
>> @request.instance = @instance
>> end
>> @@ -350,6 +350,13 @@ describe Puppet::Indirector::REST do
>> @searcher.save(@request)
>> end
>>
>> + it "should provide a Content-Type header containing the mime-type
>> of the sent object" do
>> + @connection.expects(:put).with { |path, data, args|
>> args['Content-Type'] == "mime" }.returns(@response)
>> +
>> + @instance.expects(:mime).returns "mime"
>> + @searcher.save(@request)
>> + end
>> +
>> it "should deserialize and return the network response" do
>> @searcher.expects(:deserialize).with(@response).returns
>> @instance
>> @searcher.save(@request).should equal(@instance)
>> diff --git a/spec/unit/network/format_handler.rb
>> b/spec/unit/network/format_handler.rb
>> index 54409b9..74ee4f5 100755
>> --- a/spec/unit/network/format_handler.rb
>> +++ b/spec/unit/network/format_handler.rb
>> @@ -202,6 +202,10 @@ describe Puppet::Network::FormatHandler do
>> FormatTester.new.should respond_to(:render)
>> end
>>
>> + it "should be able to get a format mime-type" do
>> + FormatTester.new.should respond_to(:mime)
>> + end
>> +
>> it "should raise a FormatError when a rendering error is
>> encountered" do
>> format = stub 'rendering format', :supported? => true
>>
>> Puppet::Network::FormatHandler.stubs(:format).with(:foo).returns format
>> @@ -233,5 +237,27 @@ describe Puppet::Network::FormatHandler do
>> format.expects(:render).with(tester)
>> tester.render
>> end
>> +
>> + it "should call the format-specific converter when asked for the
>> mime-type of a given format" do
>> + format = stub 'rendering format', :supported? => true
>> +
>> +
>> Puppet::Network::FormatHandler.stubs(:format).with(:foo).returns format
>> +
>> + tester = FormatTester.new
>> + format.expects(:mime).returns "foo"
>> +
>> + tester.mime(:foo).should == "foo"
>> + end
>> +
>> + it "should return the default format mime-type if no format is
>> provided" do
>> + format = stub 'rendering format', :supported? => true
>> +
>> Puppet::Network::FormatHandler.stubs(:format).with("foo").returns format
>> +
>> + FormatTester.expects(:default_format).returns "foo"
>> + tester = FormatTester.new
>> +
>> + format.expects(:mime).returns "foo"
>> + tester.mime.should == "foo"
>> + end
>> end
>> end
>> diff --git a/spec/unit/network/http/handler.rb
>> b/spec/unit/network/http/handler.rb
>> index 8ef6e00..97ad2d2 100755
>> --- a/spec/unit/network/http/handler.rb
>> +++ b/spec/unit/network/http/handler.rb
>> @@ -57,13 +57,14 @@ describe Puppet::Network::HTTP::Handler do
>> # Stub out the interface we require our including classes to
>> # implement.
>> def stub_server_interface
>> - @handler.stubs(:accept_header ).returns
>> "format_one,format_two"
>> - @handler.stubs(:set_content_type).returns "my_result"
>> - @handler.stubs(:set_response ).returns "my_result"
>> - @handler.stubs(:path ).returns
>> "/my_handler/my_result"
>> - @handler.stubs(:http_method ).returns("GET")
>> - @handler.stubs(:params ).returns({})
>> - @handler.stubs(:content_type
>> ...
>>
--
Brice Figureau
My Blog: http://www.masterzen.fr/
--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Puppet Developers" group.
To post to this group, send email to [email protected]
To unsubscribe from this group, send email to
[email protected]
For more options, visit this group at
http://groups.google.com/group/puppet-dev?hl=en
-~----------~----~----~----~------~----~------~--~---
