-
Notifications
You must be signed in to change notification settings - Fork 325
fix: make AccessEntry equality consistent with from_api_repr #2218
Changes from 2 commits
969f09f
0dc6b35
7687693
6ab2c3c
33c01b5
6baa50b
949aa3c
266989a
adffcf5
4230a9d
12fae55
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -506,7 +506,20 @@ def entity_id(self) -> Optional[Union[Dict[str, Any], str]]: | |
| def __eq__(self, other): | ||
| if not isinstance(other, AccessEntry): | ||
| return NotImplemented | ||
| return self._key() == other._key() | ||
|
|
||
| return ( | ||
| self.role == other.role | ||
| and self.entity_type == other.entity_type | ||
| and self._normalize_entity_id(self.entity_id) == self._normalize_entity_id(other.entity_id) | ||
| and self.condition == other.condition | ||
| ) | ||
|
|
||
| @staticmethod | ||
| def _normalize_entity_id(value): | ||
| """Ensure consistent equality for dicts like 'view'.""" | ||
| if isinstance(value, dict): | ||
| return dict(sorted(value.items())) | ||
| return value | ||
|
|
||
| def __ne__(self, other): | ||
| return not self == other | ||
|
|
@@ -542,8 +555,22 @@ def to_api_repr(self): | |
| Returns: | ||
| Dict[str, object]: Access entry represented as an API resource | ||
| """ | ||
| resource = copy.deepcopy(self._properties) | ||
| resource = {} | ||
|
|
||
| if self.role is not None: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why can't we continue using Sorry that I am only commenting on this now, somehow I missed it the last time :/ |
||
| resource["role"] = self.role | ||
| if self.entity_type is not None: | ||
| resource[self.entity_type] = self.entity_id | ||
| if self.condition is not None: | ||
| resource["condition"] = self.condition.to_api_repr() | ||
|
|
||
| # Include any extra items preserved in _properties | ||
| for k, v in self._properties.items(): | ||
| if k not in resource: | ||
| resource[k] = v | ||
|
|
||
| return resource | ||
|
|
||
|
|
||
| @classmethod | ||
| def from_api_repr(cls, resource: dict) -> "AccessEntry": | ||
|
|
@@ -557,10 +584,32 @@ def from_api_repr(cls, resource: dict) -> "AccessEntry": | |
| google.cloud.bigquery.dataset.AccessEntry: | ||
| Access entry parsed from ``resource``. | ||
| """ | ||
|
|
||
| role = resource.get("role") | ||
| condition = None | ||
| if "condition" in resource: | ||
| condition = Condition.from_api_repr(resource["condition"]) | ||
|
|
||
| entity_type = None | ||
| entity_id = None | ||
|
|
||
| for key, value in resource.items(): | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similar to https://github.com/googleapis/python-bigquery/pull/2218/files#r2162644585, could you explain why we need to manually copy the resource dict? It doesn't seem very future-proof if we add any new fields in the future. Also, we deliberately used
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I originally switched to a manual copy/deepcopy mainly to help isolate issues during tests, especially with view/routine equality, where we needed full round-trip consistency via |
||
| if key in ("role", "condition"): | ||
| continue | ||
| entity_type = key | ||
| entity_id = value | ||
| break | ||
|
|
||
| entry = cls( | ||
| role=role, | ||
| entity_type=entity_type, | ||
| entity_id=entity_id, | ||
| condition=condition, | ||
| ) | ||
|
|
||
| access_entry = cls() | ||
| access_entry._properties = resource.copy() | ||
| return access_entry | ||
| # Preserve additional _properties for roundtrip consistency: | ||
| entry._properties = dict(resource) | ||
| return entry | ||
|
|
||
|
|
||
| class Dataset(object): | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The normalization here may not work if there's nested dictionaries, and it might happen, for example here. The most reliable way might be using
json.dumps()withsort_keys=True.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @Linchin — thanks for pointing that out.
You're absolutely right that
dict(sorted(...))won't handle deeply nested dictionaries reliably. In this case though, I tested equality onAccessEntryobjects withview,routine, anddatasetentity types — including nested structures like the one indataset. The current approach passed all assertions in the test, and it seems to hold up.Totally agree that
json.dumps(..., sort_keys=True)would be the most robust and reliable method. I will change it right now👍For reference, here’s the test case I used:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for making the change, I guess all test cases passed because we didn't juggle the order of items in the nested dictionary :)