test(common-auth): add HeaderFieldSpec for header-name constants#5790
Open
lalalastella wants to merge 1 commit into
Open
test(common-auth): add HeaderFieldSpec for header-name constants#5790lalalastella wants to merge 1 commit into
lalalastella wants to merge 1 commit into
Conversation
Pins the exact value, RFC-7230-compatible format, and distinctness of every constant in HeaderField so accidental renames are caught by CI. Closes apache#5662
Contributor
|
👋 Thanks for your first contribution to Texera, @lalalastella! If you're looking for a good place to start, browse issues labeled You can drive common housekeeping yourself by commenting one of these commands on its own line:
Each command must match exactly: |
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a ScalaTest spec in common/auth to lock down the canonical HTTP header-name constants used by the auth layer, preventing accidental renames from silently breaking header-based authentication.
Changes:
- Introduce
HeaderFieldSpecto assert exact string values for eachHeaderFieldconstant. - Add validation that all header constants follow the expected
x-user-*lowercase-hyphen naming pattern. - Add a distinctness check to prevent accidental aliasing between header constants.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+43
to
+64
| "HeaderField constants" should "all match the x-user-* namespace in lowercase-hyphen format" in { | ||
| val rfcPattern = "^x-user-[a-z]+(?:-[a-z]+)*$".r | ||
| val allConstants = Seq( | ||
| HeaderField.UserComputingUnitAccess, | ||
| HeaderField.UserId, | ||
| HeaderField.UserName, | ||
| HeaderField.UserEmail | ||
| ) | ||
| allConstants.foreach { value => | ||
| rfcPattern.matches(value) shouldBe true | ||
| } | ||
| } | ||
|
|
||
| it should "all be distinct (no accidental aliasing)" in { | ||
| val allConstants = Seq( | ||
| HeaderField.UserComputingUnitAccess, | ||
| HeaderField.UserId, | ||
| HeaderField.UserName, | ||
| HeaderField.UserEmail | ||
| ) | ||
| allConstants.distinct should have size allConstants.size | ||
| } |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5790 +/- ##
============================================
- Coverage 53.30% 53.29% -0.02%
+ Complexity 2668 2663 -5
============================================
Files 1098 1098
Lines 42532 42532
Branches 4575 4575
============================================
- Hits 22673 22666 -7
- Misses 18530 18535 +5
- Partials 1329 1331 +2
*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:
|
aglinxinyuan
left a comment
aglinxinyuan
left a comment
Contributor
There was a problem hiding this comment.
Please address the comments.
Contributor
|
| config | throughput | MB/s | latency | max Δ latest / 7d | |
|---|---|---|---|---|---|
| 🔴 | bs=10 sw=10 sl=64 | 424 | 0.259 | 23,939/34,546/34,546 us | 🔴 +26.9% / ⚪ within ±5% |
| ⚪ | bs=100 sw=10 sl=64 | 932 | 0.569 | 104,294/147,693/147,693 us | ⚪ within ±5% / 🟢 -7.1% |
| 🔴 | bs=1000 sw=10 sl=64 | 1,087 | 0.664 | 919,387/1,023,222/1,023,222 us | 🔴 +9.2% / 🟢 -5.5% |
Baseline details
Latest main 731d671 from same runner
| config | metric | PR | latest main | 7d avg | Δ latest | Δ 7d |
|---|---|---|---|---|---|---|
| bs=10 sw=10 sl=64 | throughput | 424 tuples/sec | 456 tuples/sec | 410.82 tuples/sec | -7.0% | +3.2% |
| bs=10 sw=10 sl=64 | MB/s | 0.259 MB/s | 0.278 MB/s | 0.251 MB/s | -6.8% | +3.3% |
| bs=10 sw=10 sl=64 | p50 | 23,939 us | 18,864 us | 23,785 us | +26.9% | +0.6% |
| bs=10 sw=10 sl=64 | p95 | 34,546 us | 31,922 us | 34,980 us | +8.2% | -1.2% |
| bs=10 sw=10 sl=64 | p99 | 34,546 us | 31,922 us | 34,980 us | +8.2% | -1.2% |
| bs=100 sw=10 sl=64 | throughput | 932 tuples/sec | 914 tuples/sec | 891.94 tuples/sec | +2.0% | +4.5% |
| bs=100 sw=10 sl=64 | MB/s | 0.569 MB/s | 0.558 MB/s | 0.544 MB/s | +2.0% | +4.5% |
| bs=100 sw=10 sl=64 | p50 | 104,294 us | 104,885 us | 112,277 us | -0.6% | -7.1% |
| bs=100 sw=10 sl=64 | p95 | 147,693 us | 142,874 us | 139,802 us | +3.4% | +5.6% |
| bs=100 sw=10 sl=64 | p99 | 147,693 us | 142,874 us | 139,802 us | +3.4% | +5.6% |
| bs=1000 sw=10 sl=64 | throughput | 1,087 tuples/sec | 1,115 tuples/sec | 1,041 tuples/sec | -2.5% | +4.4% |
| bs=1000 sw=10 sl=64 | MB/s | 0.664 MB/s | 0.681 MB/s | 0.635 MB/s | -2.5% | +4.5% |
| bs=1000 sw=10 sl=64 | p50 | 919,387 us | 899,773 us | 972,714 us | +2.2% | -5.5% |
| bs=1000 sw=10 sl=64 | p95 | 1,023,222 us | 936,880 us | 1,023,057 us | +9.2% | +0.0% |
| bs=1000 sw=10 sl=64 | p99 | 1,023,222 us | 936,880 us | 1,023,057 us | +9.2% | +0.0% |
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,471.34,200,128000,424,0.259,23938.92,34546.20,34546.20
1,100,10,64,20,2146.87,2000,1280000,932,0.569,104293.74,147692.57,147692.57
2,1000,10,64,20,18391.39,20000,12800000,1087,0.664,919387.06,1023222.02,1023222.02
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
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?
Add
HeaderFieldSpec.scala— a dedicated unit spec forHeaderFieldincommon/auth/src/main/scala/org/apache/texera/auth/util/.HeaderFieldholds the canonical HTTP header names the auth layer reads from every request. Without a spec, accidental renames silently break header-based authentication. This PR pins three contracts:x-user-*lowercase-hyphen namespaceNo production-code changes.
Any related issues, documentation, discussions?
Closes #5662
How was this PR tested?
New spec file:
HeaderFieldSpec.scala(~50 lines, no build changes needed — ScalaTest is already a% Testdependency incommon/auth).Was this PR authored or co-authored using generative AI tooling?
Generated-by: Claude Sonnet 4.6