Code review comment for lp:~leonardr/lazr.restful/representation-cache

Revision history for this message
Gary Poster (gary) wrote :

Summary: After we verify that this general approach gives real-world benefit, I suspect that we should always populate the cache, not only when there are no redacted fields for the current user. We could do this by stripping the security proxy for getting the initial data and populating the cache, and then doing the same logic that you do now for redacting existing JSON caches to get the actual desired end result.

IRC conversation:

[10:43am] gary_poster: leonardr: in your branch, when there is no cache, did you contemplate always generating it, even if there are redacted fields? Example: if no cache, generate dict of the entire non-redacted version; else if cache and redacted fields, parse out cache to dict; else return cache. (Now we have a non-redacted dict, if we are still here.)
[10:43am] gary_poster: Now, redact dict, turn into JSON, and return. There are variations of that, some of which might be better, but I imagine you get the drift.
[11:01am] leonardr: gary, i'm not sure what the benefit would be
[11:01am] leonardr: also, if there are redacted fields we _cannot_ calculate an unredacted cache due to the security policy
[11:05am] gary_poster: leonardr: the goal would be to create a source for further cache hits. This could be particularly important for objects that frequently have one or more fields redacted. In that case, the cache would rarely or, in the worst case, never be filled (and therefore never or rarely used). Since DB access is the main expense, you discovered, I strongly suspect that loading JSON and redacting will be significantly cheaper than simply creating the JSON.
[11:05am] gary_poster: Also, I'm skeptical of "cannot"; isn't it just a matter of doing the usual work with an unproxied object?
[11:07am] leonardr: yes, we would have to strip the proxy
[11:10am] leonardr: ok, i see what you're saying. we would cache it all the time, whether we were sending a redacted version or not
[11:10am] gary_poster: right
[11:11am] leonardr: i could certainly do that in a future branch. do you know of launchpad objects that typically have redacted fields?
[11:13am] gary_poster: bac would probably know, but he's out. My first guess: anything private, or (perhaps more interesting, perhaps not) anything referring to something provate.
[11:13am] gary_poster: private
[11:14am] leonardr: if an object's url contains private information, a link to that url would be redacted
[11:14am] gary_poster: so, that's an example?
[11:14am] leonardr: but i don't know of any specific launchpad object that does that. it's something to look for
[11:15am] gary_poster: bugs that are marked as security issues
[11:15am] gary_poster: private projects
[11:15am] gary_poster: private teams
[11:15am] gary_poster: private bugs
[11:15am] leonardr: so anything that links to those objects might end up redacted
[11:16am] gary_poster: (and there's more coming, if I understand correctly)
[11:16am] gary_poster: right
[11:17am] leonardr: ok, let's get the basic cache working, make sure it improves performance in real situations, and then i'll work on that
[11:17am] gary_poster: cool, makes sense

« Back to merge proposal