zooniverse / json-api-client

Apache License 2.0
10 stars 5 forks source link

Remove unnecessary header on update #32

Closed itsravenous closed 8 years ago

itsravenous commented 8 years ago

When saving a record back to the API, the client adds two headers with values from when the subject was fetched: If-Match (using the ETag header value) and If-Unmodified-Since (using the Last-Modified header value). Panoptes doesn't send a Last-Modified header with subjects, so updating records will always fail (node catches the undefined set for If-Unmodified-Since and throws). I've therefore removed the If-Unmodified-Since header. It seems redundant if we're checking the ETag with If-Match anyway.

brian-c commented 8 years ago

node catches the undefined set for If-Unmodified-Since and throws

What's it throw? I don't think that should result in an error.

@camallen This looks fine to me, is the back end okay with just If-Match?

camallen commented 8 years ago

If we can keep it that'll be useful we use it on GET req's in one spot to test the user resource is stale?, we can do this more if we have the header (that stale method sets it in rails, the rest don't set that header).

All updates use the If-Match Etag headers exclusively. Can it just not throw the error and only set the header if it's been set in the original response?

itsravenous commented 8 years ago

On the move atm but it basically complains (somewhere in OutgoingRequest) that you can't pass undefined as a header value, which is fair enough.

itsravenous commented 8 years ago

@camallen to clarify, it's a built in node function that throws, not json-api-client. As you say, probably better instead of removing the header altogether, only set it if the resource does have a Last-Modified value.

camallen commented 8 years ago

that sounds best, that way we can still use this conditional get if we want to.

brian-c commented 8 years ago

@itsravenous Mind switching that to

getHeadersForModification: ->
  headers =
    'If-Unmodified-Since': @_getHeader 'Last-Modified'
    'If-Match': @_getHeader 'ETag'
  for header, value of headers
    unless value?
      delete headers[header]
  headers
itsravenous commented 8 years ago

Done :)

saschaishikawa commented 8 years ago

Are we good to merge this in?

brian-c commented 8 years ago

Published v3.2.1