Fix bucket name being reused between instances of different buckets - #496
Fix bucket name being reused between instances of different buckets#496minhdanh wants to merge 3 commits into
Conversation
Sorry, something went wrong.
rickdijk
left a comment
There was a problem hiding this comment.
Please add a test that two managers with different buckets get different cache keys / file services.
Sorry, something went wrong.
| /// and store them in your local cache. | ||
| class FirebaseCacheManager extends CacheManager { | ||
| static const key = 'firebaseCache'; | ||
| static const defaultKey = 'firebaseCache'; |
There was a problem hiding this comment.
key → defaultKey is a public API rename if anyone used FirebaseCacheManager.key. Prefer keeping key or a deprecated alias.
Sorry, something went wrong.
| FirebaseCacheManager._(retryOptions: _retryOptions, bucket: _bucket); | ||
| static final Map<String, FirebaseCacheManager> _instances = {}; | ||
|
|
||
| static RetryOptions? _retryOptions; |
There was a problem hiding this comment.
These statics are no longer needed once each instance owns its own bucket/retryOptions. Keeping them causes cross-instance leakage.
Sorry, something went wrong.
| _retryOptions = retryOptions; | ||
| return _instance; | ||
| final cacheKey = bucket ?? defaultKey; | ||
| if (_instances.containsKey(cacheKey)) { |
There was a problem hiding this comment.
When an instance already exists, a new retryOptions argument is silently ignored. Worth documenting, or only accepting options on first create.
Sorry, something went wrong.
| if (_instances.containsKey(cacheKey)) { | ||
| return _instances[cacheKey]!; | ||
| } | ||
| _bucket = bucket ?? _bucket; |
There was a problem hiding this comment.
_bucket = bucket ?? _bucket (and the same for _retryOptions) leaks state across instances.
FirebaseCacheManager(bucket: 'bucket-a'); FirebaseCacheManager(); // cacheKey is defaultKey, but _bucket is still 'bucket-a'
Pass the factory args directly instead:
final instance = FirebaseCacheManager._( key: cacheKey, retryOptions: retryOptions, bucket: bucket, );
(or putIfAbsent and drop the statics entirely)
Sorry, something went wrong.
| sdk: flutter | ||
| flutter_cache_manager: ^3.4.1 | ||
| firebase_storage: '>=12.0.0 <13.0.0' | ||
| firebase_storage: ^13.0.0 |
There was a problem hiding this comment.
This version bump should preferably be done in a different PR
Sorry, something went wrong.
✨ What kind of change does this PR introduce? (Bug fix, feature, docs update...)
This fixes an issue in which a subsequent FirebaseCacheManager instance cannot load or upload files correctly because the bucket name was set and cached in the first FirebaseCacheManager instance.
It also updates packages to latest versions.
⤵️ What is the current behavior?
In my case when I try to read a file from another bucket I see error " [firebase_storage/object-not-found] No object exists at the desired reference." although the file is there.
🆕 What is the new behavior (if this is a feature change)?
If we're initializing a FirebaseCacheManager with a new instance, it makes sure to use another cache key so that files from the other bucket can be read.
💥 Does this PR introduce a breaking change?
No
🐛 Recommendations for testing
Hmm, not sure
📝 Links to relevant issues/docs
🤔 Checklist before submitting