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

[persisted scope] fix: handle recursive directory correctly #455

Merged
merged 12 commits into from
Jun 29, 2023

Conversation

OrIOg
Copy link
Contributor

@OrIOg OrIOg commented Jun 25, 2023

Apparently, I must have broken something while wanting to change #32 to not use change the data structure nor a git version of Tauri.

But here we are.


PR for fixing #25 by calling the calling specific function for file/directory and handling recursion.

We can't just directly pass saved paths/patterns as recursive one such as "/my/path/**" as they will be escaped to "/my/path/[*][*]" when allowed in the scope.

We need to check on them if they are files/directories/recursive directories versions of the pattern and then edit them if necessary.

@OrIOg OrIOg requested a review from a team as a code owner June 25, 2023 16:20
@OrIOg OrIOg changed the title fix: persisted scope fix [persisted scope] fix: handle recusrive directory correctly Jun 25, 2023
@OrIOg OrIOg changed the title [persisted scope] fix: handle recusrive directory correctly [persisted scope] fix: handle recursive directory correctly Jun 25, 2023
@FabianLars
Copy link
Member

First of all, thanks for rewriting it for the x-th time 😅 I really like this one's approach!

As far as i can see, it's working nicely for recursive dirs, but "normal" dirs are missing the removal of the trailing *.

Also, since this is one of the very few plugins released to crates.io we will also need a changefile before we can merge it.

(TIL that we use glob patterns for saving dirs at runtime too, i thought we only do that for tauri.conf.json scopes...)

@OrIOg
Copy link
Contributor Author

OrIOg commented Jun 26, 2023

I'll look at it this evening, thanks for taking the time to check on it.

Might have been hyperfocused on the "**" 😅


Thanks for the reminder about the change file

@OrIOg
Copy link
Contributor Author

OrIOg commented Jun 26, 2023

Ok, that is one more step in the good direction.

I did discover another problem,

Let's imagine a multi-level bugged path in .persisted-scope file containing:
/my/super/path/[*]/[*]
it will be fixed to:
/my/super/path/*/*
then my fix will only change it to:
/my/super/path/*
while it should be:
/my/super/path/

I should be able to in somewhat the same way as you have done fix_pattern with a loop/recursion

Should be fixed now.
Edit:
Was it really a thing in the first case? Maybe it wasn't necessary. Need to rethink about this


Edit:

Another point, what if we had a file/path with [*] in it, but it's intended.
That's something to think about with the fix_pattern. Even if it's a supposed to be temporary, and that it is a strange way to name something.

Test:
Linux: Possible
Windows: No, maybe with WSL ?
Mac: I think ? Need verification.

So, do we need to escape it in some way ?

@FabianLars
Copy link
Member

Was it really a thing in the first case? Maybe it wasn't necessary. Need to rethink about this

i don't think so. This pattern will only be used in tauri.conf.json which already allows/denys the paths itself so i don't think this plugin should be concerned about /*/* or similar 🤔

Another point, what if we had a file/path with [*] in it, but it's intended.

I personally don't think this would be a real world issue. But even if, we can only handle that by removing/rewriting the current pattern fixer. So something we can keep in mind if we change the file version or something but i'd rather not remove the fix without some other kind of safeguard (like not reading the old file at all) since the fix was added to prevent OOM crashes (which weren't rare).

@OrIOg
Copy link
Contributor Author

OrIOg commented Jun 27, 2023

i don't think so. This pattern will only be used in tauri.conf.json which already allows/denys the paths itself so i don't think this plugin should be concerned about // or similar 🤔

Yeah... That's what I realised this morning...
Well, I'll check it just in case and remove if it's indeed useless.

And just realised that the yarn.lock was pushed..

@OrIOg
Copy link
Contributor Author

OrIOg commented Jun 28, 2023

I think it is done 😶

@FabianLars
Copy link
Member

Looks awesome! gonna do some last testing tomorrow morning and will merge it asap then :)

FabianLars
FabianLars previously approved these changes Jun 29, 2023
Copy link
Member

@FabianLars FabianLars left a comment

Choose a reason for hiding this comment

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

Thanks again!

@FabianLars FabianLars merged commit 9174b80 into tauri-apps:v1 Jun 29, 2023
@github-actions github-actions bot mentioned this pull request Jun 29, 2023
lucasfernog added a commit that referenced this pull request Jul 19, 2023
Co-authored-by: Fabian-Lars <[email protected]>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: FabianLars <[email protected]>
Co-authored-by: FabianLars <[email protected]>
Co-authored-by: Alexandre Dang <[email protected]>
Co-authored-by: Ludea <[email protected]>
Co-authored-by: Amr Bashir <[email protected]>
Co-authored-by: Duke Jones <[email protected]>
Co-authored-by: NaokiM03 <[email protected]>
Co-authored-by: Thibault <[email protected]>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: David Blythe <[email protected]>
Co-authored-by: Lucas Nogueira <[email protected]>
fix(stronghold): change wrong argument name for `remove` (#422)
fix(window-state): correctly set decoration state if no saved state exists, fixes #421 (#424)
fix(stronghold): return null if there is no record (#129)
fix(window-state): propagate promise (#435)
closes #432
fix(window-state): manual default implentation (#425)
fix(window-state): manual default implentation, closes #421
fix(deps): update rust crate iota-crypto to 0.21 (#438)
fix readme example (#447)
fix: handle recursive directory correctly (#455)
fix(deps): update rust crate sqlx to 0.7. plugin-sql msrv is now 1.65 (#464)
fix(persisted-scope): separately save asset protocol patterns (#459)
fix(deps): update rust crate iota-crypto to 0.22 (#475)
fix(deps): update tauri monorepo to v1.4.0 (#482)
resolve to v15.1.0 (#489)
fix(deps): update rust crate iota-crypto to 0.23 (#495)
@vasilvestre
Copy link

This got releases in v1 and v2?

@FabianLars
Copy link
Member

yes

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.

[persisted-scope] Asset protocol not configured correctly for directories after restart
3 participants