On Jul 5, 2009, at 3:47 AM, Brice Figureau wrote:
>
> 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?
If we're expecting it to be encoded and no content-type header is
provided, that's an exception IMO.
>
>>> 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/
>
>
> >
--
No one who cannot rejoice in the discovery of his own mistakes
deserves to be called a scholar. --Donald Foster
---------------------------------------------------------------------
Luke Kanies | http://reductivelabs.com | http://madstop.com
--~--~---------~--~----~------------~-------~--~----~
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
-~----------~----~----~----~------~----~------~--~---
