jakubrohleder / angular-jsonapi

Simple and lightweight, yet powerful ORM for your frontend that seamlessly integrates with your JsonAPI server.
http://jakubrohleder.github.io/angular-jsonapi/
GNU General Public License v3.0
96 stars 34 forks source link

Replace put requests with patch requests #23

Open fabiocarneiro opened 8 years ago

fabiocarneiro commented 8 years ago

Jsonapi specification uses PATCH requests, not PUT. Although it doesn't disallow us to use PUT, and they even say it can be implemented in the future, by using it now we have the risk that this implementation will not be compatible with future revisions.

https://github.com/jakubrohleder/angular-jsonapi/issues/22

Also, after that, it will be really interesting to compare the object obtained in get, to object that you're saving and only send attributes that were modified (real PATCH request).

eduardmartinez commented 8 years ago

I think it's not only changing the $http method but also the way that data is sent. When you make an update over an object, all the object is sent because it was made with the PUT intention to override the whole object. If it is changed to PATCH, it is supposed to send only attributes we need to update. I tried to do it with current version but it doesn't let me override object.form.data.attributes with the attributes I only need to update.

xpopov commented 8 years ago

There is no PUT method in JSON API 1.0 format specifications. There is only PATCH. Here is excerpt from it (http://jsonapi.org/format/#crud-updating):

Updating a Resource's Attributes

Any or all of a resource's attributes MAY be included in the resource object included in a PATCH request.

If a request does not include all of the attributes for a resource, the server MUST interpret the missing attributes as if they were included with their current values. The server MUST NOT interpret missing attributes as null values.

So basically any modifications should be done a PATCH-way. It's not a problem to send the whole object though, according to JSON API this is allowed as well.

eduardmartinez commented 8 years ago

Of course, it's not a problem to send the whole object. But what I meant is, currently, the package is not open to send only the specific attributes you need to update. And these changes only reflect a method change, just saying :)

xpopov commented 8 years ago

I'm using $jsonapi.getResource(...) and when I have object, I can use something like "angular.copy(attributes_to_update, object.form.data.attributes)", Then, on object.save it sends only fields I've set. So looks like there is a way to have partial updates.

Renaming method would help this module to be compatible with JSON API 1.0 as there is no PUT method in current specifications. For example, I use JSON API 1.0-supported web server and it does not allow PUT method at all.

Old JSON API specifications supported PUT as well, but they removed it because there was a lot confusions regarding two update methods.

Perhaps, the better way to have an option in angular-jsonapi, allowing to use PATCH instead of PUT. PATCH is not working with nginx by default.

eduardmartinez commented 8 years ago

@xpopov Thanks for the tip! I hadn't tried with it. Btw, I'm trying to unlink a relationship but it doesn't work for me. How are you doing it?

xpopov commented 8 years ago

@eduardmartinez: For now I'm developing very basic app and don't use relationships. I need to understand if angular-jsonapi suitable for offline-first support first. Looks like it has a lot of relevant functionality, sync flags, timestamps and other things that required for syncing. It's currently missing persistent queue for sync requests. When we add or update object, json api http request should be initiated, but if we offline, we cannot do http requests immediately. And one way to solve this is keeping queue of sync requests.

I think I will try to add simple relationships to test this as well.

jakubrohleder commented 8 years ago

Thanks for the PR. I'll look closely at the code at the evening.

I've used PUT instead PATCH for a simple reason - backend package that I've used did not support PUT. Nevertheless I totally agree that PATCH is a way to go (with optional PUT support)!

@xpopov you are right - this package has been created to (at some point) fully support offline apps, but as you pointed some kind of request queue and conflicts resolution mechanism has to be added. As I finish the 1.0.0 version with full support of specification, this will be the priority.

@eduardmartinez can you elaborate on the unlink issue?

LavaToaster commented 8 years ago

I've been meaning to send this in as a PR, I thought I'd share my edits to the rest source. I've updated my update method to the following to allow the ability to only send what's been changed.

      function update(config) {
        var form = angular.copy(config.object.form);
        var formAttribute = form.data.attributes;
        var changedData = {};

        for (var attribute in formAttribute) {
          if (
            formAttribute.hasOwnProperty(attribute) &&
            formAttribute[attribute] !== config.object.data.attributes[attribute]
          ) {
            changedData[attribute] = formAttribute[attribute];
          }
        }

        form.data.attributes = changedData;

        // TODO: Throw an error when there is no changed data?

        return $http({
          method: 'PATCH',
          headers: headers,
          url: url + '/' + config.object.data.id,
          data: form.toJson()
        }).then(resolveHttp, rejectHttp.bind(null, 'update'));
      }
eduardmartinez commented 8 years ago

@jakubrohleder Well, I've tried to use the unlink and link methods as you indicate in the docs, but none of them seems to work. Also, it's not clear enough for me and it's not explained in the docs the clear meaning of the link and unlink parameters. Searching in the code I found that 'key' is the name of the relationship and not of the resource type and I still don't know what do you mean with 'target' parameter, because when I pass the whole element to link/unlink, an error is thrown like Can't find schema property (I don't remember right now the exact error text). So, I had to build my own methods but that's not the idea. Maybe I should put this in a new issue. What do you think?

jakubrohleder commented 8 years ago

Yep it would be the best to create a new issue.

fabiocarneiro commented 8 years ago

By using unspecified method you force the host implementation to support it to be able to work with this client, and it makes no sense. Being able to send either full or partial requests will not break an API that is compliant with JsonApi, but this same compliant API will not work with this client.

This client is not following the specs, and I'm using my branch of it right now, since I will not modify my perfect compliant API just to support a out of spec client.