Skip to content
This repository has been archived by the owner on Apr 12, 2024. It is now read-only.

Ability to configure the encoding/decoding data for $resource #1514

Closed
wants to merge 1 commit into from
Closed

Ability to configure the encoding/decoding data for $resource #1514

wants to merge 1 commit into from

Conversation

ryanzec
Copy link

@ryanzec ryanzec commented Oct 31, 2012

Right now it seems like the $resource module excepts the data from the REST API call to be returned the content of the response however sometimes it might not be formatted that way. What I have done is instead of getting the data from the responce.data automatically, it is now a configurable function that defaults to the current process.

There is now a 4th optional parameter for $resource() that is the function used to parse for the data of the response so that if the REST API you are working with has the data formatted slightly (or completely) different than excepted, it is very easy to still use the $resource module.

An example of a REST API that has a different response the what $resource right now except for be the Jira REST API.

I have already submitted the electronic CLA.

@ryanzec
Copy link
Author

ryanzec commented Nov 1, 2012

Sorry for the broken commit but everything looks good now.

@marknadig
Copy link
Contributor

I think it is reasonable that a parseProvider is supplied - but I think that it should also be called from the resource constructor in case we need to create a resource instance from a json payload (e.g. from a websocket).
Related, I suggest we also have a encodeProvider that handles the other side of the coin - encoding the resource into json.
All that to say - is just adding another parameter on the end for responseDataParser the best, or perhaps an optional encoder object that supports 'encode'/ 'decode' - or 'toJson', 'fromJson'?

@ryanzec
Copy link
Author

ryanzec commented Nov 2, 2012

Makes sense about making it an object and adding the ability to encode the object differently. I wasn't thinking about the encode part because my though process was that if the format required the data to be in some.weird.struct, you would just create an object that looked like that but being able to keep you object normal and consistent and just encode them different, that would be better.

On the note about the constructor, I am not even sure how do do that. I though that you couldn't created an instance of a resource object as resources are just plain javascript objects. For example, to save a new resource, I thought the only way was to create a plain object and then do resourceName.save(object). Maybe if you could give me an example of the syntax on how to create a empty resource object, I would better know what you are talking about and how to go about this.

One other thing I was thinking about was also adding this to the actions. I can see cases where different actions return different data formats. Getting a list of objects might returns a format different from getting one object. Using Jira as my example again, searching for issues has the data returning in the response.issues field however getting just one object has the data returning as the response (like $resource excepts).

If I add this to the actions and the constructor, there needs to be some sort of order of precedence and I would tat this would be the logic order

action -> constructor -> factory -> default

If anyone else has any thoughts or comments of these plans, please let me know, otherwise I will probably start working on this stuff tonight or this weekend.

@marknadig
Copy link
Contributor

Thanks for taking the time to comment back. Re: encoding - our service layer is RoR so is expecting objects to be nested; e.g. {user: {name: 'filbert'}}. We also have some strange requirements around datetimes for encode/decode. Current we are wrapping the ngResource with a decorator to intercept the get/post - so being able to specify an encoder/decoder would simplify this a lot.

re: Constructor -
~ line 324 -
function Resource(value){
copy(value || {}, this);
}

This allows me to define a resource and then new one up:
var User = $resource('/user/:userId', {userId:'@id'});
var user = new User({name: 'foo'});

Notice the ctor is just doing a copy, same as the default response handler.

I need to think about the action-specific response handlers. Another option for those cases would be to make the 1 response handler dynamic enough to handle the different possibilities. Make keep the logic a little cleaner in ngResource. Just my initial thought.

@ryanzec
Copy link
Author

ryanzec commented Nov 2, 2012

The constructor thing makes sense and good to know that now.

I will experiment with having the constructor/factory encoder providers smart enough to handle different actions. My initial though is that this should be possible to pass the action name to the encode/decode providers and then have the provider decided what to do based on the action if they want. I will let you know what I come up with.

… when working with REST APIs that except different formats then $resource automaically assumes

Custom encoder/decoders can be defined in the $resource or in the resource contructor and can return different format based on different action names
@ryanzec
Copy link
Author

ryanzec commented Nov 3, 2012

I rewrote the code to incorporate the comments that Mark made. Now you can pass an optional 4th parameter to the resource factory or an optional 2nd parameter to the resource constructor that take an object that can have an encode and/or decode function defined for encoding and decoding data to and from a REST API.

The encode function takes 2 parameters, first the data and then the action name and except the data structure that the REST API is excepting to be returned..

The decode function takes 2 parameters, first the response and then the action name and except the data to be returned from the REST API formatted response.

Both functions will default to current behavior if no custom functionality is given.

The reason the action name is passed is to be able to return or parse different data structures based on the action that is called.

I have also added in additional tests to test this new functionality.

@jmaynier
Copy link

+1 to add this feature. It will help to handle complex json object and also add/remove transient data that should not be persisted (added un decode and remove in encode).

copy(value || {}, this);
constructorDataParsers = constructorDataParsers || {};
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than having a completely different constructorDataParsers, I think in most cases it would likely just use the same decode/encode as the factory definition. So, no need for a constructorDataParsers parameter - rather, the copy(value || {}, this) would use the decoder passed into the resource factory. So:
function Resource(value)
copy(dataparsers.decode(value, 'constructor') || {}, this )
}

Copy link
Author

Choose a reason for hiding this comment

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

I am good with the first point about not have the constructor (I think I misinterpreted one of your previous feedbacks) I however don't agree with the opinion that the Resource constructor should use the decoder. In my mind, that encoder/decoder is specific designed to be used with the backend service use to get and save data.

Lets just say I am using some third party crazy api that returns data like this:

{"response": {"data": {"object": {"id": 123, "username": "test", "password": "user"}}}}

Now with the way the constructor stands, to create a new user in JavaScript, all I have to do it:

var user = new User({"id": 124, "username": "test2", "password": "user2})

If the constructor is assume to use the decoder, then it looks like this:

var user = new User({"response": {"data": {"object": {"id": 124, "username": "test2", "password": "user2}}}})

Now while it is true you could have the decoder account of this but in my opinion you are starting to muck up your standard JavaScript code because of a poorly designed REST API.

Copy link
Contributor

Choose a reason for hiding this comment

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

I get your point. In my specific use-case, I have the exact same JSON payload coming across a websocket as I do from my REST API. So, I want to be able to create a new resource from the websocket payload - and hopefully going through same decode process. I could try to figure out how to expose the decoder out of the resource so I could use it from my websocket proxy.

@marknadig
Copy link
Contributor

Ryan - I'm getting ready to merge this into my branch and had a comment regarding the constructor: Rather than having constructorDataParsers, I think in most cases it would likely just use the same decode/encode as the factory definition. So, no need for a constructorDataParsers parameter - rather, the copy(value || {}, this) would use the decoder passed into the resource factory. So the constructor would look like:
function Resource(value)
copy(dataparsers.decode(value, 'constructor') || {}, this )
}

What do you think?

Having the option makes it more flexible - but seems like it may be overkill to have the constructorDataParsers.

headers: extend({}, action.headers || {})
}).then(function(response) {
var data = response.data;
var data = dataDecoder(response, name);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest pass in response.data to dataDecoder() - since decoder would only want to deal w/ data response (and facilitate sharing from ctor)

Also, to keep the decoder 'simple' - I'd suggest moving the call to it inside the forEach() loop below - so if returns an array, the decoder doesn't need to know/deal w/ that - only ever one item at a time.

Copy link
Author

Choose a reason for hiding this comment

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

About what to pass to dataDecoder(), see my comment above about this.

The part about the foreach statement does make sense, I remember trying to do that and I ran into some issue but I will give another crack at it.

@mhevery
Copy link
Contributor

mhevery commented Jan 18, 2013

This is not an approach which is consistent with the rest of the angular. A better way to do this is to allow resource to consume $http.config which already has transformRequest/Response functionality

See duplicate: #1045 (comment)

I am going to close this for now. If you would like to update it to allow full $http.config, please reopen for review.

@mhevery mhevery closed this Jan 18, 2013
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants