refactor(auth): construct User via .tap instead of positional constructor - #7057
Conversation
…} to make future migrations easier.
Automated Reviewer SuggestionsBased on the
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7057 +/- ##
============================================
+ Coverage 79.27% 79.28% +0.01%
Complexity 3787 3787
============================================
Files 1160 1160
Lines 46124 46101 -23
Branches 5114 5114
============================================
- Hits 36563 36551 -12
+ Misses 7942 7931 -11
Partials 1619 1619
*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:
|
|
| config | throughput | MB/s | latency | max Δ latest / 7d | |
|---|---|---|---|---|---|
| 🔴 | bs=10 sw=10 sl=64 | 528 | 0.322 | 18,442/24,447/24,447 us | 🔴 +6.4% / 🔴 +55.0% |
| 🔴 | bs=100 sw=10 sl=64 | 1,063 | 0.649 | 90,259/153,753/153,753 us | 🔴 +25.1% / 🔴 +43.2% |
| 🟢 | bs=1000 sw=10 sl=64 | 1,324 | 0.808 | 752,432/787,552/787,552 us | 🟢 -6.9% / 🟢 +28.4% |
Baseline details
Latest main 02ae12b from same runner
| config | metric | PR | latest main | 7d avg | Δ latest | Δ 7d |
|---|---|---|---|---|---|---|
| bs=10 sw=10 sl=64 | throughput | 528 tuples/sec | 553 tuples/sec | 786.12 tuples/sec | -4.5% | -32.8% |
| bs=10 sw=10 sl=64 | MB/s | 0.322 MB/s | 0.338 MB/s | 0.48 MB/s | -4.7% | -32.9% |
| bs=10 sw=10 sl=64 | p50 | 18,442 us | 17,327 us | 12,305 us | +6.4% | +49.9% |
| bs=10 sw=10 sl=64 | p95 | 24,447 us | 25,731 us | 15,774 us | -5.0% | +55.0% |
| bs=10 sw=10 sl=64 | p99 | 24,447 us | 25,731 us | 18,978 us | -5.0% | +28.8% |
| bs=100 sw=10 sl=64 | throughput | 1,063 tuples/sec | 1,157 tuples/sec | 999.71 tuples/sec | -8.1% | +6.3% |
| bs=100 sw=10 sl=64 | MB/s | 0.649 MB/s | 0.706 MB/s | 0.61 MB/s | -8.1% | +6.4% |
| bs=100 sw=10 sl=64 | p50 | 90,259 us | 87,230 us | 100,616 us | +3.5% | -10.3% |
| bs=100 sw=10 sl=64 | p95 | 153,753 us | 122,912 us | 107,356 us | +25.1% | +43.2% |
| bs=100 sw=10 sl=64 | p99 | 153,753 us | 122,912 us | 113,255 us | +25.1% | +35.8% |
| bs=1000 sw=10 sl=64 | throughput | 1,324 tuples/sec | 1,314 tuples/sec | 1,031 tuples/sec | +0.8% | +28.4% |
| bs=1000 sw=10 sl=64 | MB/s | 0.808 MB/s | 0.802 MB/s | 0.63 MB/s | +0.7% | +28.4% |
| bs=1000 sw=10 sl=64 | p50 | 752,432 us | 758,590 us | 980,328 us | -0.8% | -23.2% |
| bs=1000 sw=10 sl=64 | p95 | 787,552 us | 845,964 us | 1,027,528 us | -6.9% | -23.4% |
| bs=1000 sw=10 sl=64 | p99 | 787,552 us | 845,964 us | 1,054,298 us | -6.9% | -25.3% |
Raw CSV
config_idx,batch_size,schema_width,string_len,num_batches,total_ms,total_tuples,total_bytes,tuples_per_sec,mb_per_sec,lat_p50_us,lat_p95_us,lat_p99_us
0,10,10,64,20,378.54,200,128000,528,0.322,18441.81,24447.21,24447.21
1,100,10,64,20,1881.80,2000,1280000,1063,0.649,90259.42,153753.26,153753.26
2,1000,10,64,20,15100.66,20000,12800000,1324,0.808,752432.38,787552.22,787552.22|
Amber build failed on unrelated S3StorageClientSpec check, assuming flaky test. |
|
@mengw15 could you take a look when you get the chance? |
|
fixing that flaky in #7051 |
Yicong-Huang
left a comment
There was a problem hiding this comment.
🔴 1 must-fix · 1 advisory · 0 polish — clean, complete refactor; the change itself isn't blocked, the must-fix is only the title scope.
Correctness (1)
ServletAwareConfigurator.scala:96— JWT branch drops theString.valueOfwrapper around the email claim (advisory, see inline)
Conventions (1)
- Retitle →
refactor(auth): construct User via .tap instead of positional constructor— the title becomes the squash-commit message, andbackendis broader than the per-module/area scope used across merged history; all three main-code files are auth code (must-fix)
|
@Yicong-Huang All blockers fixed! |
Yicong-Huang
left a comment
There was a problem hiding this comment.
🟢 2 resolved · 0 open · 0 new — re-review of the same commit (8498bff); both prior findings are resolved and no new issues surfaced.
- Retitle (was must-fix) — resolved; the title now matches the recommended value.
String.valueOfnull-email delta (was advisory) — resolved in-thread: kept for consistency withJwtParser, and no DB migration is needed (JWT, not stored).
No positional new User(...) remains anywhere in the tree, so the #7044 refactor is complete. CI green.
Verification trace
The only correctness axis of this refactor is whether each .tap sets exactly the fields the positional call set non-null. Checked against the generated constructor order (uid, name, email, password, googleId, googleAvatar, role, comment, ...): all five conversion sites map exactly, every previously-null field stays unset, and the six referenced setters all exist on the POJO. The one intended exception is the already-resolved String.valueOf drop on the JWT email claim.
What changes were proposed in this PR?
Classes relying on the POJO User generated by jOOq uses its positional constructor which is fragile and requires us to refactor every constructor every time we edit the User table even if the new fields aren't used. Most instances of User() called it with mostly null fields which is hard to read and isn't very clear.
This PR changes that by using the .tap{} feature of Scala which allows you to construct and modify objects in place before assigned as a parameter or variable.
This allows code like this:
To be rewritten like this
Any related issues, documentation, discussions?
Closes #7044
How was this PR tested?
PR was tested against current test suite.
Was this PR authored or co-authored using generative AI tooling?
No.