On Mon, 2013-02-11 at 13:35 +0100, mfoj...@redhat.com wrote:
> From: Michal Fojtik <mfoj...@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 <mfoj...@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
