Update ETag quote logic and check headers passed in

bbc / alephant-broker

Brokers requests for alephant components

MIT License

4 stars 3 forks source link

Update ETag quote logic and check headers passed in #67

Closed samfrench closed 8 years ago

samfrench commented 8 years ago

Update ETag quote logic and check headers passed in

Single components were returning an ETag quoted. Batch responses were unquoted.
This quotes all ETag responses and unquotes them when checking if they are equal
This checks headers passed in to see if needing a modified response (as a 200 would be returned otherwise (or 404, 5xx if needed)
This also checks for nil before checking if they are equal, just to only serve a 304 if not nil and they are equal

https://jira.dev.bbc.co.uk/browse/CONNPOL-3135

samfrench commented 8 years ago

@dazoakley, I updated the logic in here (detail in PR description) and also check for headers passed in, and not component headers with last modified logic.

Points to note, my quoting and unquoting of ETags could be done in a nicer way (if you know how). I now allow for them to be either quoted or unquoted.

Also I check if they are nil before comparing the values. Quite a long line of logic, but don't know if it could look neater.

I have this running on our instance, so confident this works for all requests, and in all cases.

samfrench commented 8 years ago

@dazoakley, when we get a 304 response, there is no access control header. Do you think there is a way we can force that header to be present on the response? jQuery/browser has an error as all other responses have this header, so it stops polling. I'll look more into the jQuery options, but it would be easier if we could force that header.

samfrench commented 8 years ago

We need to decide if to remove the 304 status code, but serve a 200 instead, or if to remove the 304 status code and serve something else, (maybe a 202).

After this, then we should have our modified header logic complete.

samfrench commented 8 years ago

@dazoakley, I have made all of your suggested changes. I have swapped the 304 to a 202, and proved this works with the JS. There will be a JS PR tomorrow to cover this, once tidied up the code a bit.

I assume you'll have a suggestion for my static methods as the line lengths are quite lengthy. I look forward to the feedback from another review of this PR.

samfrench commented 8 years ago

@dazoakley, I made your suggested changes.

dazoakley commented 8 years ago

Looks good to go. :+1: