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 removeCacheFile #469

Merged
merged 2 commits into from
Oct 9, 2024
Merged

Fix removeCacheFile #469

merged 2 commits into from
Oct 9, 2024

Conversation

SmartVive
Copy link
Contributor

✨ What kind of change does this PR introduce? (Bug fix, feature, docs update...)

fix removeCacheFile

⤵️ What is the current behavior?

removeCacheFile not working. because file dir is wrong.

🆕 What is the new behavior (if this is a feature change)?

removeCacheFile is working.

💥 Does this PR introduce a breaking change?

no

🐛 Recommendations for testing

📝 Links to relevant issues/docs

🤔 Checklist before submitting

  • All projects build
  • Follows style guide lines (code style guide)
  • Relevant documentation was updated
  • Rebased onto current develop

@snoopdoggy322
Copy link

snoopdoggy322 commented Sep 25, 2024

@martijn00 pls merge it, clearing files doesn't work at all

@@ -184,7 +183,7 @@ class CacheStore {
if (_futureCache.containsKey(cacheObject.key)) {
await _futureCache.remove(cacheObject.key);
}
final file = io.File(cacheObject.relativePath);
final file = await fileSystem.createFile(cacheObject.relativePath);
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure this is correct. The old code only creates a reference to the file, and this code actually creates the file. So the check for the file will always be true, which is not correct.

Might there be a problem in your own code?

Copy link

@snoopdoggy322 snoopdoggy322 Sep 25, 2024

Choose a reason for hiding this comment

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

@martijn00
But the file reference leads to old code that never points to the actual file, resulting in a constant accumulation of files in the cache.

Is there a possible mistake in the name of the method itself? For example, the _fileExists method uses the same fileSystem.createFile to check for the existence of a file.

@SmartVive
Copy link
Contributor Author

test code:

    final oldFile = io.File(cacheObject.relativePath);
    print("oldFilePath:${oldFile.path},isExist:${await oldFile.exists()}");

    final newfile = await fileSystem.createFile(cacheObject.relativePath);
    print("newFilePath:${newfile.path},isExist:${await newfile.exists()}");
    
    final testfile = await fileSystem.createFile("test");
    print("testFilePath:${testfile.path},isExist:${await testfile.exists()}");

result:

oldFilePath:340d19b0-7b3d-11ef-ba62-6f906510429c.file,isExist:false

newFilePath:/data/user/0/xxx.xxx.xxx/cache/response/340d19b0-7b3d-11ef-ba62-6f906510429c.file,isExist:true

testFilePath:/data/user/0/xxx.xxx.xxx/cache/response/test,isExist:false

1.cacheObject.relativePath is file name. not file path
2.fileSystem.createFile("test") file will not be created

@martijn00

@wyyadd
Copy link
Contributor

wyyadd commented Oct 8, 2024

Same problem. File will never be deleted.

final file = io.File(cacheObject.relativePath);
 if (file.existsSync()) {
}

Previous code will fix that.

final file = await fileSystem.createFile(cacheObject.relativePath);

@martijn00
Copy link
Member

Reference #387

@martijn00
Copy link
Member

@SmartVive can you add some more tests to make sure the behaviour is verified?

@SmartVive
Copy link
Contributor Author

@martijn00 Do you mean adding unit tests? I did, but I'm not good at them.

@martijn00 martijn00 merged commit 515cc9e into Baseflow:develop Oct 9, 2024
10 checks passed
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