BlairAllegroTech / js-data-jsonapi

JsonApi Adapter for js-data
MIT License
15 stars 5 forks source link

Cannot read property 'id' of undefined #12

Closed rgant closed 8 years ago

rgant commented 8 years ago

After resolving #11 I'm getting this error:

TypeError: Cannot read property 'id' of undefined
    at JsonApiAdapter.update (js-data-jsonapi-0.0.0-alpha.8.js:265)
    at js-data-2.9.0.js:5637
    at processQueue (angular-1.5.7.js:16170)
    at angular-1.5.7.js:16186
    at Scope.$eval (angular-1.5.7.js:17444)
    at Scope.$digest (angular-1.5.7.js:17257)
    at Scope.$apply (angular-1.5.7.js:17552)
    at HTMLButtonElement.<anonymous> (angular-1.5.7.js:25627)
    at HTMLButtonElement.dispatch (jquery-1.12.0.min.js:3)
    at HTMLButtonElement.r.handle (jquery-1.12.0.min.js:3)(anonymous function) @ angular-1.5.7.js:13708(anonymous function) @ angular-1.5.7.js:10347processQueue @ angular-1.5.7.js:16178(anonymous function) @ angular-1.5.7.js:16186$eval @ angular-1.5.7.js:17444$digest @ angular-1.5.7.js:17257$apply @ angular-1.5.7.js:17552(anonymous function) @ angular-1.5.7.js:25627dispatch @ jquery-1.12.0.min.js:3r.handle @ jquery-1.12.0.min.js:3

This is the function raising the error:

        JsonApiAdapter.prototype.update = function (config, id, attrs, options) {
            var _this = this;
            var idString = id.toString();
            if (attrs[config.idAttribute]) {
                if (attrs[config.idAttribute].toString() !== idString) {
                    throw new Error('Json Api update expected supplied id and the primary key attribute "' + config.idAttribute +
                        '" to be the same, you may have called update on the wrong id?');
                }
            }
            else {
                attrs[config.idAttribute] = idString;
            }
            var localOptions = this.configureSerializers(options);
            return this.adapter.update(config, idString, attrs, localOptions).then(null, function (error) {
                return _this.DSUtils.Promise.reject(_this.HandleError(config, localOptions, error));
            });
        };

The error is on this line: if (attrs[config.idAttribute]) {

rgant commented 8 years ago

Here are the variable passed to update:

config = Defaults {defaultValues: Object, methods: Object, computed: Object, scopes: Object, actions: Object…}, 
id = "659", 
attrs = undefined, 
options = Options {}
rgant commented 8 years ago

Stepping through code I noticed that the attr became undefined at this last step:

screen shot 2016-06-28 at 10 02 31

Could this be the missing callback from #11?

scr: https://github.com/js-data/js-data/blob/8f4ca8ce16953c5f4e6fbe40b70cf9efbf631f7d/src/datastore/async_methods/save.js#L40

BlairAllegroTech commented 8 years ago

Yes i think it may be related to #11. I was not able to reproduce this behaviour and always had cb set whenever the lifecycle event was triggered.

I tried to reproduce this:

  1. Loading data from the server.
  2. Updating
  3. Calling save on the resource

see: describe('DS#Save', function () { ..}); here

Though the documentation says cb is not passed for synchronous actions. And beforeUpdate is called on:

rgant commented 8 years ago

Maybe this had to do with my using js-data-angular plugin? I also didn't have issues until I tried to use both AngularJS and js-data-jsonapi.

On Tue, Jun 28, 2016, 19:26 Blair notifications@github.com wrote:

Yes i think it may be related to #11 https://github.com/BlairAllegroTech/js-data-jsonapi/issues/11. I was not able to reproduce this behaviour and always had cb set whenever the lifecycle event was triggered.

I tried to reproduce this:

  1. Loading data from the server.
  2. Updating
  3. Calling save on the resource

see: describe('DS#Save', function () { ..}); here https://github.com/BlairAllegroTech/js-data-jsonapi/blob/js-data-jsonapi/test/update.spec.js#L80

Though the documentation says cb is not passed for synchronous actions. And beforeUpdate is called on:

  • save
  • update
  • updateAll

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/BlairAllegroTech/js-data-jsonapi/issues/12#issuecomment-229214241, or mute the thread https://github.com/notifications/unsubscribe/AArXmZ-LnOR2riY60hd0iBLv1wse-rJdks5qQa2TgaJpZM4JAGyE .

BlairAllegroTech commented 8 years ago

When you are debugging, can you step into the beforeUpdate call and see where the code goes? Try to find out where the attrs gets set to null.

I'm not sure if this is a bug or by design. e.g To fix this do we need to make sure that cb is always set in lifetime events or does adapter.update need to handle empty attrs

BlairAllegroTech commented 8 years ago

Ok i finally was able to reproduce this. There were a few bugs in the tests i created yesterday.

But the problem was around the way lifecycle events are invoked. It looks as though all lifecycle events are defined statically against resources are 'promisfied ' by js-data.

This test: here

Will reproduce the problem when beforeUpdateJsonApiData does not return data when no callback provided in the call.

There may be a few more things to be tested here:

  1. Do the lifetime event handlers stick, e.g. Do they remain or are they over written by js-data. It maybe that we are just assigning this callback to a temporary copy of the resource definition ?
  2. Does the call need to be promisifed?
  3. What happens when there are other lifetime events assigned, these need to be preserved.
  4. If we do 3 above then we must ensure that the lifetime event handlers only added once!
rgant commented 8 years ago

Thanks @BlairAllegroTech Sorry I didn't help with the investigation of this issue. Glad you were able to sort it.

BlairAllegroTech commented 8 years ago

@rgant No worries at the moment the more feedback i can get the better!

I would be really appreciate any feedback you can give on what works well and what doesn't.

rgant commented 8 years ago

I'll try out version 9 as soon as I can and let you know how it goes. Thanks so much!

I haven't actually gotten to relationships on the client side yet (the server side has many). First thing I was trying was to list, create and update a single model. I'll let you know how that goes.

rgant commented 8 years ago

Version 9 resolves this issue. Thanks again!