deltacloud-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From David Lutterkort <lut...@redhat.com>
Subject Re: [PATCH core 10/11] CIMI: Replaced :grab_content_type helper with :current_content_type
Date Thu, 21 Feb 2013 23:47:25 GMT
On Mon, 2013-02-11 at 13:35 +0100, mfojtik@redhat.com wrote:
> From: Michal Fojtik <mfojtik@redhat.com>
> 
> In Sinatra helpers have direct access to the 'request' method,
> so there is no need to pass any parameter to this helper.
> 
> This patch will also report unsupported media type correctly
> via Deltacloud exception handler with proper HTTP status code.

ACK. Nice; one nit:

> Signed-off-by: Michal fojtik <mfojtik@redhat.com>
> ---
>  server/lib/cimi/collections/address_templates.rb   |  2 +-
>  server/lib/cimi/collections/addresses.rb           |  2 +-
>  server/lib/cimi/collections/credentials.rb         |  2 +-
>  server/lib/cimi/collections/machine_templates.rb   |  7 +---
>  server/lib/cimi/collections/machines.rb            |  8 ++--
>  server/lib/cimi/collections/network_ports.rb       |  6 +--
>  server/lib/cimi/collections/networks.rb            |  8 ++--
>  .../lib/cimi/collections/volume_configurations.rb  |  2 +-
>  server/lib/cimi/collections/volume_templates.rb    |  5 ++-
>  server/lib/cimi/collections/volumes.rb             |  3 +-
>  server/lib/cimi/helpers/cimi_helper.rb             | 45 ++++++----------------
>  server/lib/cimi/models/machine_image.rb            |  2 +-
>  server/lib/cimi/models/volume_image.rb             |  2 +-
>  13 files changed, 35 insertions(+), 59 deletions(-)
> 
> diff --git a/server/lib/cimi/collections/address_templates.rb b/server/lib/cimi/collections/address_templates.rb
> index fea3ca1..d1bbe7d 100644
> --- a/server/lib/cimi/collections/address_templates.rb
> +++ b/server/lib/cimi/collections/address_templates.rb
> @@ -45,7 +45,7 @@ module CIMI::Collections
>        operation :create do
>          description "Create new AddressTemplate"
>          control do
> -          if grab_content_type(request.content_type, request.body) == :json
> +          if current_content_type == :json
>              new_address_template = CIMI::Model::AddressTemplate.create_from_json(request.body.read,
self)
>            else
>              new_address_template = CIMI::Model::AddressTemplate.create_from_xml(request.body.read,
self)
> diff --git a/server/lib/cimi/collections/addresses.rb b/server/lib/cimi/collections/addresses.rb
> index 9ea1764..5133894 100644
> --- a/server/lib/cimi/collections/addresses.rb
> +++ b/server/lib/cimi/collections/addresses.rb
> @@ -47,7 +47,7 @@ module CIMI::Collections
>        operation :create, :with_capability => :create_address do
>          description "Create a new Address"
>          control do
> -          if grab_content_type(request.content_type, request.body) == :json
> +          if current_content_type == :json
>              address = CIMI::Model::Address.create(request.body.read, self, :json)
>            else
>              address = CIMI::Model::Address.create(request.body.read, self, :xml)
> diff --git a/server/lib/cimi/collections/credentials.rb b/server/lib/cimi/collections/credentials.rb
> index dd0fbda..fcaf458 100644
> --- a/server/lib/cimi/collections/credentials.rb
> +++ b/server/lib/cimi/collections/credentials.rb
> @@ -46,7 +46,7 @@ module CIMI::Collections
>        operation :create, :with_capability => :create_key do
>          description "Show specific machine admin"
>          control do
> -          if grab_content_type(request.content_type, request.body) == :json
> +          if current_content_type == :json
>              new_admin = Credential.create_from_json(request.body.read, self)
>            else
>              new_admin = Credential.create_from_xml(request.body.read, self)
> diff --git a/server/lib/cimi/collections/machine_templates.rb b/server/lib/cimi/collections/machine_templates.rb
> index 99ce5cf..8734319 100644
> --- a/server/lib/cimi/collections/machine_templates.rb
> +++ b/server/lib/cimi/collections/machine_templates.rb
> @@ -45,11 +45,8 @@ module CIMI::Collections
>        operation :create do
>          description "Create new machine template"
>          control do
> -          if grab_content_type(request.content_type, request.body) == :json
> -            new_machine_template = CIMI::Model::MachineTemplate.create_from_json(request.body.read,
self)
> -          else
> -            new_machine_template = CIMI::Model::MachineTemplate.create_from_xml(request.body.read,
self)
> -          end
> +          mt = MachineTemplateCreate.parse(request.body, request.content_type)
> +          new_machine_template = mt.create(self)
>            headers_for_create new_machine_template
>            respond_to do |format|
>              format.json { new_machine_template.to_json }
> diff --git a/server/lib/cimi/collections/machines.rb b/server/lib/cimi/collections/machines.rb
> index 88b3c99..e3bada1 100644
> --- a/server/lib/cimi/collections/machines.rb
> +++ b/server/lib/cimi/collections/machines.rb
> @@ -69,7 +69,7 @@ module CIMI::Collections
>          param :id,          :string,    :required
>          control do
>            machine = Machine.find(params[:id], self)
> -          if grab_content_type(request.content_type, request.body) == :json
> +          if current_content_type == :json
>              action = Action.from_json(request.body.read)
>            else
>              action = Action.from_xml(request.body.read)
> @@ -86,7 +86,7 @@ module CIMI::Collections
>          param :id,          :string,    :required
>          control do
>            machine = Machine.find(params[:id], self)
> -          if  grab_content_type(request.content_type, request.body) == :json
> +          if current_content_type == :json
>              action = Action.from_json(request.body.read.gsub("restart", "reboot"))
>            else
>              action = Action.from_xml(request.body.read.gsub("restart", "reboot"))
> @@ -103,7 +103,7 @@ module CIMI::Collections
>          param :id,          :string,    :required
>          control do
>            machine = Machine.find(params[:id], self)
> -          if  grab_content_type(request.content_type, request.body) == :json
> +          if current_content_type == :json
>              action = Action.from_json(request.body.read)
>            else
>              action = Action.from_xml(request.body.read)
> @@ -171,7 +171,7 @@ module CIMI::Collections
>          description "Attach CIMI Volume(s) to a machine."
>          param :id,          :string,    :required
>          control do
> -          if  grab_content_type(request.content_type, request.body) == :json
> +          if current_content_type == :json
>              volume_to_attach, location = MachineVolume.find_to_attach_from_json(request.body.read,
self)
>            else
>              volume_to_attach, location = MachineVolume.find_to_attach_from_xml(request.body.read,
self)
> diff --git a/server/lib/cimi/collections/network_ports.rb b/server/lib/cimi/collections/network_ports.rb
> index ee7b83a..28c2df4 100644
> --- a/server/lib/cimi/collections/network_ports.rb
> +++ b/server/lib/cimi/collections/network_ports.rb
> @@ -47,7 +47,7 @@ module CIMI::Collections
>        operation :create, :with_capability => :create_network_port do
>          description "Create a new NetworkPort"
>          control do
> -          if grab_content_type(request.content_type, request.body) == :json
> +          if current_content_type == :json
>              network_port = CIMI::Model::NetworkPort.create(request.body.read, self,
:json)
>            else
>              network_port = CIMI::Model::NetworkPort.create(request.body.read, self,
:xml)
> @@ -73,7 +73,7 @@ module CIMI::Collections
>          control do
>            network_port = NetworkPort.find(params[:id], self)
>            report_error(404) unless network_port
> -          if grab_content_type(request.content_type, request.body) == :json
> +          if current_content_type == :json
>              action = Action.from_json(request.body.read)
>            else
>              action = Action.from_xml(request.body.read)
> @@ -91,7 +91,7 @@ module CIMI::Collections
>          control do
>            network_port = NetworkPort.find(params[:id], self)
>            report_error(404) unless network_port
> -          if grab_content_type(request.content_type, request.body) == :json
> +          if current_content_type == :json
>              action = Action.from_json(request.body.read)
>            else
>              action = Action.from_xml(request.body.read)
> diff --git a/server/lib/cimi/collections/networks.rb b/server/lib/cimi/collections/networks.rb
> index 377a868..16e4c9c 100644
> --- a/server/lib/cimi/collections/networks.rb
> +++ b/server/lib/cimi/collections/networks.rb
> @@ -46,7 +46,7 @@ module CIMI::Collections
>        operation :create, :with_capability => :create_network do
>          description "Create a new Network"
>          control do
> -          if grab_content_type(request.content_type, request.body) == :json
> +          if current_content_type == :json
>              network = Network.create(request.body.read, self, :json)
>            else
>              network = Network.create(request.body.read, self, :xml)
> @@ -72,7 +72,7 @@ module CIMI::Collections
>          control do
>            network = Network.find(params[:id], self)
>            report_error(404) unless network
> -          if grab_content_type(request.content_type, request.body) == :json
> +          if current_content_type == :json
>              action = Action.from_json(request.body.read)
>            else
>              action = Action.from_xml(request.body.read)
> @@ -90,7 +90,7 @@ module CIMI::Collections
>          control do
>            network = Network.find(params[:id], self)
>            report_error(404) unless network
> -          if grab_content_type(request.content_type, request.body) == :json
> +          if current_content_type == :json
>              action = Action.from_json(request.body.read)
>            else
>              action = Action.from_xml(request.body.read)
> @@ -108,7 +108,7 @@ module CIMI::Collections
>          control do
>            network = Network.find(params[:id], self)
>            report_error(404) unless network
> -          if grab_content_type(request.content_type, request.body) == :json
> +          if current_content_type == :json
>              action = Action.from_json(request.body.read)
>            else
>              action = Action.from_xml(request.body.read)
> diff --git a/server/lib/cimi/collections/volume_configurations.rb b/server/lib/cimi/collections/volume_configurations.rb
> index 6a03dc5..0a33794 100644
> --- a/server/lib/cimi/collections/volume_configurations.rb
> +++ b/server/lib/cimi/collections/volume_configurations.rb
> @@ -45,7 +45,7 @@ module CIMI::Collections
>        operation :create, :with_capability => :create_storage_volume do
>          description "Create new VolumeConfiguration"
>          control do
> -          if grab_content_type(request.content_type, request.body) == :json
> +          if current_content_type == :json
>              new_config = CIMI::Model::VolumeConfiguration.create_from_json(request.body.read,
self)
>            else
>              new_config = CIMI::Model::VolumeConfiguration.create_from_xml(request.body.read,
self)
> diff --git a/server/lib/cimi/collections/volume_templates.rb b/server/lib/cimi/collections/volume_templates.rb
> index 4e3dfdb..720e73b 100644
> --- a/server/lib/cimi/collections/volume_templates.rb
> +++ b/server/lib/cimi/collections/volume_templates.rb
> @@ -45,8 +45,9 @@ module CIMI::Collections
>        operation :create, :with_capability => :create_storage_volume do
>          description "Create new VolumeTemplate"
>          control do
> -          content_type = grab_content_type(request.content_type, request.body)
> -          new_template = CIMI::Model::VolumeTemplate.create(request.body.read, self,
content_type)
> +          new_template = CIMI::Model::VolumeTemplate.create(
> +            request.body.read, self, current_content_type
> +          )
>            headers_for_create new_template
>            respond_to do |format|
>              format.json { new_template.to_json }
> diff --git a/server/lib/cimi/collections/volumes.rb b/server/lib/cimi/collections/volumes.rb
> index bf1f5f0..74f8c69 100644
> --- a/server/lib/cimi/collections/volumes.rb
> +++ b/server/lib/cimi/collections/volumes.rb
> @@ -49,8 +49,7 @@ module CIMI::Collections
>        operation :create, :with_capability => :create_storage_volume do
>          description "Create a new Volume."
>          control do
> -          content_type = grab_content_type(request.content_type, request.body)
> -          new_volume = Volume.create(request.body.read, self, content_type)
> +          new_volume = Volume.create(request.body.read, self, current_content_type)
>            headers_for_create new_volume
>            respond_to do |format|
>              format.json { new_volume.to_json }
> diff --git a/server/lib/cimi/helpers/cimi_helper.rb b/server/lib/cimi/helpers/cimi_helper.rb
> index b062af7..5e74c2c 100644
> --- a/server/lib/cimi/helpers/cimi_helper.rb
> +++ b/server/lib/cimi/helpers/cimi_helper.rb
> @@ -16,6 +16,18 @@
>  module CIMI
>    module Helper
>  
> +    def current_content_type
> +      case request.content_type
> +        when 'application/json' then :json
> +        when 'text/xml', 'application/xml' then :xml
> +        else
> +          raise Deltacloud::Exceptions.exception_from_status(
> +            406,
> +            translate_error_code(406)[:message]
> +          )

Doesn't report_error do the translate_error_code automatically, i.e.,
wouldn't it be enough to 

        raise Deltacloud::Exceptions.exception_from_status(406)

David



Mime
View raw message