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

Implemented new struct 'MintUrl' which trims trailing slashes. #283

Merged
merged 1 commit into from
Aug 12, 2024

Conversation

cjbeery24
Copy link
Contributor

@cjbeery24 cjbeery24 commented Aug 10, 2024

I wasn't sure if I should insert the trim functionality into the existing "UncheckedUrl" implementation, so I created a similar but new "MintUrl" which seems more appropriately named anyway.

For:
#267

Copy link
Collaborator

@thesimplekid thesimplekid left a comment

Choose a reason for hiding this comment

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

Thanks for this. With this change we can completely remove the UncheckedUrl in favor or MintUrl.

Also since we need to account for databases that already have mints with trailing slashes in them since with this change we wont be able to get them since the lookup id will be the url without a / I suggest we add a migration that trims the trailing / from the db. In sql this can be done by adding a migration file via the cli and using sql trim. In redb we can bump the db version number and then write a function to trip urls with trailing /.

S: Into<String>,
{
let url: String = url.into();
Self(url.trim_end_matches("/").to_string())
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Self(url.trim_end_matches("/").to_string())
Self(url.trim_end_matches('/').to_string())


/// Remove trailing slashes from url
pub fn trim_trailing_slashes(&self) -> Self {
Self(self.to_string().trim_end_matches("/").to_string())
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Self(self.to_string().trim_end_matches("/").to_string())
Self(self.to_string().trim_end_matches('/').to_string())

{
fn from(url: S) -> Self {
let url: String = url.into();
Self(url.trim_end_matches("/").to_string())
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Self(url.trim_end_matches("/").to_string())
Self(url.trim_end_matches('/').to_string())

@cjbeery24
Copy link
Contributor Author

@thesimplekid I added SQL migrations to remove trailing slashes from existing mint_urls. I'm not as familiar with redb, would you be able to make the necessary changes for that?

@cjbeery24 cjbeery24 requested a review from thesimplekid August 11, 2024 03:37
Copy link
Collaborator

@thesimplekid thesimplekid left a comment

Choose a reason for hiding this comment

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

Looks like since its sqlite we need to use RTRIM and since there is a unique constraint on the mint_url we need to delete the duplicate.

Yes no problem I will do the redb.

@thesimplekid
Copy link
Collaborator

thesimplekid commented Aug 11, 2024

redb migration 9992356 if you want to cherry-pick this onto this branch

The formatting fixes for the ci are here ba5fa69

Copy link
Collaborator

@thesimplekid thesimplekid left a comment

Choose a reason for hiding this comment

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

I'm happy with this if you are. If you could just rebase and squash the commits

Implemented new struct 'MintUrl' which trims trailing slashes.

Added missing import.

Removed references to 'UncheckedUrl'. Added migrations to strip trailing slashes from mint_urls.

Update crates/cdk-sqlite/src/mint/migrations/20240811031111_update_mint_url.sql

Co-authored-by: thesimplekid <[email protected]>

Update crates/cdk-sqlite/src/wallet/migrations/20240810233905_update_mint_url.sql

Co-authored-by: thesimplekid <[email protected]>

Update crates/cdk/src/mint_url.rs

Co-authored-by: thesimplekid <[email protected]>

feat: migrate wallet db for mint_url

chore: formatting

Fixed imports.
@cjbeery24 cjbeery24 force-pushed the feature/mint-url-trim-slashes branch from 0380e88 to b762ced Compare August 12, 2024 14:38
@thesimplekid thesimplekid merged commit 4055498 into cashubtc:main Aug 12, 2024
24 checks passed
@cjbeery24 cjbeery24 deleted the feature/mint-url-trim-slashes branch August 12, 2024 20:32
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.

2 participants