Unable to access both deleted and active records

[Rick Harris]

Per https://bugs.launchpad.net/nova/+bug/701121, Vish indicates that it's intentional that read_deleted returns only deleted records and, moreover, that there is no way for a context object to have access to both deleted and undeleted records.

In cases of bad data we may have cases where something is deleted but a related object isn't. In order to perform the cleanup, we need an admin context that can see both objects.

To fix this, I'm proposing refactoring `context.read_deleted` (bool) into `context.deleted_visibility` (str).

The valid values of `deleted_visibility` would be `not_visible` (the default, read_deleted=False), `only_deleted` (this would map to read_deleted=True), and `visible` (new behavior, would display deleted and undeleted records).

[Brian Waldon (bcwaldon)]

I'm all for showing active and deleted records. i would prefer that rather than using an enum in deleted_visibility, that do this:

1) change context.read_delete to context.can_read_deleted. This indicates that the authenticated user has access to deleted records. We should return either only active or active AND deleted records based on this attribute.

2) Support a boolean 'deleted' filter on our queries that allows users to narrow down results to only deleted or only active. This allows us to keep the behavior where an admin can see only deleted records. We also shouldn't filter on a specific value for deleted by default.

[Rick Harris (rconradharris)]

I had batted around the idea of using two boolean flags to represent this. In the end, I rejected that idea (for now) based on two reasons:

1. Splitting the variable into two variables felt like it would be more difficult to get right. Given the amount of delicate code that has to change, I wasn't sure that I could manage that refactoring and ensure approx 100% correctness.

With a single variable becoming an enum, I could wrap my head around it a bit more, and found it much easier to spot check the code.

2. Having 2 boolean variables leads to 4 combinations. But only 3 combinations are valid: (cannot-read-deleted, fetch-deleted) is bogus. It felt cleaner and safer to prevent anyone from even attempting that combination by using the enum.

[Vish Ishaya (vishvananda)]

read_deleted was not intended to specify whether the user was allowed to read deleted records, so much as whether the call should read deleted records. It was added to the context object because it would otherwise require changing every single db call to support an additional flag.

Can't we just key the authorization for can_read_deleted off of is_admin, and just have read_deleted specify whether to retrieve deleted records?

[Rick Harris (rconradharris)]

Vish: if we do that, then how do we specify that we can see *both* active and deleted records?

We can't simply perform an extra query each time since all of our helper code is deeply nested and uses a single context. We really need to be able to say, at a top-level, this context can see both active and deleted records.

All that said, I really think this should just be a stop gap; ultimately we should come up with a more elegant solution that can handle all 3 scenarios (live, deleted, and both).

[Openstack Gerrit (openstack-gerrit)]

Reviewed: https://review.openstack.org/2131
Committed: http://github.com/openstack/nova/commit/c40ee5cfe75e8b1209dc53fc7eb2097812efa54e
Submitter: Jenkins
Branch: master

 status fixcommitted
 done

commit c40ee5cfe75e8b1209dc53fc7eb2097812efa54e
Author: Rick Harris <email address hidden>
Date: Wed Dec 7 16:06:31 2011 -0600

    Add ability to see deleted and active records.

    Fixes bug #900564

    Changes `Context`.`read_deleted` from a bool to an enum string with values
    "yes" (can read deleted records), "no" (cannot read deleted records), and
    "only" (can only see deleted records, for backwards compatibility).

    Change-Id: Ic81db3664c33f23f751b73973782efb06fce90d9