Add-on store: Improve refreshing the cache #15071
Conversation
|
cc: @josephsl |
|
Hi, I’ll publish Windows App Essentials dev/beta/stable releases throughout this week, so this (and some other add-ons) can be used to test the overall idea. Thanks.
|
Co-authored-by: Leonard de Ruijter <leonardder@users.noreply.github.com>
|
Thanks @josephsl and @LeonarddeR for your comments. |
|
I think that this is ready for review. |
source/_addonStore/dataManager.py
Outdated
| @@ -142,10 +157,10 @@ def _getCachedAddonData(self, cacheFilePath: str) -> Optional[CachedAddonsModel] | |||
| cacheData = json.load(cacheFile) | |||
There was a problem hiding this comment.
Could you also look at fixing #15077, at the same time as #15071 (comment)
There was a problem hiding this comment.
@nvdaes, you have asked me to test this PR in #15077 (comment).
Testing from source on latest commit (dfe1b22) with the file from #15077, the issue is still present.
You should add a try/except around the two following lines in _getCachedAddonData:
with open(cacheFilePath, 'r') as cacheFile:
cacheData = json.load(cacheFile)
is there a reason get is used here? This was to avoid an error if the "cacheHash" key doesn't exist, but now I'll see if I can fix this. |
|
I've made another commit and I'll way for further comments @seanbudd |
|
Hi @CyrilleB79 . You wrote:
I've tried to address your comment. Please try again if you can. |
Yes NVDA loads successfully now and the file seems to be recreated. Having a look at the code, I would say that it may be valuable to log the exception or at least a debugWarning message. Indeed it's not expected to fail loading the file:
I wonder if making a backup of this badly formatted file before overwriting it would be needed in case of a bug to help debugging it. Surely the current PR fix the startup issue described in #15077. But is there an issue remaining? Why was a badly formatted file created initially? @seanbudd what is your opinion on this? |
|
Thanks Cyrille: I had thought about this. Let"s wait for Sean feedback in case he has some preference about the kind of warning that maybe writen in the log. |
I think this is unnecessary as these caches are not an important state file - and can easily be wiped and replaced |
|
@seanbudd, I think we should restore _getCurrentApiVersionForURL'.
I'll do later. See this error:
file "C:\Users\nrm19\Documents\repos\nvda\source\_addonStore\dataManager.py",
line 191, in getLatestCompatibleAddons
apiData = self._getLatestAddonsDataForVersion(_getCurrentApiVersionForURL())
NameError: name '_getCurrentApiVersionForURL' is not defined
2023-07-04 3:41 GMT+02:00, Sean Budd ***@***.***>:
… @seanbudd commented on this pull request.
The changes generally look good to me
> @@ -49,7 +50,11 @@ def _getCurrentApiVersionForURL() -> str:
def _getAddonStoreURL(channel: Channel, lang: str, nvdaApiVersion: str) ->
str:
_baseURL = "https://nvaccess.org/addonStore/"
this should probably be removed as it is unused
> @@ -35,6 +35,7 @@
from gui.message import DisplayableError
+_BASE_URL = "https://nvaccess.org/addonStore/"
```suggestion
_BASE_URL = "https://nvaccess.org/addonStore"
```
> @@ -49,7 +50,11 @@ def _getCurrentApiVersionForURL() -> str:
def _getAddonStoreURL(channel: Channel, lang: str, nvdaApiVersion: str) ->
str:
_baseURL = "https://nvaccess.org/addonStore/"
- return _baseURL + f"{lang}/{channel.value}/{nvdaApiVersion}.json"
+ return _BASE_URL + f"{lang}/{channel.value}/{nvdaApiVersion}.json"
For consistency
```suggestion
return f"{_BASE_URL }/{lang}/{channel.value}/{nvdaApiVersion}.json"
```
> @@ -49,7 +50,11 @@ def _getCurrentApiVersionForURL() -> str:
def _getAddonStoreURL(channel: Channel, lang: str, nvdaApiVersion: str) ->
str:
_baseURL = "https://nvaccess.org/addonStore/"
- return _baseURL + f"{lang}/{channel.value}/{nvdaApiVersion}.json"
+ return _BASE_URL + f"{lang}/{channel.value}/{nvdaApiVersion}.json"
+
+
+def _getCacheHashURL() -> str:
+ return _BASE_URL + "cacheHash.json"
```suggestion
return f"{_BASE_URL}/cacheHash.json"
```
> @@ -139,15 +154,22 @@ def _getCachedAddonData(self, cacheFilePath: str) ->
> Optional[CachedAddonsModel]
if not os.path.exists(cacheFilePath):
return None
with open(cacheFilePath, 'r') as cacheFile:
- cacheData = json.load(cacheFile)
- if not cacheData:
+ try:
+ cacheData = json.load(cacheFile)
+ except Exception:
+ return None
I think this should log at exception level
```suggestion
log.exception(f"Invalid add-on store cache")
return None
```
> @@ -139,15 +154,22 @@ def _getCachedAddonData(self, cacheFilePath: str) ->
> Optional[CachedAddonsModel]
if not os.path.exists(cacheFilePath):
return None
with open(cacheFilePath, 'r') as cacheFile:
- cacheData = json.load(cacheFile)
- if not cacheData:
+ try:
+ cacheData = json.load(cacheFile)
+ except Exception:
+ return None
+ try:
+ data = cacheData["data"]
+ cacheHash = cacheData["cacheHash"]
+ cachedLanguage = cacheData["cachedLanguage"]
+ nvdaAPIVersion = cacheData["nvdaAPIVersion"]
+ except KeyError:
return None
same here
```suggestion
log.exception(f"Invalid add-on store cache:\n{cacheData}")
return None
```
--
Reply to this email directly or view it on GitHub:
#15071 (review)
You are receiving this because you were mentioned.
Message ID: ***@***.***>
|
See test results for failed build of commit d9a8f1ae2c |
|
@seanbudd 24o53:
I think it's all done. |
source/_addonStore/dataManager.py
Outdated
| shouldRefreshData = ( | ||
| not self._compatibleAddonCache | ||
| or self._compatibleAddonCache.nvdaAPIVersion != addonAPIVersion.CURRENT | ||
| or _DataManager._cachePeriod < (datetime.now() - self._compatibleAddonCache.cachedAt) | ||
| or cacheHash and (self._compatibleAddonCache.cacheHash != cacheHash) |
There was a problem hiding this comment.
I think we should always refresh the cache if fetching the cacheHash fails.
This assumes we can treat "None" as a valid cacheHash,
| or cacheHash and (self._compatibleAddonCache.cacheHash != cacheHash) | |
| or cacheHash is None | |
| or self._compatibleAddonCache.cacheHash != cacheHash |
source/_addonStore/dataManager.py
Outdated
| shouldRefreshData = ( | ||
| not self._latestAddonCache | ||
| or _DataManager._cachePeriod < (datetime.now() - self._latestAddonCache.cachedAt) | ||
| or cacheHash and (self._latestAddonCache.cacheHash != cacheHash) |
There was a problem hiding this comment.
same here re: cacheHash being None
| or cacheHash and (self._latestAddonCache.cacheHash != cacheHash) | |
| or cacheHash is None | |
| or self._compatibleAddonCache.cacheHash != cacheHash |
See test results for failed build of commit 28d1edcadb |
Link to issue number:
Fixes #15034
Fixes #15077
Summary of the issue:
The store data cache is refreshed every 6 hours. This may not be enough and it's desirable to add a mechanism to refresh it making a small request to the server, so we can request data just when a new commit has been done to the views branch of the nvaccess/addon-datastore repo.
Description of user facing changes
When opening the add-on store, if there has been changes on the server, the cached data should be updated.
Description of development approach
Testing strategy:
Also, manual tests have confirmed that new add-ons sent to the store have been retrieved.
Known issues with pull request:
When the checkbox to get incompatible add-ons is checked, the add-ons list is refreshed and this may take some seconds.
Change log entries:
n/a
New features
Changes
Bug fixes
For Developers
Code Review Checklist: