Fix handling the same key at multiple depths with API resource expansion.

Review Request #11667 — Created June 21, 2021 and submitted — Latest diff uploaded

Information

Djblets
release-2.x

Reviewers

API resource expansion, using ?expand=, could end up matching keys at
the wrong level of depth when serializing trees of objects. If there's a
resource hierarchy that looks like:

A
|- B
|  |-C
|- C

and expand=B,C, with A.B serialized before A.C, then we'd end up
with A.B.C expanded but not necessarily A.C.

This is because we were keeping a record of keys we could expand at each
new depth, removing any we were handling from the list, and if we ended
up recursing into a child resource that had the same key as a resource
we intended to expand on a parent resource, we'd skip the second
occurrence on the parent.

This only happened if B and C were listed in fields, rather than
item_child_resources.

This manifested in Review Board as an inability to expand
target_groups on a review_request_draft if we were also expanding
depends_on. The target_groups in depends_on would be matched
first, and we'd then skip the intended target_groups.

There were two causes for this:

  1. We defaulted the list of expanded resource keys usable for child
    resources to be the same exact list of expanded resources for the
    current object being serializes, which meant any time we removed one
    from the child-specific list, we also removed it for the current
    object's list.

  2. Manipulating this list during field iteration meant that we could end
    up serializing a child with an incorrect list of eligible expanded
    fields.

The fix for this is to build the list of viable expansion fields for
child resources up-front. We start off with a copy of the list made in
the request, and then we remove anything we could possibly expand from
the object being serialized. This is done before we begin serializing
any object contents, guaranteeing we have a correct list at all times.

Unit tests pass on Python 2 and 3.

Reproduced the issue in Review Board where a depends_on would prevent
target_groups from being expanded correctly. Verified this was fixed
with this change.

Commits

Files