refactor(cmd/permify): streamline command registration in main function #2607
Conversation
WalkthroughThis pull request restores CLI command registration (adds NewMigrateCommand, NewVersionCommand, NewConfigCommand, NewRepairCommand), renames NewGenerateASTCommand → NewGenerateAstCommand, expands the OpenID Authn struct with JWKS/jwt parser and backoff/retry fields, implements/rewrites extensive tests and fakes for OIDC and in-memory storage, refactors coverage computation into modular functions, and adds a Postgres version compatibility check in the database factory. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Authn as OpenID Authn
participant AutoRefresh as JWKS AutoRefresh
participant JWKS as JWKS Provider
Client->>Authn: Validate token request
Authn->>Authn: Check cached keys & retriedKeys
alt Key not cached
Authn->>AutoRefresh: Trigger fetch
AutoRefresh->>JWKS: GET /.well-known/jwks.json
JWKS-->>AutoRefresh: JWKS response
AutoRefresh-->>Authn: Updated jwksSet
else Key cached
Note right of Authn: Use cached keys (fast path)
end
Authn->>Authn: Parse token with jwtParser
Authn->>Authn: Validate claims (iss, aud, exp, iat, nbf)
alt Valid
Authn-->>Client: Success
else Invalid / kid mismatch
Authn-->>Client: Error (may trigger backoff/retry)
Note right of Authn: Increment globalRetryCount, apply backoffInterval
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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 |
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/storage/postgres/data_writer.go (1)
119-119: Inconsistent log level for max retries exceeded condition.Line 119 uses
slog.DebugContextfor the "max retries reached" error, but the identical error condition at line 79 (in theWritemethod) usesslog.ErrorContext. This inconsistency in severity reporting could mask failures during troubleshooting. Both should use the same log level for consistency.Suggested fix:
- slog.DebugContext(ctx, "max retries reached", slog.Any("error", errors.New(base.ErrorCode_ERROR_CODE_ERROR_MAX_RETRIES.String()))) + slog.ErrorContext(ctx, "max retries reached", slog.Any("error", errors.New(base.ErrorCode_ERROR_CODE_ERROR_MAX_RETRIES.String())))
🧹 Nitpick comments (3)
internal/storage/postgres/data_writer.go (1)
522-522: Remove redundant inline comment.The comment
// Build delete clausesat the end of line 522 is redundant with the function namebuildDeleteClausesForAttributes. Inline comments should convey non-obvious intent rather than repeating what the code already states.Suggested fix:
-func buildDeleteClausesForAttributes(attributeCollection *database.AttributeCollection) []squirrel.Eq { // Build delete clauses +func buildDeleteClausesForAttributes(attributeCollection *database.AttributeCollection) []squirrel.Eq {pkg/development/coverage/coverage.go (1)
301-336: Total coverage aggregation is correct, with minor naming clarity opportunity.The averaging logic is sound: it sums per-entity coverage percentages and divides by entity count (or by entity+scenario count for assertions) to compute the total. The zero-division guards are appropriate.
One minor note: variable names in
calculateTotalCoveragecould be clearer. For example,totalRelationshipsactually counts the number of entities (or entity+scenario pairs), not the number of relationships. Renaming torelationshipEntityCountor similar would improve readability, but this is a nice-to-have.pkg/cmd/serve.go (1)
210-218: Consider extracting duplicated header parsing logic.The header parsing logic (splitting by ":" and validating format) is duplicated across log, tracer, and meter configuration. Extracting this into a helper function would improve maintainability.
Example helper function:
// parseHeaders converts a slice of "key:value" strings into a map func parseHeaders(headers []string) (map[string]string, error) { result := map[string]string{} for _, header := range headers { parts := strings.Split(header, ":") if len(parts) != 2 { return nil, errors.New("invalid header format; expected 'key:value'") } result[parts[0]] = parts[1] } return result, nil }Then use it as:
headers, err := parseHeaders(cfg.Log.Headers) if err != nil { return err }Also applies to: 282-289, 336-344
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (26)
cmd/permify/permify.go(1 hunks)internal/authn/openid/authn.go(10 hunks)internal/authn/openid/authn_test.go(25 hunks)internal/authn/openid/fakes_test.go(1 hunks)internal/config/config.go(1 hunks)internal/config/config_test.go(3 hunks)internal/factories/database.go(1 hunks)internal/servers/schema_server.go(1 hunks)internal/servers/server.go(1 hunks)internal/storage/memory/bundle_reader.go(1 hunks)internal/storage/memory/bundle_reader_test.go(1 hunks)internal/storage/memory/bundle_writer.go(1 hunks)internal/storage/memory/bundle_writer_test.go(1 hunks)internal/storage/memory/constants/constants.go(1 hunks)internal/storage/memory/data_writer.go(1 hunks)internal/storage/memory/memory_test.go(1 hunks)internal/storage/memory/migrations/schema.go(3 hunks)internal/storage/migration.go(1 hunks)internal/storage/postgres/data_writer.go(1 hunks)pkg/cmd/ast.go(2 hunks)pkg/cmd/config.go(4 hunks)pkg/cmd/coverage.go(2 hunks)pkg/cmd/flags/coverage.go(1 hunks)pkg/cmd/serve.go(7 hunks)pkg/cmd/version.go(1 hunks)pkg/development/coverage/coverage.go(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (13)
internal/storage/memory/migrations/schema.go (1)
internal/storage/memory/constants/constants.go (2)
TenantsTable(8-8)BundlesTable(9-9)
pkg/cmd/coverage.go (1)
pkg/development/coverage/coverage.go (1)
Run(74-84)
internal/storage/memory/bundle_writer.go (3)
internal/storage/memory/constants/constants.go (1)
BundlesTable(9-9)pkg/database/memory/memory.go (1)
New(20-25)pkg/pb/base/v1/errors.pb.go (2)
ErrorCode_ERROR_CODE_EXECUTION(87-87)ErrorCode_ERROR_CODE_BUNDLE_NOT_FOUND(78-78)
internal/servers/schema_server.go (1)
pkg/database/pagination.go (2)
NewPagination(27-36)Size(7-11)
internal/storage/memory/bundle_writer_test.go (5)
pkg/database/memory/memory.go (1)
Memory(11-17)internal/storage/memory/bundle_writer.go (2)
BundleWriter(14-16)NewBundleWriter(18-22)internal/storage/memory/bundle_reader.go (2)
BundleReader(14-16)NewBundleReader(18-22)internal/storage/memory/migrations/schema.go (1)
Schema(10-181)pkg/pb/base/v1/errors.pb.go (1)
ErrorCode_ERROR_CODE_BUNDLE_NOT_FOUND(78-78)
internal/storage/memory/bundle_reader_test.go (5)
pkg/database/memory/memory.go (1)
Memory(11-17)internal/storage/memory/bundle_writer.go (2)
BundleWriter(14-16)NewBundleWriter(18-22)internal/storage/memory/bundle_reader.go (2)
BundleReader(14-16)NewBundleReader(18-22)internal/storage/memory/migrations/schema.go (1)
Schema(10-181)pkg/pb/base/v1/errors.pb.go (1)
ErrorCode_ERROR_CODE_BUNDLE_NOT_FOUND(78-78)
pkg/cmd/version.go (1)
internal/info.go (1)
Version(26-26)
pkg/cmd/serve.go (8)
internal/config/config.go (8)
Config(15-26)NewConfigWithFile(237-272)NewConfig(201-232)Log(89-99)Database(160-178)Tracer(102-110)GarbageCollection(180-185)Meter(113-122)pkg/telemetry/log.go (1)
HandlerFactory(26-35)internal/factories/database.go (1)
DatabaseFactory(32-82)pkg/telemetry/logexporters/factory.go (1)
ExporterFactory(10-17)pkg/telemetry/meterexporters/factory.go (1)
ExporterFactory(10-19)pkg/telemetry/tracerexporters/factory.go (1)
ExporterFactory(10-25)internal/storage/postgres/gc/gc.go (1)
NewGC(31-45)pkg/telemetry/meter.go (1)
NewMeter(25-62)
internal/factories/database.go (2)
internal/storage/postgres/utils/common.go (1)
EnsureDBVersion(214-224)pkg/database/postgres/postgres.go (1)
Postgres(22-35)
internal/storage/postgres/data_writer.go (4)
pkg/database/postgres/xid8.go (1)
XID8(10-10)pkg/tuple/tuple.go (1)
ELLIPSIS(19-19)internal/storage/memory/constants/constants.go (2)
RelationTuplesTable(5-5)AttributesTable(6-6)internal/storage/postgres/utils/common.go (1)
ActiveRecordTxnID(32-32)
pkg/development/coverage/coverage.go (6)
pkg/pb/base/v1/base.pb.go (15)
Attributes(1710-1715)Attributes(1728-1728)Attributes(1743-1745)EntityDefinition(815-829)EntityDefinition(842-842)EntityDefinition(857-859)Tuple(1543-1550)Tuple(1563-1563)Tuple(1578-1580)Attribute(1604-1611)Attribute(1624-1624)Attribute(1639-1641)Entity(1755-1761)Entity(1774-1774)Entity(1789-1791)pkg/development/file/shape.go (1)
Shape(4-18)pkg/dsl/parser/parser.go (1)
NewParser(59-78)pkg/dsl/compiler/compiler.go (1)
NewCompiler(25-30)pkg/tuple/tuple.go (2)
Tuple(160-182)E(215-234)pkg/attribute/attribute.go (1)
Attribute(22-131)
cmd/permify/permify.go (1)
pkg/cmd/ast.go (1)
NewGenerateAstCommand(21-37)
internal/authn/openid/authn.go (2)
internal/config/config.go (1)
Authn(59-64)pkg/pb/base/v1/errors.pb.go (5)
ErrorCode_ERROR_CODE_MISSING_BEARER_TOKEN(29-29)ErrorCode_ERROR_CODE_INVALID_BEARER_TOKEN(35-35)ErrorCode_ERROR_CODE_INVALID_CLAIMS(33-33)ErrorCode_ERROR_CODE_INVALID_ISSUER(34-34)ErrorCode_ERROR_CODE_INVALID_AUDIENCE(32-32)
⏰ 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). (3)
- GitHub Check: Test with Coverage
- GitHub Check: Analyze (go)
- GitHub Check: Test with Coverage
🔇 Additional comments (47)
internal/storage/postgres/data_writer.go (1)
432-535: Batch operation refactoring looks solid.The refactored batch operation helpers maintain correct transaction semantics and error handling. The queueing pattern (batching SQL operations then executing in a single round-trip) is appropriate for PostgreSQL performance. Retry logic with serialization error detection and backoff across Write, Delete, and RunBundle methods is consistently applied and well-structured.
pkg/development/coverage/coverage.go (5)
1-70: Solid structural foundation for the modular refactoring.Imports and type definitions are clean and appropriate. The documentation in SchemaCoverage is clear and helpful for understanding the coverage concept. No concerns.
72-99: Error handling pattern acceptable but worth noting.Line 76: When schema parsing or compilation fails,
Runsilently returns an emptySchemaCoverageInfo{}. This prevents error propagation to the caller, making it impossible to distinguish between legitimate empty coverage and a schema error. For coverage analysis (an auxiliary feature), this is acceptable; however, if robust error diagnostics are needed, consider returning anerroras well (e.g.,(SchemaCoverageInfo, error)).
101-162: Extract functions are well-decomposed and consistent.The extraction functions form a clean pipeline: schema definitions → per-entity references → per-type references. Each function has a single responsibility, and the use of format helpers ensures consistency. No concerns.
164-299: Coverage calculation logic is sound and properly defensive.The per-entity coverage calculation is straightforward and correct. The zero-division guards in
calculateCoveragePercentandcalculateAverageCoverageare appropriate. The conditional check on line 211–213 (only adding uncovered assertions if non-empty) is a nice touch that avoids unnecessary map entries. Initialization vianewEntityCoverageInfo()ensures all fields are safe. No concerns.
338-437: Extraction and formatting functions are robust and consistent.The covered-extraction functions use a sensible continue-on-error pattern to gracefully skip invalid tuples, attributes, or entities. Format helpers are simple and reusable. Line 423–425 in
formatRelationshipcorrectly handles the optional subject relation. No concerns.
Summary
Your refactoring of the coverage analysis into a modular, staged pipeline is well-executed. The code is clean, readable, and maintainable. Here are the key takeaways:
✅ Strengths:
- Excellent decomposition into focused, single-responsibility functions
- Consistent error handling throughout (parse/compile errors, malformed tuple/attribute parsing)
- Defensive initialization via
newEntityCoverageInfo()prevents nil issues- Smart conditional on line 211–213 avoids unnecessary map entries
- Format helpers ensure consistent string representation
⚠️ Minor observation (optional refactor):
- Variable names in
calculateTotalCoverage(lines 313–322) could be clearer; consider renamingtotalRelationshipsto reflect that it counts entities, not relationships.No critical or major issues detected. The code is production-ready.
internal/config/config.go (1)
69-69: LGTM!The removal of the trailing comment improves code consistency and readability without affecting functionality.
pkg/cmd/version.go (1)
1-21: LGTM!The formatting improvements enhance code organization and readability. The function implementation remains correct and unchanged functionally.
internal/storage/memory/migrations/schema.go (1)
151-181: LGTM!The formatting realignment improves code consistency without altering the table definitions or schema structure.
internal/servers/server.go (1)
398-408: LGTM!The addition of a descriptive comment and improved spacing enhances code clarity. The function behavior remains unchanged.
pkg/cmd/coverage.go (1)
1-161: LGTM!The import organization and formatting improvements enhance code readability. The coverage analysis logic, validation flow, and threshold enforcement remain functionally unchanged.
internal/storage/memory/memory_test.go (1)
16-36: LGTM!The refactored
isSameArrayfunction now performs an order-insensitive comparison by sorting copies of the input slices. This is a more robust approach for test assertions where element order doesn't matter.internal/servers/schema_server.go (1)
229-258: LGTM!The formatting improvements (removal of trailing inline comments and spacing adjustments) enhance code cleanliness without affecting the RPC logic or behavior.
internal/config/config_test.go (1)
171-221: LGTM!The removal of trailing end-of-block comments improves code cleanliness. The test logic and assertions remain unchanged.
internal/storage/memory/constants/constants.go (1)
1-10: LGTM! Clean formatting update.The comment clarification and formatting improvements enhance readability without affecting functionality.
internal/storage/memory/bundle_writer.go (2)
24-38: LGTM! Improved transaction handling.The Write method now correctly collects bundle names during insertion and commits the transaction, which aligns with the method signature returning
[]string.
40-58: LGTM! Consistent transaction management.The Delete method properly checks for bundle existence, returns a specific error when not found, and commits the transaction before returning.
internal/storage/memory/bundle_reader.go (1)
24-41: LGTM! Formatting improvement.The added whitespace improves code readability without changing functionality.
pkg/cmd/flags/coverage.go (1)
1-19: LGTM! Clean formatting update.Comment cleanup improves readability while preserving all flag binding functionality.
internal/factories/database.go (1)
64-69: LGTM! Important version validation added.Adding the Postgres version compatibility check early in the database initialization flow is a good practice. This ensures unsupported database versions are caught before any operations are performed.
pkg/cmd/serve.go (3)
159-181: LGTM! Clearer configuration loading flow.The refactored config loading logic with explicit branching for file vs. environment sources improves readability and maintains consistent error handling.
192-240: LGTM! Well-structured logger initialization.The switch-based output selection and Fanout strategy for combining multiple handlers provide a clean, maintainable approach to logger configuration.
263-265: LGTM! Consistent error logging.Error logging has been standardized across database operations, improving observability.
Also applies to: 271-272, 276-277
internal/storage/migration.go (1)
48-53: LGTM! Comment cleanup.Removing redundant inline comments improves code clarity.
internal/storage/memory/data_writer.go (2)
139-163: LGTM! Improved bundle operation handling.The RunBundle method now correctly iterates through all operations, processes each one, and commits the transaction before returning the snapshot token. The error handling is appropriate throughout.
166-172: LGTM! Explicit unused parameter.Renaming the context parameter to
_clearly indicates it's intentionally unused in this in-memory implementation, which is a good practice for code clarity.internal/storage/memory/bundle_writer_test.go (3)
21-33: LGTM! Clean test setup with proper resource management.The BeforeEach/AfterEach pattern ensures each test starts with a fresh database and properly releases resources.
36-154: Good test coverage of write and update scenarios.The test validates both initial writes and overwrites, including the important case where attributes are removed (empty arrays). The assertions at lines 152-153 correctly verify that the empty arrays replaced the previous non-empty values.
158-227: LGTM! Comprehensive deletion test.The test properly validates:
- Bundle deletion succeeds
- Reading deleted bundle returns the expected BUNDLE_NOT_FOUND error
- Other bundles remain unaffected
pkg/cmd/config.go (1)
18-117: LGTM! No functional changes detected.The refactoring maintains the existing behavior while improving code organization. The command constructor and flag setup remain semantically identical.
internal/storage/memory/bundle_reader_test.go (3)
22-33: LGTM! Consistent test setup pattern.The BeforeEach/AfterEach pattern matches the writer test, ensuring proper initialization and cleanup.
36-93: Good end-to-end validation.The test validates the complete write-then-read flow with proper assertions on all bundle properties including relationships, attributes, and their operations.
95-100: LGTM! Proper error handling test.Correctly validates that reading a non-existent bundle returns the expected BUNDLE_NOT_FOUND error.
pkg/cmd/ast.go (1)
21-37: LGTM! Consistent naming convention.The rename from
NewGenerateASTCommandtoNewGenerateAstCommandaligns with Go's MixedCaps convention for acronyms in identifiers.cmd/permify/permify.go (1)
15-56: LGTM! Clean and consistent command registration.The refactoring achieves the PR objective by establishing a clear, uniform pattern for CLI command registration. All commands (serve, validate, coverage, ast, migrate, version, config, repair) are now registered consistently.
internal/authn/openid/authn.go (4)
24-49: LGTM! Well-documented struct expansion.The new fields support JWKS auto-refresh and backoff/retry logic. Good inline documentation explains each field's purpose. The mutex ensures thread-safe access to shared retry state.
52-106: LGTM! Thorough configuration validation.The constructor properly validates all backoff configuration parameters (lines 68-81), ensuring they are positive values before use. The immediate JWKS fetch (lines 101-103) validates connectivity during initialization.
327-397: LGTM! Comprehensive HTTP error handling.The doHTTPRequest and parseOIDCConfiguration functions include proper:
- Status code validation (line 378)
- Required field validation (lines 412-420)
- Error wrapping with context
169-278: No issues found - test coverage validates all concerns.The test file includes comprehensive concurrent testing:
- Case 4 (line 401) tests concurrent requests with
sync.WaitGroup, sending multiple goroutines with invalid keys simultaneously- State reset logic (lines 220-223 in authn.go) is validated: the test verifies that valid keys successfully authenticate and trigger global state reset during the backoff period
- Race condition handling (lines 247-253) is covered by concurrent retry scenarios, ensuring snapshot comparison works correctly when multiple goroutines refresh JWKS
All three concerns raised in the review are properly addressed by existing tests. The retry logic implementation is sound.
internal/authn/openid/fakes_test.go (2)
46-88: LGTM! Comprehensive test provider setup.The fake OIDC provider properly initializes cryptographic keys (RSA, ECDSA, HMAC) and constructs a valid JWKS. The signingKeyMap and mutex ensure thread-safe key management.
146-155: LGTM! Thread-safe key ID updates.The UpdateKeyID method properly uses mutex lock/unlock to safely update both the signingKeyMap and JWKS entries, preventing race conditions during concurrent access.
internal/authn/openid/authn_test.go (6)
31-50: LGTM! Proper test fixture setup.The BeforeEach initializes a fake OIDC provider and test server on a fixed address. AfterEach properly closes the server to prevent resource leaks.
52-105: LGTM! Clear signing method validation.The test validates RS256 signing with proper token creation, authentication, and error checking. The test structure is clear and maintainable.
324-399: Excellent backoff behavior validation.This test (Case 3) thoroughly validates:
- Maximum retry attempts with proper timing (lines 377-379)
- Immediate rejection during backoff period (lines 382-385)
- State reset after backoff interval expires (lines 388-397)
The timing assertions ensure the backoff logic works as designed.
401-509: Comprehensive concurrent backoff test.This test validates complex concurrent scenarios:
- Multiple goroutines triggering retries simultaneously (lines 444-457)
- Immediate rejection during backoff (lines 463-471)
- Valid keys continuing to work (lines 474-481)
- Invalid keys remaining blocked (lines 485-493)
- Behavior after backoff expiration (lines 497-508)
Excellent coverage of the global backoff state management.
682-736: Good context cancellation test.The test properly validates that context cancellation during retry is handled gracefully. The use of goroutines and channels (lines 711-716) correctly simulates concurrent cancellation scenarios.
895-917: LGTM! Clean claims override helper.The claimOverride function provides a clear way to selectively override JWT claims for negative testing scenarios, checking each field before overriding.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
internal/authn/openid/fakes_test.go (1)
51-59: Consider removing unused cryptographic keys.The PS-RSA key (lines 51-54), ECDSA key (lines 55-58), and HMAC key (line 59) are generated but never used, since
SignIDTokenonly implements RS256 and the JWKS only contains the RS256 public key. If these keys are intended for future algorithm support, consider adding a TODO comment; otherwise, remove them to simplify the test setup.Diff to remove unused keys
- rsaPrivateKeyForPS, err := rsa.GenerateKey(rand.Reader, 2048) - if err != nil { - return nil, fmt.Errorf("failed to generate RSA key for PS: %w", err) - } - ecdsaPrivateKey, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) - if err != nil { - return nil, fmt.Errorf("failed to generate ECDSA key: %w", err) - } - hmacKey := []byte("hmackeysecret") // Initialize signing key mapping signingKeyMap := map[jwt.SigningMethod]string{ jwt.SigningMethodRS256: "rs256keyid", } jwks := []jose.JSONWebKey{ { Key: rsaPrivateKey.Public(), KeyID: signingKeyMap[jwt.SigningMethodRS256], Algorithm: "RS256", Use: "sig", }, } return &fakeOidcProvider{ issuerURL: config.IssuerURL, authPath: config.AuthPath, tokenPath: config.TokenPath, userInfoPath: config.UserInfoPath, JWKSPath: config.JWKSPath, algorithms: config.Algorithms, rsaPrivateKey: rsaPrivateKey, - rsaPrivateKeyForPS: rsaPrivateKeyForPS, - hmacKey: hmacKey, jwks: jwks, - ecdsaPrivateKey: ecdsaPrivateKey, signingKeyMap: signingKeyMap, - mu: sync.RWMutex{}, + mu: sync.RWMutex{}, // or just omit; zero value is fine }, nil }Also applies to: 80-85
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/authn/openid/fakes_test.go(1 hunks)
⏰ 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). (3)
- GitHub Check: Test with Coverage
- GitHub Check: Analyze (go)
- GitHub Check: Test with Coverage
🔇 Additional comments (1)
internal/authn/openid/fakes_test.go (1)
158-170: Good fix from previous review.The error message now accurately reflects that only RS256 is implemented in the test fake (line 168), addressing the previous review concern about inconsistency.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Refactor