Fix rename of default disk cache directory

[zhongwuzw]

New Pull Request Checklist

• I have read and understood the CONTRIBUTING guide

• I have read the Documentation

• I have searched for a similar pull request in the project and found none

• I have updated this branch with the latest master to avoid conflicts (via merge from master or rebase)

• I have added the required tests to prove the fix/feature I am adding

• I have updated the documentation (if necessary)

• I have run the tests and they pass

• I have run the lint and it passes (pod lib lint)

This merge request fixes / reffers to the following issues: ...

Pull Request Description

1. Maybe from 5.x? We changed the default disk cache name of directory, we can't just simply do the rename operation, because the older version's directory is still exist (ps. if user upgrade from older to 5.x), and may contains many images, if we do not handle these images, they would always exists, cannot access, delete or anything.
2. Remove the compiler warnings of options.

[codecov-io]

Codecov Report

Merging #2412 into 5.x will increase coverage by 0.09%.
The diff coverage is 81.81%.

@@            Coverage Diff             @@
##              5.x    #2412      +/-   ##
==========================================
+ Coverage   68.83%   68.93%   +0.09%     
==========================================
  Files          47       47              
  Lines        6649     6669      +20     
==========================================
+ Hits         4577     4597      +20     
  Misses       2072     2072

Flag	Coverage Δ
#iOS	70.02% <81.81%> (+0.06%)	⬆️
#macOS	68.61% <81.81%> (+0.07%)	⬆️

Impacted Files	Coverage Δ
SDWebImage/SDImageCache.m	69.84% <100%> (+0.75%)	⬆️
SDWebImage/SDImageCacheDefine.m	62.16% <50%> (ø)	⬆️
SDWebImage/SDImageLoader.m	86% <75%> (ø)	⬆️
SDWebImage/SDDiskCache.m	54.32% <77.77%> (+1.37%)	⬆️
SDWebImage/SDWebImageManager.m	82.11% <0%> (+0.81%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 217510e...1cbab33. Read the comment docs.

[dreampiggy]

@zhongwuzw You can still use the default path as you want. Just keep the old cache folder without any issue. See #2299

SDImageCacheConfig.defaultCacheConfig.namespacePrefix = @"com.hackemist.SDWebImageCache.";

So I don't think we need to do anything like movement, each time you call init method for SDImageCache

[zhongwuzw]

@dreampiggy , nope, we can't expect users to know everything internal. Many users would just use one shot UIImageView category method to load image. We can't just simply rename the default directory and leave many garbage files alone on the disk.

[dreampiggy]

This change is adoptable. Because this may shadow the options arg from the input arg. But this is not related to this PR, create another instead...

For each PR, it should contains the most related change for the feature/bugfix. Don't try to bundle something unrelated to a single PR, because this will make it hard to review, and may cause issue if we want to revert.

[bpoplauschi]

I agree with this approach as a generic rule of thumb, but I think we can let this one slip :)

[zhongwuzw]

@dreampiggy 👌 I just think it's trivial, so I do not create a separate PR. I'll be careful next time. 😂

[bpoplauschi]

Looks good to me.

[dreampiggy]

@zhongwuzw This change, should be already in the migration guide. For 4.x user, they should read it before upgrating, because some changes are totally break change, which not designed to be compatible. And some change may also change the semantic of same API naming. For example, the method in SDWebImagePrecher change the concept of prefetch behavior, even the API naming looks the same.

[bpoplauschi]

@dreampiggy this is not the case of changing some APIs - which indeed are subject to reading the migration guide and updating the consumer code.
In this case, I think most users will not become aware that the cache path has changed. It is not a big issue since we will just re-download everything, but I prefer we do the migration. It's the right approach for any storage upgrade. If you upgrade a storage (let's say CoreData), you don't expect the user to handle all the migration, you do as much as possible yourself under the hood. Especially here where we will just keep that folder for no usage, just taking up space.

[zhongwuzw]

Yeah, when we develop a product, we need treat users as foolish (No discriminate, it's just the truth for product 😂 ), just like #2409 , we can't expect user know everything. We just try our best to let users go to the right path, decrease mistakes. If we think users needs to know everything, maybe we don't need to fix bug anymore. Let's user do it. 😂

[dreampiggy]

@bpoplauschi I add one chapter about the important behavior changs outside API chanegs in the Migration-Guide-Notable-Behavior-Changes.

Because this changes, will been called each time you create SDImageCache instance. However, this does not do anything for 5.x new user.

If you still agree that this is big issue and need we do so, this code should not been placed in the SDImageCache itself. Because it's now a wrapper of memory cache / disk cache class. You should place this code into SDDiskCache. And user provided disk cache implementation may not even expect there are a movement of other things to my own folder. (For example, we try a disk cache implementation based on another hex code other than MD5, this will cause our own disk cache mixed up)

[bpoplauschi]

• agree that this migration procedure should be limited to SDDiskCache
• we need to find a way to easily check and start the migration only then
• any strong storage API needs to handle data format upgrade/migration, so I propose:
  • adding migrateCacheFromCachePath:toCachePath: method to SDDiskCache protocol and implement it in our SDDiskCache class
  • call that method from SDImageCache only if the com.hackemist.SDWebImageCache.default path exists

[zhongwuzw]

@dreampiggy The path you listed in Migration Guide is wrong. The correct relative path is Library/Caches/default/com.hackemist.SDImageCache.default.

I think it's up to how you understand the manager, if you think all related operation should put into SDDiskCache, it can, but maybe it still has issues, because we put the business code into the SDDiskCache, because SDDiskCache is only responsible to do disk operation. We also can handle this, just add a method to move files, and we do business logical in SDImageCache.
If you think manager needs to do these things, I think it's also acceptable.

[dreampiggy]

@zhongwuzw No. for example we have a custom disk cache implementation, and put new images in that SDImageCache folder. However, we still have SDWebImageCache folder. So, let assume you move the 4.x original folder to our new path, it will override and put something 4.x MD5-based files.

But however, our disk cache does not use MD5 hash for key-mapping. It may be a case that a cache key may be hashed to the same value, which should been a cache miss in our hash check, but it accidentilly hit the original files from 4.x folder. Which will cause issue.

4.x && 5.x's SDDiskCache behavior on naming hashing is the same now. So it's safe to directly move the 4.x folder to 5.x folder for user who use SDDiskCache. But however, for other people does not use the built-in SDDiskCache class, this will cause a mass up of files.

[zhongwuzw]

@dreampiggy If any user changed disk behavior from 4.x to 5.x, I think it's him own responsibility to handle these things, maybe he would try delete all directories if he want. I think the PR covered the people who just uses one-shot method.

[dreampiggy]

@zhongwuzw If you still need to do a disk cache migration. My consideration is that this code should not been placed into SDImageCache. Since you say If any user changed disk behavior from 4.x to 5.x, I think it's him own responsibility to handle these things.

For SDWebImage, by default it always use SDDiskCache, so if the user does not custom any disk cache implementation. This code should be placed into SDDiskCache and it works, right ?

Put any disk reading & writing code in SDImageCache level is not designed as a good point. The disk operation should happend on the disk cache level.

[zhongwuzw]

@dreampiggy Yeah, I can, what's your opinion about @bpoplauschi 's proposal, we can use dispatch_once to do one operation. I already add the check logical, so it would do move only necessary, what you think wether we need add a method in SDDiskCache protocol?

[dreampiggy]

@zhongwuzw Create a new protocol, just for a compatible method, is overcome. Actually people we use custom disk cache, may not use the default cache prefix because this will cause issue. (Like we use a separate prefix).

So, maybe we can reach a consensus , some adddtional changes should be provided:

• Aggree that this older path movement feature is adoptable
• Use dispatch_once to avoid when multiple cache initialize at the same time cause the duplicate movement.
• Move the total code into SDDiskCache.m implementation files. And do some check, like when currentCachePath == "SDWebImageCache.default", earily return.
• Polish the migration guide and let user who need migration notice this changes. User can decide whether continue to use 4.x cache path or new 5.x cache path.

[zhongwuzw]

@dreampiggy @bpoplauschi The reasons why I put only disk related operation to SDDiskCache are:

1. We use makeDiskCachePath or something to generate disk cache path in SDImageCache, other words, SDImageCache would tell SDDiskCache where to save. I think we still need keep the getter of default cache disk path in SDImageCache.
2. Keep fileManager in the same ioQueue, in SDDiskCache, almost all methods is synchronous, I think we shouldn't let the migrate operation in another secondly thread.

What's your guys think about? 🤔

[dreampiggy]

...emmm. Am I miss something ?

I means, move all code, into [SDDiskCache initWithCachePath:config:] method. Don't need to modify any code in SDImageCache.m, this method should not be visible in header files as well.

During init method of SDDiskCache, use dispatch_once + global queue to only move once.

[zhongwuzw]

@dreampiggy emm, I know you opinion, so I put the reasons in here. I can explain my thoughts further.
Why I put the default cache path in SDImageCache is we generate the default cache path in SDImageCache, right? Let's see how we generate in SDImageCache:

// class of SDImageCache
- (instancetype)init {
    return [self initWithNamespace:@"default"];
}
- (nonnull instancetype)initWithNamespace:(nonnull NSString *)ns {
    NSString *path = [self makeDiskCachePath:ns]; // first component disk cache path 
    return [self initWithNamespace:ns diskCacheDirectory:path];
}
- (nonnull instancetype)initWithNamespace:(nonnull NSString *)ns
                       diskCacheDirectory:(nonnull NSString *)directory
                                   config:(nullable SDImageCacheConfig *)config {
    if ((self = [super init])) {
        NSString *namespacePrefix = config.namespacePrefix;
        if (!namespacePrefix) {
            namespacePrefix = @"";
        }
        NSString *fullNamespace = [namespacePrefix stringByAppendingString:ns]; 
      
        // Init the disk cache
        if (directory != nil) {
            _diskCachePath = [directory stringByAppendingPathComponent:fullNamespace];
        } else {
            NSString *path = [self makeDiskCachePath:ns];
            _diskCachePath = path;
        }
    return self;
}

We combine many components to generate default cache disk path. The SDDiskCache is just a class that do disk operations, like access, delete, move, right? other words, it doesn't need to know what's the disk path it is, its only job is do disk operation, I think compare old and new disk path is not what it should do. If you still think we should put all these things to SDDiskCache, should we need to hard code the default path? or extract the methods of default cache path related methods into a single class to let both SDImageCache and SDDiskCache access it?

[bpoplauschi]

@zhongwuzw take a look at #2417, I think the approach from @dreampiggy is simpler.
But I would not be so picky in choosing the approach :) You guys should decide

[bpoplauschi]

I think calling private methods is just a bad practice (Apple discourages this) and error prone.
We can always create a private category over a class that we use internally, but do not expose through our installations. But that is just not needed. We can expose this method on SDDiskCache, no problem in it being available.

[bpoplauschi]

I just merged #2417 so closing this PR. Thanks @zhongwuzw for driving this conversation, it's great to have the migration code in place.