fix: make AccessEntry equality consistent with from_api_repr #2218
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
14bd1f0 to
969f09f
Compare
google/cloud/bigquery/dataset.py
Outdated
| def _normalize_entity_id(value): | ||
| """Ensure consistent equality for dicts like 'view'.""" | ||
| if isinstance(value, dict): | ||
| return dict(sorted(value.items())) |
There was a problem hiding this comment.
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() with sort_keys=True.
There was a problem hiding this comment.
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 on AccessEntry objects with view, routine, and dataset entity types — including nested structures like the one in dataset. 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:
def test_access_entry_view_equality(self):
from google.cloud import bigquery
entry1 = bigquery.dataset.AccessEntry(
entity_type="view",
entity_id={
"projectId": "my_project",
"datasetId": "my_dataset",
"tableId": "my_table",
},
)
entry2 = bigquery.dataset.AccessEntry.from_api_repr({
"view": {
"projectId": "my_project",
"datasetId": "my_dataset",
"tableId": "my_table",
}
})
entry3 = bigquery.dataset.AccessEntry(
entity_type="routine",
entity_id={
"projectId": "my_project",
"datasetId": "my_dataset",
"routineId": "my_routine",
},
)
entry4 = bigquery.dataset.AccessEntry.from_api_repr({
"routine": {
"projectId": "my_project",
"datasetId": "my_dataset",
"routineId": "my_routine",
}
})
entry5 = bigquery.dataset.AccessEntry(
entity_type="dataset",
entity_id={
"dataset": {
"projectId": "my_project",
"datasetId": "my_dataset",
},
"target_types": "VIEWS",
},
)
entry6 = bigquery.dataset.AccessEntry.from_api_repr({
"dataset": {
"dataset": {
"projectId": "my_project",
"datasetId": "my_dataset",
},
"target_types": "VIEWS",
}
})
entry7 = bigquery.dataset.AccessEntry(
entity_type="dataset",
entity_id={
"dataset": {
"location": {
"region": {
"zone": "asia-south1-a",
"subzone": "primary"
},
"country": "IN"
},
"datasetId": "my_dataset"
},
"target_types": "VIEWS"
},
)
entry8 = bigquery.dataset.AccessEntry.from_api_repr({
"dataset": {
"target_types": "VIEWS",
"dataset": {
"datasetId": "my_dataset",
"location": {
"country": "IN",
"region": {
"subzone": "primary",
"zone": "asia-south1-a"
}
}
}
}
})
assert entry1 == entry2
assert entry3 == entry4
assert entry5 == entry6
assert entry7 == entry8There was a problem hiding this comment.
Thanks for making the change, I guess all test cases passed because we didn't juggle the order of items in the nested dictionary :)
|
The lint test is failing, could you run |
|
Fixed it, should be fine now! |
|
I think there's more linting we need to fix |
|
Not related to the code changes in this PR. I ran a session with commenting the
|
Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
Fixes #2217 🦕
🐛 Bug Fix: AccessEntry equality for "view" entity
This PR fixes a bug where two
AccessEntryinstances representing the same view would not compare as equal:✅ Summary of Changes
__eq__method inAccessEntryto correctly handleentity_type="view"by normalizing the nested dictionary inentity_id.test_access_entry_view_equalityintests/unit/test_dataset.pyto assert correctness of equality logic for views.from_api_reprconsistency.