refactor: (BREAKING) Use unified requestAndCache method #2216
Conversation
This caches multiple static content requests in the database with a unified and simple way which reduces in memory state.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2216 +/- ##
==========================================
+ Coverage 57.53% 57.59% +0.05%
==========================================
Files 153 153
Lines 19476 19479 +3
==========================================
+ Hits 11205 11218 +13
+ Misses 8271 8261 -10
... and 1 file with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
| stackTrace, | ||
| ); | ||
|
|
||
| if (cachedResponse != null) { |
There was a problem hiding this comment.
is it really okay to return the user an expired cached data when a new request fails? I personally think we should just rethrow directly
There was a problem hiding this comment.
I would say it is better for the UX. We are doing this in a lot of other places already. And without WellKnown information for example most features of our app are not functional. And as these things very rarely change at all (maybe once a year or so or maybe even never?) having an "expired" result shouldn't be that bad.
There was a problem hiding this comment.
What if there's a config for blocking calls in well known and its changed on server side. Let's say the app fails to fetch this due to some internet issue and gives the old cached data. Later let's say the internet recovers but the app gets well known only on app startup so it has old data. This will make the user able to make calls even if it's changed on server side.
| /*async { | ||
| final wellKnownResponse = await httpClient.get( | ||
| Uri.https( | ||
| userID?.domain ?? homeserver!.host, | ||
| '/.well-known/matrix/client', | ||
| ), | ||
| ); | ||
| final wellKnown = DiscoveryInformation.fromJson( | ||
| jsonDecode(utf8.decode(wellKnownResponse.bodyBytes)) | ||
| as Map<String, Object?>, | ||
| ); | ||
| // do not reset the well known here, so super call | ||
| super.homeserver = wellKnown.mHomeserver.baseUrl.stripTrailingSlash(); | ||
| _wellKnown = wellKnown; | ||
| await database.storeWellKnown(wellKnown); | ||
| return wellKnown; | ||
| } | ||
| }*/ |
There was a problem hiding this comment.
can we remove this commented section?
|
|
||
| Future<bool> authenticatedMediaSupported() async { | ||
| return (await versionsResponse).versions.any( | ||
| return (await getVersions()).versions.any( |
There was a problem hiding this comment.
we could have a variable for await getVersions() so that it's not called twice
| if (discoveryInformation == null) { | ||
| return _clientBox.delete('discovery_information'); | ||
| } | ||
| Future<void> cacheCustomObject(String cacheKey, Map<String, Object?> object) { |
There was a problem hiding this comment.
we need to make sure that all these values are also cleared on clearCache() method in the same class. Maybe add some tests too?
This caches multiple static
content requests in the database
with a unified and simple way
which reduces in memory
state.
closes https://github.com/famedly/product-management/issues/2358
This simplifies some things a lot but also introduces an inconveniency: You can no longer access the wellKnown cache in and synchronious way by
client.wellKnownbut have to always await client.getWellKnown(). IMO this is definitely worth it to get rid of the state and win simplicity and also have these static content stored in the exactly same manner: