-
Notifications
You must be signed in to change notification settings - Fork 4.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
UI: Maintain HTTP Headers from all API requests as JSON-API meta data #4946
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
johncowen
commented
Nov 12, 2018
@@ -0,0 +1,3 @@ | |||
export const HEADERS_SYMBOL = '__consul_ui_http_headers__'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't quite sure where this should live at first, this seems to make most sense though, it's http 'related' and specific to consul
hanshasselberg
approved these changes
Nov 16, 2018
This was referenced Nov 22, 2018
johncowen
added a commit
that referenced
this pull request
Nov 26, 2018
johncowen
added a commit
that referenced
this pull request
Dec 5, 2018
This was referenced Dec 7, 2018
johncowen
added a commit
that referenced
this pull request
Jan 28, 2019
johncowen
added a commit
that referenced
this pull request
Jan 28, 2019
johncowen
added a commit
that referenced
this pull request
Jan 29, 2019
johncowen
added a commit
that referenced
this pull request
Feb 21, 2019
johncowen
added a commit
that referenced
this pull request
Apr 29, 2019
johncowen
added a commit
that referenced
this pull request
May 1, 2019
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I've been mulling over for quite a while about whether to PR this as a separate PR or to do it as part of my upcoming work. The reasons behind this change are tied up with my next piece of feature work, and in order to understand that, there is a reason to say it should be with that work. But this morning I finally decided I had a lot to say about this and additionally it has a large impact on the work I'll be doing when I next do a refactoring series of PR's (not for a while maybe).
Why?
This PR essentially makes the HTTP header information outside of the Adapters/Serializers. I'll be using this information when implementing my blocking query support. I would rather do this outside of Adapters as I see adapters as describing the shape of the API requests (specifically how resource locators are defined) and using a transport to interact with the API (in the case of the ember-data RESTAdapter
jQuery.ajax
). Serializers describe or normalise the shape of the request payload and the response.It could be argued that blocking query support is related to Adapters, and that's fine, but in my case I would definitely like my blocking query functionality to be completely independent of the Adapter, meaning you can switch in and out different Adapters without having to rewrite the blocking query functionality for every type of Adapter. The
ember-data
approach withAdapter
s does exactly this, it abstracts the shape of the locators and the transport away from thestore
meaning you can just usequeryRecord
and things work whether you are using HTTP REST or something else. More than that I would like my blocking query support to be a almost independent ofember-data
as whilst it uses it, I don't think it should depend on it.How
I do this by following
ember-data
conventions as much as possible.ember-data
follows a JSON-API and uses a meta property in various places in the codebase, here's one:https://github.com/emberjs/data/blob/079a28a2d56bfa3fea600cca978fb1d66e5721d3/addon/-private/system/record-arrays/adapter-populated-record-array.js#L83
JSON-API specifies a
meta
property ( https://jsonapi.org/format/#document-meta )that should be at the same JSON level/indentation as thedata
. Consul's blocking queries HTTP response header returns 2 types of information required to implement blocking queries depending on the endpoint:X-Consul-Index
: https://www.consul.io/api/index.html#blocking-queriesX-Consul-ContentHash
: https://www.consul.io/api/index.html#hash-based-blocking-queriesCurrently in the UI we don't use any endpoints that give us hash based blocking queries, but I might soon so I've included this below.
Following
ember-data
practices I want to abstract these header values off from 'http' and serialise them tometa
instead, I also abstract these away fromconsul
specifically by usingindex
anddigest
properties instead of the HTTP specific custom names. I've also includeddate
in here, as it might be useful later to know when the last update of data was, such as for reconciliation of theember-data
store with deleted data in the backend. Right now for later reconciliation I save a list of id's received in the request so I can compare these with what I have in theember-data
store later. I have more ideas on this reconciliation and plan on perf testing all of them at a later date.In order to work with the
ember-data
RESTAdapter
I save the headers into the payload (see below for concerns about this) and pass them through to the serializer, which feels like the best place to further normalise them to what is required.I normalise them with a custom
normalizeMeta
method which I've modelled on otherember-data
functions so I can do this perSerializer
if I ever need to. I was actually surprised thatember-data
didn't have a method like this already. It does have anextractMeta
method, which lets you extract meta data form the payload itself, so that's post serialization - and it doesn't have access to the JSON-APImeta
property nor the HTTP headers themselves.Problems
ember-data
'drops' the HTTP headers very very early:https://github.com/emberjs/data/blob/079a28a2d56bfa3fea600cca978fb1d66e5721d3/addon/adapters/rest.js#L974-L976
..literally just after it has access to the response, and it doesn't do anything with them, or save them anywhere, so the headers/meta information is 'lost' at this point.
The only safe/non-racey way to get these headers through to the serializer is to hang them off the response (the response if the only thing you have access to in the serializer and
handleResponse
). I do this in a relatively safe way by defining a 'symbol-like' property that is highly unlikely to overwrite anything in the original response. I then pick this off the response in the serializer and delete it so its not polluting the actual response when it comes out of the serializer.This works, and is relatively safe, but for me its another 'yuk', and for me this is the final straw with the
RESTAdapter
. TheRESTAdapter
is fine (I assume) if you have a perfectly RESTful JSON-API API, but I don't have that (and I don't think they are that common) - so we are entering 'wrong tool for the job' territory. I am aware and understand why there is generally a preference to useRESTAdapter
, but I would like to investigate writing a less specificHTTPAdapter
that fits a more generic API so I have to jump through less hoops. This is what I refer to above when I mention my next round of refactoring work (at a later date), so I won't go into this in too much detail right now, although I felt it was important to point out the problems encountered here so you can understand my reasoning better when that work comes.Last note here, there are no tests specifically testing this (not yet at least). There is no logic here, only reshaping data and not a lot else to test.
Right, I'm off to figure out how to change my spellchecker so I can write
izer
instead ofiser
😂I'll see if I can add some inline comments also.