Skip to content

Conversation

@SageGJ
Copy link
Contributor

@SageGJ SageGJ commented Oct 16, 2025

Problem:

The error messages displayed when a user tried to delete rows from a table and the rows did not exist were unclear and confusing.

Solution:

The way discrepant rows were identified and formatted for display were changed. A new test was added to ensure that the error message accurately conveys what the error was and where the problem lies.

Testing:

Unit tests were updated, new integration tests were added, and the validation script was used afterwards.

Continuation of #1254

@SageGJ SageGJ changed the title Exception handling synpy 1667 Update Error Messaging when Attempting to Delete Rows that do not exist from Tables Oct 16, 2025
@SageGJ SageGJ changed the title Update Error Messaging when Attempting to Delete Rows that do not exist from Tables [SYNPY-1667]Update Error Messaging when Attempting to Delete Rows that do not exist from Tables Oct 16, 2025
@SageGJ SageGJ marked this pull request as ready for review October 17, 2025 17:18
@SageGJ SageGJ requested a review from a team as a code owner October 17, 2025 17:18
Copy link
Member

@thomasyu888 thomasyu888 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔥 LGTM! I'll defer to the team for final review

Copy link
Member

@BryanFauble BryanFauble left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks for this patch

@danlu1
Copy link
Contributor

danlu1 commented Oct 17, 2025

LGTM, thanks for working on this.

@SageGJ SageGJ force-pushed the exception-handling-SYNPY-1667 branch from 8cc052e to 172071d Compare October 17, 2025 18:24
@SageGJ
Copy link
Contributor Author

SageGJ commented Oct 20, 2025

The last failure was a single test that had permission issues:

FAILED tests/integration/synapseclient/models/synchronous/test_permissions.py::TestDeletePermissions::test_delete_permissions_multiple_nested_branches - synapseclient.core.exceptions.SynapseHTTPError: 409 Client Error: An entity with the name: beta_level_0_e30018be-5aec-405c-8156-1329098df08e already exists with a parentId: syn18180277

I'm going to go ahead and merge this pr given that this was a known issue.
cc @BryanFauble

@BryanFauble
Copy link
Member

The last failure was a single test that had permission issues:

FAILED tests/integration/synapseclient/models/synchronous/test_permissions.py::TestDeletePermissions::test_delete_permissions_multiple_nested_branches - synapseclient.core.exceptions.SynapseHTTPError: 409 Client Error: An entity with the name: beta_level_0_e30018be-5aec-405c-8156-1329098df08e already exists with a parentId: syn18180277

I'm going to go ahead and merge this pr given that this was a known issue. cc @BryanFauble

The other versions passed so I dont have an issue merging just because of this failure. Either way - I did go ahead and reran the failed test in github action.

@SageGJ SageGJ merged commit 41b0c30 into develop Oct 20, 2025
44 of 45 checks passed
@SageGJ SageGJ deleted the exception-handling-SYNPY-1667 branch October 20, 2025 17:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants