Hi Marios, I like your patch but the return code you picked raises questions (seems to introduce inconsistencies).
We used (still do?) return a 405 from the Deltacloud API when trying to start an instance that is already started, stopping an instance that is already stopped, etc., according to: https://issues.apache.org/jira/browse/DTACLOUD-214 I don't understand how this case is any different. I don't know the background of returning 405 for the case above, and the cases the spec writers envisioned. I read the 409 description (also due to Wikipedia's interpretation) to mean a case where the action would put the resource in an inconsistent state, and the client can change their request and retry to prevent the inconsistency. In our case the action is not possible and applicable at all to the resource at the moment, and it requires more than changing the request to reach the intended outcome. In cimi (as in DC), we return actions that are allowed on resources. It is my understanding (from a discussion I had with Doug last week) that actions that are not allowed because the user doesn't have privileges or because the resource is not in the right state, are not included. In that case, it makes sense to me to return "405 Method Not Allowed" to an action not included in that list. So I find the introduction of 409 for this particular case look inconsistent with what we do in other similar cases. Regards, Dies Koper > -----Original Message----- > From: [email protected] [mailto:[email protected]] > Sent: Monday, 10 December 2012 11:51 PM > To: [email protected] > Cc: Koper, Dies; Joe Vlcek > Subject: Re: [PATCH] DTACLOUD-379 catch and handle in haml > > Hi Dies - as always thanks for your input here - more inline: > > On 08/12/12 05:49, Koper, Dies wrote: > > Nack. > > I don't think it's a good idea to start putting messages specific to one > > provider in the common haml files. > > If we start doing that with all messages from all providers, it's going > > to be a big mess. > > > > yes indeed - and I have to claim ownership of this great idea :/ - Joe's > initial solution was to raise a new 5XX error and then catch this so it > silences the error logging. I didn't want to 'mess around' too much with > the error handling code, hence my 'do it in haml' suggestion. Looking at > it, you are right, its not the way to go. > > > Maybe the driver can raise a particular class of (common) error for > > which the backtrace is not logged? > > > > Before that, I don't think the driver should be raising a 5xx for this > > error. 5xx's are for problems with the server. They're of the type that > > you would contact your system administrator for to fix the server. > > Trying to delete a template that is in use, sounds like a user error; > > "412 Precondition Failed" or "405 Method Not Allowed" seems more > > appropriate. > > Indeed. I spent some time looking at the status code definitions [1]. I > think 405 or 412 could both be used without too much drama, but since > we're being specific about it... 405 says 'method not allowed' which imo > isn't accurate since it *is* allowed, but not for current resource > state. For 412, we don't user preconditions in the request headers here > (or at all afaik). I think perhaps 409 is a good fit: > > "The request could not be completed due to a conflict with the current > state of the resource. This code is only allowed in situations where it > is expected that the user might be able to resolve the conflict and > resubmit the request" ... > > I have a patch at http://tracker.deltacloud.org/set/193 - Joe will be > testing that and then update the JIRA ticket accordingly, > > thanks, marios > > [1] http://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html > > > > > > Regards, > > Dies Koper > > > >> -----Original Message----- > >> From: [email protected] [mailto:[email protected]] > >> Sent: Saturday, 8 December 2012 4:10 AM > >> To: [email protected] > >> Subject: [PATCH] DTACLOUD-379 catch and handle in haml > >> > >> From: Joe VLcek <[email protected]> > >> > >> --- > >> server/views/errors/500.html.haml | 3 ++- > >> server/views/errors/500.xml.haml | 3 ++- > >> 2 files changed, 4 insertions(+), 2 deletions(-) > >> > >> diff --git a/server/views/errors/500.html.haml > >> b/server/views/errors/500.html.haml > >> index 8cd3c74..0fa4563 100644 > >> --- a/server/views/errors/500.html.haml > >> +++ b/server/views/errors/500.html.haml > >> @@ -16,7 +16,8 @@ > >> %em No details > >> %li{ :'data-role' => 'list-divider'} Backtrace > >> %li > >> - %pre= bt @error.backtrace > >> + - unless @error.message.downcase.gsub(/\s+/," > > ").include?('cannot > >> delete template. template is being used') > >> + %pre= bt @error.backtrace > >> > >> - if @error.backtrace > >> %div{ 'data-role' => :collapsible, 'data-collapsed' => "true"} > >> diff --git a/server/views/errors/500.xml.haml > >> b/server/views/errors/500.xml.haml > >> index b3e71e2..11aa4e1 100644 > >> --- a/server/views/errors/500.xml.haml > >> +++ b/server/views/errors/500.xml.haml > >> @@ -6,7 +6,8 @@ > >> %code=response.status > >> %message< #{cdata @error.message} > >> - if @error.respond_to?(:backtrace) and [email protected]? > >> - %backtrace=cdata(@error.backtrace.join("\n")) > >> + - unless @error.message.downcase.gsub(/\s+/," ").include?('cannot > >> delete template. template is being used') > >> + %backtrace=cdata(@error.backtrace.join("\n")) > >> - if params > >> %request > >> - params.each do |k, v| > >> -- > >> 1.7.11.7 > >> > > > > >
