feat: Authoritative plugin fields #1435
Conversation
📝 WalkthroughWalkthroughIntroduces a device field lock/unlock feature with authoritative source-tracking to prevent plugins from overwriting manually set device values. Adds database schema columns, backend authorization logic, frontend UI elements, API endpoints, plugin configurations, and comprehensive test coverage. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User Interface
participant API as Backend API
participant Auth as Authoritative Handler
participant DB as Database
participant Plugin as Plugin (Update)
User->>API: Lock field (devName)
API->>Auth: lock_field(mac, "devName")
Auth->>DB: UPDATE Devices SET devNameSource='LOCKED'
DB-->>Auth: Success
Auth-->>API: Lock confirmed
Plugin->>API: Update device (devName='NewName')
API->>Auth: can_overwrite_field("devName", "LOCKED", plugin_prefix)
Auth-->>API: False (field locked)
API-->>Plugin: Field not updated (already at source value)
User->>API: Unlock field (devName)
API->>Auth: unlock_field(mac, "devName")
Auth->>DB: UPDATE Devices SET devNameSource=''
DB-->>Auth: Success
Auth-->>API: Unlock confirmed
Plugin->>API: Update device (devName='NewName')
API->>Auth: can_overwrite_field("devName", "", plugin_prefix)
Auth-->>API: True (field unlocked, source empty)
API->>DB: UPDATE Devices SET devName='NewName', devNameSource='PLUGIN_PREFIX'
DB-->>API: Success
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 9
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
server/models/device_instance.py (1)
593-609: Make the update + source enforcement atomic.
get_temp_db_connection()usesisolation_level=None, enabling SQLite autocommit mode. In autocommit,cur.execute(sql, values)at line 595 commits immediately beforeenforce_source_on_user_update()runs at line 604. If enforcement fails, the attemptedconn.rollback()cannot undo the already-committed UPDATE, leaving device data and source tracking out of sync. Wrap both the UPDATE and enforcement in an explicit transaction usingBEGIN IMMEDIATEto ensure both succeed or both fail atomically.🧩 Proposed fix
- cur = conn.cursor() - cur.execute(sql, values) - conn.commit() + conn.execute("BEGIN IMMEDIATE") + cur = conn.cursor() + cur.execute(sql, values) # Enforce source tracking on user updates # User-updated fields should have their *Source set to "USER" user_updated_fields = {k: v for k, v in data.items() if k in FIELD_SOURCE_MAP} if user_updated_fields and not data.get("createNew", False): try: enforce_source_on_user_update(normalized_mac, user_updated_fields, conn) except Exception as e: mylog("none", [f"[DeviceInstance] Failed to enforce source tracking: {e}"]) conn.rollback() conn.close() return {"success": False, "error": f"Source tracking failed: {e}"} + + conn.commit()server/api_server/api_server_start.py (2)
65-76: Fix source-field resolution; concatenation breaks several columns.
field_name + "Source"won’t matchdevLastIpSource,devFqdnSource,devSsidSource, ordevParentMacSource. UseFIELD_SOURCE_MAPto avoid mismatches and keep the allowlist in sync.🐛 Proposed fix
-from .openapi.schemas import ( # noqa: E402 [flake8 lint suppression] +from .openapi.schemas import ( # noqa: E402 [flake8 lint suppression] DeviceSearchRequest, DeviceSearchResponse, @@ RecentEventsRequest, SetDeviceAliasRequest ) +from db.authoritative_handler import FIELD_SOURCE_MAP # noqa: E402 [flake8 lint suppression] @@ - source_field = field_name + "Source" - allowed_tracked_fields = { - "devMac", "devName", "devLastIP", "devVendor", "devFQDN", - "devSSID", "devParentMAC", "devParentPort", "devParentRelType", "devVlan" - } - if field_name not in allowed_tracked_fields: + source_field = FIELD_SOURCE_MAP.get(field_name) + if not source_field: return jsonify({"success": False, "error": f"Field '{field_name}' cannot be locked"}), 400Also applies to: 472-479
47-47: Normalize MAC before the DB write.This endpoint writes device data but doesn’t validate/normalize the MAC before calling
updateDeviceColumn.✅ Proposed fix
-from plugin_helper import is_mac # noqa: E402 [flake8 lint suppression] +from plugin_helper import is_mac, normalize_mac # noqa: E402 [flake8 lint suppression] @@ def api_device_field_lock(mac, payload=None): @@ - device_handler = DeviceInstance() + normalized_mac = normalize_mac(mac) + if not normalized_mac: + return jsonify({"success": False, "error": "Invalid MAC address"}), 400 + + device_handler = DeviceInstance() @@ - result = device_handler.updateDeviceColumn(mac, source_field, new_source) + result = device_handler.updateDeviceColumn(normalized_mac, source_field, new_source)As per coding guidelines, please validate MAC addresses with
normalize_mac()before database writes.Also applies to: 463-488
🤖 Fix all issues with AI agents
In `@front/php/templates/language/sv_sv.json`:
- Around line 300-305: The Swedish translation file contains empty values for
the FieldLock_* keys (FieldLock_Error, FieldLock_Lock_Tooltip, FieldLock_Locked,
FieldLock_Source_Label, FieldLock_Unlock_Tooltip, FieldLock_Unlocked) which
results in blank UI because fallback only checks key existence; either provide
proper Swedish translations for those keys (replace empty strings with localized
text) or remove these keys from the sv_sv JSON (or remove sv_sv from the
available languages) so the app can fall back to English—update the keys in the
same JSON object where FieldLock_* are defined and ensure no empty string values
remain.
In `@front/plugins/dig_scan/config.json`:
- Around line 85-87: The JSON settings array is invalid because object entries
are missing commas between them; add a trailing comma after each settings object
where you see entries like { "function": "SET_ALWAYS", "type": ... } and the
neighboring object literals (also at the other occurrences around the sections
referenced near the second and third SET_ALWAYS blocks) so each object in the
array is separated by a comma, ensuring the arrays containing
"function":"SET_ALWAYS" and adjacent objects are properly comma-delimited.
In `@front/plugins/luci_import/config.json`:
- Around line 571-632: The JSON entries for "SET_ALWAYS" and "SET_EMPTY" are
currently placed under database_column_definitions but are settings schema
entries and should be moved into the manifest's settings array; locate the
objects with "function": "SET_ALWAYS" and "function": "SET_EMPTY" and remove
them from database_column_definitions, then re-add them into the settings array
(for example after "RUN_TIMEOUT") so the plugin reads them via
get_setting_value(); ensure their schema (type, default_value, options,
localized/name/description) is preserved exactly when relocating.
In `@front/plugins/pihole_scan/config.json`:
- Around line 219-344: There are duplicate configuration blocks for the
functions "SET_ALWAYS" and "SET_EMPTY" which creates ambiguity; remove the first
pair (the blocks whose default_value is ["devLastIP"] and [] respectively) and
keep a single authoritative "SET_ALWAYS" block with default_value
["devMac","devLastIP"] and a single "SET_EMPTY" block with default_value
["devName","devVendor"], ensuring each retained block's options include
["devMac","devLastIP","devName","devVendor"] and keep their localized
name/description entries intact.
In `@front/plugins/unifi_api_import/config.json`:
- Around line 42-44: The JSON is invalid due to stray closing tokens after the
"params" property; locate the object that contains the "params" key in
front/plugins/unifi_api_import/config.json (the entry showing `"params": []`)
and remove the extra trailing `}` and `]` shown after that property so the
surrounding object/array syntax is correct (ensure no dangling commas remain and
the enclosing object/array are properly closed).
In `@server/api_server/api_server_start.py`:
- Around line 499-501: Replace the illegal mylog("error", ...) call in the
exception handler with one of the allowed levels
(none/minimal/verbose/debug/trace) and pass the message as a list payload per
the logging contract; specifically update the catch block that references
field_name and mac to call e.g. mylog("minimal", [f"Error locking field
{field_name} for {mac}", str(e)]) and then return the same jsonify response —
ensure the first argument is one of the permitted level strings and the second
argument is a list containing the descriptive message and the error detail.
In `@server/models/device_instance.py`:
- Around line 681-705: Both lockDeviceField and unlockDeviceField must normalize
the mac and always close the DB connection; update lockDeviceField and
unlockDeviceField to call normalize_mac(mac) before using it with
lock_field/unlock_field (and still validate field_name against
FIELD_SOURCE_MAP), obtain conn via get_temp_db_connection(), run
lock_field(mac_normalized, field_name, conn) / unlock_field(mac_normalized,
field_name, conn) inside try, and move conn.close() into a finally block so the
connection is closed whether an exception occurs; preserve the same
success/error return shapes.
In `@server/scan/device_handling.py`:
- Around line 544-548: The IPv6 branch currently bypasses validation (cur_IP is
passed through when ":" in cur_IP), so add IPv6 validation consistent with IPv4
handling by reusing or mirroring the logic in format_ip_long from
server/helper.py: when cur_IP contains ":", call the IPv6 validator (e.g.,
format_ip_long or a new helper wrapper) to normalize/validate the address and
only assign primary_ipv6 if the validator returns a valid normalized value; keep
primary_ipv4 assignment unchanged and ensure invalid IPv6 values result in an
empty primary_ipv6 string (or the same error/empty behavior as invalid IPv4 via
check_IP_format).
In `@test/authoritative_fields/test_field_lock_scan_integration.py`:
- Around line 689-702: The test has an IndentationError because the results dict
and following for loop are over-indented; adjust indentation so the variable
results and the for mac, expected in results.items(): loop align with
surrounding test code (same block level as other setup/assert sections) and
ensure the cur.execute(...) call and its fetchone() remain inside the loop body;
update the block around results, for loop, and the assertion that references
cur.execute, devName/devVendor to match the file's surrounding indentation and
remove the extra leading spaces.
🟡 Minor comments (12)
front/php/templates/language/it_it.json-206-209 (1)
206-209: Provide Italian (or fallback) text for Device_Save_ strings.*These are empty, so success/error messages will render blank in the UI. Please add translations or copy the en_us strings as a temporary fallback.
front/php/templates/language/it_it.json-300-305 (1)
300-305: Fill FieldLock_ tooltips/status labels (currently empty).*Empty values will hide lock/unlock tooltips and status labels for Italian users. Please translate or fallback to en_us to avoid blank UI.
front/php/templates/language/pt_br.json-300-305 (1)
300-305: Missing Portuguese translations for field lock feature.These new keys have empty placeholder values for the field locking UI. Consider adding proper translations such as:
FieldLock_Error: "Erro ao bloquear campo"FieldLock_Lock_Tooltip: "Clique para bloquear este campo"FieldLock_Locked: "Bloqueado"FieldLock_Source_Label: "Fonte"FieldLock_Unlock_Tooltip: "Clique para desbloquear este campo"FieldLock_Unlocked: "Desbloqueado"front/php/templates/language/pt_br.json-206-209 (1)
206-209: Missing Portuguese translations for device save messages.These new keys have empty placeholder values. Without translations, Portuguese-speaking users will see blank or fallback text when device save operations succeed or fail. Consider adding proper translations such as:
Device_Save_Failed: "Falha ao salvar dispositivo"Device_Save_Unauthorized: "Não autorizado a salvar dispositivo"Device_Saved_Success: "Dispositivo salvo com sucesso"Device_Saved_Unexpected: "Erro inesperado ao salvar dispositivo"front/php/templates/language/pl_pl.json-300-305 (1)
300-305: Empty Polish translations for field lock feature.The field lock UI elements lack Polish translations. Users will see empty tooltips and status labels for the new field locking functionality.
Suggested translations:
"FieldLock_Error": "Błąd blokady pola", "FieldLock_Lock_Tooltip": "Zablokuj to pole", "FieldLock_Locked": "Zablokowane", "FieldLock_Source_Label": "Źródło", "FieldLock_Unlock_Tooltip": "Odblokuj to pole", "FieldLock_Unlocked": "Odblokowane",front/php/templates/language/pl_pl.json-206-209 (1)
206-209: Empty Polish translations for device save messages.These translation keys have empty placeholder values. Polish users will see empty text or fallback behavior for device save status messages until translations are provided.
Suggested translations:
"Device_Save_Failed": "Zapisanie urządzenia nie powiodło się", "Device_Save_Unauthorized": "Brak autoryzacji do zapisania urządzenia", "Device_Saved_Success": "Urządzenie zapisane pomyślnie", "Device_Saved_Unexpected": "Nieoczekiwany błąd podczas zapisywania urządzenia",front/php/templates/language/sv_sv.json-206-209 (1)
206-209: Add Swedish strings for these keys or confirm that empty values reliably fall back to the default locale.New
Device_Save_*andFieldLock_*keys have empty string values in the Swedish translation file. Without a confirmed fallback mechanism, these will display as blank UI text. Either provide Swedish translations or verify that your i18n system handles empty values by falling back to English.docs/PLUGINS_DEV_CONFIG.md-180-259 (1)
180-259: Docs examples don’t match the config schema used in this PR.The examples show per-field
"SET_ALWAYS": "1"/"SET_EMPTY": "1", but the manifests here defineSET_ALWAYS/SET_EMPTYas settings arrays of column names. This will confuse plugin authors. Consider updating the examples and decision flow to reflect the actual manifest format.docs/API_DEVICE_FIELD_LOCK.md-136-148 (1)
136-148: Fix inconsistent LOCKED color description.
You describe LOCKED as orange above, but the workflow says it turns red. Please align to one color.✏️ Suggested fix
-4. Button changes to unlock (🔓) and source indicator turns red (LOCKED) +4. Button changes to unlock (🔓) and source indicator turns orange (LOCKED)test/authoritative_fields/test_ip_update_logic.py-84-156 (1)
84-156: Use@pytest.mark.usefixturesfor side-effect-only fixtures.The
mock_device_handlingfixture is applied only for its patching side effects and never referenced in the test body. Ruff flags it as ARG001 (unused argument). Follow pytest conventions by moving it to a decorator.🔧 Proposed fix
+@pytest.mark.usefixtures("mock_device_handling") -def test_primary_ipv6_is_set_and_ipv4_preserved(in_memory_db, mock_device_handling): +def test_primary_ipv6_is_set_and_ipv4_preserved(in_memory_db): """Setting IPv6 in CurrentScan should update devPrimaryIPv6 without changing devPrimaryIPv4."""Also applies to:
test_primary_ipv4_is_set_and_ipv6_preservedtest/authoritative_fields/FIELD_LOCK_TEST_SUMMARY.md-279-282 (1)
279-282: Align status count with “Total Tests: 33.”The status line states “24 comprehensive tests,” which conflicts with the earlier total of 33.
📝 Proposed fix
-**Status:** ✅ 24 comprehensive tests created covering all scenarios +**Status:** ✅ 33 comprehensive tests created covering all scenariosfront/deviceDetailsEdit.php-597-605 (1)
597-605: Source indicator not updated when unlocking a field.When
shouldLockis false (unlocking), the source indicator text and styling are not updated. The user will see stale "LOCKED" state until page refresh.🐛 Proposed fix to update source indicator on unlock
// Update source indicator if locked if (shouldLock) { const sourceIndicator = lockBtn.next(); if (sourceIndicator.hasClass("input-group-addon")) { sourceIndicator.text("LOCKED"); sourceIndicator.attr("class", "input-group-addon text-danger"); sourceIndicator.attr("title", getString("FieldLock_Source_Label") + "LOCKED"); } + } else { + // Update source indicator on unlock (source reverts to previous state, typically NEWDEV) + const sourceIndicator = lockBtn.next(); + if (sourceIndicator.hasClass("input-group-addon")) { + sourceIndicator.html('<i class="fa-solid fa-tag"></i> NEWDEV'); + sourceIndicator.attr("class", "input-group-addon text-muted"); + sourceIndicator.attr("title", getString("FieldLock_Source_Label") + "NEWDEV"); + } }Note: The actual source after unlocking may vary (could be NEWDEV or the previous source). Consider fetching the updated source from the API response for accuracy.
🧹 Nitpick comments (19)
front/php/templates/language/nb_no.json (1)
778-778: Consider adding a trailing newline.The file ends without a trailing newline. Adding one improves compatibility with POSIX tools and avoids "no newline at end of file" warnings in diffs.
front/php/templates/language/cs_cz.json (1)
206-209: Empty translations for new Device save status keys.These new keys follow the existing pattern of placeholder translations. Consider adding Czech translations before release to ensure a consistent localized user experience, or verify that empty strings fall back gracefully to a default language.
server/db/db_upgrade.py (2)
86-96: Guard blocks schema upgrades for non-Devices tables due to empty listThe guard at line 88 uses an empty list
[]as the fallback for non-Devices tables. Since[]is falsy in Python, the conditionif not expected or column_name not in expected:will always trigger (returning False) when the function is called with any table other than "Devices". While this is currently not an issue sinceensure_columnis only called with"Devices"(37 call sites inserver/database.py), the overly restrictive logic makes the function unsuitable for evolution of other tables. Consider checkingif expected is not None and column_name not in expected:instead, so the validation only applies when a table has defined expected columns.
263-277: Remove unusedcur_devVlancolumn or align naming with table patternThe
cur_devVlancolumn in the CurrentScan table definition breaks the consistentcur_*naming pattern (e.g.,cur_MAC,cur_IP,cur_SSID). However, this column is never populated in any INSERT statements and never referenced in SELECT queries, making it dead code. Either remove the column or rename it tocur_Vlanto match the naming convention used throughout the table.front/css/app.css (1)
2442-2473: Consider reusing existing color CSS variables for lock indicators.
Keeps palette consistent and reduces duplicate hex values.♻️ Suggested tweak
.input-group-addon.text-warning { - color: `#f39c12`; + color: var(--color-yellow); background-color: rgba(243, 156, 18, 0.1); } .input-group-addon.text-danger { - color: `#dd4b39`; + color: var(--color-red); background-color: rgba(221, 75, 57, 0.1); } .input-group-addon.text-muted { - color: `#8c8c8c`; + color: var(--color-gray); background-color: rgba(140, 140, 140, 0.05); }docs/QUICK_REFERENCE_FIELD_LOCK.md (1)
86-88: Tighten wording to avoid repeated “again”.✏️ Suggested tweak
- ✅ You can lock it again anytime + ✅ You can lock it anytimeserver/api_server/api_server_start.py (1)
463-468: Use the validated payload to avoid bypassing schema checks.
payloadis unused, which bypasses theLockDeviceFieldRequestvalidation and triggers ARG001.♻️ Proposed refactor
- data = request.get_json() or {} + data = payload if payload is not None else (request.get_json() or {}) + if hasattr(data, "model_dump"): + data = data.model_dump(exclude_unset=True) + elif hasattr(data, "dict"): + data = data.dict(exclude_unset=True)test/test_device_field_lock.py (3)
42-58: Consider logging cleanup failures for debugging.The cleanup fixture silently swallows all exceptions. While this is acceptable for test teardown, logging the exception could help diagnose test environment issues.
♻️ Suggested improvement
`@pytest.fixture`(autouse=True) def cleanup_test_device(test_mac): """Clean up test device before and after test.""" device_handler = DeviceInstance() # Clean before test try: device_handler.deleteDeviceByMAC(test_mac) - except Exception: - pass + except Exception as e: + print(f"[cleanup] Pre-test cleanup note: {e}") yield # Clean after test try: device_handler.deleteDeviceByMAC(test_mac) - except Exception: - pass + except Exception as e: + print(f"[cleanup] Post-test cleanup note: {e}")
272-290: Remove unusedauth_headersparameter.The
auth_headersfixture is not used in this test since it interacts directly withDeviceInstancerather than the API client.♻️ Suggested fix
- def test_locked_field_blocks_plugin_overwrite(self, test_mac, auth_headers): + def test_locked_field_blocks_plugin_overwrite(self, test_mac): """Verify locked fields prevent plugin source overwrites.""" device_handler = DeviceInstance()
292-316: Remove unusedauth_headersparameter.Same as above - this integration test uses
DeviceInstancedirectly.♻️ Suggested fix
- def test_field_source_tracking(self, test_mac, auth_headers): + def test_field_source_tracking(self, test_mac): """Verify field source is tracked correctly.""" device_handler = DeviceInstance()front/plugins/snmp_discovery/config.json (1)
589-650: Missing localization for non-English languages.The
SET_ALWAYSandSET_EMPTYsettings only includeen_ustranslations, while other settings in this plugin (e.g.,RUN,CMD,RUN_SCHD) includees_esandde_detranslations. Consider adding translations for consistency.front/plugins/nbtscan_scan/config.json (3)
86-86: JSON formatting inconsistency.Line 86 has inconsistent formatting with extra spaces before
"function". This appears to be a merge artifact. While valid JSON, it breaks the file's formatting consistency.Suggested fix
- { "function": "SET_ALWAYS", + { + "function": "SET_ALWAYS",
145-145: Same formatting issue on CMD block.Suggested fix
- { "function": "CMD", + { + "function": "CMD",
86-144: Missing localization for non-English languages.Similar to snmp_discovery, the
SET_ALWAYSandSET_EMPTYsettings only includeen_ustranslations, while other settings in this plugin includees_esandde_detranslations.server/api_server/openapi/schemas.py (1)
276-280: Consider validatingfieldNameagainst allowed lockable fields.The
fieldNamefield accepts any string, but only certain fields support locking (those inFIELD_SOURCE_MAPinauthoritative_handler.py). Consider adding validation to provide better error messages at the schema level.Suggested improvement
+# At the top of the file, define allowed lockable fields +ALLOWED_LOCKABLE_FIELDS = Literal[ + "devMac", "devName", "devFQDN", "devLastIP", "devVendor", + "devSSID", "devParentMAC", "devParentPort", "devParentRelType", "devVlan" +] + class LockDeviceFieldRequest(BaseModel): """Request to lock/unlock a device field.""" - fieldName: str = Field(..., description="Field name to lock/unlock (devMac, devName, devLastIP, etc.)") + fieldName: ALLOWED_LOCKABLE_FIELDS = Field(..., description="Field name to lock/unlock") lock: bool = Field(True, description="True to lock the field, False to unlock")server/db/authoritative_handler.py (4)
18-19: Remove unusednoqadirectives.Static analysis indicates these
noqa: E402directives are not needed. The E402 rule (module level import not at top of file) is either not enabled or not triggered here.Suggested fix
-from logger import mylog # noqa: E402 [flake8 lint suppression] -from helper import get_setting_value # noqa: E402 [flake8 lint suppression] +from logger import mylog +from helper import get_setting_value
75-117: Unusedplugin_prefixparameter.The
plugin_prefixparameter is declared but never used incan_overwrite_field. Consider either using it for logging/debugging or removing it from the signature. Based on learnings from relevant code snippets, logging could help troubleshoot overwrite decisions.Option 1: Remove unused parameter
-def can_overwrite_field(field_name, current_source, plugin_prefix, plugin_settings, field_value): +def can_overwrite_field(field_name, current_source, plugin_settings, field_value):Option 2: Use for debug logging
# Rule 1: USER and LOCKED are protected if current_source in ("USER", "LOCKED"): + mylog("trace", [f"[can_overwrite_field] {plugin_prefix} blocked from overwriting {field_name}: source is {current_source}"]) return False
120-135: Unusedfield_nameparameter.The
field_nameparameter is declared but never used inget_source_for_field_update. If it's intended for future use or consistency with other function signatures, consider prefixing with underscore to indicate intentional non-use.Suggested fix
-def get_source_for_field_update(field_name, plugin_prefix, is_user_override=False): +def get_source_for_field_update(_field_name, plugin_prefix, is_user_override=False):
156-156: Unused loop variable.The
new_valuevariable is not used in the loop body. Rename to_new_valueto indicate intentional non-use.Suggested fix
- for field_name, new_value in updates_dict.items(): + for field_name, _new_value in updates_dict.items():
| "FieldLock_Error": "", | ||
| "FieldLock_Lock_Tooltip": "", | ||
| "FieldLock_Locked": "", | ||
| "FieldLock_Source_Label": "", | ||
| "FieldLock_Unlock_Tooltip": "", | ||
| "FieldLock_Unlocked": "", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if the file exists and get context around lines 300-305
find . -name "sv_sv.json" -type fRepository: netalertx/NetAlertX
Length of output: 104
🏁 Script executed:
# Also search for language file handling to understand the fallback mechanism
find . -name "*.php" -o -name "*.js" | head -20Repository: netalertx/NetAlertX
Length of output: 568
🏁 Script executed:
# Search for how language files are loaded and handled
rg -t php "sv_sv|language.*fallback|translation.*fallback" --max-count=20Repository: netalertx/NetAlertX
Length of output: 505
🏁 Script executed:
# Read sv_sv.json around lines 300-305 to verify the content
sed -n '298,307p' ./front/php/templates/language/sv_sv.jsonRepository: netalertx/NetAlertX
Length of output: 365
🏁 Script executed:
# Find and check en_us.json for the same FieldLock_* keys
find . -name "en_us.json" -type fRepository: netalertx/NetAlertX
Length of output: 104
🏁 Script executed:
# Read the fallback implementation in lang.php
rg -A 10 "If key not found in selected language" ./front/php/templates/language/lang.phpRepository: netalertx/NetAlertX
Length of output: 509
🏁 Script executed:
# Extract FieldLock_* entries from en_us.json
rg 'FieldLock_' ./front/php/templates/language/en_us.json -A 0Repository: netalertx/NetAlertX
Length of output: 393
🏁 Script executed:
# Get the actual values for FieldLock_* keys in en_us.json
rg '"FieldLock_' ./front/php/templates/language/en_us.jsonRepository: netalertx/NetAlertX
Length of output: 393
🏁 Script executed:
# Check how many untranslated (empty string) keys exist in sv_sv.json
rg '": "",' ./front/php/templates/language/sv_sv.json | wc -lRepository: netalertx/NetAlertX
Length of output: 66
🏁 Script executed:
# Compare to en_us.json to assess translation completeness
rg '": "",' ./front/php/templates/language/en_us.json | wc -lRepository: netalertx/NetAlertX
Length of output: 64
🏁 Script executed:
# Check file sizes to understand translation coverage
wc -l ./front/php/templates/language/sv_sv.json ./front/php/templates/language/en_us.jsonRepository: netalertx/NetAlertX
Length of output: 170
Complete Swedish translations or remove untranslated keys.
The FieldLock_* UI labels are empty strings in the Swedish translation file. Because the fallback logic checks for key existence (not value content), empty strings will render as blank UI instead of falling back to English. Note that the Swedish translation file is incomplete overall—775 out of 777 keys are empty strings, suggesting this file needs either full translation or to be removed from the language list until translations are ready.
🤖 Prompt for AI Agents
In `@front/php/templates/language/sv_sv.json` around lines 300 - 305, The Swedish
translation file contains empty values for the FieldLock_* keys
(FieldLock_Error, FieldLock_Lock_Tooltip, FieldLock_Locked,
FieldLock_Source_Label, FieldLock_Unlock_Tooltip, FieldLock_Unlocked) which
results in blank UI because fallback only checks key existence; either provide
proper Swedish translations for those keys (replace empty strings with localized
text) or remove these keys from the sv_sv JSON (or remove sv_sv from the
available languages) so the app can fall back to English—update the keys in the
same JSON object where FieldLock_* are defined and ensure no empty string values
remain.
| }, | ||
| { "function": "SET_ALWAYS", | ||
| "type": { |
There was a problem hiding this comment.
Fix missing commas between settings objects (invalid JSON).
The settings array lacks commas between consecutive objects, which makes the JSON invalid and will prevent the plugin config from loading.
🛠️ Proposed fix
- }
+ },
@@
- }
+ },
@@
- }
+ },Also applies to: 115-117, 146-148
🤖 Prompt for AI Agents
In `@front/plugins/dig_scan/config.json` around lines 85 - 87, The JSON settings
array is invalid because object entries are missing commas between them; add a
trailing comma after each settings object where you see entries like {
"function": "SET_ALWAYS", "type": ... } and the neighboring object literals
(also at the other occurrences around the sections referenced near the second
and third SET_ALWAYS blocks) so each object in the array is separated by a
comma, ensuring the arrays containing "function":"SET_ALWAYS" and adjacent
objects are properly comma-delimited.
| }, | ||
| { | ||
| "function": "SET_ALWAYS", | ||
| "type": { | ||
| "dataType": "array", | ||
| "elements": [ | ||
| { | ||
| "elementType": "select", | ||
| "elementOptions": [{ "multiple": "true", "orderable": "true" }], | ||
| "transformers": [] | ||
| } | ||
| ] | ||
| }, | ||
| "default_value": ["devMac", "devLastIP"], | ||
| "options": [ | ||
| "devMac", | ||
| "devLastIP" | ||
| ], | ||
| "localized": ["name", "description"], | ||
| "name": [ | ||
| { | ||
| "language_code": "en_us", | ||
| "string": "Set always columns" | ||
| } | ||
| ], | ||
| "description": [ | ||
| { | ||
| "language_code": "en_us", | ||
| "string": "These columns are always overwritten by this plugin, unless the user locks or overwrites the value." | ||
| } | ||
| ] | ||
| }, | ||
| { | ||
| "function": "SET_EMPTY", | ||
| "type": { | ||
| "dataType": "array", | ||
| "elements": [ | ||
| { | ||
| "elementType": "select", | ||
| "elementOptions": [{ "multiple": "true", "orderable": "true" }], | ||
| "transformers": [] | ||
| } | ||
| ] | ||
| }, | ||
| "default_value": [], | ||
| "options": [ | ||
| "devMac", | ||
| "devLastIP" | ||
| ], | ||
| "localized": ["name", "description"], | ||
| "name": [ | ||
| { | ||
| "language_code": "en_us", | ||
| "string": "Set empty columns" | ||
| } | ||
| ], | ||
| "description": [ | ||
| { | ||
| "language_code": "en_us", | ||
| "string": "These columns are only overwritten if they are empty or set to NEWDEV." | ||
| } | ||
| ] |
There was a problem hiding this comment.
SET_ALWAYS/SET_EMPTY are in the wrong section.
These blocks use the settings schema (function, type, default_value) but are placed inside database_column_definitions, which expects column entries. They will likely be ignored or cause validation errors. Move them into the settings array with the other RUN/CMD entries. As per coding guidelines, plugin settings should live in the manifest’s settings section and be read via get_setting_value().
🛠️ Proposed fix (remove from database_column_definitions)
- {
- "function": "SET_ALWAYS",
- "type": {
- "dataType": "array",
- "elements": [
- {
- "elementType": "select",
- "elementOptions": [{ "multiple": "true", "orderable": "true" }],
- "transformers": []
- }
- ]
- },
- "default_value": ["devMac", "devLastIP"],
- "options": [
- "devMac",
- "devLastIP"
- ],
- "localized": ["name", "description"],
- "name": [
- {
- "language_code": "en_us",
- "string": "Set always columns"
- }
- ],
- "description": [
- {
- "language_code": "en_us",
- "string": "These columns are always overwritten by this plugin, unless the user locks or overwrites the value."
- }
- ]
- },
- {
- "function": "SET_EMPTY",
- "type": {
- "dataType": "array",
- "elements": [
- {
- "elementType": "select",
- "elementOptions": [{ "multiple": "true", "orderable": "true" }],
- "transformers": []
- }
- ]
- },
- "default_value": [],
- "options": [
- "devMac",
- "devLastIP"
- ],
- "localized": ["name", "description"],
- "name": [
- {
- "language_code": "en_us",
- "string": "Set empty columns"
- }
- ],
- "description": [
- {
- "language_code": "en_us",
- "string": "These columns are only overwritten if they are empty or set to NEWDEV."
- }
- ]
- }You can re-add these blocks under settings (e.g., after RUN_TIMEOUT).
🤖 Prompt for AI Agents
In `@front/plugins/luci_import/config.json` around lines 571 - 632, The JSON
entries for "SET_ALWAYS" and "SET_EMPTY" are currently placed under
database_column_definitions but are settings schema entries and should be moved
into the manifest's settings array; locate the objects with "function":
"SET_ALWAYS" and "function": "SET_EMPTY" and remove them from
database_column_definitions, then re-add them into the settings array (for
example after "RUN_TIMEOUT") so the plugin reads them via get_setting_value();
ensure their schema (type, default_value, options, localized/name/description)
is preserved exactly when relocating.
| { | ||
| "function": "SET_ALWAYS", | ||
| "type": { | ||
| "dataType": "array", | ||
| "elements": [ | ||
| { | ||
| "elementType": "select", | ||
| "elementOptions": [{ "multiple": "true"}], | ||
| "transformers": [] | ||
| } | ||
| ] | ||
| }, | ||
| "default_value": ["devLastIP"], | ||
| "options": [ | ||
| "devLastIP" | ||
| ], | ||
| "localized": ["name", "description"], | ||
| "name": [ | ||
| { | ||
| "language_code": "en_us", | ||
| "string": "Set always columns" | ||
| } | ||
| ], | ||
| "description": [ | ||
| { | ||
| "language_code": "en_us", | ||
| "string": "These columns are always overwritten by this plugin, unless the user locks or overwrites the value." | ||
| } | ||
| ] | ||
| }, | ||
| { | ||
| "function": "SET_EMPTY", | ||
| "type": { | ||
| "dataType": "array", | ||
| "elements": [ | ||
| { | ||
| "elementType": "select", | ||
| "elementOptions": [{ "multiple": "true"}], | ||
| "transformers": [] | ||
| } | ||
| ] | ||
| }, | ||
| "default_value": [], | ||
| "options": [ | ||
| "devLastIP" | ||
| ], | ||
| "localized": ["name", "description"], | ||
| "name": [ | ||
| { | ||
| "language_code": "en_us", | ||
| "string": "Set empty columns" | ||
| } | ||
| ], | ||
| "description": [ | ||
| { | ||
| "language_code": "en_us", | ||
| "string": "These columns are only overwritten if they are empty or set to NEWDEV." | ||
| } | ||
| ] | ||
| }, | ||
| { | ||
| "function": "SET_ALWAYS", | ||
| "type": { | ||
| "dataType": "array", | ||
| "elements": [ | ||
| { | ||
| "elementType": "select", | ||
| "elementOptions": [{ "multiple": "true"}], | ||
| "transformers": [] | ||
| } | ||
| ] | ||
| }, | ||
| "default_value": ["devMac", "devLastIP"], | ||
| "options": [ | ||
| "devMac", | ||
| "devLastIP", | ||
| "devName", | ||
| "devVendor" | ||
| ], | ||
| "localized": ["name", "description"], | ||
| "name": [ | ||
| { | ||
| "language_code": "en_us", | ||
| "string": "Set always columns" | ||
| } | ||
| ], | ||
| "description": [ | ||
| { | ||
| "language_code": "en_us", | ||
| "string": "These columns are always overwritten by this plugin, unless the user locks or overwrites the value." | ||
| } | ||
| ] | ||
| }, | ||
| { | ||
| "function": "SET_EMPTY", | ||
| "type": { | ||
| "dataType": "array", | ||
| "elements": [ | ||
| { | ||
| "elementType": "select", | ||
| "elementOptions": [{ "multiple": "true"}], | ||
| "transformers": [] | ||
| } | ||
| ] | ||
| }, | ||
| "default_value": ["devName", "devVendor"], | ||
| "options": [ | ||
| "devMac", | ||
| "devLastIP", | ||
| "devName", | ||
| "devVendor" | ||
| ], | ||
| "localized": ["name", "description"], | ||
| "name": [ | ||
| { | ||
| "language_code": "en_us", | ||
| "string": "Set empty columns" | ||
| } | ||
| ], | ||
| "description": [ | ||
| { | ||
| "language_code": "en_us", | ||
| "string": "These columns are only overwritten if they are empty or set to NEWDEV." | ||
| } | ||
| ] | ||
| }, |
There was a problem hiding this comment.
Duplicate SET_ALWAYS/SET_EMPTY settings cause ambiguous behavior
You define SET_ALWAYS and SET_EMPTY twice with different defaults/options. This will likely render duplicate UI entries or create non-deterministic precedence when settings are read. Please keep only one authoritative block and consolidate the intended defaults/options.
🛠️ Suggested fix (remove the earlier duplicate pair)
- {
- "function": "SET_ALWAYS",
- "type": {
- "dataType": "array",
- "elements": [
- {
- "elementType": "select",
- "elementOptions": [{ "multiple": "true"}],
- "transformers": []
- }
- ]
- },
- "default_value": ["devLastIP"],
- "options": [
- "devLastIP"
- ],
- "localized": ["name", "description"],
- "name": [
- {
- "language_code": "en_us",
- "string": "Set always columns"
- }
- ],
- "description": [
- {
- "language_code": "en_us",
- "string": "These columns are always overwritten by this plugin, unless the user locks or overwrites the value."
- }
- ]
- },
- {
- "function": "SET_EMPTY",
- "type": {
- "dataType": "array",
- "elements": [
- {
- "elementType": "select",
- "elementOptions": [{ "multiple": "true"}],
- "transformers": []
- }
- ]
- },
- "default_value": [],
- "options": [
- "devLastIP"
- ],
- "localized": ["name", "description"],
- "name": [
- {
- "language_code": "en_us",
- "string": "Set empty columns"
- }
- ],
- "description": [
- {
- "language_code": "en_us",
- "string": "These columns are only overwritten if they are empty or set to NEWDEV."
- }
- ]
- },📝 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.
| { | |
| "function": "SET_ALWAYS", | |
| "type": { | |
| "dataType": "array", | |
| "elements": [ | |
| { | |
| "elementType": "select", | |
| "elementOptions": [{ "multiple": "true"}], | |
| "transformers": [] | |
| } | |
| ] | |
| }, | |
| "default_value": ["devLastIP"], | |
| "options": [ | |
| "devLastIP" | |
| ], | |
| "localized": ["name", "description"], | |
| "name": [ | |
| { | |
| "language_code": "en_us", | |
| "string": "Set always columns" | |
| } | |
| ], | |
| "description": [ | |
| { | |
| "language_code": "en_us", | |
| "string": "These columns are always overwritten by this plugin, unless the user locks or overwrites the value." | |
| } | |
| ] | |
| }, | |
| { | |
| "function": "SET_EMPTY", | |
| "type": { | |
| "dataType": "array", | |
| "elements": [ | |
| { | |
| "elementType": "select", | |
| "elementOptions": [{ "multiple": "true"}], | |
| "transformers": [] | |
| } | |
| ] | |
| }, | |
| "default_value": [], | |
| "options": [ | |
| "devLastIP" | |
| ], | |
| "localized": ["name", "description"], | |
| "name": [ | |
| { | |
| "language_code": "en_us", | |
| "string": "Set empty columns" | |
| } | |
| ], | |
| "description": [ | |
| { | |
| "language_code": "en_us", | |
| "string": "These columns are only overwritten if they are empty or set to NEWDEV." | |
| } | |
| ] | |
| }, | |
| { | |
| "function": "SET_ALWAYS", | |
| "type": { | |
| "dataType": "array", | |
| "elements": [ | |
| { | |
| "elementType": "select", | |
| "elementOptions": [{ "multiple": "true"}], | |
| "transformers": [] | |
| } | |
| ] | |
| }, | |
| "default_value": ["devMac", "devLastIP"], | |
| "options": [ | |
| "devMac", | |
| "devLastIP", | |
| "devName", | |
| "devVendor" | |
| ], | |
| "localized": ["name", "description"], | |
| "name": [ | |
| { | |
| "language_code": "en_us", | |
| "string": "Set always columns" | |
| } | |
| ], | |
| "description": [ | |
| { | |
| "language_code": "en_us", | |
| "string": "These columns are always overwritten by this plugin, unless the user locks or overwrites the value." | |
| } | |
| ] | |
| }, | |
| { | |
| "function": "SET_EMPTY", | |
| "type": { | |
| "dataType": "array", | |
| "elements": [ | |
| { | |
| "elementType": "select", | |
| "elementOptions": [{ "multiple": "true"}], | |
| "transformers": [] | |
| } | |
| ] | |
| }, | |
| "default_value": ["devName", "devVendor"], | |
| "options": [ | |
| "devMac", | |
| "devLastIP", | |
| "devName", | |
| "devVendor" | |
| ], | |
| "localized": ["name", "description"], | |
| "name": [ | |
| { | |
| "language_code": "en_us", | |
| "string": "Set empty columns" | |
| } | |
| ], | |
| "description": [ | |
| { | |
| "language_code": "en_us", | |
| "string": "These columns are only overwritten if they are empty or set to NEWDEV." | |
| } | |
| ] | |
| }, | |
| { | |
| "function": "SET_ALWAYS", | |
| "type": { | |
| "dataType": "array", | |
| "elements": [ | |
| { | |
| "elementType": "select", | |
| "elementOptions": [{ "multiple": "true"}], | |
| "transformers": [] | |
| } | |
| ] | |
| }, | |
| "default_value": ["devMac", "devLastIP"], | |
| "options": [ | |
| "devMac", | |
| "devLastIP", | |
| "devName", | |
| "devVendor" | |
| ], | |
| "localized": ["name", "description"], | |
| "name": [ | |
| { | |
| "language_code": "en_us", | |
| "string": "Set always columns" | |
| } | |
| ], | |
| "description": [ | |
| { | |
| "language_code": "en_us", | |
| "string": "These columns are always overwritten by this plugin, unless the user locks or overwrites the value." | |
| } | |
| ] | |
| }, | |
| { | |
| "function": "SET_EMPTY", | |
| "type": { | |
| "dataType": "array", | |
| "elements": [ | |
| { | |
| "elementType": "select", | |
| "elementOptions": [{ "multiple": "true"}], | |
| "transformers": [] | |
| } | |
| ] | |
| }, | |
| "default_value": ["devName", "devVendor"], | |
| "options": [ | |
| "devMac", | |
| "devLastIP", | |
| "devName", | |
| "devVendor" | |
| ], | |
| "localized": ["name", "description"], | |
| "name": [ | |
| { | |
| "language_code": "en_us", | |
| "string": "Set empty columns" | |
| } | |
| ], | |
| "description": [ | |
| { | |
| "language_code": "en_us", | |
| "string": "These columns are only overwritten if they are empty or set to NEWDEV." | |
| } | |
| ] | |
| }, |
🤖 Prompt for AI Agents
In `@front/plugins/pihole_scan/config.json` around lines 219 - 344, There are
duplicate configuration blocks for the functions "SET_ALWAYS" and "SET_EMPTY"
which creates ambiguity; remove the first pair (the blocks whose default_value
is ["devLastIP"] and [] respectively) and keep a single authoritative
"SET_ALWAYS" block with default_value ["devMac","devLastIP"] and a single
"SET_EMPTY" block with default_value ["devName","devVendor"], ensuring each
retained block's options include ["devMac","devLastIP","devName","devVendor"]
and keep their localized name/description entries intact.
| "params": [], | ||
| } | ||
| ], |
There was a problem hiding this comment.
Fix invalid JSON after params.
The stray } and ] make this file unparsable.
🛠️ Proposed fix
- "params": [],
- }
- ],
+ "params": [],🧰 Tools
🪛 Biome (2.1.2)
[error] 43-43: Expected a property but instead found '}'.
Expected a property here.
(parse)
[error] 44-44: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
🤖 Prompt for AI Agents
In `@front/plugins/unifi_api_import/config.json` around lines 42 - 44, The JSON is
invalid due to stray closing tokens after the "params" property; locate the
object that contains the "params" key in
front/plugins/unifi_api_import/config.json (the entry showing `"params": []`)
and remove the extra trailing `}` and `]` shown after that property so the
surrounding object/array syntax is correct (ensure no dangling commas remain and
the enclosing object/array are properly closed).
| except Exception as e: | ||
| mylog("error", f"Error locking field {field_name} for {mac}: {str(e)}") | ||
| return jsonify({"success": False, "error": str(e)}), 500 |
There was a problem hiding this comment.
Use allowed log levels in mylog.
mylog("error", ...) violates the permitted levels and also skips the expected list payload.
🔧 Proposed fix
- except Exception as e:
- mylog("error", f"Error locking field {field_name} for {mac}: {str(e)}")
+ except Exception as e:
+ mylog("verbose", [f"Error locking field {field_name} for {mac}: {e!s}"])
return jsonify({"success": False, "error": str(e)}), 500As per coding guidelines, use only none/minimal/verbose/debug/trace with mylog().
📝 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.
| except Exception as e: | |
| mylog("error", f"Error locking field {field_name} for {mac}: {str(e)}") | |
| return jsonify({"success": False, "error": str(e)}), 500 | |
| except Exception as e: | |
| mylog("verbose", [f"Error locking field {field_name} for {mac}: {e!s}"]) | |
| return jsonify({"success": False, "error": str(e)}), 500 |
🧰 Tools
🪛 Ruff (0.14.13)
499-499: Do not catch blind exception: Exception
(BLE001)
500-500: Use explicit conversion flag
Replace with conversion flag
(RUF010)
🤖 Prompt for AI Agents
In `@server/api_server/api_server_start.py` around lines 499 - 501, Replace the
illegal mylog("error", ...) call in the exception handler with one of the
allowed levels (none/minimal/verbose/debug/trace) and pass the message as a list
payload per the logging contract; specifically update the catch block that
references field_name and mac to call e.g. mylog("minimal", [f"Error locking
field {field_name} for {mac}", str(e)]) and then return the same jsonify
response — ensure the first argument is one of the permitted level strings and
the second argument is a list containing the descriptive message and the error
detail.
| def lockDeviceField(self, mac, field_name): | ||
| """Lock a device field so it won't be overwritten by plugins.""" | ||
| if field_name not in FIELD_SOURCE_MAP: | ||
| return {"success": False, "error": f"Field {field_name} does not support locking"} | ||
|
|
||
| try: | ||
| conn = get_temp_db_connection() | ||
| lock_field(mac, field_name, conn) | ||
| conn.close() | ||
| return {"success": True, "message": f"Field {field_name} locked"} | ||
| except Exception as e: | ||
| return {"success": False, "error": str(e)} | ||
|
|
||
| def unlockDeviceField(self, mac, field_name): | ||
| """Unlock a device field so plugins can overwrite it again.""" | ||
| if field_name not in FIELD_SOURCE_MAP: | ||
| return {"success": False, "error": f"Field {field_name} does not support unlocking"} | ||
|
|
||
| try: | ||
| conn = get_temp_db_connection() | ||
| unlock_field(mac, field_name, conn) | ||
| conn.close() | ||
| return {"success": True, "message": f"Field {field_name} unlocked"} | ||
| except Exception as e: | ||
| return {"success": False, "error": str(e)} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, verify that normalize_mac() exists in the expected location
find . -name "plugin_helper.py" -type f | head -20Repository: netalertx/NetAlertX
Length of output: 95
🏁 Script executed:
# Search for normalize_mac function definition
rg -n "def normalize_mac" --type pyRepository: netalertx/NetAlertX
Length of output: 218
🏁 Script executed:
# Check the current implementation of lockDeviceField and unlockDeviceField
cat -n server/models/device_instance.py | sed -n '681,705p'Repository: netalertx/NetAlertX
Length of output: 1356
🏁 Script executed:
# Find all callers of lockDeviceField and unlockDeviceField
rg -n "lockDeviceField\(|unlockDeviceField\(" -C 2 --type pyRepository: netalertx/NetAlertX
Length of output: 850
🏁 Script executed:
# Search for lock_field and unlock_field function implementations
rg -n "def lock_field|def unlock_field" --type py -A 5Repository: netalertx/NetAlertX
Length of output: 861
🏁 Script executed:
# Check imports in device_instance.py
head -50 server/models/device_instance.py | grep -n "import\|from"Repository: netalertx/NetAlertX
Length of output: 699
🏁 Script executed:
# Check full lock_field and unlock_field implementations
rg -n "def lock_field|def unlock_field" --type py -A 20Repository: netalertx/NetAlertX
Length of output: 2903
🏁 Script executed:
# Search for all callers of these methods to see if they normalize
rg -n "\.lockDeviceField\(|\.unlockDeviceField\(" --type py -B 2 -A 2Repository: netalertx/NetAlertX
Length of output: 45
🏁 Script executed:
# Check if normalize_mac is already imported or used elsewhere in device_instance.py
grep -n "normalize_mac" server/models/device_instance.pyRepository: netalertx/NetAlertX
Length of output: 256
Normalize MACs and always close DB connections on lock/unlock.
Both lockDeviceField and unlockDeviceField have two issues:
-
Missing MAC normalization: The
macparameter is passed directly tolock_field()andunlock_field()without normalization, causing database queries to miss rows when callers supply mixed-case MACs.normalize_mac()is already imported in this file and used elsewhere (lines 504-505) for the same purpose. -
Connection leak on exception: If an exception occurs in the
tryblock,conn.close()is never called because there's nofinallyblock, leaking database connections.
Apply the proposed fix to normalize the MAC before the database call and ensure the connection is closed regardless of exceptions:
🔧 Proposed fix
def lockDeviceField(self, mac, field_name):
"""Lock a device field so it won't be overwritten by plugins."""
if field_name not in FIELD_SOURCE_MAP:
return {"success": False, "error": f"Field {field_name} does not support locking"}
- try:
- conn = get_temp_db_connection()
- lock_field(mac, field_name, conn)
- conn.close()
- return {"success": True, "message": f"Field {field_name} locked"}
- except Exception as e:
- return {"success": False, "error": str(e)}
+ normalized_mac = normalize_mac(mac)
+ conn = None
+ try:
+ conn = get_temp_db_connection()
+ lock_field(normalized_mac, field_name, conn)
+ return {"success": True, "message": f"Field {field_name} locked"}
+ except Exception as e:
+ return {"success": False, "error": str(e)}
+ finally:
+ if conn:
+ conn.close()
def unlockDeviceField(self, mac, field_name):
"""Unlock a device field so plugins can overwrite it again."""
if field_name not in FIELD_SOURCE_MAP:
return {"success": False, "error": f"Field {field_name} does not support unlocking"}
- try:
- conn = get_temp_db_connection()
- unlock_field(mac, field_name, conn)
- conn.close()
- return {"success": True, "message": f"Field {field_name} unlocked"}
- except Exception as e:
- return {"success": False, "error": str(e)}
+ normalized_mac = normalize_mac(mac)
+ conn = None
+ try:
+ conn = get_temp_db_connection()
+ unlock_field(normalized_mac, field_name, conn)
+ return {"success": True, "message": f"Field {field_name} unlocked"}
+ except Exception as e:
+ return {"success": False, "error": str(e)}
+ finally:
+ if conn:
+ conn.close()🧰 Tools
🪛 Ruff (0.14.13)
690-690: Consider moving this statement to an else block
(TRY300)
691-691: Do not catch blind exception: Exception
(BLE001)
703-703: Consider moving this statement to an else block
(TRY300)
704-704: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
In `@server/models/device_instance.py` around lines 681 - 705, Both
lockDeviceField and unlockDeviceField must normalize the mac and always close
the DB connection; update lockDeviceField and unlockDeviceField to call
normalize_mac(mac) before using it with lock_field/unlock_field (and still
validate field_name against FIELD_SOURCE_MAP), obtain conn via
get_temp_db_connection(), run lock_field(mac_normalized, field_name, conn) /
unlock_field(mac_normalized, field_name, conn) inside try, and move conn.close()
into a finally block so the connection is closed whether an exception occurs;
preserve the same success/error return shapes.
| # Derive primary IP family values | ||
| cur_IP = str(cur_IP).strip() if cur_IP else "" | ||
| cur_IP_normalized = check_IP_format(cur_IP) if ":" not in cur_IP else cur_IP | ||
| primary_ipv4 = cur_IP_normalized if cur_IP_normalized and ":" not in cur_IP_normalized else "" | ||
| primary_ipv6 = cur_IP_normalized if cur_IP_normalized and ":" in cur_IP_normalized else "" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's see the actual code at lines 544-548 in device_handling.py
sed -n '540,555p' server/scan/device_handling.pyRepository: netalertx/NetAlertX
Length of output: 825
🏁 Script executed:
# Locate the check_IP_format function to understand what it validates
rg -n "def check_IP_format" --type=py -A 20Repository: netalertx/NetAlertX
Length of output: 1123
🏁 Script executed:
# Search for any IPv6 validation or ipaddress module usage
rg -n "ipv6|IPv6|ipaddress" --type=py server/Repository: netalertx/NetAlertX
Length of output: 2496
🏁 Script executed:
# Check what validation functions exist in helper files
rg -n "def.*[Ii][Pp]" --type=py server/ | head -20Repository: netalertx/NetAlertX
Length of output: 1739
🏁 Script executed:
# Let's look at the format_ip_long function which appears to use ipaddress validation
sed -n '675,690p' server/helper.pyRepository: netalertx/NetAlertX
Length of output: 550
🏁 Script executed:
# Check if format_ip_long is called in device_handling.py
rg -n "format_ip_long" server/scan/device_handling.pyRepository: netalertx/NetAlertX
Length of output: 45
🏁 Script executed:
# Let's see the complete context around the IPv6 validation in helper.py
sed -n '675,695p' server/helper.pyRepository: netalertx/NetAlertX
Length of output: 741
🏁 Script executed:
# Check what happens to primary_ipv6 after it's set - is it validated before database insert?
sed -n '544,580p' server/scan/device_handling.pyRepository: netalertx/NetAlertX
Length of output: 1813
Add IPv6 format validation before database insert.
The code at lines 544-548 validates IPv4 addresses using check_IP_format() but passes IPv6 addresses through without validation. The format_ip_long() function in server/helper.py already provides IPv6 validation using the ipaddress module; consider applying similar validation to IPv6 addresses before inserting into the database to ensure data integrity and consistency with IPv4 handling.
🤖 Prompt for AI Agents
In `@server/scan/device_handling.py` around lines 544 - 548, The IPv6 branch
currently bypasses validation (cur_IP is passed through when ":" in cur_IP), so
add IPv6 validation consistent with IPv4 handling by reusing or mirroring the
logic in format_ip_long from server/helper.py: when cur_IP contains ":", call
the IPv6 validator (e.g., format_ip_long or a new helper wrapper) to
normalize/validate the address and only assign primary_ipv6 if the validator
returns a valid normalized value; keep primary_ipv4 assignment unchanged and
ensure invalid IPv6 values result in an empty primary_ipv6 string (or the same
error/empty behavior as invalid IPv4 via check_IP_format).
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@test/authoritative_fields/test_device_field_lock.py`:
- Around line 272-290: The test test_locked_field_blocks_plugin_overwrite only
sets devNameSource to "LOCKED" and re-reads it but never attempts a
plugin/authoritative overwrite; update the test to simulate an overwrite (call
the authoritative update path or mimic a plugin write) after
device_handler.updateDeviceColumn(test_mac, "devNameSource", "LOCKED")—for
example use the authoritative handler or a DeviceInstance method that applies a
source="PLUGIN" update (or call device_handler.setDeviceData with a plugin-like
payload) and then assert device_handler.getDeviceData(test_mac)["devNameSource"]
remains "LOCKED"; also remove the unused auth_headers parameter from the test
signature if you don’t need it or use it in the overwrite call.
- Around line 9-10: Remove the hardcoded '/app' default for NETALERTX_APP and
make the code fail fast when the environment variable is missing: read
os.getenv('NETALERTX_APP') into INSTALL_PATH, check if INSTALL_PATH is falsy and
raise a clear exception (or call sys.exit with an error) rather than falling
back to '/app', then use INSTALL_PATH in the existing sys.path.extend([...])
call so imports cannot be silently misconfigured.
- Around line 178-204: The test_lock_prevents_field_updates currently only
asserts response codes; after calling the update endpoint with update_payload,
add a follow-up GET (e.g., call the same client to GET /device/{test_mac} or use
existing helper test_create_test_device/get_device helper) to retrieve the
device record and assert that devName remains the original value created by
test_create_test_device; reference the test function name
test_lock_prevents_field_updates, the lock endpoint used with lock_payload, and
the update POST using update_payload so you place the assertion right after the
update response check.
- Around line 42-58: The fixture cleanup_test_device currently swallows all
exceptions during pre/post cleanup which hides failures; update it to stop
catching exceptions silently—either let exceptions from
DeviceInstance.deleteDeviceByMAC propagate or explicitly fail the test using
pytest.fail with the caught exception message; modify the try/except blocks
around deleteDeviceByMAC in cleanup_test_device to remove the bare excepts (or
replace them with except Exception as e: pytest.fail(f"...{e}")) so cleanup
failures surface, and keep references to DeviceInstance and deleteDeviceByMAC to
locate the calls.
In `@test/authoritative_fields/test_field_lock_scan_integration.py`:
- Around line 1-12: The test module test_field_lock_scan_integration.py is too
large; split it into smaller modules by scenario groups (e.g. name_vendor
updates, ip_updates, presence_mixed_cases) so each file stays under 500 LOC;
move related test functions and any helper fixtures/classes into the new files
(e.g. test_field_lock_scan_integration_name_vendor.py,
test_field_lock_scan_integration_ip.py,
test_field_lock_scan_integration_presence.py), update any relative imports or
conftest references to keep fixtures (e.g. fixtures used by tests in this file)
available, and ensure pytest discovery still finds tests by keeping top-level
test_*.py filenames and preserving the original test function and class names
(so functions referenced by device_handling scan update integration tests remain
unchanged).
🧹 Nitpick comments (3)
test/authoritative_fields/test_device_field_lock.py (2)
12-14: Drop unused# noqa: E402directives.Ruff reports these as unused; removing them keeps lint clean.
♻️ Proposed cleanup
-from helper import get_setting_value # noqa: E402 -from api_server.api_server_start import app # noqa: E402 -from models.device_instance import DeviceInstance # noqa: E402 +from helper import get_setting_value +from api_server.api_server_start import app +from models.device_instance import DeviceInstance
292-316: Remove unusedauth_headersparameter.Ruff flags the argument as unused; dropping it (or prefixing with
_) will keep lint clean.test/authoritative_fields/test_field_lock_scan_integration.py (1)
110-183: Avoid unused fixture parameters; use@pytest.mark.usefixtures.The fixture is used for side effects only, so passing it as an unused argument triggers ARG001. This applies to all tests in this module.
♻️ Proposed pattern
+@pytest.mark.usefixtures("mock_device_handlers") -def test_scan_updates_newdev_device_name(scan_db, mock_device_handlers): +def test_scan_updates_newdev_device_name(scan_db):
| INSTALL_PATH = os.getenv('NETALERTX_APP', '/app') | ||
| sys.path.extend([f"{INSTALL_PATH}/front/plugins", f"{INSTALL_PATH}/server"]) |
There was a problem hiding this comment.
Avoid hardcoded default for NETALERTX_APP.
The /app fallback can silently misconfigure imports when the env var isn’t set. Prefer requiring the environment variable explicitly. As per coding guidelines, avoid hardcoded paths.
🛠️ Proposed fix
-INSTALL_PATH = os.getenv('NETALERTX_APP', '/app')
+INSTALL_PATH = os.getenv("NETALERTX_APP")
+if not INSTALL_PATH:
+ raise RuntimeError("NETALERTX_APP must be set for tests")🤖 Prompt for AI Agents
In `@test/authoritative_fields/test_device_field_lock.py` around lines 9 - 10,
Remove the hardcoded '/app' default for NETALERTX_APP and make the code fail
fast when the environment variable is missing: read os.getenv('NETALERTX_APP')
into INSTALL_PATH, check if INSTALL_PATH is falsy and raise a clear exception
(or call sys.exit with an error) rather than falling back to '/app', then use
INSTALL_PATH in the existing sys.path.extend([...]) call so imports cannot be
silently misconfigured.
| @pytest.fixture(autouse=True) | ||
| def cleanup_test_device(test_mac): | ||
| """Clean up test device before and after test.""" | ||
| device_handler = DeviceInstance() | ||
| # Clean before test | ||
| try: | ||
| device_handler.deleteDeviceByMAC(test_mac) | ||
| except Exception: | ||
| pass | ||
|
|
||
| yield | ||
|
|
||
| # Clean after test | ||
| try: | ||
| device_handler.deleteDeviceByMAC(test_mac) | ||
| except Exception: | ||
| pass |
There was a problem hiding this comment.
Don’t silently ignore cleanup failures.
Swallowing exceptions can hide DB/setup issues and make tests flaky. Consider letting failures surface.
🛠️ Proposed fix
- try:
- device_handler.deleteDeviceByMAC(test_mac)
- except Exception:
- pass
+ device_handler.deleteDeviceByMAC(test_mac)
@@
- try:
- device_handler.deleteDeviceByMAC(test_mac)
- except Exception:
- pass
+ device_handler.deleteDeviceByMAC(test_mac)📝 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.
| @pytest.fixture(autouse=True) | |
| def cleanup_test_device(test_mac): | |
| """Clean up test device before and after test.""" | |
| device_handler = DeviceInstance() | |
| # Clean before test | |
| try: | |
| device_handler.deleteDeviceByMAC(test_mac) | |
| except Exception: | |
| pass | |
| yield | |
| # Clean after test | |
| try: | |
| device_handler.deleteDeviceByMAC(test_mac) | |
| except Exception: | |
| pass | |
| `@pytest.fixture`(autouse=True) | |
| def cleanup_test_device(test_mac): | |
| """Clean up test device before and after test.""" | |
| device_handler = DeviceInstance() | |
| # Clean before test | |
| device_handler.deleteDeviceByMAC(test_mac) | |
| yield | |
| # Clean after test | |
| device_handler.deleteDeviceByMAC(test_mac) |
🧰 Tools
🪛 Ruff (0.14.13)
49-50: try-except-pass detected, consider logging the exception
(S110)
49-49: Do not catch blind exception: Exception
(BLE001)
57-58: try-except-pass detected, consider logging the exception
(S110)
57-57: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
In `@test/authoritative_fields/test_device_field_lock.py` around lines 42 - 58,
The fixture cleanup_test_device currently swallows all exceptions during
pre/post cleanup which hides failures; update it to stop catching exceptions
silently—either let exceptions from DeviceInstance.deleteDeviceByMAC propagate
or explicitly fail the test using pytest.fail with the caught exception message;
modify the try/except blocks around deleteDeviceByMAC in cleanup_test_device to
remove the bare excepts (or replace them with except Exception as e:
pytest.fail(f"...{e}")) so cleanup failures surface, and keep references to
DeviceInstance and deleteDeviceByMAC to locate the calls.
| def test_lock_prevents_field_updates(self, client, test_mac, auth_headers): | ||
| """Locked field should not be updated through API.""" | ||
| # Create device with initial name | ||
| self.test_create_test_device(client, test_mac, auth_headers) | ||
|
|
||
| # Lock the field | ||
| lock_payload = {"fieldName": "devName", "lock": True} | ||
| resp = client.post( | ||
| f"/device/{test_mac}/field/lock", | ||
| json=lock_payload, | ||
| headers=auth_headers | ||
| ) | ||
| assert resp.status_code == 200 | ||
|
|
||
| # Try to update the locked field | ||
| update_payload = {"devName": "New Name"} | ||
| resp = client.post( | ||
| f"/device/{test_mac}", | ||
| json=update_payload, | ||
| headers=auth_headers | ||
| ) | ||
|
|
||
| # Update should succeed at API level but authoritative handler should prevent it | ||
| # The field update logic checks source in the database layer | ||
| # For now verify the API accepts the request | ||
| assert resp.status_code in [200, 201] | ||
|
|
There was a problem hiding this comment.
Add an assertion that the locked field stays unchanged.
This test currently only checks the response code, so it doesn’t validate the locking behavior it describes.
🛠️ Proposed assertion
assert resp.status_code in [200, 201]
+
+ # Verify locked field remains unchanged
+ resp = client.get(f"/device/{test_mac}", headers=auth_headers)
+ assert resp.status_code == 200
+ device_data = resp.json
+ assert device_data.get("devName") == "Test Device"
+ assert device_data.get("devNameSource") == "LOCKED"🤖 Prompt for AI Agents
In `@test/authoritative_fields/test_device_field_lock.py` around lines 178 - 204,
The test_lock_prevents_field_updates currently only asserts response codes;
after calling the update endpoint with update_payload, add a follow-up GET
(e.g., call the same client to GET /device/{test_mac} or use existing helper
test_create_test_device/get_device helper) to retrieve the device record and
assert that devName remains the original value created by
test_create_test_device; reference the test function name
test_lock_prevents_field_updates, the lock endpoint used with lock_payload, and
the update POST using update_payload so you place the assertion right after the
update response check.
| def test_locked_field_blocks_plugin_overwrite(self, test_mac, auth_headers): | ||
| """Verify locked fields prevent plugin source overwrites.""" | ||
| device_handler = DeviceInstance() | ||
|
|
||
| # Create device | ||
| create_result = device_handler.setDeviceData(test_mac, { | ||
| "devName": "Original Name", | ||
| "devLastIP": "192.168.1.100", | ||
| "createNew": True | ||
| }) | ||
| assert create_result.get("success") is True | ||
|
|
||
| # Lock the field | ||
| device_handler.updateDeviceColumn(test_mac, "devNameSource", "LOCKED") | ||
|
|
||
| # Try to overwrite with plugin source (this would be done by authoritative handler) | ||
| # For now, verify the source is stored correctly | ||
| device_data = device_handler.getDeviceData(test_mac) | ||
| assert device_data.get("devNameSource") == "LOCKED" |
There was a problem hiding this comment.
Test doesn’t actually validate overwrite blocking.
It only sets the source and re-reads it; consider triggering an authoritative overwrite attempt (e.g., via authoritative handler or scan update) and asserting the value stays unchanged. Also, auth_headers is unused here.
🧰 Tools
🪛 Ruff (0.14.13)
272-272: Unused method argument: auth_headers
(ARG002)
🤖 Prompt for AI Agents
In `@test/authoritative_fields/test_device_field_lock.py` around lines 272 - 290,
The test test_locked_field_blocks_plugin_overwrite only sets devNameSource to
"LOCKED" and re-reads it but never attempts a plugin/authoritative overwrite;
update the test to simulate an overwrite (call the authoritative update path or
mimic a plugin write) after device_handler.updateDeviceColumn(test_mac,
"devNameSource", "LOCKED")—for example use the authoritative handler or a
DeviceInstance method that applies a source="PLUGIN" update (or call
device_handler.setDeviceData with a plugin-like payload) and then assert
device_handler.getDeviceData(test_mac)["devNameSource"] remains "LOCKED"; also
remove the unused auth_headers parameter from the test signature if you don’t
need it or use it in the overwrite call.
| """ | ||
| Integration tests for device field locking during actual scan updates. | ||
| Simulates real-world scenarios by: | ||
| 1. Setting up Devices table with various source values | ||
| 2. Populating CurrentScan with new discovery data | ||
| 3. Running actual device_handling scan updates | ||
| 4. Verifying field updates respect authorization rules | ||
| Tests all combinations of field sources (LOCKED, USER, NEWDEV, plugin name) | ||
| with realistic scan data. | ||
| """ |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Split this test module to stay under 500 LOC.
This file exceeds the 500-line guideline; consider splitting by scenario group (name/vendor updates, IP updates, presence/mixed cases). As per coding guidelines, keep files <=500 lines.
🤖 Prompt for AI Agents
In `@test/authoritative_fields/test_field_lock_scan_integration.py` around lines 1
- 12, The test module test_field_lock_scan_integration.py is too large; split it
into smaller modules by scenario groups (e.g. name_vendor updates, ip_updates,
presence_mixed_cases) so each file stays under 500 LOC; move related test
functions and any helper fixtures/classes into the new files (e.g.
test_field_lock_scan_integration_name_vendor.py,
test_field_lock_scan_integration_ip.py,
test_field_lock_scan_integration_presence.py), update any relative imports or
conftest references to keep fixtures (e.g. fixtures used by tests in this file)
available, and ensure pytest discovery still finds tests by keeping top-level
test_*.py filenames and preserving the original test function and class names
(so functions referenced by device_handling scan update integration tests remain
unchanged).
Authoritative overwrite checks should be implemented inside SQL UPDATE conditions wherever possible, avoiding per-row Python logic.
A plugin may overwrite a field if:
Summary by CodeRabbit
Release Notes
New Features
Documentation
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.