Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FIX] Prevent keys from starting with slashes #172

Merged
merged 17 commits into from
Feb 28, 2024

Conversation

Firepup6500
Copy link
Contributor

If a key starts with a slash, then it becomes undeletable and prevents database purges from working properly as well. This prevents that from occuring by stripping slashes from the left of the key name.

Why

At least two separate occurrences where users have accidentally set a key that starts with a slash, and therefore can no longer remove the key or cleared their database.

What changed

Changed all references to key or k when setting a key-value pair to keyStrip(key) and keyStrip(k) respectively.

Test plan

Attempted to set keys starting with slashes, then subsequently remove them and/or purge the database.

Rollout

Prevents keys from being set with a slash in the future, which was is less breaking than the existing behavior (which is to not process the keys at all)

  • This is fully backward and forward compatible

@Firepup6500 Firepup6500 requested a review from a team as a code owner September 11, 2023 21:07
@Firepup6500 Firepup6500 requested review from airportyh and removed request for a team September 11, 2023 21:07
@airportyh
Copy link
Contributor

@Firepup6500
Copy link
Contributor Author

Firepup6500 commented Sep 13, 2023

Code seems to be working as running pip install git+https://github.com/Firepup6500/replit-py, then running the following code works:

from replit import db
db["/abc"] = "d"
print(db["/abc"])

I'm not sure what's causing the JSONDecodeError.

Edit: Not using get_raw for _raw set methods was the issue here.

@Firepup6500
Copy link
Contributor Author

Alright, finally figured it out. @airportyh Could you review this again?

Copy link
Contributor

@airportyh airportyh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Beautiful! Thank you.

Copy link
Collaborator

@blast-hardcheese blast-hardcheese left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@blast-hardcheese blast-hardcheese merged commit 4a5a7ac into replit:master Feb 28, 2024
6 checks passed
@lafkpages
Copy link

Wouldn't it be better to figure out why adding a leading forward slash causes problems instead of removing them? Sounds like an issue with escaping the keys or something.

@blast-hardcheese
Copy link
Collaborator

@lafkpages The way I read this initially suggested that it was impossible for keys to have started with a slash, so this would effectively be a no-op.

Upon rereading the initial report, it may indeed have been an error to make this change without similar changes in the other libraries, as well as the requisite changes on the backend to establish consistency.

I'll raise your point with the team next week, we'll find a way forward; for the time being, if this has impacted your program, please stay on v3.6.0 while we get this sorted.

Thank you!

blast-hardcheese added a commit that referenced this pull request May 2, 2024
This was a good first attempt, but still prevented users from deleting the keys they'd created
@blast-hardcheese
Copy link
Collaborator

@lafkpages It took a bit longer to get back to this than anticipated, but I'm happy to say #218 is up for review. Thanks again for pointing this out!

blast-hardcheese added a commit that referenced this pull request May 2, 2024
This was a good first attempt, but still prevented users from deleting the keys they'd created
blast-hardcheese added a commit that referenced this pull request May 3, 2024
* poetry lock

* Formatting

* Avoid object has no attribute '_refresh_timer'

When called without a refresh function we end up getting

    AttributeError: 'AsyncDatabase' object has no attribute '_refresh_timer'

on attempted close. Fix this.

* Revert #172

This was a good first attempt, but still prevented users from deleting the keys they'd created

* Switch delete method to use new multipart/form-data delete method that supports deleting keys with leading slashes

* Adjusting tests to reflect the gap in getting slashy keys
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants