adopted-ember-addons / ember-data-model-fragments

Ember Data addon to support nested JSON documents
MIT License
370 stars 114 forks source link

5.0.0-beta.0 - normalization incomplete #375

Open basz opened 4 years ago

basz commented 4 years ago

I have a model with a cloudDrive status fragment that in turn has two more fragments. called context and failure.

Over a webstomp connection I receive updates to my models via store.pushPayload(payload). The model correctly updates except if it does not exists in the store. When it does exist in the store all properties are correctly updated except for the nested fragments... (note the state property is correctly updated.

"status" : { "state": "checked-out", "context": { "adapter": "dropbox", "identity_id": "xxx-xxx-xxx-xxx-xxx", "order_number": "2.4", "order_status": "cad", "checkout_time": "2020-08-19T11:51:26.000Z" }, "failure": { "message_code": null, "message": null, "details": [] } }

So. a button instructs the backend to perform a 'check-in'... This might fail due to some reason and the whole model is transmitted via webstomp as a json API payload. Pushing that payload does not update the failure fragment.

Any ideas where to look/debug. I have confirmed the received payload is correct.

"data": {
    "type": "dossier/order",
    "id": "xxx-xxx-xxx-xxx-xxx",
    "attributes": {
      // redacted
      "cloud-drive": {
        "state": "checked-out",
        "context": {
          "checkout_time": "2020-08-19T11:51:26+00:00",
          "identity_id": "xxx-xxx-xxx-xxx-xxx",
          "adapter": "dropbox",
          "order_status": "cad",
          "order_number": "2.4"
        },
        "failure": {
          "message_code": "dossier.checkInFailure.noMutationsDetected",
          "message": "No changes where detected. Has the cloud drive been fully synchronized?",
          "details": []
        }
      }
    },
  }
}
basz commented 4 years ago

setting a fragment to null on a model seems to be the culprit here. I would guess the fragment is then either deleted or recreated from the defaultValue. Is that indeed what I may expect to happen?

patocallaghan commented 3 years ago

@basz I think I'm seeing a similar issue to you. While it's not exactly the same it has the same characteristic where the fragment isn't populated after being null initially.

In our case we are creating a record which is only sparsely populated but the rest of the attributes are filled in by the response from the backend.

I've created a failing test in https://github.com/lytics/ember-data-model-fragments/pull/384 to replicate it and have confirmed it did work before the 5.0 refactor (my tests passed).

patocallaghan commented 2 years ago

So I've come back to this issue after a long time as we're in the midst of trying get ourselves unblocked to upgrade Ember Data (currently stuck on ED 3.12).

As I mentioned in the comment above I have some failing tests which replicate the buggy behaviour we are seeing. Essentially if you have a model/fragment which has a fragment array/fragment property set to null it will never update when a http response has those properties updated. I have confirmed running these failing tests on a v4 branch pass just fine.

I've tracked the bug down to the following lines of code in the FragmentRecordData#didCommit hook:

https://github.com/adopted-ember-addons/ember-data-model-fragments/blob/4f77b2d5ea08685230ec60b5a3ddd2b3d3a7ec6c/addon/record-data.js#L426-L433

Basically because the existing property is set to null the fragment = this.serverFragments[key]; evaluates to null. This means the if (fragment) check fails and we don't commit the new data to the fragment with fragment._didCommit(attributes[key]);.

I've been looking at it and what I'm imagining is that if the fragment doesn't exist we should recreate it using something like setupFragment or setupFragmentArray but I'm hitting a few stumbling blocks there. The API of createFragment, for example, is createFragment(store, declaredModelName, record, key, options, data) but I can't find any possible way to get access to the record at this point in the stacktrace so can't recreate the fragment.

image

@igorT @runspired as you folks have context on the RecordData implementation here, do you have any pointers?

patocallaghan commented 2 years ago

I've added a potential direction for a fix for this issue in https://github.com/adopted-ember-addons/ember-data-model-fragments/pull/384 but it seems a bit hacky.

patocallaghan commented 2 years ago

@basz I'm not sure if you're issue was exactly the same as mine but could you try out https://github.com/adopted-ember-addons/ember-data-model-fragments/releases/tag/v5.0.0-beta.3 where I shipped my fix to make sure null fragments are updated from the server?