Added cache to get_settings #1151
Merged
+46
−1
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The settings file is read about 30 times per second and parsed from json. Cache function added for now.
Contributor
WalkthroughIntroduces module-level caching for table_settings.json in server/helper.py by adding globals LASTCACHEDATE and CACHE. get_setting now checks file mtime, populates cache from data["data"], and returns values from CACHE. Adds error handling for missing/invalid JSON. Includes an unreachable fallback search block after an early return. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant Helper as helper.get_setting(key)
participant FS as table_settings.json
Caller->>Helper: get_setting(key)
alt Cache valid (mtime == LASTCACHEDATE) and key in CACHE
Helper-->>Caller: return CACHE[key]
else Cache stale/empty
Helper->>FS: stat mtime + read JSON
FS-->>Helper: data["data"]
rect rgba(200,230,255,0.3)
note right of Helper: Rebuild CACHE keyed by item.setKey<br/>Update LASTCACHEDATE
end
alt key in CACHE
Helper-->>Caller: return CACHE[key]
else key missing
Helper-->>Caller: return None (log error)
end
end
note over Helper: An additional fallback search block exists<br/>but is unreachable due to earlier return.
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/helper.py (1)
339-359: Remove unreachable fallback; it also changes CACHE type to listThis entire block is unreachable due to earlier returns. If it ever executes,
CACHE = data.get("data", [])rebinds CACHE from dict to list, breaking all lookups. Delete it.- try: - - with open(settingsFile, 'r') as json_file: - - data = json.load(json_file) - CACHE=data.get("data",[]) - - for item in data.get("data",[]): - if item.get("setKey") == key: - return item - - mylog('debug', [f'[Settings] ⚠ ERROR - setting_missing - Setting not found for key: {key} in file {settingsFile}']) - - return None - - except (FileNotFoundError, json.JSONDecodeError, ValueError) as e: - # Handle the case when the file is not found, JSON decoding fails, or data is not in the expected format - mylog('none', [f'[Settings] ⚠ ERROR - JSONDecodeError or FileNotFoundError for file {settingsFile}']) - - return None + # (removed unreachable legacy path)
🧹 Nitpick comments (3)
server/helper.py (3)
31-33: Initialize cache metadata with sane types; consider thread-safety
- Use a non-string initial value for LASTCACHEDATE to match mtime type.
- Optional: guard updates with a lock if this runs under multi-threaded server workloads.
-LASTCACHEDATE='' -CACHE={} +LASTCACHEDATE = None # last file mtime (float) or None +CACHE = {}Additionally (outside this hunk), you can add:
# near imports import threading CACHE_LOCK = threading.RLock()and wrap mutations:
# inside get_setting, around CACHE.clear() / writes / LASTCACHEDATE assignment with CACHE_LOCK: CACHE.clear() ... LASTCACHEDATE = fileModifiedTime
301-314: Log level and message consistencyMinor: the three “[Import table_settings.json] …” debug lines are helpful. Consider consolidating into a single structured log call to reduce noise at high call rates.
299-299: Prefer pathlib Path for constructing settingsFile
server/helper.py:299
- Replace
settingsFile = apiPath + 'table_settings.json'
with either:
• after changing the import toimport const:
settingsFile = Path(const.apiPath) / 'table_settings.json'
• or, if keeping the star-import, at minimum:
settingsFile = Path(apiPath) / 'table_settings.json'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
server/helper.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
server/helper.py (1)
server/logger.py (1)
mylog(82-93)
🪛 Ruff (0.12.2)
server/helper.py
299-299: apiPath may be undefined, or defined from star imports
(F405)
324-324: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: docker_dev
Comment on lines
+305
to
+334
| fileModifiedTime = os.path.getmtime(settingsFile) | ||
| mylog('debug', ['[Import table_settings.json] checking table_settings.json file ']) | ||
| mylog('debug', ['[Import table_settings.json] LASTCACHEDATE :', LASTCACHEDATE]) | ||
| mylog('debug', ['[Import table_settings.json] fileModifiedTime :', fileModifiedTime]) | ||
| if fileModifiedTime == LASTCACHEDATE and CACHE: | ||
| mylog('debug', ['[Import table_settings.json] using cached version of table_settings.json ']) | ||
| try: | ||
| return CACHE[key] | ||
| except KeyError: | ||
| mylog('none', [f'[Settings] ⚠ ERROR - setting_missing - Setting not found for key: {key} in cached file {settingsFile}']) | ||
|
|
||
| LASTCACHEDATE = fileModifiedTime | ||
|
|
||
| try: | ||
| with open(settingsFile, 'r') as json_file: | ||
| data = json.load(json_file) | ||
| for item in data.get("data",[]): | ||
| # map setkey to item | ||
| CACHE[item["setKey"]] = item | ||
| except (FileNotFoundError, json.JSONDecodeError, ValueError) as e: | ||
| # Handle the case when the file is not found, JSON decoding fails, or data is not in the expected format | ||
| mylog('none', [f'[Settings] ⚠ ERROR - JSONDecodeError or FileNotFoundError for file {settingsFile}']) | ||
| return None | ||
|
|
||
| try: | ||
| return CACHE[key] | ||
| except KeyError: | ||
| mylog('none', [f'[Settings] ⚠ ERROR - setting_missing - Setting not found for key: {key} in file {settingsFile}']) | ||
| return None | ||
|
|
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix: handle missing file (getmtime), avoid unnecessary reloads, rebuild cache atomically, and log exception details
- os.path.getmtime can raise FileNotFoundError before the try/except. This will crash the server.
- When mtime is unchanged and CACHE exists but key is missing, you still re-read the file; just return None.
- Update LASTCACHEDATE only after a successful load; otherwise you can pin a bad mtime and never refresh.
- Rebuild CACHE from scratch to drop stale keys; also guard missing setKey.
- Use the caught exception (e) in the log to satisfy Ruff F841.
@@
- fileModifiedTime = os.path.getmtime(settingsFile)
- mylog('debug', ['[Import table_settings.json] checking table_settings.json file '])
- mylog('debug', ['[Import table_settings.json] LASTCACHEDATE :', LASTCACHEDATE])
- mylog('debug', ['[Import table_settings.json] fileModifiedTime :', fileModifiedTime])
- if fileModifiedTime == LASTCACHEDATE and CACHE:
- mylog('debug', ['[Import table_settings.json] using cached version of table_settings.json '])
- try:
- return CACHE[key]
- except KeyError:
- mylog('none', [f'[Settings] ⚠ ERROR - setting_missing - Setting not found for key: {key} in cached file {settingsFile}'])
-
- LASTCACHEDATE = fileModifiedTime
-
- try:
- with open(settingsFile, 'r') as json_file:
- data = json.load(json_file)
- for item in data.get("data",[]):
- # map setkey to item
- CACHE[item["setKey"]] = item
- except (FileNotFoundError, json.JSONDecodeError, ValueError) as e:
- # Handle the case when the file is not found, JSON decoding fails, or data is not in the expected format
- mylog('none', [f'[Settings] ⚠ ERROR - JSONDecodeError or FileNotFoundError for file {settingsFile}'])
- return None
-
- try:
- return CACHE[key]
- except KeyError:
- mylog('none', [f'[Settings] ⚠ ERROR - setting_missing - Setting not found for key: {key} in file {settingsFile}'])
- return None
+ try:
+ fileModifiedTime = os.path.getmtime(settingsFile)
+ except FileNotFoundError as e:
+ mylog('none', [f'[Settings] ⚠ ERROR - File not found: {settingsFile}: {e}'])
+ return CACHE.get(key) if CACHE else None
+
+ mylog('debug', ['[Import table_settings.json] checking table_settings.json file '])
+ mylog('debug', ['[Import table_settings.json] LASTCACHEDATE :', LASTCACHEDATE])
+ mylog('debug', ['[Import table_settings.json] fileModifiedTime :', fileModifiedTime])
+
+ if fileModifiedTime == LASTCACHEDATE and CACHE:
+ # Serve from cache without re-reading the file
+ if key in CACHE:
+ mylog('debug', ['[Import table_settings.json] using cached version of table_settings.json '])
+ return CACHE[key]
+ mylog('none', [f'[Settings] ⚠ ERROR - setting_missing - Setting not found for key: {key} in cached file {settingsFile}'])
+ return None
+
+ try:
+ with open(settingsFile, 'r') as json_file:
+ data = json.load(json_file)
+ # Rebuild cache from scratch to avoid stale keys persisting
+ CACHE.clear()
+ for item in data.get("data", []):
+ set_key = item.get("setKey")
+ if set_key is None:
+ mylog('minimal', [f'[Settings] ⚠ WARNING - Skipping item without setKey in {settingsFile}'])
+ continue
+ CACHE[set_key] = item
+ # Update mtime only after successful reload
+ LASTCACHEDATE = fileModifiedTime
+ except (FileNotFoundError, json.JSONDecodeError, ValueError) as e:
+ mylog('none', [f'[Settings] ⚠ ERROR - JSONDecodeError or FileNotFoundError for file {settingsFile}: {e}'])
+ return None
+
+ # Lookup after (re)load
+ if key in CACHE:
+ return CACHE[key]
+ mylog('none', [f'[Settings] ⚠ ERROR - setting_missing - Setting not found for key: {key} in file {settingsFile}'])
+ return None📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| fileModifiedTime = os.path.getmtime(settingsFile) | |
| mylog('debug', ['[Import table_settings.json] checking table_settings.json file ']) | |
| mylog('debug', ['[Import table_settings.json] LASTCACHEDATE :', LASTCACHEDATE]) | |
| mylog('debug', ['[Import table_settings.json] fileModifiedTime :', fileModifiedTime]) | |
| if fileModifiedTime == LASTCACHEDATE and CACHE: | |
| mylog('debug', ['[Import table_settings.json] using cached version of table_settings.json ']) | |
| try: | |
| return CACHE[key] | |
| except KeyError: | |
| mylog('none', [f'[Settings] ⚠ ERROR - setting_missing - Setting not found for key: {key} in cached file {settingsFile}']) | |
| LASTCACHEDATE = fileModifiedTime | |
| try: | |
| with open(settingsFile, 'r') as json_file: | |
| data = json.load(json_file) | |
| for item in data.get("data",[]): | |
| # map setkey to item | |
| CACHE[item["setKey"]] = item | |
| except (FileNotFoundError, json.JSONDecodeError, ValueError) as e: | |
| # Handle the case when the file is not found, JSON decoding fails, or data is not in the expected format | |
| mylog('none', [f'[Settings] ⚠ ERROR - JSONDecodeError or FileNotFoundError for file {settingsFile}']) | |
| return None | |
| try: | |
| return CACHE[key] | |
| except KeyError: | |
| mylog('none', [f'[Settings] ⚠ ERROR - setting_missing - Setting not found for key: {key} in file {settingsFile}']) | |
| return None | |
| try: | |
| fileModifiedTime = os.path.getmtime(settingsFile) | |
| except FileNotFoundError as e: | |
| mylog('none', [f'[Settings] ⚠ ERROR - File not found: {settingsFile}: {e}']) | |
| return CACHE.get(key) if CACHE else None | |
| mylog('debug', ['[Import table_settings.json] checking table_settings.json file ']) | |
| mylog('debug', ['[Import table_settings.json] LASTCACHEDATE :', LASTCACHEDATE]) | |
| mylog('debug', ['[Import table_settings.json] fileModifiedTime :', fileModifiedTime]) | |
| if fileModifiedTime == LASTCACHEDATE and CACHE: | |
| # Serve from cache without re-reading the file | |
| if key in CACHE: | |
| mylog('debug', ['[Import table_settings.json] using cached version of table_settings.json ']) | |
| return CACHE[key] | |
| mylog('none', [f'[Settings] ⚠ ERROR - setting_missing - Setting not found for key: {key} in cached file {settingsFile}']) | |
| return None | |
| try: | |
| with open(settingsFile, 'r') as json_file: | |
| data = json.load(json_file) | |
| # Rebuild cache from scratch to avoid stale keys persisting | |
| CACHE.clear() | |
| for item in data.get("data", []): | |
| set_key = item.get("setKey") | |
| if set_key is None: | |
| mylog('minimal', [f'[Settings] ⚠ WARNING - Skipping item without setKey in {settingsFile}']) | |
| continue | |
| CACHE[set_key] = item | |
| # Update mtime only after successful reload | |
| LASTCACHEDATE = fileModifiedTime | |
| except (FileNotFoundError, json.JSONDecodeError, ValueError) as e: | |
| mylog('none', [f'[Settings] ⚠ ERROR - JSONDecodeError or FileNotFoundError for file {settingsFile}: {e}']) | |
| return None | |
| # Lookup after (re)load | |
| if key in CACHE: | |
| return CACHE[key] | |
| mylog('none', [f'[Settings] ⚠ ERROR - setting_missing - Setting not found for key: {key} in file {settingsFile}']) | |
| return None |
🧰 Tools
🪛 Ruff (0.12.2)
324-324: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
🤖 Prompt for AI Agents
In server/helper.py around lines 305-334: handle FileNotFoundError from
os.path.getmtime by wrapping the getmtime call in its own try/except and logging
the caught exception; if mtime is unchanged and CACHE exists, immediately return
CACHE.get(key) (which will be None) instead of re-reading the file; only set
LASTCACHEDATE after a successful load; rebuild the cache atomically by
populating a new temporary dict and then assigning it to CACHE (skip entries
missing "setKey"), and when catching exceptions include the exception variable
(e) in the mylog message so the actual error details are logged.
Contributor
Author
There was a problem hiding this comment.
Sweet suggestions, applying and testing.
Although some logic in CodeRabbot would have broken the routine.
Contributor
There was a problem hiding this comment.
@ingoratsdorf Thanks for the feedback! I'm glad the suggestions were helpful even if some adjustments were needed. You're absolutely right to adapt the logic to fit the actual requirements - you know the codebase and runtime behavior better than I do.
Feel free to ping me if you run into any specific issues while testing or if you'd like me to review the final implementation. Looking forward to seeing how the caching performance improvement works out!
🐰💭 curious about the performance gains
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The settings file is read about 30 times per second and parsed from json. Cache function added for now.
This is more like a proof of concept.
We should investigate if the file 'table_settings.json' actually changes at all during the runtime of the server.
If not, we could eliminate the file change check as well and gain even more savings.
Summary by CodeRabbit