refactor: make gateway health non-nullable and unify challenge types#4428
refactor: make gateway health non-nullable and unify challenge types#4428
Conversation
chronark
requested review from
Flo4604,
MichaelUnkey,
imeyer,
mcstepp,
ogzhanolguncu and
perkinsjr
as code owners
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
📝 WalkthroughWalkthroughThis pull request refactors database models and schema by renaming challenge type fields, reorganizing ACME challenge enum types, simplifying gateway health logic, and updating type signatures across generated Go code and TypeScript schemas to reflect these structural changes. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/db/src/schema/acme_challenges.ts (1)
3-26: Break the new circular dependency betweenacme_challenges.tsandcustom_domains.ts.
acme_challenges.tsimportscustomDomains(foracmeChallengeRelations), andcustom_domains.tsnow importschallengeTypefrom./acme_challenges. This creates a runtime import cycle where:
challengeTypeis used at top level to build table schemas incustom_domains.ts, andcustomDomainsis used at top level for relations inacme_challenges.ts.In both CJS and ESM, this pattern can leave one side uninitialized/undefined at module initialization time, leading to hard‑to‑debug runtime errors or incorrect schema wiring.
Consider one of these options to break the cycle:
- Move
challengeTypeinto a small, dependency‑free module (e.g../challenge_typeor./util/challenge_type) imported by bothacme_challenges.tsandcustom_domains.ts, or- Keep
challengeTypehere but moveacmeChallengeRelationsinto a separate relations file (or intocustom_domains.ts) that imports both tables, soacme_challenges.tsno longer needs to importcustomDomains.Once the cycle is broken, the shared enum +
.notNull()and the constraint array look good.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
go/apps/ingress/services/router/service.go(1 hunks)go/cmd/dev/seed/ingress.go(1 hunks)go/pkg/db/acme_challenge_find_by_token.sql_generated.go(2 hunks)go/pkg/db/acme_challenge_insert.sql_generated.go(4 hunks)go/pkg/db/acme_challenge_list_executable.sql_generated.go(2 hunks)go/pkg/db/bulk_acme_challenge_insert.sql_generated.go(2 hunks)go/pkg/db/gateway_insert.sql_generated.go(1 hunks)go/pkg/db/models_generated.go(5 hunks)go/pkg/db/querier_generated.go(3 hunks)go/pkg/db/queries/acme_challenge_insert.sql(1 hunks)go/pkg/db/queries/acme_challenge_list_executable.sql(1 hunks)go/pkg/db/schema.sql(3 hunks)internal/db/src/schema/acme_challenges.ts(1 hunks)internal/db/src/schema/custom_domains.ts(2 hunks)internal/db/src/schema/gateways.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 4190
File: go/internal/services/keys/verifier.go:51-53
Timestamp: 2025-10-30T15:10:52.743Z
Learning: PR #4190 for unkeyed/unkey is focused solely on database schema and query changes for identity-based credits. It adds IdentityCredits and KeyCredits fields to structs and queries, but does not implement the priority enforcement logic in the usagelimiter. The logic implementation is intentionally deferred to a later PR in the stack.
📚 Learning: 2025-07-17T14:24:20.403Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3631
File: go/pkg/db/bulk_keyring_insert.sql.go:23-25
Timestamp: 2025-07-17T14:24:20.403Z
Learning: In go/pkg/db/bulk_keyring_insert.sql.go and similar bulk insert generated files, hardcoded zero values for fields like size_approx and size_last_updated_at are intentional and reflect the original SQL query structure, not missing parameters.
Applied to files:
go/pkg/db/gateway_insert.sql_generated.gogo/pkg/db/bulk_acme_challenge_insert.sql_generated.go
🧬 Code graph analysis (7)
go/apps/ingress/services/router/service.go (1)
go/pkg/db/models_generated.go (1)
GatewaysHealthHealthy(286-286)
go/pkg/db/gateway_insert.sql_generated.go (1)
go/pkg/db/models_generated.go (1)
GatewaysHealth(281-281)
go/pkg/db/acme_challenge_find_by_token.sql_generated.go (3)
go/pkg/db/queries.go (1)
Queries(3-3)go/pkg/db/interface.go (1)
DBTX(29-34)go/pkg/db/models_generated.go (1)
AcmeChallenge(627-637)
go/cmd/dev/seed/ingress.go (1)
go/pkg/db/models_generated.go (1)
GatewaysHealthHealthy(286-286)
go/pkg/db/acme_challenge_insert.sql_generated.go (1)
go/pkg/db/models_generated.go (2)
AcmeChallengesStatus(58-58)AcmeChallengesChallengeType(16-16)
internal/db/src/schema/custom_domains.ts (1)
internal/db/src/schema/acme_challenges.ts (1)
challengeType(7-7)
go/pkg/db/acme_challenge_list_executable.sql_generated.go (3)
go/pkg/db/models_generated.go (1)
AcmeChallengesChallengeType(16-16)go/pkg/db/queries.go (1)
Queries(3-3)go/pkg/db/interface.go (1)
DBTX(29-34)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Test Dashboard / Test Dashboard
- GitHub Check: Test Go API Local / Test
- GitHub Check: Test API / API Test Local
- GitHub Check: Test Packages / Test
- GitHub Check: Build / Build
🔇 Additional comments (15)
internal/db/src/schema/custom_domains.ts (1)
1-19: SharedchallengeTypeenum changes behavior forcustom_domains.Switching to the shared
challengeTypeenum is good for consistency, but it has two notable behavior changes compared to the previous inline definition:
- Enum values are now
HTTP-01/DNS-01(different spelling/casing from the oldhttp01/dns01style mentioned in the summary).- The previous default is gone;
challenge_typeis now strictly NOT NULL with no default, so every insert must set it explicitly.Please double‑check that:
- Existing rows (if any) are migrated to the new enum values, and
- All write paths that insert into
custom_domainsnow explicitly setchallenge_type, otherwise inserts will fail.Also, this import contributes to a circular dependency with
acme_challenges.ts; see the other comment suggesting how to break that cycle.go/pkg/db/queries/acme_challenge_list_executable.sql (1)
2-6: Column rename tochallenge_typeis consistent and non‑breaking.Updating both the SELECT list and
IN (sqlc.slice(verification_types))to usedc.challenge_typecleanly aligns this query with the new column and Go enum type, without changing the filter semantics.go/pkg/db/schema.sql (1)
392-425: Schema enums now match the refactor; verify data/migration coverage.The schema updates look internally consistent:
custom_domains.challenge_typeandacme_challenges.challenge_typeboth useenum('HTTP-01','DNS-01') NOT NULL, matching the shared TS enum.gateways.healthis now a non‑nullable enum with an'unknown'default, which works with the router logic that only routes whenhealth = 'healthy'.Please verify that:
- Existing
custom_domains/acme_challengesrows (and any migrations) are updated to the newHTTP-01/DNS-01values.- All insert/update code paths for
gatewayseither sethealthexplicitly or intentionally rely on'unknown'being treated as non‑routable.From a schema perspective this aligns with the refactor; the remaining risk is purely around data migration and callers.
go/pkg/db/queries/acme_challenge_insert.sql (1)
2-22: INSERT now targetschallenge_typeconsistently.Swapping
typeforchallenge_typein the column list keeps the INSERT aligned with the new schema and generatedInsertAcmeChallengeParams.ChallengeTypefield. Placeholders and argument order remain consistent.go/apps/ingress/services/router/service.go (1)
100-108: Health check simplification matches the new non‑nullable enum.Filtering with
if gw.Health != db.GatewaysHealthHealthy { continue }correctly reflects the new non‑nullableGatewaysHealthenum: only'healthy'gateways are considered, while'paused','unhealthy', and the new'unknown'default are excluded from routing as intended.go/pkg/db/bulk_acme_challenge_insert.sql_generated.go (1)
12-41: Bulk insert updated cleanly to usechallenge_type.The bulk insert base query and argument mapping now include
challenge_typeand usearg.ChallengeTypein the correct slot, keeping the placeholder count and argument order consistent with the schema and single‑row insert.go/pkg/db/acme_challenge_find_by_token.sql_generated.go (1)
12-38: Find‑by‑token query now aligned withChallengeTypefield.Updating the SELECT list and
row.Scanto usechallenge_type→i.ChallengeTypematches theAcmeChallengestruct and the renamed column, with the column order preserved.go/cmd/dev/seed/ingress.go (1)
130-130: LGTM!The direct use of
db.GatewaysHealthHealthyaligns correctly with the updated non-nullableHealthfield type inInsertGatewayParams.go/pkg/db/gateway_insert.sql_generated.go (1)
36-45: LGTM!The generated
InsertGatewayParamsstruct correctly reflects the non-nullableHealthfield type change fromNullGatewaysHealthtoGatewaysHealth.go/pkg/db/querier_generated.go (1)
1560-1567: LGTM!The
ListExecutableChallengesinterface signature correctly reflects the column rename fromtypetochallenge_typeand the updated parameter type[]AcmeChallengesChallengeType.go/pkg/db/acme_challenge_insert.sql_generated.go (1)
37-47: LGTM!The generated
InsertAcmeChallengeParamsstruct correctly reflects the column rename fromtypetochallenge_typewith the appropriate typeAcmeChallengesChallengeTypeand updated db tag.go/pkg/db/acme_challenge_list_executable.sql_generated.go (1)
21-25: LGTM!The
ListExecutableChallengesRowstruct and associated query logic correctly reflect the column rename tochallenge_typewith proper type and scanning.go/pkg/db/models_generated.go (3)
281-288: LGTM!The
GatewaysHealthenum correctly includes the newunknownstate, which serves as the default value for the non-nullable health column.
785-794: LGTM!The
Gatewaystruct correctly reflects the non-nullableHealthfield asGatewaysHealthinstead ofNullGatewaysHealth.
16-21: I'll help you verify the review comment about the ACME challenge type enum values. Let me start by exploring the codebase to confirm the database schema definitions and usage patterns.
<function_calls>
#!/bin/bashSearch for database schema files and migrations
find . -type f ( -name "*.sql" -o -name "migration" -o -name "schema" ) | head -20
</function_calls>Now let me search for references to the challenge type values in the codebase:
<function_calls>
#!/bin/bashSearch for challenge type references and enum values
rg -n "HTTP-01|DNS-01|http01|dns01" -t go -t sql -C 2 | head -100
</function_calls>Let me also check the models_generated.go file directly to understand the complete context:
<function_calls>
#!/bin/bashCheck the models_generated.go file to see the challenge type definitions
head -100 go/pkg/db/models_generated.go | cat -n
</function_calls>Let me search more specifically for database table definitions and create statements:
<function_calls>
#!/bin/bashSearch for acme_challenges table definition
rg -n "acme_challenges|AcmeChallenge" -t go -t sql -B 5 -A 5 | head -150
</function_calls>Let me also check if there are test files that show expected values:
<function_calls>
#!/bin/bashSearch for test files and example usage
find . -type f -name "test" | grep -E ".(go|sql)" | head -10
</function_calls><function_calls>
#!/bin/bashSearch for any hardcoded challenge type values in the codebase
rg -n '"HTTP-01"|"DNS-01"|"dns01"|"http01"' --type go -C 3 | head -80
</function_calls>
|
|
Graphite Automations"Post a GIF when PR approved" took an action on this PR • (11/26/25)1 gif was posted to this PR based on Andreas Thomas's automation. |
3 participants
No description provided.