Re: REST: ETag handling

Hi Ralf:

In message <[email protected]>,
Ralf Schlatterbeck writes:
>On Fri, Mar 22, 2019 at 03:05:20PM -0400, John P. Rouillard wrote:
>> >will fail because the resulting value includes quotes. The @etag value
>> >in the json representation does *not* include quotes.
>> 
>> Maybe it should be: { "@etag": ""etagvalue"", ... } I am not sure how to
>> encode " inside a json value but you get the idea.
>I'd leave the encoding to the json serializer. So just add the quotes to
>the string like you do in the header.

Fair enough.

I would strip quotes everywhere they occur so we should never have
quotes in any variable/tuple etc. that hold an etag value.

Originally the reason I put the @etag value in the json body was that
the json body could be reused as the payload for a put request.

I provided two ways of performing etag/If-Match checks.

 1) the @etag parameter in the json payload.
 2) using Etag from the http header in the http header of the put command.
    (Yes, it should be If-Match: "etag value" in the header for a
     PUT/DELETE/PATCH request, not etag. This was a thinko from handling
     the @etag parameter in json).

Both can be used and both must match if they are both used. But they
are separate mechanisms and shouldn't be mixed.

>> >I think the quotes should be removed in the header, too.
>> 
>> I thought the quotes in the header were required by spec.
>> See: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/ETag
>
>Thanks.
>This also says that in the request we should send an 'If-Match' header
>not an ETag.

Quite right. Thinko on my part.

>> >Another question regarding ETag: The 'PUT' method is supposed to be
>> >idempotent. Requiring an ETag kills this property: A second equivalent
>> >PUT request where the change is already applied will fail because the
>> >ETag doesn't match. Shouldn't we optionally allow a PUT without an ETag
>> >and document that using an ETag will make the method non-idempotent?
>> 
>> Well we can't remove etag as that is required to avoid clobbering an
>> update.  Consider the following:
>> 
>>   app1 gets issue/27
>>   app2 gets issue/27
>>   app2 changes status to open from new
>>   app2 puts change
>>   app1 changes status to close from new
>>   app1 puts change
>
>I know and understand this. But sometimes a client may want to make sure
>it got the last update and wins. So sending the If-Match header should
>probably made optional.

If so then it should re-request the data to get the new etag and
submit a new PUT request.

I can see the possible need for this "I will always win"
mechanism. Maybe that's what a POST to a /rest/data/{class}/{id} or
/rest/data/{class}/{id}/{property} url should do? It looks like POST
is not used for those URL's currently.

>> Regarding idempotentcy. I read that as: if the preconditions of the put
>> are met, multiple applications of the same PUT data should not change
>> the state of the resource.
>
>Wikipedia say otherwise: "whereby they can be applied multiple times
>without changing the result beyond the initial application."
>https://en.wikipedia.org/wiki/Idempotence
>This doesn't apply in this case because after first applying it once the
>ETag will change because the PUT modified the data.
>
>> Does this make sense?

I see what you are saying it just sounds wrong.

https://evertpot.com/idempotence-in-http/

states:

   Idempotent methods are distinguished because the request can be
   repeated automatically if a communication failure occurs before the
   client is able to read the server’s response. For example, if a
   client sends a PUT request and the underlying connection is closed
   before any response is received, then it can establish a new
   connection and retry the idempotent request because it knows that
   repeating the request will have the same effect even if the
   original request succeeded.

In this case, if the value of the resource on the server was unchanged
(i.e. the PUT request was not received/completed), the second PUT
works with code 200. If the first was received, the second PUT has no
effect keeping the result of the original change.

Compare that to POST, where there will be a new resource created on
the second request if the first succeeded.

I think we are allowed to do this as well:

  make the change implied by the PUT.

  if the new state of the resource is the same as the state before the PUT,
    return 200 with an empty attributes object.

  if the new state is different from the (original) state before the
    PUT, check the etag. If the etag doesn't match the original state,
    we have a lost update. Return code 412.

This would be ideal as a re-apply would return a 200 code (although
without the attribute payload that would have returned for the first
PUT). This makes it easier to determine that the state on the server
is what the client requested.

To check server state when you get a 412, you need to do a GET.
Compare your values to the values in the get. Determine if you need to
send another PUT to update the representation.

Also
https://stackoverflow.com/questions/52964467/how-put-is-idempotent has
a different quote from
http://www.w3.org/Protocols/rfc2616/rfc2616-sec9.html:

  Methods can also have the property of "idempotence" in that
  (aside from error or expiration issues) the side-effects of
  N > 0 identical requests is the same as for a single request.

I claim the Etag/If-Match allows for "expiration" issues.

>I'd change it as follows:
>- Change incoming check for ETag to check for If-Match

Agreed.

>- Check that the incoming If-Match matches the outgoing ETag *WITH
>  QUOTES*: The quotes should be there in both cases. So I can still use
>  my naive client implementation as the If-Match header also includes
>  quotes.

I would strip quotes everywhere internally. I am surprised that
header.get() doesn't strip the quotes but...

>  So I'd change the etag computation function to include the quotes and
>  not apply the quotes in several cases and not in others. Point to the
>  spec in the documentation of the ETag computation.

I would apply the quotes at the use point of the etag. Keep the etag
as the MD5 string. The quotes are not part of the etag.

>- Make the check optional, so if no If-Match header is found apply the
>  change unconditionally.

I claim POST should be used for this case.

>- Point to this in the documentation and suggest an ETag is always used.

I would say require etag use. I think the lost update issue is more of
a problem (even though we do have the journal) than the possibility of
a change to a resource when the change is occurring at the speed of an
API (as opposed to editing a web form).
 
>But I'm not religious about that, so if you think that an If-Match
>header should always be present, I'm fine.

Yeah, that's my feeling. I was going to say this matches the web form
and the @lastactivity field. However, it appears that removing that
field is supported and in that case, we have a lost update issue 8-/.
 
>I can provide a patch if you like.

That would be great. I think I finally chased down the last of the
crashes with rest in python 3, but I am still testing manually.

Have a great weekend.

--
				-- rouilj
John Rouillard
===========================================================================
My employers don't acknowledge my existence much less my opinions.


_______________________________________________
Roundup-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/roundup-devel