asl-node-auth-check: node changed to s in lookup#183
Conversation
Robustify failures in `get_node_regtime()`
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
bin/asl-node-auth-check (1)
360-365:⚠️ Potential issue | 🔴 CriticalHandle empty array responses from the API endpoint.
The new endpoint at
https://www.allstarlink.org/nodelist/nodelist-server.php?s={node}can return an empty JSON array[], but the code at line 398 directly accessesnode_reginfo[0]without checking if the list is empty. This will raise anIndexErrorwhen the API returns no registration data. Additionally, if'regseconds'is missing from the response,.get('regseconds')will returnNone, causing aTypeErrorwhen passed todatetime.fromtimestamp()on line 399.Add a check to ensure
node_reginfois not an empty list before accessing the first element, and handle the case where'regseconds'is missing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/asl-node-auth-check` around lines 360 - 365, The API response handling must guard against empty arrays and missing 'regseconds': after calling response.json() (assign to node_reginfo), check if not node_reginfo or len(node_reginfo) == 0 and return None (or otherwise abort processing) before accessing node_reginfo[0]; then extract regseconds = node_reginfo[0].get('regseconds') and verify it is a numeric value (int/float) before passing it to datetime.fromtimestamp(); if regseconds is missing or not numeric, handle by returning None or skipping timestamp conversion to avoid TypeError.
🧹 Nitpick comments (2)
bin/asl-node-auth-check (2)
407-408: Similar KeyError risk oniptimeaccess.
stats['node']['iptime']can also raiseKeyErrorif the field is missing. Consider wrapping with a try-except or using.get()with a default:🛡️ Defensive access for iptime
- iptime = stats['node']['iptime'] - print_info(f"Last time IP changed was {iptime} UTC") + iptime = stats.get('node', {}).get('iptime') + if iptime: + print_info(f"Last time IP changed was {iptime} UTC") + else: + print_warning("IP change time not available in stats")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/asl-node-auth-check` around lines 407 - 408, Accessing stats['node']['iptime'] can raise a KeyError; update the code that reads iptime (the stats variable and the print_info call) to defensively fetch the value, e.g. use stats.get('node', {}).get('iptime') with a sensible default or wrap in try/except KeyError and handle missing iptime before calling print_info so you don't crash when the field is absent.
382-391: Consider defensive access for nested dictionary keys.The direct key access
stats['node']['server']['udpport']will raise aKeyErrorif the response structure differs from expected. Given the PR's goal to robustify failure handling, consider using defensive access:🛡️ Proposed defensive access
- - stats_udpport = stats['node']['server']['udpport'] + + try: + stats_udpport = stats['node']['server']['udpport'] + except (KeyError, TypeError): + print_error("Stats response missing expected UDP port structure") + n_errors += 1 + return n_errors, n_warnings🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/asl-node-auth-check` around lines 382 - 391, The code directly indexes stats['node']['server']['udpport'] which can raise KeyError if the response shape changes; update the access to be defensive (e.g., use nested .get calls or try/except) when reading stats_udpport from stats, handle the missing-key case by logging a clear error via print_error (or print_info) and incrementing n_errors, and only perform the comparison against bind_udpport when stats_udpport is present; reference the stats_udpport variable, the stats dict, bind_udpport, print_error/print_ok, and n_errors when making these changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bin/asl-node-auth-check`:
- Around line 394-399: The current code assumes node_reginfo contains an element
with a valid 'regseconds' value; update the block that computes reg_since and
reg_since_dt to first verify node_reginfo is a non-empty list and that
node_reginfo[0] contains a non-null numeric 'regseconds' before calling
datetime.fromtimestamp; if the list is empty or 'regseconds' is missing/None/not
a number, call print_error with a descriptive message, increment n_errors and
return n_errors, n_warnings (same as the existing null-case path). Reference the
existing variables and functions: node_reginfo, node_reginfo[0], reg_since,
reg_since_dt, datetime.fromtimestamp, UTC, print_error, n_errors, n_warnings.
---
Outside diff comments:
In `@bin/asl-node-auth-check`:
- Around line 360-365: The API response handling must guard against empty arrays
and missing 'regseconds': after calling response.json() (assign to
node_reginfo), check if not node_reginfo or len(node_reginfo) == 0 and return
None (or otherwise abort processing) before accessing node_reginfo[0]; then
extract regseconds = node_reginfo[0].get('regseconds') and verify it is a
numeric value (int/float) before passing it to datetime.fromtimestamp(); if
regseconds is missing or not numeric, handle by returning None or skipping
timestamp conversion to avoid TypeError.
---
Nitpick comments:
In `@bin/asl-node-auth-check`:
- Around line 407-408: Accessing stats['node']['iptime'] can raise a KeyError;
update the code that reads iptime (the stats variable and the print_info call)
to defensively fetch the value, e.g. use stats.get('node', {}).get('iptime')
with a sensible default or wrap in try/except KeyError and handle missing iptime
before calling print_info so you don't crash when the field is absent.
- Around line 382-391: The code directly indexes
stats['node']['server']['udpport'] which can raise KeyError if the response
shape changes; update the access to be defensive (e.g., use nested .get calls or
try/except) when reading stats_udpport from stats, handle the missing-key case
by logging a clear error via print_error (or print_info) and incrementing
n_errors, and only perform the comparison against bind_udpport when
stats_udpport is present; reference the stats_udpport variable, the stats dict,
bind_udpport, print_error/print_ok, and n_errors when making these changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a4ebde32-aa2b-4c45-8523-4d9985d19050
📒 Files selected for processing (1)
bin/asl-node-auth-check
| if node_reginfo is None: | ||
| print_error(f"No registration information available.") | ||
| n_errors += 1 | ||
| return n_errors, n_warnings | ||
| reg_since = node_reginfo[0].get('regseconds') | ||
| reg_since_dt = datetime.fromtimestamp(reg_since, UTC) |
There was a problem hiding this comment.
Incomplete robustness: handle empty list and missing regseconds.
The null check on line 394 is good, but two edge cases remain unhandled:
- Empty list: If the API returns
[],node_reginfo[0]raisesIndexError. - Missing key: If
regsecondsis absent,.get()returnsNone, anddatetime.fromtimestamp(None, UTC)raisesTypeError.
🐛 Proposed fix for complete robustness
node_reginfo = get_node_regtime(node)
- if node_reginfo is None:
+ if node_reginfo is None or len(node_reginfo) == 0:
print_error(f"No registration information available.")
n_errors += 1
return n_errors, n_warnings
- reg_since = node_reginfo[0].get('regseconds')
+
+ reg_since = node_reginfo[0].get('regseconds')
+ if reg_since is None:
+ print_error("Registration info missing 'regseconds' field.")
+ n_errors += 1
+ return n_errors, n_warnings
+
reg_since_dt = datetime.fromtimestamp(reg_since, UTC)📝 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.
| if node_reginfo is None: | |
| print_error(f"No registration information available.") | |
| n_errors += 1 | |
| return n_errors, n_warnings | |
| reg_since = node_reginfo[0].get('regseconds') | |
| reg_since_dt = datetime.fromtimestamp(reg_since, UTC) | |
| if node_reginfo is None or len(node_reginfo) == 0: | |
| print_error(f"No registration information available.") | |
| n_errors += 1 | |
| return n_errors, n_warnings | |
| reg_since = node_reginfo[0].get('regseconds') | |
| if reg_since is None: | |
| print_error("Registration info missing 'regseconds' field.") | |
| n_errors += 1 | |
| return n_errors, n_warnings | |
| reg_since_dt = datetime.fromtimestamp(reg_since, UTC) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bin/asl-node-auth-check` around lines 394 - 399, The current code assumes
node_reginfo contains an element with a valid 'regseconds' value; update the
block that computes reg_since and reg_since_dt to first verify node_reginfo is a
non-empty list and that node_reginfo[0] contains a non-null numeric 'regseconds'
before calling datetime.fromtimestamp; if the list is empty or 'regseconds' is
missing/None/not a number, call print_error with a descriptive message,
increment n_errors and return n_errors, n_warnings (same as the existing
null-case path). Reference the existing variables and functions: node_reginfo,
node_reginfo[0], reg_since, reg_since_dt, datetime.fromtimestamp, UTC,
print_error, n_errors, n_warnings.
|
Ugh, I forgot this was using |
Allan-N
changed the title
node changed to s in lookup.node changed to s in lookup
2 participants
Robustify failures from
get_node_regtime()Summary by CodeRabbit
Release Notes