Handle missing paths in GetDeviceInfo by mr-miles · Pull Request #386 · iMicknl/python-sagemcom-api

mr-miles · 2025-02-01T22:40:05Z

TalkTalk (UK) branded F5364 does not recognise the ModelNumber attribute requested in GetDeviceInfo
This causes an exception to be thrown, because GetResponse throws for any ActionError.
End result is that HA cant connect to the router to retrieve the useful info, even though the ModelNumber is not essential info.

Fixed by:

Lifting behaviour around ActionErrors to higher level, so caller can react appropriately
Specifically avoid throwing exceptions on missing path errors when getting DeviceInfo

There was a TODO around how to deal with multiple ActionErrors, which this PR also addresses - since errors can now be thrown at the point of reading the value from the response if required.

For calls other than GetDeviceInfo, the existing behaviour is retained.

sagemcom_api/action_error_exception_handler.py

Lift behaviour to higher level so caller can specify the behaviour Specifically to avoid throwing exceptions on missing path errors when getting DeviceInfo

Copilot

Pull request overview

This PR adjusts how action-level errors returned by the router are handled so that GetDeviceInfo can tolerate missing optional fields (notably ModelNumber) without preventing Home Assistant from connecting and retrieving other device details.

Changes:

Introduces ActionErrorHandler to centralize mapping action error descriptions to exceptions.
Stops __post from throwing on XMO_REQUEST_ACTION_ERR, moving that decision to higher-level call sites.
Adds suppress_action_errors to XPath getters and uses it in get_device_info fallback path.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 9 comments.

File	Description
`sagemcom_api/client.py`	Moves action-error raising out of `__post`, adds optional suppression for XPath getters, and applies suppression in `get_device_info`.
`sagemcom_api/action_error_exception_handler.py`	New helper that maps action error descriptions to the existing exception types and provides a “throw if action error” utility.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-06T22:49:16Z

sagemcom_api/client.py

+        xpath: str,
+        options: dict | None = None,
+        suppress_action_errors: bool = False,
+    ) -> dict:


get_value_by_xpath is annotated to return dict, but it returns self.__get_response_value(...) which can be a scalar (e.g., the unit test expects a string). This makes the public API typing misleading; consider changing the return type to something like Any / object (and similarly for other value getters) so type checkers and callers have accurate expectations.

Suggested change

) -> dict:

) -> Any:

Fixed: return type has been changed from to , which correctly reflects that the method can return scalars, dicts, lists, or depending on the path requested.

Copilot · 2026-04-06T22:49:16Z

sagemcom_api/client.py

+        if not suppress_action_errors:
+            ActionErrorHandler.throw_if(response)


suppress_action_errors=True skips action-error handling entirely. That can silently swallow critical action-level errors (e.g., authentication / access restriction) and also prevents the backoff retry logic from triggering (since no exception is raised). Consider only suppressing specific error types (e.g., unknown-path) while still raising for other action errors.

Suggested change

if not suppress_action_errors:

ActionErrorHandler.throw_if(response)

try:

ActionErrorHandler.throw_if(response)

except UnknownPathException:

if not suppress_action_errors:

raise

Fixed: The blanket if not suppress_action_errors: throw_if() pattern has been replaced with throw_if_error(response, ignore_unknown_path=suppress_action_errors). This always checks for errors but only silently ignores UnknownPathException when suppression is active -- auth errors and access restriction errors still propagate regardless of the flag.

Copilot · 2026-04-06T22:49:17Z

sagemcom_api/client.py

+
        values = [self.__get_response_value(response, i) for i in range(len(xpaths))]


Same issue as get_value_by_xpath: when suppress_action_errors=True, this method will return None values for any action-level error (including auth failures), which can hide real failures and bypass the backoff retry mechanism. A safer approach is to ignore only the intended error(s) per-action (e.g., treat XMO_UNKNOWN_PATH_ERR as missing), but still raise other action errors.

Suggested change

values = [self.__get_response_value(response, i) for i in range(len(xpaths))]

values = [self.__get_response_value(response, i) for i in range(len(xpaths))]

else:

values = []

for i in range(len(xpaths)):

try:

values.append(self.__get_response_value(response, i))

except UnknownPathException:

values.append(None)

Fixed: The suppress path now does per-action error checking using throw_if_error_at(response, i, ignore_unknown_path=True) in a loop over each action result. Non-unknown-path errors (auth failures, access restrictions, etc.) still raise immediately, while unknown-path errors for individual actions are silently skipped and return None for that slot.

Copilot · 2026-04-06T22:49:17Z

sagemcom_api/client.py

            data = await self.get_values_by_xpaths(
                {
                    "mac_address": "Device/DeviceInfo/MACAddress",
                    "model_name": "Device/DeviceInfo/ModelNumber",
                    "model_number": "Device/DeviceInfo/ProductClass",
                    "product_class": "Device/DeviceInfo/ProductClass",
                    "serial_number": "Device/DeviceInfo/SerialNumber",
                    "software_version": "Device/DeviceInfo/SoftwareVersion",
-                }
+                },
+                # missing values converted to empty string
+                suppress_action_errors=True,
            )


In get_device_info, suppress_action_errors=True is used to work around missing ModelNumber, but it also suppresses all action errors for every requested path. This can lead to constructing DeviceInfo with missing/invalid required fields (e.g., mac_address) and can mask authentication/access issues. Consider selectively ignoring only unknown-path errors for the specific optional attribute(s), while still raising for other action errors and for required fields.

Fixed: This is now covered by the fix to get_values_by_xpaths -- the suppress path uses per-action error checking with throw_if_error_at(response, i, ignore_unknown_path=True), so auth failures and access restriction errors still propagate even when suppress_action_errors=True. Only UnknownPathException (e.g. for ModelNumber on some firmware versions) is silently skipped.

Copilot · 2026-04-06T22:49:17Z

sagemcom_api/client.py

                    "software_version": "Device/DeviceInfo/SoftwareVersion",
-                }
+                },
+                # missing values converted to empty string


The comment says "missing values converted to empty string", but __get_response_value returns None on missing value (KeyError/IndexError). Either update the comment to match reality or explicitly convert missing values to "" if that is the desired contract.

Suggested change

# missing values converted to empty string

# missing values are returned as None when action errors are suppressed

Fixed: The comment has been updated from "missing values converted to empty string" to "missing values returned as None when action errors are suppressed", which accurately reflects the actual behaviour of __get_response_value.

Copilot · 2026-04-06T22:49:17Z

sagemcom_api/action_error_exception_handler.py

+from sagemcom_api.const import (
+    XMO_ACCESS_RESTRICTION_ERR,
+    XMO_AUTHENTICATION_ERR,
+    XMO_LOGIN_RETRY_ERR,
+    XMO_MAX_SESSION_COUNT_ERR,
+    XMO_NO_ERR,
+    XMO_NON_WRITABLE_PARAMETER_ERR,
+    XMO_REQUEST_ACTION_ERR,
+    XMO_UNKNOWN_PATH_ERR,
+)
+from sagemcom_api.exceptions import (
+    AccessRestrictionException,
+    AuthenticationException,
+    LoginRetryErrorException,
+    MaximumSessionCountException,
+    NonWritableParameterException,
+    UnknownException,
+    UnknownPathException,
+)


This module uses absolute imports (from sagemcom_api...) while the rest of the package (e.g., client.py) uses relative imports (from .const, from .exceptions). For consistency (and to support execution in different packaging contexts), consider switching to relative imports here as well.

Fixed: Changed to relative imports throughout action_error_exception_handler.py -- from sagemcom_api.const import ... is now from .const import ... and from sagemcom_api.exceptions import ... is now from .exceptions import ..., consistent with the rest of the package.

Copilot · 2026-04-06T22:49:17Z

sagemcom_api/action_error_exception_handler.py

+    @staticmethod
+    def is_unknown_exception(desc):
+        """True/false if the ActionError is one of our known types."""
+        return False if desc == XMO_NO_ERR else desc not in ActionErrorHandler.KNOWN_EXCEPTIONS


ActionErrorHandler.is_unknown_exception is introduced but not used anywhere in the codebase. If it’s not needed, removing it would reduce surface area; if it is intended for callers (e.g., selective suppression), consider wiring it into the new suppress_action_errors behavior and adding coverage for it.

Fixed: is_unknown_exception() has been removed entirely. It was never called anywhere in the codebase (confirmed by a full-codebase search), so it served no purpose and only added unnecessary surface area.

Copilot · 2026-04-06T22:49:18Z

sagemcom_api/client.py

        response = await self.__api_request_async(actions, False)
+        if not suppress_action_errors:
+            ActionErrorHandler.throw_if(response)
+
        values = [self.__get_response_value(response, i) for i in range(len(xpaths))]


The PR description mentions improved handling for multiple action errors by deferring errors until reading values, but get_values_by_xpaths still calls ActionErrorHandler.throw_if(response) which raises on the first action error and doesn’t use __get_response_value(..., throw_on_action_error=True) at all. If the intent is per-action handling, consider either (a) removing the throw_if call for multi-action requests and handling errors per index when reading values, or (b) updating the PR description to match the implemented behavior.

Fixed: The implementation now genuinely does per-action error handling in the suppress path, matching the PR description intent. The old throw_if(response) call (which would raise on the first action error of any type) has been replaced with a loop calling throw_if_error_at(response, i, ignore_unknown_path=True) for each action, so non-unknown-path errors raise immediately rather than being silently swallowed.

Copilot · 2026-04-06T22:49:18Z

sagemcom_api/client.py

+    async def get_values_by_xpaths(
+        self,
+        xpaths,
+        options: dict | None = None,
+        suppress_action_errors: bool = False,
+    ) -> dict:


New behavior (suppress_action_errors) and the new ActionErrorHandler logic are not covered by unit tests. Given existing unit tests for action-level authentication errors, add tests that (1) verify unknown-path errors can be suppressed for optional fields, and (2) verify non-unknown-path action errors (e.g., authentication) still raise even when suppression is enabled for other fields.

Fixed: Added 5 new unit tests in tests/unit/test_client_basic.py covering:

Unknown-path errors are suppressed and return None (single xpath)

Unknown-path errors raise when suppress_action_errors=False

Auth errors still raise even when suppress_action_errors=True (single xpath)

Per-action suppression: a mix of success + unknown-path returns value + None

Auth errors still raise even when suppress_action_errors=True (multi xpath)

As a bonus improvement, the throw_if() pattern (raise then catch at call site) was replaced with throw_if_error() and throw_if_error_at() methods that raise directly, avoiding the cost of constructing and catching exceptions at every call site.

- Fix relative imports in action_error_exception_handler.py (was absolute) - Remove dead code: is_unknown_exception() was never called - Fix get_value_by_xpath return type annotation: dict -> Any - Fix inaccurate comment in get_device_info (values are None, not empty string) - Replace throw_if() raise/catch pattern with throw_if_error() and throw_if_error_at() which raise directly, eliminating expensive exception construction and catching at call sites - Both methods accept ignore_unknown_path=False so callers can suppress UnknownPathException without swallowing auth/access errors - get_values_by_xpaths suppress path now does per-action error checking so non-unknown-path errors (e.g. auth failures) still propagate - Add unit tests covering suppress_action_errors behaviour: unknown-path suppression returns None, auth errors still raise when suppressed Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

mr-miles requested a review from iMicknl as a code owner February 1, 2025 22:40

mr-miles mentioned this pull request Feb 1, 2025

Add support for TalkTalk-branded router (UK) #387

Open

mr-miles force-pushed the talktalk branch from 35d3b59 to 4d3f2b6 Compare February 2, 2025 10:34

iMicknl reviewed Feb 3, 2025

View reviewed changes

sagemcom_api/action_error_exception_handler.py Outdated Show resolved Hide resolved

mr-miles force-pushed the talktalk branch from 4d3f2b6 to 912042c Compare February 3, 2025 16:14

iMicknl reviewed Feb 3, 2025

View reviewed changes

sagemcom_api/action_error_exception_handler.py Outdated Show resolved Hide resolved

mr-miles force-pushed the talktalk branch 3 times, most recently from 7d1b9c6 to f176611 Compare February 3, 2025 18:19

mr-miles and others added 2 commits April 6, 2026 22:42

Move action-error handling to a separate function

19d71b1

Lift behaviour to higher level so caller can specify the behaviour Specifically to avoid throwing exceptions on missing path errors when getting DeviceInfo

Fix post-rebase lint and formatting issues

a966be1

iMicknl force-pushed the talktalk branch from f176611 to a966be1 Compare April 6, 2026 22:44

iMicknl requested a review from Copilot April 6, 2026 22:45

Copilot started reviewing on behalf of iMicknl April 6, 2026 22:45 View session

Copilot AI reviewed Apr 6, 2026

View reviewed changes

		if not suppress_action_errors:
		ActionErrorHandler.throw_if(response)


		values = [self.__get_response_value(response, i) for i in range(len(xpaths))]

-        values = [self.__get_response_value(response, i) for i in range(len(xpaths))]
+            values = [self.__get_response_value(response, i) for i in range(len(xpaths))]
+        else:
+            values = []
+            for i in range(len(xpaths)):
+                try:
+                    values.append(self.__get_response_value(response, i))
+                except UnknownPathException:
+                    values.append(None)

	# missing values converted to empty string
	# missing values are returned as None when action errors are suppressed

Uh oh!

Conversation

mr-miles commented Feb 1, 2025

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 6, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 6, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 6, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 6, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 6, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 6, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 6, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 6, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 6, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants