Add encryptcookie middleware migration#212
Conversation
|
Thanks for opening this pull request! 🎉 Please check out our contributing guidelines. If you need help or want to chat with us, join us on Discord https://gofiber.io/discord |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a new v3 migration that rewrites encryptcookie.Config blocks to include updated Encryptor/Decryptor parameters, registers it in the v3 migrations list, and provides tests covering single-line, multi-line, and idempotent cases. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer (CLI)
participant CMD as cobra.Command
participant Mig as MigrateEncryptcookieConfig
participant FS as File System
participant RW as internal.ChangeFileContent
Dev->>CMD: run migration (v3)
CMD->>Mig: invoke with cwd and versions
Mig->>FS: read .go files in cwd
Mig->>Mig: find encryptcookie.Config{...} via regex
alt Matches found
Mig->>Mig: add Encryptor/Decryptor extra param
Mig->>RW: write updated content
RW-->>Mig: success/error
alt Success
Mig-->>CMD: print "Migrating encryptcookie middleware configs"
else Error
Mig-->>CMD: return wrapped error
end
else No matches
Mig-->>CMD: no-op
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🧰 Additional context used🧠 Learnings (3)📓 Common learnings📚 Learning: 2024-07-02T13:29:56.992ZApplied to files:
📚 Learning: 2024-10-08T19:06:06.583ZApplied to files:
🧬 Code graph analysis (3)cmd/internal/migrations/lists.go (1)
cmd/internal/migrations/v3/encryptcookie_config_test.go (1)
cmd/internal/migrations/v3/encryptcookie_config.go (1)
🔇 Additional comments (6)
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 |
LOL 😂 |
There was a problem hiding this comment.
Pull Request Overview
This PR adds migration functionality for the encryptcookie middleware to update function signatures by adding a new cookie name parameter to Encryptor and Decryptor hooks.
- Adds
MigrateEncryptcookieConfigfunction to inject a new string parameter into encryptcookie function signatures - Registers the migration with the existing v3 migration list
- Provides comprehensive test coverage for inline, multiline, and idempotent scenarios
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| cmd/internal/migrations/v3/encryptcookie_config.go | Implements the core migration logic to add cookie name parameter to encryptcookie functions |
| cmd/internal/migrations/v3/encryptcookie_config_test.go | Comprehensive test suite covering various function signature formats and migration scenarios |
| cmd/internal/migrations/lists.go | Registers the new encryptcookie migration with the v3 migration list |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting
| args := splitArgs(params) | ||
| var nonEmpty []string | ||
| for _, arg := range args { | ||
| if strings.TrimSpace(arg) != "" { | ||
| nonEmpty = append(nonEmpty, arg) | ||
| } | ||
| } | ||
|
|
||
| if len(nonEmpty) != 2 { | ||
| return match | ||
| } |
There was a problem hiding this comment.
The guard len(nonEmpty) != 2 relies on splitArgs returning exactly two entries, but splitArgs treats comment-only segments as arguments. If a user formats the Encryptor/Decryptor literal with inline comments (e.g., Encryptor: func( value string, // cookie value key string, )), splitArgs produces three entries (value string, the comment block, and key string) and the migration returns the original code without adding the new _ string parameter. Such signatures remain on the old two-argument form and will fail to compile after the middleware expects three parameters. Consider ignoring comment-only tokens (e.g., by stripping // and /* */ blocks) before the length check or by counting parameters syntactically rather than string splitting.
Useful? React with 👍 / 👎.
|
Congrats on merging your first pull request! 🎉 We here at Fiber are proud of you! If you need help or want to chat with us, join us on Discord https://gofiber.io/discord |
Summary
Summary by CodeRabbit