Skip to content
This repository was archived by the owner on Mar 6, 2026. It is now read-only.

fix: make AccessEntry equality consistent with from_api_repr - #2218

Merged
Linchin merged 11 commits into
googleapis:mainfrom
drokeye:fix-accessentry-equality
Jun 26, 2025
Merged

fix: make AccessEntry equality consistent with from_api_repr#2218
Linchin merged 11 commits into
googleapis:mainfrom
drokeye:fix-accessentry-equality

Conversation

@drokeye

@drokeyedrokeye commented Jun 18, 2025

Copy link
Copy Markdown
Contributor

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:

  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)

Fixes#2217 🦕

🐛 Bug Fix: AccessEntry equality for "view" entity

This PR fixes a bug where two AccessEntry instances representing the same view would not compare as equal:

AccessEntry(entity_type="view", entity_id={"projectId": ..., ...}) !=AccessEntry.from_api_repr({...})

✅ Summary of Changes

  • Updated the __eq__ method in AccessEntry to correctly handle entity_type="view" by normalizing the nested dictionary in entity_id.
  • Added a new unit test test_access_entry_view_equality in tests/unit/test_dataset.py to assert correctness of equality logic for views.
  • Ensured compatibility with existing functionality and from_api_repr consistency.

@drokeye
drokeye requested review from a team and PhongChuongJune 18, 2025 08:13
@google-cla

Copy link
Copy Markdown

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.

@product-auto-labelproduct-auto-labelBot added the size: m Pull request size is medium. label Jun 18, 2025
@product-auto-labelproduct-auto-labelBot added the api: bigquery Issues related to the googleapis/python-bigquery API. label Jun 18, 2025
@drokeye
drokeyeforce-pushed the fix-accessentry-equality branch from 14bd1f0 to 969f09fCompareJune 18, 2025 08:28
@LinchinLinchin self-assigned this Jun 18, 2025
Comment threadgoogle/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()))

Copy link
Copy Markdown
Contributor

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() with sort_keys=True.

@drokeyedrokeyeJun 19, 2025

Copy link
Copy Markdown
ContributorAuthor

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 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:

deftest_access_entry_view_equality(self):
fromgoogle.cloudimportbigqueryentry1=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"
}
}
}
}
})
assertentry1==entry2assertentry3==entry4assertentry5==entry6assertentry7==entry8

Copy link
Copy Markdown
Contributor

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 :)

@chalmerlowechalmerlowe removed their assignment Jun 18, 2025
@drokeyedrokeye changed the title fix: make AccessEntry equality consistent for view entity typefix: make AccessEntry equality consistent with from_api_reprJun 19, 2025
@drokeye
drokeye requested a review from LinchinJune 19, 2025 02:55
@LinchinLinchin added kokoro:run Add this label to force Kokoro to re-run the tests. kokoro:force-run Add this label to force Kokoro to re-run the tests. labels Jun 20, 2025
@yoshi-kokoroyoshi-kokoro removed kokoro:run Add this label to force Kokoro to re-run the tests. kokoro:force-run Add this label to force Kokoro to re-run the tests. labels Jun 20, 2025
@Linchin

Copy link
Copy Markdown
Contributor

The lint test is failing, could you run nox -r -s lint in the root folder to fix it? Thanks

@drokeye

Copy link
Copy Markdown
ContributorAuthor

Fixed it, should be fine now!

@LinchinLinchin added kokoro:run Add this label to force Kokoro to re-run the tests. kokoro:force-run Add this label to force Kokoro to re-run the tests. labels Jun 23, 2025
@yoshi-kokoroyoshi-kokoro removed kokoro:run Add this label to force Kokoro to re-run the tests. kokoro:force-run Add this label to force Kokoro to re-run the tests. labels Jun 23, 2025
@Linchin

Copy link
Copy Markdown
Contributor

I think there's more linting we need to fix

@LinchinLinchin added the kokoro:run Add this label to force Kokoro to re-run the tests. label Jun 23, 2025
@yoshi-kokoroyoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jun 24, 2025
@LinchinLinchin added kokoro:run Add this label to force Kokoro to re-run the tests. kokoro:force-run Add this label to force Kokoro to re-run the tests. labels Jun 24, 2025
@yoshi-kokoroyoshi-kokoro removed kokoro:run Add this label to force Kokoro to re-run the tests. kokoro:force-run Add this label to force Kokoro to re-run the tests. labels Jun 24, 2025
@LinchinLinchin added kokoro:run Add this label to force Kokoro to re-run the tests. kokoro:force-run Add this label to force Kokoro to re-run the tests. labels Jun 24, 2025
@yoshi-kokoroyoshi-kokoro removed kokoro:run Add this label to force Kokoro to re-run the tests. kokoro:force-run Add this label to force Kokoro to re-run the tests. labels Jun 24, 2025
@drokeye

drokeye commented Jun 25, 2025

Copy link
Copy Markdown
ContributorAuthor

nox -r -s lint fails locally due to a flake8 error in docs/samples, which contains non-python or partial files.

image

Not related to the code changes in this PR. I ran a session with commenting the session.run("flake8", os.path.join("docs", "samples")) from noxfile.py and everything seems fine👍🏻

image

@LinchinLinchin added kokoro:run Add this label to force Kokoro to re-run the tests. kokoro:force-run Add this label to force Kokoro to re-run the tests. labels Jun 26, 2025
@yoshi-kokoroyoshi-kokoro removed kokoro:run Add this label to force Kokoro to re-run the tests. kokoro:force-run Add this label to force Kokoro to re-run the tests. labels Jun 26, 2025
@LinchinLinchin added kokoro:run Add this label to force Kokoro to re-run the tests. kokoro:force-run Add this label to force Kokoro to re-run the tests. labels Jun 26, 2025
@yoshi-kokoroyoshi-kokoro removed kokoro:run Add this label to force Kokoro to re-run the tests. kokoro:force-run Add this label to force Kokoro to re-run the tests. labels Jun 26, 2025
@Linchin
Linchin merged commit 4941de4 into googleapis:mainJun 26, 2025
@release-pleaserelease-pleaseBot mentioned this pull request Jun 26, 2025
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

api: bigqueryIssues related to the googleapis/python-bigquery API.size: mPull request size is medium.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

AccessEntry.from_api_repr != AccessEntry when entity_type = view

5 participants

@drokeye@Linchin@shollyman@chalmerlowe@yoshi-kokoro