chore: Reduces the form_data_key length #31905
Conversation
![korbit-ai[bot]](https://avatars.githubusercontent.com/in/322216?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review by Korbit AI
Korbit automatically attempts to detect when you fix issues in new commits.
| Category | Issue | Fix Detected |
|---|---|---|
 |
Missing function parameter implementation ▹ view |
Files scanned
| File Path | Reviewed |
|---|---|
| superset/key_value/shared_entries.py | ✅ |
| superset/key_value/utils.py | ✅ |
Explore our documentation to understand the languages and file types we support and the files we ignore.
Need a new review? Comment
/korbit-reviewon this PR and I'll review your latest changes.Korbit Guide: Usage and Customization
Interacting with Korbit
- You can manually ask Korbit to review your PR using the
/korbit-reviewcommand in a comment at the root of your PR.- You can ask Korbit to generate a new PR description using the
/korbit-generate-pr-descriptioncommand in any comment on your PR.- Too many Korbit comments? I can resolve all my comment threads if you use the
/korbit-resolvecommand in any comment on your PR.- Chat with Korbit on issues we post by tagging @korbit-ai in your reply.
- Help train Korbit to improve your reviews by giving a 👍 or 👎 on the comments Korbit posts.
Customizing Korbit
- Check out our docs on how you can make Korbit work best for you and your team.
- Customize Korbit for your organization through the Korbit Console.
Current Korbit Configuration
General Settings
Setting Value Review Schedule Automatic excluding drafts Max Issue Count 10 Automatic PR Descriptions ❌ Issue Categories
Category Enabled Naming ✅ Database Operations ✅ Documentation ✅ Logging ✅ Error Handling ✅ Systems and Environment ✅ Objects and Data Structures ✅ Readability and Maintainability ✅ Asynchronous Processing ✅ Design Patterns ✅ Third-Party Libraries ✅ Performance ✅ Security ✅ Functionality ✅ Feedback and Support
Note
Korbit Pro is free for open source projects 🎉
Looking to add Korbit to your team? Get started with a free 2 week trial here
| @@ -42,6 +42,7 @@ def set_shared_value(key: SharedKey, value: Any) -> None: | |||
| def get_permalink_salt(key: SharedKey) -> str: | |||
| salt = get_shared_value(key) | |||
| if salt is None: | |||
| salt = random_key() | |||
| # Use a 48 bytes salt | |||
| salt = random_key(48) | |||
There was a problem hiding this comment.
Missing function parameter implementation 
Tell me more
What is the issue?
The code assumes that random_key() has been updated to accept a length parameter, but the function definition change is not included in this diff.
Why this matters
This could lead to a runtime error if the random_key() function hasn't been modified to accept parameters, causing the application to crash when generating permalink salts.
Suggested change ∙ Feature Preview
Ensure the random_key() function in 'superset/key_value/utils.py' is updated to accept a length parameter before merging this change:
def random_key(length: int = 8) -> str:
# ImplementationChat with Korbit by mentioning @korbit-ai, and give a 👍 or 👎 to help Korbit improve your reviews.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #31905 +/- ##
===========================================
+ Coverage 60.48% 83.45% +22.96%
===========================================
Files 1931 540 -1391
Lines 76236 39140 -37096
Branches 8568 0 -8568
===========================================
- Hits 46114 32663 -13451
+ Misses 28017 6477 -21540
+ Partials 2105 0 -2105
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
SUMMARY
When the
form_data_keywas created, it was used to share state between Superset users and required a secure key length. When permalinks were introduced, theform_data_keyrole changed to only be used for holding and transferring temporary state for a user session and not more for sharing state between Superset users. This PR reduces theform_data_keylength given that an 8 bytes key is sufficient for its new role.As an example, this will be the diff:
TESTING INSTRUCTIONS
CI should be sufficient.
ADDITIONAL INFORMATION