Opened 7 years ago

Last modified 3 years ago

#16558 new defect

Cache checks if put() returned an object. Null (empty response) IS an object, it crashes

Reported by: Tony Merc Mobily Owned by: Kris Zyp
Priority: undecided Milestone: 1.15
Component: Data Version: 1.8.3
Keywords: Cc:
Blocked By: Blocking:

Description

Hi,

Please check self-contained attached file. You will need to refine a PUT in your server that returns 200 and an empty response.

Basically, with "put", Cache.js decides that whatever is returned by the call is to be considered the "current" version of the put object:

cachingStore.put(typeof result == "object" ? result : object, directives);

The trouble here is that "null" *is* an object! So, this happens:

"Cannot use 'in' operator to search for '_id' in null"

Now... It would be fine if the line in Cache looked like this:

cachingStore.put(typeof result == "object" && result !== null ? result : object, directives);

Note that Observable does indeed have that check:

action((typeof results == "object" && results)
value);

My initial idea was that for each successful PUT, I would return an JSON object containing an "OK" message, maybe a list to emit in the application, and so on. However, looking at the code it looks like both Cache and Observable (and maybe other things in Dojo?) will assume that if anything comes back from a PUT, that's the "good" version of the object. Oh well, I will live with that (probably a good thing anyway).

However, the fact that Cache.js will treat "null" as an object actually renders it completely unworkable with JsonRest?... doesn't it?

Thanks for listening!

Merc.

Attachments (1)

putError.html (2.2 KB) - added by Tony Merc Mobily 7 years ago.

Download all attachments as: .zip

Change History (12)

Changed 7 years ago by Tony Merc Mobily

Attachment: putError.html added

comment:1 Changed 7 years ago by bill

Component: GeneralData
Owner: set to Kris Zyp

comment:2 Changed 7 years ago by Tony Merc Mobily

Hi,

Meh I stuffed up the title! Can somebody with the right permissions please fix it? It should be:

Cache.js checks if put() returned an object. Null (empty response) IS an object, it crashes

(It's not Observable that has the problem)

Thanks,

Merc.

comment:3 Changed 7 years ago by bill

Summary: Observable checks if put() returned an object. Null (empty response) IS an object, it crashesCache checks if put() returned an object. Null (empty response) IS an object, it crashes

comment:4 Changed 7 years ago by Kenneth G. Franqueiro

While I'll leave it to kzyp to determine whether this is worth adding code to fix, it seems more to me like you are using an underlying store (in this case JsonRest?) whose expectations of your service differ from how your service actually operates. I don't believe Cache is unworkable with JsonRest? at all, in cases where the service behaves as it expects.

kzyp would also be the best one to speak to the expectations of JsonRest?, but I don't think it ever expects the server to respond to a PUT with an object other than the updated item itself. (Generally, I thought it usually expects to just be told the ID.) If you want to pass along other data in the response, that sounds like grounds for store customization.

comment:5 Changed 7 years ago by Tony Merc Mobily

Hi,

While I'll leave it to kzyp to determine whether this is worth adding code to fix, it seems more to me like you are using an underlying store (in this case JsonRest??) whose expectations of your service differ from how your service actually operates. I don't believe Cache is unworkable with JsonRest?? at all, in cases where the service behaves as it expects.

OK. but... what _are_ the expectations then *exactly*? Do I *always* have to return the updated object? Without the check I suggested, returning the updated object is a *requirement* to start with -- unless I decide to return *something* random, like a string, just to make sure I don't get that line of code to trigger the error.

I must be missing something here... but right now, it doesn't feel right.

(Plus, Observable *does* have that check. So there is a lack of consistency...)

Merc.

comment:6 Changed 7 years ago by Tony Merc Mobily

Hi,

kzyp would also be the best one to speak to the expectations of JsonRest??, but I don't think it ever expects the server to respond to a PUT with an object other than the updated item itself.

Does the current JsonRest? implementation _always_ expect to return an object representing the updated object? So, effectively returning an empty response isn't actually "valid"...?

Merc.

comment:7 Changed 6 years ago by bugbot

I run into the same problem. From RFC2616 it doesn't look the PUT has to return an object (but not everybody agrees):

If an existing resource is modified, either the 200 (OK) or 204 (No Content) response codes SHOULD be sent to indicate successful completion of the request

Going to apply Merc's patch by myself :)

Last edited 6 years ago by bugbot (previous) (diff)

comment:8 Changed 6 years ago by Tony Merc Mobily

Hi,

I realise that you just have much more important things to deal with and that this is a minor bug. However, it _is_ in my opinion a bug (the check is in Observer.js) but not in Cache.js...), and a very easy one to fix too...

Would you be able to consider it?

Thanks,

Merc.

comment:9 Changed 6 years ago by miker

Also have the same problem. RFC 2616 is pretty specific about the response for a PUT and the Cache.js module does not comply. PUT response for a newly created entry 201, PUT accepted but not yet acted upon (something like a definite 'maybe') 202, PUT complete 204 or 200.

Only the last, 200 OK, works with Cache.js when using dojo/store/JsonRest.js

Merc's patch fixes this. Please implement!

comment:10 Changed 4 years ago by dylan

Milestone: tbd1.12

@mercmobily, is this still an issue? Do you want to create a pull request for this?

comment:11 Changed 3 years ago by dylan

Milestone: 1.131.15

Ticket planning... move current 1.13 tickets out to 1.15 to make it easier to move tickets into the 1.13 milestone.

Note: See TracTickets for help on using tickets.