Skip to content
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: Change signature of request/respond methods to take serialized and unserialized data #6285

Merged
merged 8 commits into from
Aug 23, 2019

Conversation

johncowen
Copy link
Contributor

@johncowen johncowen commented Aug 7, 2019

This is part of #5637

We'd like to slightly tweak the API so that unserialized and unserialized data is available to the requestFor... methods, currently only unserialized is.

Both requestFor and respondFor methods now take both serialized data
and unserialized data.

We also leave it up to the adapter author to add (or omit) the serialized
data from the request payload, instead of it being automatically added
within the http client.

Included here is a small refactor to dry out the http adapter slightly.

Tests where changed only slightly to follow the new method signature.

As a side note here we are very tempted to write the requests like:

// https://www.consul.io/api/acl/legacy.html#update-acl-token
return request`
  PUT /v1/acl/update?${{ [API_DATACENTER_KEY]: data[DATACENTER_KEY] }}

  ${
    {
      ID: serialized.SecretID,
      Name: serialized.Description,
      Type: serialized.Type,
      Rules: serialized.Rules
    }
   }
`;

instead of:

// https://www.consul.io/api/acl/legacy.html#update-acl-token
return request`
  PUT /v1/acl/update?${{ [API_DATACENTER_KEY]: data[DATACENTER_KEY] }}

  ${serialized}
`;

and then the 'serialization' happening elsewhere in the app (in the Serializer).

Doing the former in our mind make every request easier to read, its easier to see what is happening in one place, and it also looks exactly like the documentation https://www.consul.io/api/acl/legacy.html#update-acl-token

Right now, as part of this refactor is about moving serialization to the app Serializers where folks expect them, we've left it as the latter. But we are considering (in the more distant future), moving everything to the former example. If data values need transforming (as is the case with KV values and base64-ness), that would be done in the Serializers still, although the final structure of the JSON/request payload would be done in the Adapter. Which, actually as I write this, feels like it actually makes more sense, with the idea that the request payload should be structured like so to be done in the Adapter layer.

For now this will be left as is, the above is more to communicate future ideas more than anything.

Lastly, again this is going down onto out http-adapter refactor branch, not master or ui-staging - there is still some decisions to be made/cleanup to be done once the entire refactor is over.

@johncowen johncowen requested a review from a team August 7, 2019 07:00
@johncowen johncowen added the theme/ui Anything related to the UI label Aug 7, 2019
@johncowen johncowen mentioned this pull request Aug 7, 2019
John Cowen added 4 commits August 11, 2019 21:39
Both `requestFor` and `respondFor` methods now take both serialized data
and unserialized data.

We also leave it upto the adapter author to add (or omit) the serialized
data from the request payload, instead of it being automatically added
within the http client.

Included here is a small refactor to dry out the http adapter slightly.

Tests where changed only slightly to follow the new method signature.
Uses the newer method signature, and adds acceptance tests to catch
regressions while we are here.
This adds some integration tests to double check the requestFor... URLs
for self and clone.
Custom adapter methods (like clone and self) needed the full promise
chain written out. This can be repetitive and harder to refactor things
and make things consitent. We've added a possibly temporary
`application.request` method here to dry this out slightly. This may be
moved in the future once we decide exactly where it should live.

Now everything is much simpler, a few wrinkles are more visible, so we
iron out these wrinkles to make everythinbg as consistent as possible.
@johncowen johncowen force-pushed the feature/ui-http-adapter-sig-change branch from ca4c65b to 9c31df4 Compare August 11, 2019 21:40
@johncowen johncowen mentioned this pull request Aug 22, 2019
5 tasks
Copy link

@backspace backspace left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left some comments but nothing I’d consider blocking. I give my approval with the caveat that this is a set of changes that I definitely don’t claim to understand deeply, but it looks mostly mechanical, so since the tests are passing, it seems like it must have worked!

// workable way to decide whether this is a snapshot
// essentially 'is attributable'.
// Snapshot is private so we can't do instanceof here
if (typeof obj.attributes === 'function') {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’m not sure if this would be considered scandalous, but obj.constructor.name would be Snapshot when obj is a snapshot, maybe that’s a nicer way to check this?

Copy link
Contributor Author

@johncowen johncowen Aug 23, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worked a treat! Thanks @backspace ! (gotta love a scandal 😂 )

// TODO: When HTTPAdapter:responder changes, this will also need to change
return resp(serializer, response, serialized, unserialized);
});
// TODO: Potentially add specific serializer errors here

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these TODOs for future PRs?

Copy link
Contributor Author

@johncowen johncowen Aug 23, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably yes. I have another bunch of code for a similar Serializer refactor 😬 , I'm not ready to PR it yet though (this series of Adapter refactoring PR's gets me over the line for the ember upgrade so no need to PR the serializer stuff just yet) - this relates to the first TODO here.

I'm also half sticking to previous ember-data functionality here (I'm keeping the jQuery usage for now even though I don't want to etc). Specifically here - the ember-data provided adapters/serializers have specific adapter errors, but not specific serializer errors, and I'm thinking that potentially it might be beneficial to add some here, I'm not 100% sold on that idea yet - and I don't really want to add it here, but maybe later - this relates to the second TODO here.

I wish I had a better way to hint to folks 2 different types of TODO's 😢 i.e:

  1. TODO-type-1: I need to do this during this PR - but I'm looking for any suggestions/advice/information on this TODO because I'm not happy with what I've done myself, which is why I've PR'ed it with the TODO in.
  2. TODO-type-2: This stuff is longer term TODO, i.e. TODO in another PR, possibly by someone else, potentially before or after going into master.

Both of the above would be TODO-type-2.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I use FIXME for the first kind of todo, because I never want to commit FIXMEs to the primary branch, but I’m fine with leaving TODOs hehe

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh sweet of course yeah, FIXME has a better ring to it than TODO-type-1 😂 .

Will try to use that moving forwards, thanks!

return request`
PUT /v1/kv/${keyToArray(data[SLUG_KEY])}?${{ [API_DATACENTER_KEY]: data[DATACENTER_KEY] }}
Content-Type: application/x-www-form-urlencoded
Content-Type: text/plain; charset=utf-8

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’m curious why the content type needs to change…?? I’m surprised that it would be different for create vs update.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah weird, I was sure I'd done this! I put the TODO in a while ago to revisit, but I thought I'd resolved this.

As I remember.. when I started this refactor/PR back in April I remembered this issue #3804 and started setting the KV responses to be form-urlencoded. Then when I was tyre kicking all this lately to try to ensure no behaviour had changed, I noticed that this ever so slightly changed the behaviour of the KV API interaction, so I changed it back to text/plain to make it exactly the same - typically I only changed it in one place 😂 :doh:

Once we get past this refactor I'm going to look more deeply at what the correct header to use actually is (i.e. what Consul itself expects)

Short term, i.e. now - I need to change it in both API calls 😂 , thanks for spotting this!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, this is done, also added some more detail in the code incase its not me who gets to look at it. Thanks again for spotting!

`;
},
requestForDeleteRecord: function(request, data) {
requestForDeleteRecord: function(request, serialized, data) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This wouldn’t need to happen in this PR, but seeing how all these adapters had to be updated in very similar ways and how create/update/delete all seem to follow the same pattern in most of the adapters, I wonder if you could generalise using something like BuildURLMixin?

if (method !== 'GET' && headers['Content-Type'].indexOf('json') !== -1) {
options.data = JSON.stringify(body);
} else {
// TODO: Does this need urlencoding? Assuming jQuery does this

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this is a question to resolve before merging this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yeah, I dropped this in just to triple check this before the entire refactor goes onto master. A little bit of background info here (it's a little hard to remember as its from things I did back in April):

The client here tries to replicate exactly what the ember-data RESTAdapter does. I'm pretty sure I copied its behaviour pretty much exactly, with a little bit of refactoring to simplify the code somewhat. As I know I did it like this I'm pretty sure I don't need to do any extra url encoding here, but you know what happens whenever you are 'pretty sure you don't need to do something' - you probably need to 😂, plus I did this back in April so I suppose I was thinking I want to double check now so much time has passed (I might be completely misremembering here) .

Anyway, this is just a little reminder for me to check - not part of this PR (I think its actually part of the previous one) but this will be checked before this entire refactor goes down onto master.

Actually probably worth stating here, there'll probably be a final 'cleanup' PR of this entire refactor that you'll see soonish before it goes into master.. Sorry.. I really need to work on my 'PR Series' fu, michael's really good at communicating those, I'm gonna try harder with that next time I have a big thing like this!

John Cowen added 4 commits August 23, 2019 10:04
This header could be absolutely anything but application/json right now
(application/json means the data gets passed through JSON encoding which
we don't want here)

Future plans involve sending the correct header, but we what to stick to
previous behaviour in this series of commits/PR.
@johncowen
Copy link
Contributor Author

Hey @backspace

Thanks for looking at this appreciate the caveat, I know its always a bit difficult without a lot of context into the entire story, I'd be exactly the same!

I added a few more comments on the end here, one of which as a result of your constructor.name suggestion, but I'm gonna merge this down to the feature branch it belongs on.

I'm hoping that there is potentially one more PR to come on this feature (mainly cleanup), and then It'll be ready to be merged down for real 😅

Thanks!

@johncowen johncowen merged commit 0929202 into feature/ui-http-adapter Aug 23, 2019
johncowen added a commit that referenced this pull request Aug 23, 2019
…d unserialized data (#6285)

* Change signature of request/respond methods to take serialized,data

Both `requestFor` and `respondFor` methods now take both serialized data
and unserialized data.

We also leave it upto the adapter author to add (or omit) the serialized
data from the request payload, instead of it being automatically added
within the http client.

Included here is a small refactor to dry out the http adapter slightly.

Tests where changed only slightly to follow the new method signature.

We spotted missing tests for token cloning, and the self API methods,
so whilst refactoring this we added the test for those while we are here.

* Drys out custom adapter methods and irons out a few wrinkles

Custom adapter methods (like clone and self) needed the full promise
chain written out. This can be repetitive and harder to refactor things
and make things consitent. We've added a possibly temporary
`application.request` method here to dry this out slightly. This may be
moved in the future once we decide exactly where it should live.

Now everything is much simpler, a few wrinkles are more visible, so we
iron out these wrinkles to make everything as consistent as possible.
@johncowen johncowen deleted the feature/ui-http-adapter-sig-change branch September 12, 2019 12:38
@johncowen johncowen mentioned this pull request Dec 9, 2019
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
theme/ui Anything related to the UI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants