JR 0.9 ignores #records_for, crashes from nonstandard #records

[NuckChorris]

We've moved our main app to JR 0.9 (yay caching!) but are facing an issue where #records_for isn't being called to get associated records for a request with include param. Instead, the associated resource's #records method is being called.

I'm inclined to call this a regression, honestly, since #records_for is still documented as a potential hookup location for authorization code. I was actually just about to override this so that certain associations would ignore our authorization code, and found that it wasn't being called at all.

It doesn't appear to be a security issue for us (due to the way our authorization works), but this behavior could potentially cause security issues in other apps, depending on how they are architected (namely, if their #records_for authorized differently from their #records)

Strongly related: if the #records call on the associated resource causes some of the associations to be nonexistent, it causes an exception, since it can't find the ID of the associated record in the Hash it's built up for preload data

[lgebhardt]

I'm sorry I didn't catch this during testing. I'm looking into the best way to handle it in a future 0.9.x release. For now my advice is to not use caching for resources which implement records_for or records_for_#{relationship_name}.

[NuckChorris]

@lgebhardt I haven't verified, but I think it might not even call records_for when there isn't caching.

[lgebhardt]

@NuckChorris In my testing it's still being called.

[lgebhardt]

@NuckChorris Looks like I might be wrong, going by the caveats section of the caching documentation:

• If resource caching is enabled at all, then custom relationship methods on any resource might not always be used, even resources that are not cached. For example, if you manually define a comments method or records_for_comments method on a Resource that has_many :comments, you cannot expect it to be used when caching is enabled, even if you never call caching on that particular Resource. Instead, you should use relationship name lambdas.
• The above also applies to custom find or find_by_key methods. Instead, if you are using resource caching anywhere in your app, try overriding the find_records method to return an appropriate ActiveRecord::Relation.

[NuckChorris]

That seems like a pretty huge caveat. Couldn't it just call records_for and then pluck the columns for cache keys from that? Seems strange to completely ignore these methods when they return relatioms

[lgebhardt]

@NuckChorris I agree, it's a big caveat. I have updated the docs with a note in the finders section. Hopefully we'll get a fix in place to remove the limitation, but if not I'll update the docs at a minimum.

[NuckChorris]

Were either of these fixed by @DavidMikeSimon in #1023? The patchset's a bit dense, and I'm not 100% up to speed on the new Accessor system in 0.10 (though it sounds like it'll make a lot of our custom code less brittle and that's very welcome)

[DavidMikeSimon]

@NuckChorris Caching code doesn't use records_for because it's a Resource instance method which would not be available when the cache delivers a hit.

One possibility would be to have a class-level variation on the records_for method that takes a relation to the source table and returns another relation to the associated table. That might get confusing, though; if both were available, which one would JR call?

I've been dealing with view security stuff in my own code purely with the records method, and it works reasonably well but the code can get a little hairy. :-( OTOH, it may be better to only have one place where access to a resource is limited; that way, there's no risk of a user getting at an unauthorized resource through an unexpected path.

I have better news about the second bug you reported, the crash on failure to preload association: that's fixed in master. :-) Now, if that happens (e.g. due to polymorphic associations, which cannot be preloaded), then it just falls back to instantiating the Resources manually.

[DavidMikeSimon]

@lgebhardt BTW, I have a PR for the caching docs which spells out that records_for is not used: cerebris/jsonapi-resources-site#8