fix(computing-unit, v1.2): fix 500s in share/revoke (unknown email + privilege change) - #7008
Open
Yicong-Huang wants to merge 1 commit into
Open
fix(computing-unit, v1.2): fix 500s in share/revoke (unknown email + privilege change) #7008Yicong-Huang wants to merge 1 commit into
Yicong-Huang wants to merge 1 commit into
Conversation
…ege change) (#6446) ### What changes were proposed in this PR? Fixes two HTTP 500s in ComputingUnitAccessResource's sharing endpoints, both hit by ordinary user actions. 1. Sharing/revoking to an unknown email -> 500 (NPE). grantAccess and revokeAccess read userDao.fetchOneByEmail(email).getUid before null-checking: val granteeId = userDao.fetchOneByEmail(email).getUid // NPE when email is unknown if (granteeId == null) { ... } // dead code, never reached fetchOneByEmail returns null for an unknown email, so .getUid threw an NPE before the guard, surfacing as an opaque 500, which is the intended "User with the given email does not exist" error was unreachable. Both methods now null-check the fetched user first, so an unknown email returns a clear IllegalArgumentException (4xx). Mistyping an email when sharing is common, so this replaces a server error with an actionable message. 2. Changing an existing collaborator's access level -> 500 (duplicate key). grantAccess used insert, so re-granting a user who already had access - what the share dialog does when you change a privilege via the dropdown, violated the (cuid, uid) primary key. Switched to merge (upsert) so re-granting updates the privilege in place, matching DatasetAccessResource/WorkflowAccessResource/ProjectAccessResource, from which this resource had diverged. ### Any related issues, documentation, discussions? Closes #6445 ### How was this PR tested? Added ComputingUnitAccessResourceSpec (embedded Postgres via MockTexeraDB), 8 tests covering both methods: happy path, unknown email (fix 1), re-grant privilege change (fix 2), and no-write-access. Verified red->green: against the old code the unknown-email cases fail with NullPointerException and the re-grant case with a duplicate-key IntegrityConstraintViolationException; after the fixes all 8 pass. The module had no DB-backed test wiring, so build.sbt adds DAO % "test->test" (for MockTexeraDB + embedded Postgres) and forks the test JVM with COMPUTING_UNIT_SHARING_ENABLED=true and the repo root as the working directory (so MockTexeraDB can resolve sql/texera_ddl.sql). Run: sbt "ComputingUnitManagingService/testOnly org.apache.texera.service.resource.ComputingUnitAccessResourceSpec" ### Was this PR authored or co-authored using generative AI tooling? Co-authored with Claude Opus 4.8 in compliance with ASF. (backported from commit 8df38af)
Contributor
Automated Reviewer SuggestionsBased on the
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## release/v1.2 #7008 +/- ##
===============================================
Coverage 53.95% 53.95%
Complexity 1441 1441
===============================================
Files 809 809
Lines 34144 34144
Branches 3448 3447 -1
===============================================
Hits 18421 18421
Misses 14815 14815
Partials 908 908
*This pull request uses carry forward flags. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this PR?
Backport of #6446 to
release/v1.2, cherry-picked from 8df38af.Source fix only. #'s test and its test-only build dependency (
configs(Test).dependsOn(DAO % "test->test")) are omitted — the test spec does not exist onrelease/v1.2. Only the source fix is carried over, per maintainer guidance.Any related issues, documentation, discussions?
Backport of #6446. Originally linked #6445.
How was this PR tested?
Release-branch CI runs on this PR.
Was this PR authored or co-authored using generative AI tooling?
Yes — backport prepared with Claude Code (cherry-pick + manual conflict resolution; the change itself is #6446 by its original author).
🤖 Generated with Claude Code