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

Check for objects with a truthy clause to avoid nulls, fixes #16158 #40

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
4 participants
@thehousecat
Copy link
Contributor

commented Nov 20, 2013

The fix for #16158 does not appear to be correct. Please accept my updated fix.

The previous fix tests 'object' for thruthyness and the typeof 'result'. Presumably 'result' should be tested for truthyness instead.

Currenly I get error "Cannot use 'in' operator to search for 'id' in null". This happens because the REST server responds to a PUT with status 202 and the response header fields but no response data. This is correct behaviour according to RFC2616. It is also desirable for efficiency - I don't want the REST server sending back the same data I just sent it every time. null is always passed to Cache.put because 'object' is always true and typeof is always an object.

I have tested my change and it fixes the bug.

Thanks
Allen

@neonstalwart

This comment has been minimized.

Copy link
Contributor

commented Nov 20, 2013

i'm curious if you can tell me how result is null if your server provides an empty response? i would have expected result to be undefined in that case.

@thehousecat

This comment has been minimized.

Copy link
Contributor Author

commented Nov 21, 2013

Hi Ben

I can see that if result was undefined then object would be passed into cacheStore.put instead of results. I still don't understand why the ternary test is object && typeof result == "object" instead of result && typeof result == "object" though.

To answer you question Dojo xhr.js does a good job of making sure the response is null instead of undefined:

See this code from xhr.js:

    var _deferredOk = function(/*Deferred*/dfd){
        // summary:
        //      okHandler function for dojo._ioSetArgs call.

        var ret = handlers[dfd.ioArgs.handleAs](dfd.ioArgs.xhr);
        return ret === undefined ? null : ret;
    };

And the handler the above code uses is:

        "json": function(xhr){
            // summary:
            //      A contentHandler which returns a JavaScript object created from the response data
            return json.fromJson(xhr.responseText || null);
        },

The REST server doesn't return a response body (which it isn't required to do) so xhr.responseText is an empty string. json.fromJson returns null, and you can see in _deferredOk that even if it returned undefined it would still return a null result.

Regards
Allen

@neonstalwart

This comment has been minimized.

Copy link
Contributor

commented Nov 22, 2013

thanks for the explanation of how result becomes null. i think your fix is probably the right thing to do.

/cc @kriszyp

@thehousecat

This comment has been minimized.

Copy link
Contributor Author

commented Feb 4, 2014

@neonstalwart @kriszyp are you waiting for anything else from me? I think it would be good if this fix can be applied to the official repository. Thanks.

@thehousecat

This comment has been minimized.

Copy link
Contributor Author

commented Apr 2, 2014

@dylans dylans added this to the 1.10 milestone Apr 9, 2014

@dylans

This comment has been minimized.

Copy link
Member

commented Apr 9, 2014

@thehousecat , I've marked this for the 1.10 release, this should be reviewed soon.

@thehousecat

This comment has been minimized.

Copy link
Contributor Author

commented May 5, 2014

Thanks @dylans , I look forward to it being included in 1.10.

Please see the top answer to this SO question http://stackoverflow.com/questions/797834/should-a-restful-put-operation-return-something should you need any more information on my assertion that REST PUT requests don't need to return any data. At present he Dojo Cache with Memory and JsonRest store does not support this.

@dylans

This comment has been minimized.

Copy link
Member

commented May 5, 2014

@thehousecat this was fixed 9 days ago in 9b974ae

@dylans dylans closed this May 5, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.