goodeggs / angular-cached-resource

An AngularJS module to interact with RESTful resources, even when browser is offline
MIT License
216 stars 29 forks source link

Resources without boundParams get a "default" cache key #36

Open hazeledmands opened 10 years ago

hazeledmands commented 10 years ago

(and maybe a warning that can be disabled).

From a comment on the bites blog:

Dear Max, I am having such a problem:

ngCachedResource instance [object Object] doesn't have any boundParams. Please, make sure you specified them in your resource's initialization, f.e. {id: "@id"}, or it won't be cached.

Why is that error happens? The requests are done but seemingly no caching happening. My URL does not have any bound Params so I left the field empty, like so:

// URL has no params var containerUrl = ConstantValueService.get('webApiUri') + '/api/containertype'; // trying to get it cached return $cachedResource('sw_containerTypes',containerUrl, {}, { // rest methods getAll: {...}}

My response:

Hi Askar, sorry you're having difficulties with our module!

$cachedResource uses the bound parameters of the objects returned by an endpoint as a cache key (that's how it differentiates between objects!).

In your use case, do you only ever call getAll? Does the getAll endpoint have "isArray: true" set? If so, then you should be fine setting a bound parameter even though the parameter does not show up in the URL; set it to anything that could be a unique identifier returned by the getAll endpoint, like {id: '@id"}.

I've added an issue on github describing your use case; it seems like we could relatively easily allow for "localy defined" default cache keys, to alleviate this problem.

Hope that helps!

~Max

The proposed solution would only work for isArray cache keys, and is not amazingly ideal, but it would make the module useful for more people.

geoah commented 9 years ago

How would you propose to properly solve this issue?

We seem to be having the same issue with @superdecimal so I was playing around and was very tempted to try and force a default pair on the params.

I was thinking something along the lines of https://github.com/geoah/angular-cached-resource/commit/4631bcfc70a22e42d374f750e10e206fbcd70118 which seems to be working.

Probably adding a providerParams.forceParam option to trigger the param enforcement would be needed. eg providerParams.forceParam = true; or providerParams.forceParam = { key: "_fc", value: "default" };

At least this way I can get the forceParam and pass the tests. Any better ideas? I've been only playing with this for half an hour so maybe I'm missing something.

hazeledmands commented 9 years ago

Seems like you're on the right track, though in your code, every instance in the array returned from the server would have the same cache key -- so the cache would turn a response like this:

[ {a : 1}, {b: 2}, {c: 3} ]

into this, on the second request to the same resource:

[ { c: 3}, { c: 3 }, { c: 3 } ]

If you changed your code just a little bit, to have the default param use the array index as well, it'd fix that issue. Something like this:

    addInstances: (instances, dirty) ->
      cacheArrayReferences = []
      for instance, index in instances
        cacheInstanceParams = instance.$params()
        if Object.keys(cacheInstanceParams).length is 0
          cacheInstanceParams["_fc"] = "default-#{index}"
        cacheArrayReferences.push cacheInstanceParams
        cacheInstanceEntry = new ResourceCacheEntry(@key, cacheInstanceParams).load()
        cacheInstanceEntry.set instance, dirty
      @set cacheArrayReferences, dirty

Want to write a pull request? You're halfway there! Some thoughts, to make it perfect:

geoah commented 9 years ago

First of all thank you for a very fast response. :)

I've started working on it. The easy part is done. https://github.com/geoah/angular-cached-resource/commit/b2e7ae1ea6b31a253ed9161a782836bd4f498e55 It's a bit more verbose than it should but you are most welcome to make any changes you'd like before considering merging.

Now to the tests... I'll try to write tests to check that

Anything else I should check?

ps. As I've never tested anything like this before I promise that this will work! :P As you can see https://gist.github.com/geoah/7477ddbfad915554423f it isn't working well so far. ~hehe.

pencilcheck commented 9 years ago

Somehow I am getting the warning when I have all boundParam specified. Using 1.4.1

joaom182 commented 9 years ago

I have the same issue, my endpoint return an array instead of object, so i get the error

ngCachedResource 'Tag' instance doesn't have any boundParams. Please, make sure you specified them in your resource's initialization, f.e.{id: "@id"}, or it won't be cached

+1