Hi Dies: On 11/12/12 08:20, Koper, Dies wrote: > Hi Marios, > > I like your patch but the return code you picked raises questions (seems > to introduce inconsistencies).
thanks for checking that out. > > 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. > Perhaps we need to look at the status codes returned from there - I mean it may be that 405 was not the right code to be returning in those cases - though changing them now may not be so easy as it would endanger backwards compatibility. In '405 Method not allowed' - Method is referring to the HTTP verb [1] ... i.e. GET, POST etc. and not 'restart'/'reboot'. > 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. I looked at wikipedia - though the definition there is just a one line interpretation of the definition in RFC2616. Looking at that [1] I still think 409 is more appropriate here. In '409' - the 'resource inconsistency' ('edit conflict') is given as an example and reading [1] again, it does say "most likely to occur in response to a PUT request ... For example, if versioning were being used and the entity being PUT included changes to a resource which conflict with those made by an earlier (third-party) request,". > 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. API action ('restart/reboot') vs 'GET/POST/PUT' - from [1] I understand that 'method' in 'method not allowed' in about the verb. 409 is not about getting the client to change the request but rather "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". > > 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. again, API actions vs HTTP verb. Not sure how to proceed - do you still think 405 is more appropriate here? I understand your concern that "this is what we return for instance state actions" but *in my opinion* and IF that was an error, we shouldn't repeat it here. On a final note - this is awesome! Having this much scrutiny/interest/discussion about the status code returned for a particular action can only mean good things for this project moving forward! all the best, marios [1] http://www.w3.org/Protocols/rfc2616/rfc2616.txt > > 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 >>>> >>> >>> >> > >
