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

1.13.4 JSON API Adapter/Serializer PATCH #3467

Closed
lstyles opened this issue Jun 30, 2015 · 36 comments
Closed

1.13.4 JSON API Adapter/Serializer PATCH #3467

lstyles opened this issue Jun 30, 2015 · 36 comments

Comments

@lstyles
Copy link

lstyles commented Jun 30, 2015

I've successfully upgraded my application to 1.13 in preparation for 2.0. The app is currently in development and I am able to still make changes to the API the app uses.

I've changed my app to use JSONAPISerializer/Adapter and got some of it working. I am however struggling with updateRecord functionality because it send a PATCH request to the server with a standard POST payload.

I think it either needs to send POST request with POST payload or PATCH request with PATCH payload (according to JSON API 1.0 spec).

Currently, whatever I do to work around it, will most likely break in future updates. If I develop my backend to use POST payload for PATCH method I will have to redevelop the backend when the payload (inevitably) changes. And the other way around, I can't develop the backend to accept and process PATCH operations because the payload sent by Ember-Data is a full record.

I can see the changedAttributes property on the snapshot which makes me believe this is work in progress and updateRecord will use that property to build a PATCH payload. This sounds perfect but at the moment, Ember-Data is no longer in Beta, but the functionality isn't correct/consistent.

What should I do to sort this out and not have to rewrite all server side PATCH endpoints later?

@lstyles
Copy link
Author

lstyles commented Jun 30, 2015

May I just add that this happens when the following code runs:

myObject.set('someProperty', true);
myObject.save();

@wecc
Copy link
Contributor

wecc commented Jun 30, 2015

Hi @lpaluszk!

Updating resources in JSON API is done using the PATCH verb, see http://jsonapi.org/format/#crud-updating.

This is not to be confused with the upcoming JSON Patch Extension.

@lstyles
Copy link
Author

lstyles commented Jun 30, 2015

Hi @wecc

Thanks for this. I feel embarrassed for not seeing this sooner. Are there plans for ember-data to support JSON Patch Extension in the near future (which is clearly what I was looking at)?

The changedAttributes property and diff makes it look so.

Or should I not worry about it and it will remain as it is (Full model sent to PATCH endpoint) for the foreseeable future?

@wecc
Copy link
Contributor

wecc commented Jun 30, 2015

In 1.13 there have been some steps taken towards being able to implement JSON Patch (like you said, snapshot.changedAttributes) but I think this is still in RFC stage. See emberjs/rfcs#5

As JSON API is committed to making additive changes only you should feel safe using the current behavior.

@lstyles
Copy link
Author

lstyles commented Jun 30, 2015

Thanks for this. It makes complete sense and gives me a clear way of proceeding with my development.

AFAIK, currently the PATCH request sends the full object back to the server and not only the properties that have changed. Is this something that's likely to change? (Not talking about the JSON Patch extension here).

@wecc
Copy link
Contributor

wecc commented Jun 30, 2015

@lpaluszk Hm, that's a very good question :)

@igorT, @bmac TL;DR: do we want to modify the JSONAPISerializer to only serialize changed attributes when saving?

@bmac
Copy link
Member

bmac commented Jul 2, 2015

Only sending changed attributes and relationships sounds like a good idea. Unfortunately, I don't think Ember Data is quite ready to do that right now since it only tracks changed attributes and only sending changes attributes and all the relationships seems like it would be more confusing then helpful.

@alageman
Copy link

alageman commented Dec 4, 2015

One argument that I could see for doing this would be the following scenario.

You have a data that contains information that is restricted based on user level permissions. So on the get request you only return certain fields. Now the user edits the data they are allowed to touch and then saves the data. The way it's currently setup it's going to try and send the entire model back with blank or null fields in the restricted areas. This can potentially overwrite the database entry incorrectly.

A example of where this first bit me was a password hash. I allowed users to post or patch the password if it was their own account, hashed it, and then never sent it back to the front end. However since the password was part of the data model of a user and user is allowed to change their own password this caused a issue any time they tried to edit their user data and the patch would set their password as a blank string even though the password attribute was never changed by them.

@vsymguysung
Copy link

@wecc @bmac, Is there any example codes of JSONAPISerializer to send only changed attributes & relationships?

@sevarubbo
Copy link

It would be extreeeemly cool to be able to send only changed attributes and relationships in PATCH requests.

@mattmcginnis
Copy link

@alageman

The way it's currently setup it's going to try and send the entire model back with blank or null fields in the restricted areas. This can potentially overwrite the database entry incorrectly.

According to the docs at jsonapi.org

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.

@webark
Copy link

webark commented Feb 11, 2016

@vsymguysung this is a quick way that I accomplished it. It will probably need to be tweeked as it gets used more, but could be a place to start with you.

  serializeAttribute(snapshot, json, key) {
    if (snapshot.changedAttributes()[key] || snapshot.record.get('isNew')) {
      this._super(...arguments);
    }
  },

@sevarubbo
Copy link

There's another way to do this. It's pretty simple for attributes, but I don't know how to do the same with relationships.

serialize (snapshot) {

    let json = this._super(...arguments),
        changedAttributes = snapshot.changedAttributes(),
        attributes = {};

    Object.keys(changedAttributes).forEach(key => {
        attributes[this.keyForAttribute(key)] = changedAttributes[key][1];
    });

    json["data"]["attributes"] = attributes;

    return json;
}

@bmac
Copy link
Member

bmac commented Feb 12, 2016

The current blocker on supporting this is the fact that Ember Data doesn't track changed relationships on records (only changed attributes). Once that feature is implemented support for sending only the changed attributes and relationships should be easy to add to Ember Data.

@alageman
Copy link

@mattmcginnis

Yes that is correct for PATCH requests. The problem ember data is sending them as a POST (I think, It's definitely not a PATCH). Also when I said

blank or null fields

I was wrong. It will use empty strings (At least for string values). Which could or could not be correct values.

I think the argument is to use PATCH rather then POST if partially updating a single item. If they used the PATCH request the server would be able to distinguish the difference.

There is a plugin that looks like it does this. But I have not had a chance to try it yet.
https://www.npmjs.com/package/ember-data-patch-persistance

@vsymguysung
Copy link

@webark @sevarubbo Thank you for sharing the codes ! 👍 @bmac hope that ember-data implement tracking changes on relationships soon.

@wecc
Copy link
Contributor

wecc commented Oct 22, 2016

Rollback and dirtiness of relationships are being tracked in emberjs/rfcs#21

@Redsandro
Copy link

Are there any updates on this?

When you have multiple clients (or the server) mutating records, and everyone is mutating unique properties, this should technically be no problem. On Ember however, the record ends up broken.

Imagine a new log record appears. One user pushes labels, another user pushes a description, and the server is collecting events.

This would all work, except because the clients (using Ember Data) push the complete document from the time it was loaded, all other users' mutations, including the ones on the server, will be undone.

It took me a while to figure out Ember Data was doing this, because knowing the JSON:API spec, I assumed Ember would only send the mutated fields as being updated. Surely I'm not the only one that has multiple clients mutating unique properties on the same record, yet this issue is still open. So I'm wondering if there is some well-known solution that I didn't come across.

I can reload the record before every mutation, but (1) it causes visible lag for clients and (2) it still has a (small) time frame in which the record can still be broken. I.e. if two users mutate at the same time, or if the server pushes a stream of events to an array as they happen, it is likely that one will be 'dropped'.

This came up in a search.

@Redsandro
Copy link

Redsandro commented Oct 17, 2019

@webark although it is not documented as such, I have observed that serializeAttribute, or at least your specific trick, does not work from within the application serializer. It only works from a model-specific serializer. So you need to create new serializers for every model. This might save people some time.

Once again I was thrown off the scent, this time by ember-infinity which causes all loaded records to be marked as isNew for some reason. adopted-ember-addons/ember-infinity#393

@slimee
Copy link

slimee commented Feb 7, 2020

Hi,
In 2020 is there a way to tell ember-data: produce PATCH requests with dirty attributes only?

Regards

@runspired
Copy link
Contributor

runspired commented Feb 7, 2020

@slimee yes, how you serialize is entirely up to you. When you implement updateRecord in your adapter you can choose to serialize only things you want to be part of the patch.

This has always been the case, this issue is because there was some desire for this to be the automatic behavior. It is unlikely we ever support this behavior automatically in the current adapters and serializers as that would be a major breaking change.

With the above in mind I am going to close this ticket.

@slimee
Copy link

slimee commented Feb 8, 2020

Thanks you.
You said yes but I understand the answer is no, I requested for an automatic way of course. If it's a breaking change it should be optional, isn't it?

@webark
Copy link

webark commented Feb 8, 2020

@runspired Why are we settling on having the default behavior be one that is against the (intended, prescribed, desired) use of the spec?

** Yes technically you “may” send all, but that is not why you are making a patch request. It is to send partial documents.

@Redsandro
Copy link

Redsandro commented Feb 8, 2020

I agree that this is not ideal. I've learned to deal with it, but it's always something I have to deal with. It feels permanently awkward/buggy. The art of a good implementation is preventing you from having to deal with the same thing repetitively.

So I'm not satisfied that we have to forego the sane, expected, documented way of PATCHing things, just because 2 major versions ago we forgot to implement it, even though the groundwork has been laid out by tracking all properties' state. The JSONAPI spec is clear, it's not a breaking change when people don't run a broken JSONAPI server.

We are incorrectly using PATCH requests as if they are PUT requests, which is an issue, even if our implementation is preferred (by some) over the intended implementation of the PATCH verb.

RFC 5789

In a PUT request, the enclosed entity is considered to be a modified version of the resource stored on the origin server, and the client is requesting that the stored version be replaced.

With PATCH, however, the enclosed entity contains a set of instructions describing how a resource currently residing on the origin server should be modified to produce a new version.

RFC 5789 section 2

JSONAPI spec 1.0

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.

JSON:API v1.0 CRUD updating


@runspired I don't think this issue should be closed unless it's also being tracked somewhere else.

@runspired
Copy link
Contributor

@Redsandro @webark the solution here is for someone to write an addon that provides an adapter for JSON:API that is spec specific. This is easier than ever now that the reference adapters that EmberData shipped eons ago can be svelted away.

@Redsandro
Copy link

Redsandro commented Feb 9, 2020

Yes, I use something close to the above solution by @webark in every app. It works nicely. Egoistically speaking, my problem is solved by having this insider knowledge.

It could probably be made into an adapter that we'd have to include rather dan adding the lines of code, but I think the point is that it is our opinion that a spec specific implementation could/should be the default implementation.

It would remove the unexpected deviation from the spec that other users will encounter at some point, which will cause them to chase a ghost for a couple of hours, reading the JSON:API spec, reading the PATCH spec, wondering what they do wrong, until they figure out the deviation.

If in your experience this is really a bad idea to do such a change, it could be a milestone item for the 4.0 version

@runspired
Copy link
Contributor

the naming of the adapters is confusing especially given they are used for mostly not the specs they are named for.

I agree it would be nice to have an official spec conforming adapter supported but that’s not something that belongs in the core of EmberData. An official but non-core spec driven adapter replacement is planned for once we’ve finished the designed for replacing adapters.

I should have been more clear: I closed this ticket since there is no chance we change the existing adapters in a breaking way. We will eventually introduce new things or help the wider community build new things that we help support once there’s better infra for making requests in place. Those new things would be closer to the specs they support, and JSON:API will remain one of the specs we try to provide first class tooling for.

@Redsandro
Copy link

Thank you for the clarification @runspired.

Out of curiosity, considering that the official implementation of EmberData does track the state of individual properties, is using those states to determine the PATCH request once forgotten and since grown into the official adapter where it is policy to never change, or is there a conscious reason for building the PATCH requests this way?

@webark
Copy link

webark commented Feb 9, 2020

and @runspired is there a reason for there being “no chance” that a breaking change will every be made to an existing serializer? (i assume by “adapter” you where speaking generically).

There has been consideration over the years of brining things like batching support to json api, so will these never be added into ember data proper? Is the idea to move all “adapters” (rest, jsonapi, etc)
out of the core ember data, and simply only having tgfbase json skeleton?

@runspired
Copy link
Contributor

@webark I said adapter because the adapter is what serializes the record for a request, and in this case the reference adapters (json:api and rest) both have a behavior that would break most apps if we were to change it.

It’s easy to provide your own adapter that doesn’t extend either of these either by providing a native class with a static create method or extending EmberObject. The API docs go into detail on this.

@webark
Copy link

webark commented Feb 10, 2020

@runspired wait. I was just trying to clarify before, but now i’m confused.. Why should we need to do a whole new base adapter, rather then just extending and augmenting the jsonapi serializer? Are these types of things not supposed to be done in serializers now? Are serializers going away in a new version of ember data? (i looked through all of the ember data RFCs, and saw no mention of any major modification in this area 😅)

@slimee
Copy link

slimee commented Feb 10, 2020

Can someone @runspired @Redsandro share a sample code of such an adapter that produce json PATCH requests?
Even not official or supported. Just to egoistically unlock the situation of my project :D

@Redsandro
Copy link

@slimee there are mutiple solutions to the same problem. Two of them outlined earlier in this issue. When a fix is part of your own code (as opposed to vendor), it's tempting to mix in additional application-specific code. So I've stripped that and I believe this will do the trick if it's okay to do it in the entire application.

serializers/application.js

import DS from 'ember-data'

export default DS.JSONAPISerializer.extend({
    /**
     * Do not patch attributes if the value is not dirty.
     * @see https://github.com/emberjs/data/issues/3467#issuecomment-543176123
     */
    serializeAttribute(snapshot, json, key, attributes) {
        if (snapshot.record.get('isNew') || snapshot.changedAttributes()[key]) {
            this._super(...arguments)
        }
    }
})

@runspired
Copy link
Contributor

@webark it isn't an RFC but a misunderstanding. Folks have been using adapters and serializers wrong for years. I'd read through the API guide for them, it's a bit more clear.

It appears the adapter docs didn't get published as part of 3.16 which is surprising to me, I'll check on that but they are here in the source code: https://github.com/emberjs/data/blob/master/packages/store/addon/-private/ts-interfaces/minimum-adapter-interface.ts#L12

@hoIIer
Copy link

hoIIer commented Sep 13, 2020

running into this in Sept 2020, @Redsandro, thanks for that gist, how would I strip non-dirty relationships as well?

@Redsandro
Copy link

Redsandro commented Sep 14, 2020

@burritoIand I'm not sure. You can disable serialization of a relationship like so:

export default ApplicationSerializer.extend({
  attrs: {
    someRelationship: { serialize: false },
    otherRelationship: { serialize: false },
  },

But that's dirty-agnostic. If you do find a method that's working in sept 2020, please share the code here. 💪

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests