Conversation
WalkthroughThis pull request adds a new GitHub Actions workflow and a comprehensive Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant M as Odoo Client (main)
participant G as get_connector
participant CN as Connector
participant CO as Connection
participant S as Servicer
C->>M: get_connection(hostname, protocol, port, …)
M->>G: get_connector(hostname, protocol, port)
G-->>M: Return appropriate Connector (XMLRPC/JSONRPC)
M->>CO: Instantiate Connection(connector, database, login, …)
CO->>S: Delegate authentication & context retrieval
S-->>CO: Return user context or model service
CO-->>M: Return established Connection
M-->>C: Connection ready for RPC calls
sequenceDiagram
participant C as Client
participant Srv as Service
participant Sd as Sender
participant RS as Remote Service
C->>Srv: Invoke method (e.g., read or search_read)
Srv->>Srv: __getattr__ intercepts call and creates proxy
Srv->>Sd: Calls send (or async_send) with service/method & args
Sd->>RS: Forward RPC request
RS-->>Sd: Return response/result
Sd-->>Srv: Send back result
Srv-->>C: Return method call result
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Nitpick comments (42)
src/mccann_hub/odoolib/connection/authentication_error.py (1)
8-8: Update the copyright year to the current yearThe copyright year is set to 2025, which is in the future. Update it to the current year (2024).
- Copyright (C) 2025 Jimmy McCann + Copyright (C) 2024 Jimmy McCannsrc/mccann_hub/odoolib/connector/json_rpcs.py (4)
8-8: Verify copyright yearThe copyright year is set to 2025, which is in the future. Consider updating to the current year.
-# Copyright (C) 2025 Jimmy McCann +# Copyright (C) 2024 Jimmy McCann
36-41: Update class docstring to mention secure protocolThe current docstring states that the class uses "the JsonRPC protocol", but it doesn't mention that it specifically uses the secure HTTPS variant. Update the docstring to be more specific about the secure nature of this connector.
class JsonRpcsConnector(JsonRpcConnector): """ - A type of connector that uses the JsonRPC protocol. + A type of connector that uses the secure JsonRPC protocol over HTTPS. """ PROTOCOL = "jsonrpcs"
43-50: Addversionparameter to docstringThe
versionparameter is missing from the docstring. Include it to fully document all parameters.def __init__(self, hostname: str, port=8069, version="2.0"): """ Initialize by specifying the hostname and the port. :param hostname: The hostname of the computer holding the instance of Odoo. :param port: The port used by the Odoo instance for JsonRPC (default to 8069). + :param version: The JsonRPC protocol version (default to "2.0"). """ super(JsonRpcsConnector, self).__init__(hostname, port, version) self.url: str = "https://%s:%d/jsonrpc" % (hostname, port)
49-50: Avoid overriding parent class attributesThe
urlattribute is set in the parent class constructor but then immediately overridden here. This pattern can be inefficient and error-prone if the parent class behavior changes. Consider alternative approaches to avoid duplication.def __init__(self, hostname: str, port=8069, version="2.0"): """ Initialize by specifying the hostname and the port. :param hostname: The hostname of the computer holding the instance of Odoo. :param port: The port used by the Odoo instance for JsonRPC (default to 8069). """ - super(JsonRpcsConnector, self).__init__(hostname, port, version) - self.url: str = "https://%s:%d/jsonrpc" % (hostname, port) + # Initialize parent without setting url + super(JsonRpcConnector, self).__init__() + self.version = version + self.url: str = "https://%s:%d/jsonrpc" % (hostname, port)Alternatively, you could modify the parent class to accept a protocol parameter that determines whether to use HTTP or HTTPS.
src/mccann_hub/odoolib/dates.py (1)
36-39: Consistent string formatting approachThe formatting changes to
DEFAULT_SERVER_DATETIME_FORMATimprove readability and consistency. Consider using an f-string for modern Python formatting if you're using Python 3.6+.-DEFAULT_SERVER_DATETIME_FORMAT = "%s %s" % ( - DEFAULT_SERVER_DATE_FORMAT, - DEFAULT_SERVER_TIME_FORMAT, -) +DEFAULT_SERVER_DATETIME_FORMAT = f"{DEFAULT_SERVER_DATE_FORMAT} {DEFAULT_SERVER_TIME_FORMAT}"src/mccann_hub/odoolib/connector/xml_rpcs.py (1)
53-58: Consider reducing URL construction duplicationThe URL construction code is duplicated from the parent class with only the protocol change from "http" to "https". Consider refactoring to avoid duplication by extending the parent's implementation.
def __init__( self, hostname: str, port=8069, version: Optional[str] = "2", transport: Optional[SafeTransport] = None, ): super(XmlRpcsConnector, self).__init__(hostname, port, version, transport) - self.url = ( - "https://%s:%d/xmlrpc" % (hostname, port) - if version is None - else "https://%s:%d/xmlrpc/%s" % (hostname, port, version) - ) + # Replace 'http' with 'https' in the URL + self.url = self.url.replace("http://", "https://")tests/test_xml_rpc.py (1)
42-50: Consider adding tests for different connector configurationsThe current test only verifies one configuration (default port and with version). Consider adding tests for different parameter combinations like custom port or no version.
def test_initialization_with_custom_port(self): connector = XmlRpcConnector(self.hostname, port=9000) self.assertEqual( connector.url, f"http://{self.hostname}:9000/xmlrpc/{self.version}", ) def test_initialization_without_version(self): connector = XmlRpcConnector(self.hostname, version=None) self.assertEqual( connector.url, f"http://{self.hostname}:{self.port}/xmlrpc", )test_containers/compose.yaml (1)
24-24: Remove trailing whitespaceThere's a trailing space at the end of line 24.
- - POSTGRES_PASSWORD=odoo + - POSTGRES_PASSWORD=odoo🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 24-24: trailing spaces
(trailing-spaces)
src/mccann_hub/odoolib/connector/_connector.py (2)
47-52: Consider simplifying the logger name constructionThe logger name construction is somewhat complex with string splitting and joining. Consider simplifying this for better readability.
- def __init__(self): - self._logger = logging.getLogger( - f"{str.join('.', __name__.split('.')[:-1])}" - + (f".{self.PROTOCOL}" if self.PROTOCOL is not None else "") - ) - self.url: Optional[str] = None + def __init__(self): + # Get the package name (without the module part) + package_name = ".".join(__name__.split(".")[:-1]) + logger_name = package_name + if self.PROTOCOL is not None: + logger_name = f"{logger_name}.{self.PROTOCOL}" + self._logger = logging.getLogger(logger_name) + self.url: Optional[str] = None
8-8: Future copyright dateThe copyright notice includes the year 2025, which is a future date.
-# Copyright (C) 2025 Jimmy McCann +# Copyright (C) 2024 Jimmy McCanntests/test_json_rpc.py (1)
4-4: Future copyright dateThe copyright notice includes the year 2025, which is a future date.
-# Copyright (C) 2025 Jimmy McCann +# Copyright (C) 2024 Jimmy McCann.github/workflows/test.yml (2)
26-28: Consider adding container cleanup stepThe workflow starts Docker containers but doesn't explicitly shut them down after tests, which could leave resources running unnecessarily.
- name: Start up containers run: | docker compose -f ./test_containers/compose.yaml up --detach --wait --wait-timeout 600 + + # Other steps... + + - name: Cleanup containers + if: always() + run: | + docker compose -f ./test_containers/compose.yaml down --volumes --remove-orphans
35-37: Consider adding dependency cachingAdding caching for Python dependencies can significantly speed up workflow execution by reusing previously installed packages.
- name: Setup Python uses: actions/setup-python@v4 with: python-version: ${{ matrix.python_version }} + - name: Cache pip dependencies + uses: actions/cache@v3 + with: + path: ~/.cache/pip + key: ${{ runner.os }}-pip-${{ matrix.python_version }}-${{ hashFiles('requirements.txt') }} + restore-keys: | + ${{ runner.os }}-pip-${{ matrix.python_version }}- + ${{ runner.os }}-pip- - name: Install requirements run: | pip install -r requirements.txttests/test_json_rpcs.py (2)
52-88: Consider refactoring duplicate test codeThe test methods
test_send_successandtest_send_errorare nearly identical to those intest_json_rpc.py. Consider creating a base test class with common functionality to reduce duplication.# Example refactoring approach: import unittest from unittest.mock import MagicMock, patch import json class BaseJsonRpcConnectorTest(unittest.TestCase): connector_class = None # To be overridden by subclasses protocol = None # To be overridden by subclasses def setUp(self): self.hostname = "localhost" self.port = 8069 self.version = "2.0" self.connector = self.connector_class(self.hostname, self.port, self.version) def test_send_success(self): with patch(f"mccann_hub.odoolib.connector.json_rpc.requests.post") as mock_post: mock_response = MagicMock() mock_response.json.return_value = {"result": "mock_response"} mock_post.return_value = mock_response response = self.connector.send("common", "some_method", "arg1", "arg2") # Verify request data _, called_kwargs = mock_post.call_args sent_data = json.loads(called_kwargs["data"]) # Verify all expected fields except 'id' self.assertEqual(sent_data["jsonrpc"], self.version) self.assertEqual(sent_data["method"], "call") self.assertEqual( sent_data["params"], {"service": "common", "method": "some_method", "args": ["arg1", "arg2"]}, ) # Ensure 'id' exists and is an integer self.assertIn("id", sent_data) self.assertIsInstance(sent_data["id"], int) self.assertEqual(response, "mock_response") def test_send_error(self): with patch(f"mccann_hub.odoolib.connector.json_rpc.requests.post") as mock_post: mock_response = MagicMock() mock_response.json.return_value = {"error": "mock_error"} mock_post.return_value = mock_response with self.assertRaises(JsonRpcException) as context: self.connector.send("common", "some_method", "arg1", "arg2") self.assertEqual(str(context.exception), "'mock_error'") # Then in test_json_rpc.py: class TestJsonRpcConnector(BaseJsonRpcConnectorTest): connector_class = JsonRpcConnector protocol = "jsonrpc" def test_initialization(self): self.assertEqual( self.connector.url, f"http://{self.hostname}:{self.port}/jsonrpc" ) self.assertEqual(self.connector.version, self.version) self.assertEqual( self.connector._logger.name, f"mccann_hub.odoolib.connector.{self.protocol}" ) # And in test_json_rpcs.py: class TestJsonRpcsConnector(BaseJsonRpcConnectorTest): connector_class = JsonRpcsConnector protocol = "jsonrpcs" def test_initialization(self): self.assertEqual( self.connector.url, f"https://{self.hostname}:{self.port}/jsonrpc" ) self.assertEqual(self.connector.version, self.version) self.assertEqual( self.connector._logger.name, f"mccann_hub.odoolib.connector.{self.protocol}" )
4-4: Future copyright dateThe copyright notice includes the year 2025, which is a future date.
-# Copyright (C) 2025 Jimmy McCann +# Copyright (C) 2024 Jimmy McCannsrc/mccann_hub/odoolib/main.py (4)
8-8: Update the copyright year to the current year.The copyright date is set to 2025, which is in the future. Consider updating it to the current year.
-# Copyright (C) 2025 Jimmy McCann +# Copyright (C) 2024 Jimmy McCann
33-38: Consider updating the docstring with your own repository information.The module docstring still contains references to the original Odoo client library's URLs. It would be more accurate to update this with your repository information if this is a fork or customized version.
52-61: Improve conditional flow consistency.The conditional structure switches from if/elif to a fresh if/elif pattern. This works functionally but is stylistically inconsistent.
if protocol == "xmlrpc": return XmlRpcConnector(hostname, port) elif protocol == "xmlrpcs": return XmlRpcsConnector(hostname, port) -if protocol == "jsonrpc": +elif protocol == "jsonrpc": return JsonRpcConnector(hostname, port) elif protocol == "jsonrpcs": return JsonRpcsConnector(hostname, port) else: raise ValueError("You must choose xmlrpc, xmlrpcs, jsonrpc or jsonrpcs")
73-85: Fix incorrect parameter documentation.The docstring mentions a
connectorparameter that doesn't exist in the function signature. The connector is created internally usingget_connector.""" A shortcut method to easily create a connection to a remote Odoo server. :param hostname: The hostname to the remote server. :param protocol: The name of the protocol, must be "xmlrpc", "xmlrpcs", "jsonrpc" or "jsonrpcs". :param port: The number of the port. Defaults to 8069. -:param connector: A valid Connector instance to send messages to the remote server. :param database: The name of the database to work on. :param login: The login of the user. :param password: The password of the user. :param user_id: The user id is a number identifying the user. This is only useful if you already know it, in most cases you don't need to specify it. """src/mccann_hub/odoolib/service.py (2)
8-8: Update the copyright year to the current year.The copyright date is set to 2025, which is in the future. Consider updating it to the current year.
-# Copyright (C) 2025 Jimmy McCann +# Copyright (C) 2024 Jimmy McCann
52-78: Consider adding error handling for RPC calls.The proxy functions do not include any error handling for failures that might occur during RPC calls. Consider adding try/except blocks to handle and properly log potential errors.
def proxy(*args): """ :param args: A list of values for the method """ self._logger.debug("args (sync): %r", args) - result = self.connector.send(self.service_name, method, *args) - self._logger.debug("result (sync): %r", result) - return result + try: + result = self.connector.send(self.service_name, method, *args) + self._logger.debug("result (sync): %r", result) + return result + except Exception as e: + self._logger.error("Error calling %s.%s: %s", self.service_name, method, e) + raise async def async_proxy(*args): """ :param args: A list of values for the method """ self._logger.debug("args (async): %r", args) - result = await self.connector.async_send(self.service_name, method, *args) - self._logger.debug("result (async): %r", result) - return result + try: + result = await self.connector.async_send(self.service_name, method, *args) + self._logger.debug("result (async): %r", result) + return result + except Exception as e: + self._logger.error("Error calling %s.%s asynchronously: %s", self.service_name, method, e) + raisetests/test_async.py (2)
4-4: Update the copyright year to the current year.The copyright date is set to 2025, which is in the future. Consider updating it to the current year.
-# Copyright (C) 2025 Jimmy McCann +# Copyright (C) 2024 Jimmy McCann
38-45: Extract hardcoded connection parameters to constants or configuration.The connection parameters are hardcoded, which makes the tests less flexible and harder to run in different environments. Consider extracting these to constants at the top of the file, or better yet, use environment variables.
+# At the top of the file +# Default connection parameters - can be overridden by environment variables +HOST = os.environ.get("ODOO_TEST_HOST", "localhost") +DATABASE = os.environ.get("ODOO_TEST_DB", "test") +LOGIN = os.environ.get("ODOO_TEST_LOGIN", "admin") +PASSWORD = os.environ.get("ODOO_TEST_PASSWORD", "a") def _conn(self, protocol): return odoolib.get_connection( - hostname="localhost", + hostname=HOST, protocol=protocol, - database="test", - login="admin", - password="a", + database=DATABASE, + login=LOGIN, + password=PASSWORD, )tests/test_connection.py (1)
1-27: Copyright year in the futureThe copyright notice shows 2025 which is a future date (current year is 2024).
-# Copyright (C) 2025 Jimmy McCann +# Copyright (C) 2024 Jimmy McCannsrc/mccann_hub/odoolib/connection/_connection.py (1)
1-31: Copyright year in the futureThe copyright notice includes 2025 which is a future date (current year is 2024).
-# Copyright (C) 2025 Jimmy McCann +# Copyright (C) 2024 Jimmy McCanntests/test_xml_rpcs.py (1)
1-27: Copyright year in the futureThe copyright notice shows 2025 which is a future date (current year is 2024).
-# Copyright (C) 2025 Jimmy McCann +# Copyright (C) 2024 Jimmy McCanntests/test_model.py (1)
1-27: Copyright year in the futureThe copyright notice shows 2025 which is a future date (current year is 2024).
-# Copyright (C) 2025 Jimmy McCann +# Copyright (C) 2024 Jimmy McCannsrc/mccann_hub/odoolib/connector/xml_rpc.py (1)
39-65: Class initialization looks good, consider adding type annotationsThe
XmlRpcConnectorclass is well-structured with clear documentation, but missing some type annotations:def __init__( self, hostname: str, - port=8069, + port: int = 8069, version: Optional[str] = "2", transport: Optional[Transport] = None, ):tests/test.py (1)
61-84: Test cases maintained but could be expandedThe test methods are maintained with minor formatting improvements, but they are minimal and might not provide enough coverage for the library's functionality.
Consider adding more comprehensive tests for:
- Error handling scenarios
- Different authentication methods
- Tests for the new connector implementations
- Edge cases for different API calls
src/mccann_hub/odoolib/connector/json_rpc.py (2)
56-65: Add type annotations to constructor parametersSimilar to the XmlRpcConnector, some type annotations are missing:
- def __init__(self, hostname: str, port=8069, version="2.0"): + def __init__(self, hostname: str, port: int = 8069, version: str = "2.0"):
66-69: Add documentation to the send methodThe
sendmethod lacks documentation describing its purpose and parameters:- def send(self, service_name: str, method: str, *args): + def send(self, service_name: str, method: str, *args) -> any: + """ + Send a request to the specified service and method with the given arguments. + + :param service_name: The name of the service to call + :param method: The method name to call on the service + :param args: Additional arguments to pass to the method + :return: The result of the method call + :raises: JsonRpcException if the remote call returns an error + """tests/test_service.py (1)
54-63: Consider renaming the test to avoid referencing 'caching'
Currently, this test validates multiple calls but does not actually test caching. Renaming it for clarity or adding a caching mechanism would better reflect the test objective.-def test_sync_method_caching_and_multiple_calls(self): +def test_sync_method_multiple_calls(self):test_containers/postgres/docker-ensure-initdb.sh (1)
30-30: Use$BASH_SOURCE[0]for better clarity and to address ShellCheck SC2128.
Expanding$BASH_SOURCEwithout an index can cause unintended behavior in certain shells or nested calls.-exec gosu postgres "$BASH_SOURCE" "$@" +exec gosu postgres "${BASH_SOURCE[0]}" "$@"🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 30-30: Expanding an array without an index only gives the first element.
(SC2128)
src/mccann_hub/odoolib/model.py (1)
98-115: Refactor nested conditional checks.
You can combineif method == "read"andif isinstance(result, list) ...into a single check for slightly cleaner code.-if method == "read": - if isinstance(result, list) and len(result) > 0 and "id" in result[0]: +if method == "read" and isinstance(result, list) and len(result) > 0 and "id" in result[0]: index = {} ...🧰 Tools
🪛 Ruff (0.8.2)
102-103: Use a single
ifstatement instead of nestedifstatements(SIM102)
src/mccann_hub/odoolib/connection/_servicer.py (3)
8-8: Check the copyright year.The copyright year is set to 2025, which is in the future. Consider updating it to reflect the current year 2024.
-# Copyright (C) 2025 Jimmy McCann +# Copyright (C) 2024 Jimmy McCann
67-73: Remove incorrect parameter in docstring.The
connectorparameter is mentioned in the docstring but it's not part of the method signature forset_login_info.""" Set login information after the initialisation of this object. - :param connector: A valid Connector instance to send messages to the remote server. :param database: The name of the database to work on. :param login: The login of the user. :param password: The password of the user. :param user_id: The user id is a number identifying the user. This is only useful if you already know it, in most cases you don't need to specify it. """
78-91: Update docstring to reference Servicer.The docstring references "Connection" but should reference "Servicer" class since that's the actual class name.
""" - Checks if this Connection was already validated previously. + Checks if this Servicer was already validated previously. :param force: Force to re-check even if this Connection was already validated previously. Default to True. """setup.py (1)
8-8: Check the copyright year.The copyright year is set to 2025, which is in the future. Consider updating it to reflect the current year 2024.
-# Copyright (C) 2025 Jimmy McCann +# Copyright (C) 2024 Jimmy McCanntest_containers/postgres/docker-entrypoint.sh (3)
163-163: Unused variable warning.The
psqlvariable appears to be unused. If it's intended for external use, consider exporting it.- psql=( docker_process_sql ) + local psql=( docker_process_sql ) # Define as local if only used in this functionor if used externally:
- psql=( docker_process_sql ) + export psql=( docker_process_sql )🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 163-163: psql appears unused. Verify use (or export if used externally).
(SC2034)
201-201: Fix syntax for empty variable assignment.Remove spaces after
=when assigning empty values. According to shellcheck, this can cause issues.- PGHOST= PGHOSTADDR= "${query_runner[@]}" "$@" + PGHOST="" PGHOSTADDR="" "${query_runner[@]}" "$@"Similar fixes should be applied to lines 209, 214, and 274-275.
Also applies to: 209-209, 214-214, 274-275
🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 201-201: Remove space after = if trying to assign a value (for empty string, use var='' ... ).
(SC1007)
[warning] 201-201: Remove space after = if trying to assign a value (for empty string, use var='' ... ).
(SC1007)
316-316: Fix array expansion.Expanding an array without an index only gives the first element. This might not be the intended behavior.
- exec gosu postgres "$BASH_SOURCE" "$@" + exec gosu postgres "${BASH_SOURCE[0]}" "$@"🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 316-316: Expanding an array without an index only gives the first element.
(SC2128)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (35)
.github/workflows/test.yml(1 hunks).gitignore(1 hunks)odoolib/main.py(0 hunks)pyproject.toml(1 hunks)requirements.txt(1 hunks)setup.py(2 hunks)src/mccann_hub/odoolib/connection/__init__.py(1 hunks)src/mccann_hub/odoolib/connection/_connection.py(1 hunks)src/mccann_hub/odoolib/connection/_servicer.py(1 hunks)src/mccann_hub/odoolib/connection/authentication_error.py(1 hunks)src/mccann_hub/odoolib/connector/__init__.py(1 hunks)src/mccann_hub/odoolib/connector/_connector.py(1 hunks)src/mccann_hub/odoolib/connector/_sender.py(1 hunks)src/mccann_hub/odoolib/connector/json_rpc.py(1 hunks)src/mccann_hub/odoolib/connector/json_rpcs.py(1 hunks)src/mccann_hub/odoolib/connector/xml_rpc.py(1 hunks)src/mccann_hub/odoolib/connector/xml_rpcs.py(1 hunks)src/mccann_hub/odoolib/dates.py(5 hunks)src/mccann_hub/odoolib/main.py(1 hunks)src/mccann_hub/odoolib/model.py(1 hunks)src/mccann_hub/odoolib/service.py(1 hunks)test_containers/compose.yaml(1 hunks)test_containers/config/odoo.conf(1 hunks)test_containers/postgres/Dockerfile(1 hunks)test_containers/postgres/docker-ensure-initdb.sh(1 hunks)test_containers/postgres/docker-entrypoint.sh(1 hunks)tests/test.py(2 hunks)tests/test_async.py(1 hunks)tests/test_connection.py(1 hunks)tests/test_json_rpc.py(1 hunks)tests/test_json_rpcs.py(1 hunks)tests/test_model.py(1 hunks)tests/test_service.py(1 hunks)tests/test_xml_rpc.py(1 hunks)tests/test_xml_rpcs.py(1 hunks)
💤 Files with no reviewable changes (1)
- odoolib/main.py
🧰 Additional context used
🧬 Code Definitions (19)
src/mccann_hub/odoolib/connector/__init__.py (4)
src/mccann_hub/odoolib/connector/json_rpc.py (2)
JsonRpcConnector(49-88)JsonRpcException(41-46)src/mccann_hub/odoolib/connector/json_rpcs.py (1)
JsonRpcsConnector(36-50)src/mccann_hub/odoolib/connector/xml_rpc.py (1)
XmlRpcConnector(39-69)src/mccann_hub/odoolib/connector/xml_rpcs.py (1)
XmlRpcsConnector(39-58)
src/mccann_hub/odoolib/connection/__init__.py (2)
src/mccann_hub/odoolib/connection/_connection.py (1)
Connection(40-90)src/mccann_hub/odoolib/connection/authentication_error.py (1)
AuthenticationError(34-39)
tests/test_xml_rpc.py (1)
src/mccann_hub/odoolib/connector/xml_rpc.py (2)
XmlRpcConnector(39-69)send(66-69)
src/mccann_hub/odoolib/connector/json_rpcs.py (1)
src/mccann_hub/odoolib/connector/json_rpc.py (1)
JsonRpcConnector(49-88)
src/mccann_hub/odoolib/connector/xml_rpcs.py (1)
src/mccann_hub/odoolib/connector/xml_rpc.py (1)
XmlRpcConnector(39-69)
tests/test_json_rpc.py (1)
src/mccann_hub/odoolib/connector/json_rpc.py (3)
JsonRpcConnector(49-88)JsonRpcException(41-46)send(66-69)
src/mccann_hub/odoolib/connector/_connector.py (3)
src/mccann_hub/odoolib/service.py (1)
Service(38-78)src/mccann_hub/odoolib/connector/_sender.py (1)
Sender(33-51)src/mccann_hub/odoolib/connection/_servicer.py (1)
get_service(131-139)
tests/test_async.py (3)
tests/test.py (2)
setUp(45-46)_conn(48-55)src/mccann_hub/odoolib/main.py (1)
get_connection(64-88)src/mccann_hub/odoolib/connection/_connection.py (2)
get_model(84-90)async_get_user_context(76-82)
src/mccann_hub/odoolib/main.py (5)
src/mccann_hub/odoolib/connection/_connection.py (1)
Connection(40-90)src/mccann_hub/odoolib/connector/json_rpc.py (1)
JsonRpcConnector(49-88)src/mccann_hub/odoolib/connector/json_rpcs.py (1)
JsonRpcsConnector(36-50)src/mccann_hub/odoolib/connector/xml_rpc.py (1)
XmlRpcConnector(39-69)src/mccann_hub/odoolib/connector/xml_rpcs.py (1)
XmlRpcsConnector(39-58)
src/mccann_hub/odoolib/connection/_connection.py (3)
src/mccann_hub/odoolib/connector/_connector.py (1)
Connector(40-60)src/mccann_hub/odoolib/model.py (1)
Model(38-160)src/mccann_hub/odoolib/connection/_servicer.py (1)
Servicer(41-139)
tests/test_json_rpcs.py (2)
src/mccann_hub/odoolib/connector/json_rpc.py (2)
JsonRpcException(41-46)send(66-69)src/mccann_hub/odoolib/connector/json_rpcs.py (1)
JsonRpcsConnector(36-50)
src/mccann_hub/odoolib/service.py (3)
src/mccann_hub/odoolib/connector/_sender.py (3)
Sender(33-51)send(38-42)async_send(44-51)src/mccann_hub/odoolib/connector/json_rpc.py (1)
send(66-69)src/mccann_hub/odoolib/connector/xml_rpc.py (1)
send(66-69)
tests/test_service.py (1)
src/mccann_hub/odoolib/service.py (2)
Service(38-78)proxy(58-65)
tests/test_xml_rpcs.py (2)
src/mccann_hub/odoolib/connector/xml_rpcs.py (1)
XmlRpcsConnector(39-58)src/mccann_hub/odoolib/connector/xml_rpc.py (1)
send(66-69)
tests/test_model.py (1)
src/mccann_hub/odoolib/model.py (2)
Model(38-160)search_read(116-136)
tests/test.py (2)
src/mccann_hub/odoolib/main.py (1)
get_connection(64-88)src/mccann_hub/odoolib/connection/_connection.py (2)
get_model(84-90)get_user_context(68-74)
src/mccann_hub/odoolib/connection/_servicer.py (3)
src/mccann_hub/odoolib/connector/_connector.py (2)
Connector(40-60)get_service(54-60)src/mccann_hub/odoolib/service.py (1)
Service(38-78)src/mccann_hub/odoolib/connection/authentication_error.py (1)
AuthenticationError(34-39)
tests/test_connection.py (2)
src/mccann_hub/odoolib/connection/_connection.py (3)
Connection(40-90)get_model(84-90)get_user_context(68-74)src/mccann_hub/odoolib/model.py (1)
Model(38-160)
src/mccann_hub/odoolib/model.py (2)
src/mccann_hub/odoolib/connection/_servicer.py (4)
Servicer(41-139)check_login(93-110)get_service(131-139)async_check_login(112-129)src/mccann_hub/odoolib/connector/_connector.py (1)
get_service(54-60)
🪛 Ruff (0.8.2)
src/mccann_hub/odoolib/connector/__init__.py
1-1: .json_rpc.JsonRpcConnector imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
1-1: .json_rpc.JsonRpcException imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
2-2: .json_rpcs.JsonRpcsConnector imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
3-3: .xml_rpc.XmlRpcConnector imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
4-4: .xml_rpcs.XmlRpcsConnector imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
src/mccann_hub/odoolib/connection/__init__.py
1-1: ._connection.Connection imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
2-2: .authentication_error.AuthenticationError imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
src/mccann_hub/odoolib/model.py
102-103: Use a single if statement instead of nested if statements
(SIM102)
🪛 actionlint (1.7.4)
.github/workflows/test.yml
31-31: the runner of "actions/setup-python@v4" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🪛 YAMLlint (1.35.1)
test_containers/compose.yaml
[error] 24-24: trailing spaces
(trailing-spaces)
[warning] 31-31: too many blank lines
(1 > 0) (empty-lines)
🪛 Shellcheck (0.10.0)
test_containers/postgres/docker-ensure-initdb.sh
[warning] 30-30: Expanding an array without an index only gives the first element.
(SC2128)
test_containers/postgres/docker-entrypoint.sh
[warning] 163-163: psql appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 177-177: ShellCheck can't follow non-constant source. Use a directive to specify location.
(SC1090)
[warning] 201-201: Remove space after = if trying to assign a value (for empty string, use var='' ... ).
(SC1007)
[warning] 201-201: Remove space after = if trying to assign a value (for empty string, use var='' ... ).
(SC1007)
[warning] 209-209: This assignment is only seen by the forked process.
(SC2097)
[warning] 209-209: Remove space after = if trying to assign a value (for empty string, use var='' ... ).
(SC1007)
[warning] 209-209: This expansion will not see the mentioned assignment.
(SC2098)
[warning] 214-214: This assignment is only seen by the forked process.
(SC2097)
[warning] 214-214: Remove space after = if trying to assign a value (for empty string, use var='' ... ).
(SC1007)
[warning] 214-214: This expansion will not see the mentioned assignment.
(SC2098)
[warning] 274-275: Remove space after = if trying to assign a value (for empty string, use var='' ... ).
(SC1007)
[warning] 316-316: Expanding an array without an index only gives the first element.
(SC2128)
🔇 Additional comments (62)
src/mccann_hub/odoolib/connection/authentication_error.py (1)
34-39: LGTM: Good implementation of custom exceptionThe
AuthenticationErrorclass is well-defined with an appropriate docstring that clearly explains its purpose.test_containers/config/odoo.conf (1)
5-8: Consider adding security restrictions for production useThe current configuration enables both XML-RPC and JSON-RPC interfaces on port 8069 without any IP restrictions. While this is acceptable for development or testing environments, you should consider adding security restrictions for production deployments.
Consider adding the following settings for production environments:
xmlrpc_interfaceandjsonrpc_interfaceto restrict which network interfaces Odoo listens onproxy_modeif running behind a reverse proxydbfilterto restrict database accessIs this configuration intended for development/testing only, or will it be used in production?
requirements.txt (2)
2-2:Details
✅ Verification successful
Verify the future date in certifi version
The certifi version is dated 2025.1.31, which appears to be a future date. This may cause installation issues as this version likely doesn't exist yet.
🌐 Web query:
What is the latest version of certifi package available?💡 Result:
The latest version of the
certifiPython package is 2025.1.31, released on January 30, 2025[1][7][8]. This version provides Mozilla's curated root certificate bundle for TLS/SSL validation and is available on PyPI[1].For context:
- Debian/Ubuntu systems have this version packaged as
2025.1.31+ds-1(including Debian-specific modifications)[8].- Security vulnerabilities in earlier versions (like CVE-2024-39689 affecting versions before 2024.07.04) have been addressed in subsequent releases[3][4].
To install or upgrade:
pip install certifi --upgradeThe package continues to follow Mozilla's root certificate updates and maintains compatibility with Python deployments[1][9].
Citations:
- 1: https://pypi.org/project/certifi/
- 2: https://tracker.debian.org/python-certifi
- 3: https://www.tenable.com/plugins/nessus/204790
- 4: https://discuss.python.org/t/request-package-upgrade-is-not-updating-the-certifi-package/68271
- 5: https://github.com/certifi
- 6: https://pypi.org/project/python-certifi-win32/
- 7: https://www.piwheels.org/project/certifi/
- 8: https://launchpad.net/ubuntu/+source/python-certifi
- 9: https://certifi.io
- 10: https://github.com/certifi/python-certifi
Certifi Version Validated
The certifi version specified in requirements.txt (
certifi==2025.1.31) is confirmed to be the latest release (January 30, 2025). There is no issue with this version, and no modifications are necessary.
5-6:Details
❓ Verification inconclusive
Verify latest versions of dependent packages
Make sure the versions of requests and urllib3 are compatible with each other. Sometimes specific versions of urllib3 are required for certain requests versions.
🏁 Script executed:
#!/bin/bash # Check if the specified versions are the latest available echo "Checking latest versions of packages..." pip index versions requests | head -n 5 pip index versions urllib3 | head -n 5 # Check if our specified versions are compatible echo "Checking compatibility requirements..." pip install requests==2.32.3 -v | grep -A 5 "Requires"Length of output: 2482
Compatibility check for requests and urllib3 packages
The specified versions in requirements.txt (requests==2.32.3 and urllib3==2.3.0) are indeed the latest releases. However, the compatibility check using
pip install requests==2.32.3 -v | grep -A 5 "Requires"did not output any dependency requirements. This makes it unclear whether the verbose output properly confirms that requests declares the necessary version—or range—of urllib3.Please manually verify that the installed versions are compatible. For example, consider running additional checks (like
pip checkor reviewing requests’ installation metadata) to ensure no dependency conflicts exist.src/mccann_hub/odoolib/dates.py (2)
72-74: Good formatting improvement for readabilityThe multi-line formatting of the
strptimemethod call improves readability and makes the code more maintainable. This is a good change.
9-18: Consistent whitespace in commentsThe changes to add blank lines within the license comment block improve readability. These formatting changes are good.
src/mccann_hub/odoolib/connector/xml_rpcs.py (1)
39-58: LGTM - Well implemented secure XML-RPC connectorThe
XmlRpcsConnectorclass properly extendsXmlRpcConnectorto provide secure HTTPS communication. It correctly overrides thePROTOCOLconstant and the URL construction to use "https://" instead of "http://".tests/test_xml_rpc.py (1)
35-66: Thorough test implementation for XmlRpcConnectorThe test class provides good coverage of the connector functionality, including initialization and proper request handling.
test_containers/compose.yaml (1)
19-30:Details
❓ Verification inconclusive
Verify the database image and health check configuration
The database service is using a custom image (
ghcr.io/jabez007/odoo-client-lib) instead of a standard PostgreSQL image. Additionally, the health check's start period is set to 300s (5 minutes), which seems excessively long.
🏁 Script executed:
#!/bin/bash # Check the custom image details CUSTOM_IMAGE="ghcr.io/jabez007/odoo-client-lib" echo "Checking image: $CUSTOM_IMAGE" curl -s "https://api.github.com/users/jabez007/packages/container/odoo-client-lib/versions" | jq '.[0]' # Check if this is a modified PostgreSQL image or something else curl -s "https://api.github.com/repos/jabez007/odoo-client-lib/readme" | jq -r '.content' | base64 -d || echo "No README found"Length of output: 554
Review: Verify Custom Image and Health Check Settings
- The database service uses a custom image (
ghcr.io/jabez007/odoo-client-lib). The GitHub API check did not yield clear metadata (JSON indexing error and no README), so please manually verify that this image is a modified PostgreSQL container and meets our requirements.- The health check’s start period is set to 300s, which may be excessively long. Confirm if this duration is necessary for proper initialization or if a shorter interval is feasible.
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 24-24: trailing spaces
(trailing-spaces)
src/mccann_hub/odoolib/connector/_sender.py (2)
33-43: Well-designed base class for RPC operationsThe
Senderclass provides a solid foundation with a sensible stub implementation that can be overridden by child classes.
44-51: Well-implemented asynchronous wrapperThe
async_sendmethod correctly implements an asynchronous wrapper around the synchronoussendmethod usingrun_in_executor, allowing for non-blocking execution while maintaining compatibility.src/mccann_hub/odoolib/connector/_connector.py (4)
33-37: Well-structured imports with proper orderingThe imports are well-organized, with standard library imports (
logging,typing) followed by relative imports from the package.
40-43: Good use of inheritance and clear documentationThe
Connectorclass properly inherits fromSenderand includes a descriptive docstring explaining its purpose.
45-45: Appropriate use of Optional typingThe
PROTOCOLclass variable is properly typed asOptional[str], allowing for None values and providing clear information about expected types.
54-60: Well-implementedget_servicemethodThe
get_servicemethod properly creates and returns a Service instance, which aligns with the similar method seen in the_servicer.pyfile.tests/test_json_rpc.py (4)
36-42: Good test class setupThe test class is well-organized with a proper
setUpmethod that initializes the test object with relevant parameters.
43-50: Thorough initialization testThe
test_initializationmethod properly verifies that all connector attributes are initialized correctly, including the URL, version, and logger name.
52-77: Comprehensive success path testingThe
test_send_successmethod effectively tests the success path by:
- Mocking the external dependency (requests.post)
- Setting up the expected response
- Verifying that the correct data was sent in the request
- Checking that the response matches the expected result
79-88: Proper error path testingThe
test_send_errormethod correctly tests the error handling by:
- Mocking an error response
- Verifying that the appropriate exception is raised
- Checking the exception message matches expectations
.github/workflows/test.yml (1)
13-20:Details
✅ Verification successful
Consider Python version compatibility
The workflow tests against Python 3.13, which is not yet released. This might cause workflow failures until Python 3.13 is available.
🌐 Web query:
Is Python 3.13 released yet? What is the latest Python version available?💡 Result:
Python 3.13 was officially released on October 7, 2024[3][4], after a brief delay from its original October 1 target due to performance issues with garbage collector changes[1][8]. The latest version available as of April 2025 is Python 3.13.2, released on February 4, 2025, which includes nearly 250 bug fixes and improvements over previous 3.13 releases[2][5][9].
Key details about Python 3.13:
- Current stable version: 3.13.2 (maintenance release)
- Support timeline: Security updates guaranteed until October 2029[6][7][9]
- Major new features:
- Experimental free-threaded mode (GIL-disabled builds)[1][3][4]
- Preliminary JIT compiler groundwork[2][3][4]
- Enhanced REPL with multi-line editing and color support[2][4][10]
- Improved error messages with colorized tracebacks[3][10]
Python 3.13 marked the end of security support for Python 3.8[1][7], while development continues on Python 3.14 (currently in alpha)[5][7]. The release introduced significant under-the-hood changes to prepare for future performance enhancements, even though immediate speed improvements remain modest[3][10].
Citations:
- 1: https://flyaps.com/blog/update-python-3-13/
- 2: https://www.python.org/downloads/release/python-3132/
- 3: https://www.infoworld.com/article/2337441/the-best-new-features-and-fixes-in-python-313.html
- 4: https://www.python.org/downloads/release/python-3130/
- 5: https://realpython.com/python-news-march-2025/
- 6: https://peps.python.org/pep-0719/
- 7: https://devguide.python.org/versions/
- 8: https://www.python.org/downloads/release/python-3130rc3/
- 9: https://www.python.org/downloads/
- 10: https://realpython.com/python313-new-features/
Python 3.13 is now officially supported
Your workflow configuration in.github/workflows/test.ymlis valid as-is. Python 3.13 was officially released in October 2024 (with the current stable release being 3.13.2 as of February 2025), so testing against it no longer poses compatibility risks.tests/test_json_rpcs.py (1)
36-50: Test verifies secure HTTPS protocol usageThe
test_initializationmethod correctly verifies that theJsonRpcsConnectoruses HTTPS instead of HTTP, which is a good security practice for RPC connections.src/mccann_hub/odoolib/main.py (1)
86-88: The get_connection implementation looks good.The implementation properly uses the get_connector function to create a connector and passes all the necessary parameters to the Connection constructor.
src/mccann_hub/odoolib/service.py (2)
38-51: The Service class initialization looks good.The Service class is well-designed with a clear initialization process that properly sets up logging for monitoring RPC calls.
76-78: Elegant implementation of async support.Attaching the async_proxy directly to the proxy function as an attribute is an elegant design pattern that provides a clean API for both synchronous and asynchronous usage.
tests/test_async.py (1)
67-80: The test_search_count_async implementation is well-structured.This test correctly validates that the count of search results matches the length of the results array, which is a good verification of the async functionality.
test_containers/postgres/Dockerfile (5)
1-5: This is a generated Dockerfile.The comment indicates this file is generated via "apply-templates.sh" and should not be edited directly. Consider using the template generation process for any changes instead of directly modifying this file.
9-16: Well-configured PostgreSQL user setup.The user and group setup follows best practices with explicit IDs and proper home directory permissions.
189-190: Consider the implications of removing the VOLUME directive.The commented-out VOLUME directive is unconventional for PostgreSQL containers, which typically need persistent storage. This is explicitly done to enable database snapshots, but make sure you understand the implications for data persistence.
Is there a specific reason you're not using a volume for PostgreSQL data? This approach means data will be lost when the container is removed unless you're handling persistence elsewhere.
192-194:Details
✅ Verification successful
Verify that the necessary entrypoint scripts exist.
The Dockerfile copies
docker-entrypoint.shanddocker-ensure-initdb.shinto the container, but these files are not shown in the PR. Ensure these scripts exist and are properly configured.
🏁 Script executed:
#!/bin/bash # Check if the referenced entrypoint scripts exist ls -la test_containers/postgres/docker-entrypoint.sh test_containers/postgres/docker-ensure-initdb.shLength of output: 298
Entry Point Scripts Verified: Both necessary scripts are present and properly configured.
- Verified that
test_containers/postgres/docker-entrypoint.shandtest_containers/postgres/docker-ensure-initdb.shexist in the repository with appropriate executable permissions.- No further action is required regarding these files.
89-173:Details
❓ Verification inconclusive
Consider implications of using PostgreSQL 17.
PostgreSQL 17 is a very recent version and might not be compatible with applications designed for older versions. Verify that your Odoo installation is compatible with this PostgreSQL version.
🏁 Script executed:
#!/bin/bash # Check if Odoo officially supports PostgreSQL 17 # Look for PostgreSQL version requirements in the documentation # First, let's look for documentation files that might mention PostgreSQL fd -t f -e md -e rst -e txt | xargs rg -l "postgres|PostgreSQL" | head -n 10 # Then, let's check specifically for version requirements rg -i "postgres.*version|postgresql.*version|requires.*postgres" -A 3 -B 3Length of output: 2058
Verify PostgreSQL 17 Compatibility for Odoo
The PostgreSQL 17 configuration in this Dockerfile is a recent update, and there isn’t any conclusive evidence from the documentation that Odoo fully supports this version. Please review the official Odoo compatibility guidelines to ensure:
- All required extensions and dependencies are compatible with PostgreSQL 17.
- The build and runtime behaviors (as seen from the Dockerfile configuration) don’t negatively impact your Odoo installation.
tests/test_connection.py (5)
36-45: LGTM! Good test setupThe test setup correctly mocks the connector and initializes the Connection with appropriate test parameters.
47-51: LGTM! Good initialization testThe test properly verifies that the Connection constructor correctly initializes all expected attributes.
53-62: LGTM! Good context testing techniqueGreat use of mocking to test the get_user_context method. The test verifies both the return value and that the expected methods are called with the correct parameters.
64-68: LGTM! Well-structured model testThe test properly verifies that get_model returns an instance of the correct type with the expected attributes.
71-72: LGTM! Standard unittest entry pointGood practice to include the standard unittest entry point for running tests individually.
src/mccann_hub/odoolib/connection/_connection.py (2)
40-67: LGTM! Well-documented Connection classThe Connection class is well-structured with clear documentation. It correctly inherits from Servicer and initializes the user_context attribute.
84-90: LGTM! Well-implemented get_model methodThe get_model method correctly creates and returns a Model instance with the appropriate parameters.
tests/test_xml_rpcs.py (2)
35-50: LGTM! Good test structure and initialization testThe test class is well-structured with appropriate setup and initialization testing. The assertions verify the URL construction, logger name, and transport initialization.
69-70: LGTM! Standard unittest entry pointGood practice to include the standard unittest entry point for running tests individually.
tests/test_model.py (5)
35-42: LGTM! Comprehensive test setupThe test setup thoroughly configures the mock objects and initializes the Model with appropriate test parameters.
44-47: LGTM! Good logger name verificationThe test correctly verifies that the logger name is properly constructed.
49-67: LGTM! Thorough proxy method testsThe tests for proxy method calls cover important scenarios: single ID retrieval, multiple ID retrieval, and handling non-existent IDs. The assertions and mock configurations are appropriate.
69-78: LGTM! Well-structured search_read testsThe tests for search_read properly verify both the case where results are found and the case where no results are found.
80-82: LGTM! Standard unittest entry pointGood practice to include the standard unittest entry point for running tests individually.
.gitignore (1)
1-175: Comprehensive and well-structured .gitignore fileThis .gitignore file is complete and follows standard Python project practices, covering all necessary exclusions like byte-compiled files, packages, testing artifacts, and environment-specific files.
Some configuration options are commented out that you might consider uncommenting based on your project's needs:
- Line 168:
.idea/for PyCharm/IntelliJ- Lines 95-108: Various lock files for dependency management tools (Pipfile.lock, poetry.lock)
These are typically project decisions based on whether you want consistent dependency versions checked into your repository.
tests/test.py (2)
40-41: Namespace package import is a good improvementUpdating the import from
import odoolibtoimport mccann_hub.odoolib as odoolibis a positive change that follows better namespace package practices.
49-55: Connection method refactoring improves readabilityThe reformatted
_connmethod is more readable with proper indentation and parameter alignment.src/mccann_hub/odoolib/connector/json_rpc.py (2)
41-47: Custom exception provides good error handling foundationThe
JsonRpcExceptionclass is well-defined and allows for clear error propagation from JSON-RPC calls.
71-88:Details
✅ Verification successful
Improve error handling in _json_rpc method
While the method handles JSON-RPC errors, it doesn't handle HTTP request failures:
def _json_rpc(self, fct_name, params): + """ + Execute a JSON-RPC call with the given function name and parameters. + + :param fct_name: The RPC function to call + :param params: The parameters to pass to the function + :return: The result of the RPC call + :raises: JsonRpcException if the server returns an error + :raises: requests.RequestException if the HTTP request fails + """ data = { "jsonrpc": self.version, "method": fct_name, "params": params, "id": random.randint(0, 1000000000), } - result_req = requests.post( - self.url, - data=json.dumps(data), - headers={ - "Content-Type": "application/json", - }, - ) - result = result_req.json() + try: + result_req = requests.post( + self.url, + data=json.dumps(data), + headers={ + "Content-Type": "application/json", + }, + timeout=60, # Add a reasonable timeout + ) + result_req.raise_for_status() # Raise exception for 4XX/5XX responses + result = result_req.json() + except requests.RequestException as e: + # Handle network or HTTP errors + raise JsonRpcException({"code": -32603, "message": f"Transport error: {str(e)}"}) + except json.JSONDecodeError: + raise JsonRpcException({"code": -32700, "message": "Invalid JSON response"}) if result.get("error", None): raise JsonRpcException(result["error"]) return result.get("result", False)Also, consider using
secretsinstead ofrandomfor generating the request ID in security-sensitive applications:- "id": random.randint(0, 1000000000), + "id": secrets.randbelow(1000000000),
🏁 Script executed:
#!/bin/bash # Check whether the request ID generation is used in a secure context # Look for any authentication or sensitive data processing in the codebase rg -A 5 -B 5 "login|password|token|auth|credential" --type pyLength of output: 21445
Refactor _json_rpc Method for Robust Error Handling and Secure ID Generation
Please update the
_json_rpcmethod insrc/mccann_hub/odoolib/connector/json_rpc.py(lines 71–88) as follows:
- Add a concise docstring describing the parameters, return values, and raised exceptions.
- Wrap the HTTP request in a try/except block:
- Call
raise_for_status()on the response to catch HTTP errors,- Catch
requests.RequestExceptionfor transport-level issues and wrap them in aJsonRpcException,- Catch
json.JSONDecodeErrorto handle invalid JSON responses.- Specify a reasonable timeout (e.g., 60 seconds) for the HTTP request.
- Replace the use of
random.randintwithsecrets.randbelowfor generating the request ID in security-sensitive scenarios, given the presence of authentication details elsewhere in the codebase.Suggested diff snippet:
def _json_rpc(self, fct_name, params): + """ + Execute a JSON-RPC call with the given function name and parameters. + + :param fct_name: The RPC function to call + :param params: The parameters to pass to the function + :return: The result of the RPC call + :raises: JsonRpcException if the server returns an error or transport issue occurs + """ data = { - "id": random.randint(0, 1000000000), + "id": secrets.randbelow(1000000000), "jsonrpc": self.version, "method": fct_name, "params": params, } - result_req = requests.post( - self.url, - data=json.dumps(data), - headers={ - "Content-Type": "application/json", - }, - ) - result = result_req.json() + try: + result_req = requests.post( + self.url, + data=json.dumps(data), + headers={"Content-Type": "application/json"}, + timeout=60, + ) + result_req.raise_for_status() + result = result_req.json() + except requests.RequestException as e: + raise JsonRpcException({"code": -32603, "message": f"Transport error: {str(e)}"}) + except json.JSONDecodeError: + raise JsonRpcException({"code": -32700, "message": "Invalid JSON response"}) if result.get("error", None): raise JsonRpcException(result["error"]) return result.get("result", False)This refactor improves the resilience of the JSON-RPC call by handling network, HTTP, and JSON parsing errors robustly, while also enhancing security for request ID generation.
tests/test_service.py (4)
1-34: No issues found in license headers and imports.
35-43: TestService setup and initialization.
The class and logger validation test are clear and straightforward.
44-53: Synchronous method call test is well-structured.
The mock usage and assertion logic look correct.
64-68: Method name reflection test is valid.
Verifying the existence of.async_is a solid check.test_containers/postgres/docker-ensure-initdb.sh (2)
1-29: Initial script and environment setup look fine.
No issues found regarding environment configuration or Docker entrypoint usage in these lines.
31-72: No additional issues identified.
Overall, the script handles initialization logic cleanly.src/mccann_hub/odoolib/model.py (4)
1-52: File setup, imports, constructor, and logging approach look good.
53-96: Dynamic method generation and async proxy are well-structured.
The approach for synchronous and asynchronous calls is consistent and aligns well with Odoo’s dynamic RPC style.
116-137: Thesearch_readmethod is concise and follows Odoo conventions.
138-161: The asynchronoussearch_readflow is consistent and correct.
No concerns regarding the async logic.src/mccann_hub/odoolib/connection/_servicer.py (1)
41-140: LGTM! The Servicer class is well-structured with clear separation of concerns.The class provides both synchronous and asynchronous authentication methods, which is good practice for modern Python applications. The error handling is appropriate and the logging will be helpful for debugging.
setup.py (1)
35-60: The setup configuration looks good!The package configuration has been properly updated to reflect the new namespace structure and version. The classifiers and metadata are accurately specified.
test_containers/postgres/docker-entrypoint.sh (1)
1-360: This looks like a standard PostgreSQL Docker entrypoint script.The script follows common patterns for initializing a PostgreSQL database in a Docker container. It handles environment variables, database initialization, and user permissions appropriately.
🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 163-163: psql appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 177-177: ShellCheck can't follow non-constant source. Use a directive to specify location.
(SC1090)
[warning] 201-201: Remove space after = if trying to assign a value (for empty string, use var='' ... ).
(SC1007)
[warning] 201-201: Remove space after = if trying to assign a value (for empty string, use var='' ... ).
(SC1007)
[warning] 209-209: This assignment is only seen by the forked process.
(SC2097)
[warning] 209-209: Remove space after = if trying to assign a value (for empty string, use var='' ... ).
(SC1007)
[warning] 209-209: This expansion will not see the mentioned assignment.
(SC2098)
[warning] 214-214: This assignment is only seen by the forked process.
(SC2097)
[warning] 214-214: Remove space after = if trying to assign a value (for empty string, use var='' ... ).
(SC1007)
[warning] 214-214: This expansion will not see the mentioned assignment.
(SC2098)
[warning] 274-275: Remove space after = if trying to assign a value (for empty string, use var='' ... ).
(SC1007)
[warning] 316-316: Expanding an array without an index only gives the first element.
(SC2128)
| @patch("mccann_hub.odoolib.connector.xml_rpc.ServerProxy") | ||
| def test_send(self, mock_server_proxy): | ||
| mock_service = MagicMock() | ||
| mock_method = MagicMock(return_value="mock_response") | ||
| mock_service.some_method = mock_method | ||
| mock_server_proxy.return_value = mock_service | ||
|
|
||
| response = self.connector.send("common", "some_method", "arg1", "arg2") | ||
|
|
||
| mock_server_proxy.assert_called_once_with( | ||
| f"https://{self.hostname}:{self.port}/xmlrpc/{self.version}/common", | ||
| transport=None, | ||
| ) | ||
| mock_method.assert_called_once_with("arg1", "arg2") | ||
| self.assertEqual(response, "mock_response") |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Missing error handling test
The test for send covers the success case, but there's no test for error handling. Based on the method's expected behavior, there should be tests for error conditions.
Add a test method for error handling:
@patch("mccann_hub.odoolib.connector.xml_rpc.ServerProxy")
def test_send_error(self, mock_server_proxy):
mock_service = MagicMock()
mock_method = MagicMock(side_effect=Exception("Test error"))
mock_service.some_method = mock_method
mock_server_proxy.return_value = mock_service
with self.assertRaises(Exception):
self.connector.send("common", "some_method", "arg1", "arg2")
mock_server_proxy.assert_called_once_with(
f"https://{self.hostname}:{self.port}/xmlrpc/{self.version}/common",
transport=None,
)
mock_method.assert_called_once_with("arg1", "arg2")| class TestModel(unittest.TestCase): | ||
| def setUp(self): | ||
| self.mock_connection = MagicMock() | ||
| self.mock_connection.database = "test_db" | ||
| self.mock_connection.user_id = 1 | ||
| self.mock_connection.password = "pass" | ||
| self.mock_connection.get_service.return_value.execute_kw = MagicMock() | ||
| self.model = Model(self.mock_connection, "res.partner") | ||
|
|
||
| def test_initialization(self): | ||
| self.assertEqual( | ||
| self.model._logger.name, "mccann_hub.odoolib.model.res.partner" | ||
| ) | ||
|
|
||
| def test_proxy_method_call(self): | ||
| self.mock_connection.get_service.return_value.execute_kw.return_value = [ | ||
| {"id": 1, "name": "John"} | ||
| ] | ||
| result = self.model.read([1]) | ||
| self.assertEqual(result, [{"id": 1, "name": "John"}]) | ||
|
|
||
| def test_proxy_method_call_list_ids(self): | ||
| self.mock_connection.get_service.return_value.execute_kw.return_value = [ | ||
| {"id": 1, "name": "John"}, | ||
| {"id": 2, "name": "Jane"}, | ||
| ] | ||
| result = self.model.read([1, 2]) | ||
| self.assertEqual(result, [{"id": 1, "name": "John"}, {"id": 2, "name": "Jane"}]) | ||
|
|
||
| def test_proxy_method_call_id_not_found(self): | ||
| self.mock_connection.get_service.return_value.execute_kw.return_value = [] | ||
| result = self.model.read([999]) | ||
| self.assertFalse(result) | ||
|
|
||
| def test_search_read(self): | ||
| self.model.search = MagicMock(return_value=[1]) | ||
| self.model.read = MagicMock(return_value=[{"id": 1, "name": "X"}]) | ||
| result = self.model.search_read([("is_company", "=", True)], ["name"]) | ||
| self.assertEqual(result, [{"id": 1, "name": "X"}]) | ||
|
|
||
| def test_search_read_no_result(self): | ||
| self.model.search = MagicMock(return_value=[]) | ||
| result = self.model.search_read([("is_company", "=", False)]) | ||
| self.assertEqual(result, []) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Missing async method tests
The Model class supports both synchronous and asynchronous operations, but the tests only cover the synchronous methods. Add tests for the asynchronous methods to ensure complete coverage.
Here's an example of how to test an async method:
@patch('asyncio.run')
def test_async_proxy_method_call(self, mock_run):
# Setup mock response
async_result = [{"id": 1, "name": "John"}]
mock_run.return_value = async_result
# Create a coroutine mock
mock_coro = MagicMock()
mock_coro.__await__ = lambda self: async_result.__await__()
# Setup the async_ method to return our coroutine mock
self.model.read.async_ = MagicMock(return_value=mock_coro)
# Run the test
result = asyncio.run(self.model.read.async_([1]))
self.assertEqual(result, [{"id": 1, "name": "John"}])
# Verify the async method was called
self.model.read.async_.assert_called_once_with([1])Consider adding similar tests for other async methods like search.async_ and search_read.async_ if they exist.
| def send(self, service_name: str, method: str, *args): | ||
| url = "%s/%s" % (self.url, service_name) | ||
| service = ServerProxy(url, transport=self._transport) | ||
| return getattr(service, method)(*args) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and documentation to send method
The send method lacks error handling for connection issues or remote exceptions, and is missing return type annotation and documentation.
- def send(self, service_name: str, method: str, *args):
+ def send(self, service_name: str, method: str, *args) -> any:
+ """
+ Send a request to the specified service and method with the given arguments.
+
+ :param service_name: The name of the service to call (e.g., 'common', 'object', etc.)
+ :param method: The method name to call on the service
+ :param args: Additional arguments to pass to the method
+ :return: The result of the method call
+ :raises: xmlrpc.client.Fault if the remote call fails
+ """
url = "%s/%s" % (self.url, service_name)
- service = ServerProxy(url, transport=self._transport)
- return getattr(service, method)(*args)
+ service = ServerProxy(url, transport=self._transport)
+ try:
+ return getattr(service, method)(*args)
+ except Exception as e:
+ # Log the error or transform it to a more specific exception
+ raise📝 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.
| def send(self, service_name: str, method: str, *args): | |
| url = "%s/%s" % (self.url, service_name) | |
| service = ServerProxy(url, transport=self._transport) | |
| return getattr(service, method)(*args) | |
| def send(self, service_name: str, method: str, *args) -> any: | |
| """ | |
| Send a request to the specified service and method with the given arguments. | |
| :param service_name: The name of the service to call (e.g., 'common', 'object', etc.) | |
| :param method: The method name to call on the service | |
| :param args: Additional arguments to pass to the method | |
| :return: The result of the method call | |
| :raises: xmlrpc.client.Fault if the remote call fails | |
| """ | |
| url = "%s/%s" % (self.url, service_name) | |
| service = ServerProxy(url, transport=self._transport) | |
| try: | |
| return getattr(service, method)(*args) | |
| except Exception as e: | |
| # Log the error or transform it to a more specific exception | |
| raise |
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (12)
test_containers/config/odoo.conf (1)
4-4: Avoid hardcoding the admin passwordHardcoding passwords in configuration files is a security risk, especially when those files might be committed to version control. Consider using environment variables or a secrets management system instead.
- admin_passwd = P@$$w0rd + admin_passwd = ${ODOO_ADMIN_PASSWORD}Then provide the password via environment variable in your deployment process.
requirements.txt (1)
1-1:asynciopackage is unnecessaryThe
asynciopackage is part of the Python standard library since Python 3.4 and shouldn't be explicitly installed. It's already available in any Python 3.4+ environment without requiring a separate package installation.-asyncio==3.4.3test_containers/compose.yaml (1)
22-24: Avoid hardcoded credentials in configuration filesThe database credentials are hardcoded in the configuration. Consider using environment variables or secrets management.
environment: - POSTGRES_DB=postgres - - POSTGRES_USER=odoo - - POSTGRES_PASSWORD=odoo + - POSTGRES_USER=${POSTGRES_USER:-odoo} + - POSTGRES_PASSWORD=${POSTGRES_PASSWORD:-odoo}🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 24-24: trailing spaces
(trailing-spaces)
.github/workflows/test.yml (1)
30-33: Update GitHub Actions setup-python versionThe static analysis hints are correct—using
actions/setup-python@v4is outdated. The recommended version is nowv5.- name: Setup Python - uses: actions/setup-python@v4 + name: Setup Python + uses: actions/setup-python@v5 with: python-version: ${{ matrix.python_version }}🧰 Tools
🪛 actionlint (1.7.4)
31-31: the runner of "actions/setup-python@v4" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
tests/test_xml_rpcs.py (1)
52-66: 🛠️ Refactor suggestionMissing error handling test
The test for
sendcovers the success case, but there's no test for error handling. Based on the method's expected behavior, there should be tests for error conditions.Add a test method for error handling:
@patch("mccann_hub.odoolib.connector.xml_rpc.ServerProxy") def test_send_error(self, mock_server_proxy): mock_service = MagicMock() mock_method = MagicMock(side_effect=Exception("Test error")) mock_service.some_method = mock_method mock_server_proxy.return_value = mock_service with self.assertRaises(Exception): self.connector.send("common", "some_method", "arg1", "arg2") mock_server_proxy.assert_called_once_with( f"https://{self.hostname}:{self.port}/xmlrpc/{self.version}/common", transport=None, ) mock_method.assert_called_once_with("arg1", "arg2")tests/test_model.py (1)
44-78: 🛠️ Refactor suggestionMissing async method tests for the Model class.
The test suite thoroughly tests synchronous methods but is missing tests for the asynchronous methods available in the Model class.
Here's an example of how to test an async method:
import asyncio from unittest.mock import MagicMock, patch def test_async_proxy_method_call(self): # Setup mock response async_result = [{"id": 1, "name": "John"}] # Mock the async execute_kw call mock_execute_kw_async = MagicMock() async def mock_async(*args, **kwargs): return async_result mock_execute_kw_async.async_ = mock_async self.mock_connection.get_service.return_value.execute_kw = mock_execute_kw_async # Run the test with asyncio.run result = asyncio.run(self.model.read.async_([1])) # Verify results self.assertEqual(result, [{"id": 1, "name": "John"}])Consider adding similar tests for other async methods like
search.async_andsearch_read.async_.src/mccann_hub/odoolib/connector/xml_rpc.py (1)
66-78: 🛠️ Refactor suggestionAdd error handling to the send method.
While the method documentation has been added as suggested in the previous review, there's still no explicit error handling for connection issues or remote exceptions.
def send(self, service_name: str, method: str, *args): """ Send a request to the specified service and method with the given arguments. :param service_name: The name of the service to call (e.g., 'common', 'object', etc.) :param method: The method name to call on the service :param args: Additional arguments to pass to the method :return: The result of the method call :raises: xmlrpc.client.Fault if the remote call fails """ url = "%s/%s" % (self.url, service_name) - service = ServerProxy(url, transport=self._transport) - return getattr(service, method)(*args) + service = ServerProxy(url, transport=self._transport) + try: + return getattr(service, method)(*args) + except Exception as e: + self._logger.error("XML-RPC call failed: %s", str(e)) + raisesrc/mccann_hub/odoolib/connection/_connection.py (1)
68-82: (duplicate) Potential race onuser_contextcacheThe cached
user_contextis written by both sync and async getters with no locking, which can lead to inconsistent state under concurrent access. Although earlier you noted that callers should use either sync or async APIs, multithreaded sync use cases (or concurrentawaitscheduling) can still interleave.Re-consider adding a (re-entrant) lock or an immutable snapshot to guarantee safety.
pyproject.toml (1)
31-34: Removeasynciofrom dependencies (standard library)
asyncioships with Python ≥ 3.4 and must not be declared as an external requirement. Keeping it confuses tooling and users:dependencies = [ "requests", - "asyncio", ]No functional change, but cleans up packaging metadata.
src/mccann_hub/odoolib/connection/_servicer.py (2)
104-110: Address the TODO comment about usingauthenticateinstead oflogin.There's a TODO comment about using
authenticateinstead oflogin. This needs to be implemented or the TODO should be documented with a rationale for keeping the current approach.This issue was previously identified in a review comment. Either implement the
authenticatemethod in the common service, document whyloginis preferred, or remove the TODO comment if it's not planned.
123-129: Address the TODO comment in the async implementation as well.The same TODO comment about using
authenticateinstead ofloginappears in the asynchronous implementation. Both instances should be addressed consistently.This is related to the same issue flagged earlier in the synchronous implementation. If you update the synchronous method, make sure to update the asynchronous one as well.
setup.py (1)
46-48: 🛠️ Refactor suggestionRemove unnecessary dependency on built-in module.
asynciois a built-in module in Python 3.4+ and should not be listed as an external dependency since your package already requires Python 3.9+ (as specified inpython_requires).install_requires=[ "requests", - "asyncio", ],
🧹 Nitpick comments (26)
src/mccann_hub/odoolib/connection/authentication_error.py (1)
8-8: Future copyright dateThe copyright year 2025 is in the future. Consider updating to the current year or using a range (e.g., 2024-2025) if this is intentional.
- # Copyright (C) 2025 Jimmy McCann + # Copyright (C) 2024 Jimmy McCannsrc/mccann_hub/odoolib/connector/json_rpcs.py (1)
36-40: Update docstring to specify secure protocolThe current docstring doesn't indicate that this connector uses the secure JsonRPC protocol over HTTPS, which is the key difference from its parent class.
- A type of connector that uses the JsonRPC protocol. + A type of connector that uses the secured JsonRPC protocol over HTTPS.tests/test_xml_rpc.py (2)
4-4: Update the copyright yearThe copyright year 2025 is in the future. Consider updating it to the current year (2024) for accuracy.
-# Copyright (C) 2025 Jimmy McCann +# Copyright (C) 2024 Jimmy McCann
40-40: Consider initializing with explicit parameters for better test coverageThe connector is initialized with only the hostname, relying on default values for port and version. For better test coverage, consider explicitly passing all parameters.
- self.connector = XmlRpcConnector(self.hostname) + self.connector = XmlRpcConnector(self.hostname, self.port, self.version)test_containers/compose.yaml (2)
13-17: Consider making healthcheck more robustThe current healthcheck relies on curl being available in the container. For improved reliability, consider using a more robust approach or ensuring curl is installed.
healthcheck: - test: curl --fail http://localhost:8069/web/login || exit 1 + test: ["CMD", "wget", "--spider", "--quiet", "http://localhost:8069/web/login"] start_period: 10s interval: 10s timeout: 5s retries: 5
26-30: Reduce health check start periodThe current start period of 300s (5 minutes) for the database health check seems excessive and could significantly delay startup in testing environments.
healthcheck: test: ["CMD-SHELL", "pg_isready -U odoo -d postgres"] - start_period: 300s + start_period: 60s interval: 10s timeout: 5s retries: 5src/mccann_hub/odoolib/connector/_sender.py (1)
4-4: Update the copyright yearThe copyright year 2025 is in the future. Consider updating it to the current year (2024) for accuracy.
-# Copyright (C) 2025 Jimmy McCann +# Copyright (C) 2024 Jimmy McCann.github/workflows/publish.yml (3)
15-19: Consider specifying a more precise Python versionUsing
"3.x"will use the latest Python 3 version, which might lead to unexpected behavior with future Python releases. Consider specifying a more precise version range.- name: Set up Python uses: actions/setup-python@v5 with: - python-version: "3.x" + python-version: "3.9"
20-27: Add caching for pip dependenciesAdding a caching step for pip dependencies can significantly improve workflow execution time by avoiding repeated downloads.
- name: Set up Python uses: actions/setup-python@v5 with: python-version: "3.x" + - name: Cache pip dependencies + uses: actions/cache@v3 + with: + path: ~/.cache/pip + key: ${{ runner.os }}-pip-${{ hashFiles('**/pyproject.toml') }} + restore-keys: | + ${{ runner.os }}-pip- - name: Install build tools run: | python -m pip install --upgrade build
76-78: Add pip upgrade and consider pinning versionIt's a good practice to ensure pip is updated before installing packages. Also, for verification purposes, consider pinning the package version.
- name: Install package from PyPI run: | - python -m pip install mccann_hub-odoo_client_lib + python -m pip install --upgrade pip + python -m pip install mccann_hub-odoo_client_lib==${{ github.event.release.tag_name }}tests/test_json_rpc.py (1)
79-88: Consider adding test for async_send method.While the current tests cover the synchronous
sendmethod well, there's no coverage for the asynchronousasync_sendmethod inherited from theSenderclass.+ @patch("mccann_hub.odoolib.connector.json_rpc.requests.post") + async def test_async_send_success(self, mock_post): + mock_response = MagicMock() + mock_response.json.return_value = {"result": "mock_response"} + mock_post.return_value = mock_response + + response = await self.connector.async_send("common", "some_method", "arg1", "arg2") + + _, called_kwargs = mock_post.call_args + sent_data = json.loads(called_kwargs["data"]) + + self.assertEqual(response, "mock_response").github/workflows/test-publish.yml (2)
19-20: Consider specifying a more precise Python version.Using "3.x" will pull the latest Python 3 version, which might lead to unexpected behavior if new Python versions introduce breaking changes. Consider specifying a specific version range that matches your compatibility requirements.
- python-version: "3.x" + python-version: "3.8 - 3.11"
69-70: Consider validating more functionality in the verification step.The current verification only checks if the package can be imported. Consider adding a basic smoke test that exercises core functionality to ensure the published package works as expected.
- python -c "import mccann_hub.odoolib; print('✅ Import successful')" + python -c "import mccann_hub.odoolib as odoolib; connector = odoolib.get_connector('localhost', 'jsonrpc'); print('✅ Import and basic functionality successful')"src/mccann_hub/odoolib/connector/_connector.py (1)
47-52: Add constructor docstring and enforce URL initialization.The constructor lacks a docstring explaining its purpose, and the
urlattribute is initialized toNonebut each subclass is expected to set it to a valid value. Consider adding a docstring and requiring subclasses to set the URL.def __init__(self): + """ + Initialize the connector with a logger. + Subclasses must set the `url` attribute to a valid URL string. + """ self._logger = logging.getLogger( f"{str.join('.', __name__.split('.')[:-1])}" + (f".{self.PROTOCOL}" if self.PROTOCOL is not None else "") ) self.url: Optional[str] = Nonesrc/mccann_hub/odoolib/main.py (2)
46-51: Update docstring to reflect all supported protocols.The docstring only mentions XMLRPC, but the function supports xmlrpc, xmlrpcs, jsonrpc, and jsonrpcs protocols.
""" -A shortcut method to easily create a connector to a remote server using XMLRPC. +A shortcut method to easily create a connector to a remote server using various protocols. :param hostname: The hostname to the remote server. :param protocol: The name of the protocol, must be "xmlrpc", "xmlrpcs", "jsonrpc" or "jsonrpcs". :param port: The number of the port. Defaults to 8069. """
79-84: Remove non-existent parameter from docstring.The docstring includes a
connectorparameter that doesn't exist in the function signature.:param protocol: The name of the protocol, must be "xmlrpc", "xmlrpcs", "jsonrpc" or "jsonrpcs". :param port: The number of the port. Defaults to 8069. -:param connector: A valid Connector instance to send messages to the remote server. :param database: The name of the database to work on. :param login: The login of the user. :param password: The password of the user.tests/test.py (1)
8-8: Future copyright date needs revision.The copyright year is set to 2025, which is in the future. Consider updating it to the current year (2024) to accurately reflect when the code was written.
-# Copyright (C) 2025 Jimmy McCann +# Copyright (C) 2024 Jimmy McCannsrc/mccann_hub/odoolib/service.py (1)
8-8: Future copyright date needs revision.The copyright year is set to 2025, which is in the future. Consider updating it to the current year (2024) to accurately reflect when the code was written.
-# Copyright (C) 2025 Jimmy McCann +# Copyright (C) 2024 Jimmy McCanntests/test_connection.py (1)
4-4: Future copyright date needs revision.The copyright year is set to 2025, which is in the future. Consider updating it to the current year (2024) to accurately reflect when the code was written.
-# Copyright (C) 2025 Jimmy McCann +# Copyright (C) 2024 Jimmy McCanntest_containers/postgres/docker-ensure-initdb.sh (1)
28-31: Consider handling shell safety in user switching.The shellcheck static analysis flagged line 30 with SC2128 about expanding an array without an index. While this is actually a false positive for
$@which correctly expands to all arguments, it's worth double-checking your shell script practices.- exec gosu postgres "$BASH_SOURCE" "$@" + exec gosu postgres "$BASH_SOURCE" "$@"🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 30-30: Expanding an array without an index only gives the first element.
(SC2128)
src/mccann_hub/odoolib/connector/xml_rpc.py (1)
33-36: Consider more precise typing imports.The import for Optional from typing is good, but you might also want to add Any for the return type of the send method to be more explicit about the type.
-from typing import Optional +from typing import Any, Optionalsrc/mccann_hub/odoolib/connector/json_rpc.py (2)
66-70: Consider supporting keyword parameters
send()currently forwards only positional*args. Some Odoo endpoints accept keyword arguments (e.g.context={...}) that callers may want to pass explicitly:- def send(self, service_name: str, method: str, *args): - return self._json_rpc( - "call", {"service": service_name, "method": method, "args": args} - ) + def send(self, service_name: str, method: str, *args, **kwargs): + return self._json_rpc( + "call", + { + "service": service_name, + "method": method, + "args": args, + "kwargs": kwargs, + }, + )The server will ignore
kwargsif unused, while enabling richer calls when needed.
76-77: Use uuid for request IDs
random.randint(…)can collide across processes and offers no semantic meaning. Usinguuid.uuid4().hex(orsecrets.randbits) eliminates that risk and communicates intent:- "id": random.randint(0, 1000000000), + "id": uuid.uuid4().hex,Since the value is opaque to the server, changing type to
stris safe.src/mccann_hub/odoolib/model.py (1)
102-113: Simplify nested conditionals & leverage dict-comprehensionRuff flag
SIM102indicates the nestedifchain can be flattened, improving readability and performance slightly:- if method == "read": - if isinstance(result, list) and len(result) > 0 and "id" in result[0]: - index = {} - for r in result: - index[r["id"]] = r - if isinstance(args[0], list): - result = [index[x] for x in args[0] if x in index] - elif args[0] in index: - result = index[args[0]] - else: - result = False + if ( + method == "read" + and isinstance(result, list) + and result + and "id" in result[0] + ): + index = {r["id"]: r for r in result} + key = args[0] + if isinstance(key, list): + result = [index[x] for x in key if x in index] + else: + result = index.get(key, False)Same behaviour, fewer branches, clearer intent.
🧰 Tools
🪛 Ruff (0.8.2)
102-103: Use a single
ifstatement instead of nestedifstatements(SIM102)
test_containers/postgres/docker-entrypoint.sh (2)
201-201: Fix syntax in variable assignments.There are spaces after equal signs in variable assignments that should be removed according to Shellcheck. In Bash, spaces around
=in variable assignments can cause unexpected behavior.- PGHOST= PGHOSTADDR= "${query_runner[@]}" "$@" + PGHOST="" PGHOSTADDR="" "${query_runner[@]}" "$@" - POSTGRES_DB= docker_process_sql --dbname postgres --set db="$POSTGRES_DB" --tuples-only <<-'EOSQL' + POSTGRES_DB="" docker_process_sql --dbname postgres --set db="$POSTGRES_DB" --tuples-only <<-'EOSQL' - POSTGRES_DB= docker_process_sql --dbname postgres --set db="$POSTGRES_DB" <<-'EOSQL' + POSTGRES_DB="" docker_process_sql --dbname postgres --set db="$POSTGRES_DB" <<-'EOSQL' - NOTIFY_SOCKET= \ + NOTIFY_SOCKET="" \Also applies to: 209-209, 214-214, 274-275
🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 201-201: Remove space after = if trying to assign a value (for empty string, use var='' ... ).
(SC1007)
[warning] 201-201: Remove space after = if trying to assign a value (for empty string, use var='' ... ).
(SC1007)
316-316: Array expansion without index warning.Shellcheck warns about expanding
$BASH_SOURCEwithout an index. In Bash, expanding an array without an index only gives the first element. If this is intentional, consider making it explicit.- exec gosu postgres "$BASH_SOURCE" "$@" + exec gosu postgres "$BASH_SOURCE[0]" "$@"However, if
$BASH_SOURCEis not intended to be an array in this context, you can ignore this warning.🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 316-316: Expanding an array without an index only gives the first element.
(SC2128)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (38)
.github/workflows/publish.yml(1 hunks).github/workflows/test-publish.yml(1 hunks).github/workflows/test.yml(1 hunks).gitignore(1 hunks)README.rst(2 hunks)odoolib/main.py(0 hunks)pyproject.toml(1 hunks)requirements.txt(1 hunks)setup.py(2 hunks)src/mccann_hub/odoolib/connection/__init__.py(1 hunks)src/mccann_hub/odoolib/connection/_connection.py(1 hunks)src/mccann_hub/odoolib/connection/_servicer.py(1 hunks)src/mccann_hub/odoolib/connection/authentication_error.py(1 hunks)src/mccann_hub/odoolib/connector/__init__.py(1 hunks)src/mccann_hub/odoolib/connector/_connector.py(1 hunks)src/mccann_hub/odoolib/connector/_sender.py(1 hunks)src/mccann_hub/odoolib/connector/json_rpc.py(1 hunks)src/mccann_hub/odoolib/connector/json_rpcs.py(1 hunks)src/mccann_hub/odoolib/connector/xml_rpc.py(1 hunks)src/mccann_hub/odoolib/connector/xml_rpcs.py(1 hunks)src/mccann_hub/odoolib/dates.py(5 hunks)src/mccann_hub/odoolib/main.py(1 hunks)src/mccann_hub/odoolib/model.py(1 hunks)src/mccann_hub/odoolib/service.py(1 hunks)test_containers/compose.yaml(1 hunks)test_containers/config/odoo.conf(1 hunks)test_containers/postgres/Dockerfile(1 hunks)test_containers/postgres/docker-ensure-initdb.sh(1 hunks)test_containers/postgres/docker-entrypoint.sh(1 hunks)tests/test.py(2 hunks)tests/test_async.py(1 hunks)tests/test_connection.py(1 hunks)tests/test_json_rpc.py(1 hunks)tests/test_json_rpcs.py(1 hunks)tests/test_model.py(1 hunks)tests/test_service.py(1 hunks)tests/test_xml_rpc.py(1 hunks)tests/test_xml_rpcs.py(1 hunks)
💤 Files with no reviewable changes (1)
- odoolib/main.py
🧰 Additional context used
🧬 Code Graph Analysis (15)
src/mccann_hub/odoolib/main.py (5)
src/mccann_hub/odoolib/connection/_connection.py (1)
Connection(40-90)src/mccann_hub/odoolib/connector/json_rpc.py (1)
JsonRpcConnector(49-88)src/mccann_hub/odoolib/connector/json_rpcs.py (1)
JsonRpcsConnector(36-50)src/mccann_hub/odoolib/connector/xml_rpc.py (1)
XmlRpcConnector(39-78)src/mccann_hub/odoolib/connector/xml_rpcs.py (1)
XmlRpcsConnector(39-58)
tests/test_json_rpc.py (1)
src/mccann_hub/odoolib/connector/json_rpc.py (3)
JsonRpcConnector(49-88)JsonRpcException(41-46)send(66-69)
src/mccann_hub/odoolib/connector/json_rpcs.py (1)
src/mccann_hub/odoolib/connector/json_rpc.py (1)
JsonRpcConnector(49-88)
tests/test_xml_rpc.py (1)
src/mccann_hub/odoolib/connector/xml_rpc.py (2)
XmlRpcConnector(39-78)send(66-78)
src/mccann_hub/odoolib/connector/xml_rpcs.py (1)
src/mccann_hub/odoolib/connector/xml_rpc.py (1)
XmlRpcConnector(39-78)
tests/test_model.py (1)
src/mccann_hub/odoolib/model.py (2)
Model(38-160)search_read(116-136)
src/mccann_hub/odoolib/connection/__init__.py (2)
src/mccann_hub/odoolib/connection/_connection.py (1)
Connection(40-90)src/mccann_hub/odoolib/connection/authentication_error.py (1)
AuthenticationError(34-39)
src/mccann_hub/odoolib/connector/_sender.py (2)
src/mccann_hub/odoolib/connector/xml_rpc.py (1)
send(66-78)src/mccann_hub/odoolib/connector/json_rpc.py (1)
send(66-69)
src/mccann_hub/odoolib/connector/_connector.py (3)
src/mccann_hub/odoolib/service.py (1)
Service(38-78)src/mccann_hub/odoolib/connector/_sender.py (1)
Sender(33-51)src/mccann_hub/odoolib/connection/_servicer.py (1)
get_service(131-139)
tests/test_xml_rpcs.py (3)
src/mccann_hub/odoolib/connector/xml_rpcs.py (1)
XmlRpcsConnector(39-58)tests/test_xml_rpc.py (3)
setUp(36-40)test_initialization(42-50)test_send(53-66)src/mccann_hub/odoolib/connector/xml_rpc.py (1)
send(66-78)
src/mccann_hub/odoolib/service.py (3)
src/mccann_hub/odoolib/connector/_sender.py (3)
Sender(33-51)send(38-42)async_send(44-51)src/mccann_hub/odoolib/connector/xml_rpc.py (1)
send(66-78)src/mccann_hub/odoolib/connector/json_rpc.py (1)
send(66-69)
src/mccann_hub/odoolib/connection/_connection.py (3)
src/mccann_hub/odoolib/connector/_connector.py (1)
Connector(40-60)src/mccann_hub/odoolib/model.py (1)
Model(38-160)src/mccann_hub/odoolib/connection/_servicer.py (1)
Servicer(41-139)
src/mccann_hub/odoolib/connector/xml_rpc.py (3)
src/mccann_hub/odoolib/connector/_connector.py (1)
Connector(40-60)src/mccann_hub/odoolib/connector/_sender.py (1)
send(38-42)src/mccann_hub/odoolib/connector/json_rpc.py (1)
send(66-69)
tests/test_async.py (3)
tests/test.py (3)
setUp(45-46)_conn(48-55)_get_protocols(57-59)src/mccann_hub/odoolib/main.py (1)
get_connection(64-88)src/mccann_hub/odoolib/connection/_connection.py (2)
get_model(84-90)async_get_user_context(76-82)
src/mccann_hub/odoolib/connection/_servicer.py (3)
src/mccann_hub/odoolib/connector/_connector.py (2)
Connector(40-60)get_service(54-60)src/mccann_hub/odoolib/service.py (1)
Service(38-78)src/mccann_hub/odoolib/connection/authentication_error.py (1)
AuthenticationError(34-39)
🪛 YAMLlint (1.35.1)
test_containers/compose.yaml
[error] 24-24: trailing spaces
(trailing-spaces)
[warning] 31-31: too many blank lines
(1 > 0) (empty-lines)
🪛 Shellcheck (0.10.0)
test_containers/postgres/docker-ensure-initdb.sh
[warning] 30-30: Expanding an array without an index only gives the first element.
(SC2128)
test_containers/postgres/docker-entrypoint.sh
[warning] 163-163: psql appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 177-177: ShellCheck can't follow non-constant source. Use a directive to specify location.
(SC1090)
[warning] 201-201: Remove space after = if trying to assign a value (for empty string, use var='' ... ).
(SC1007)
[warning] 201-201: Remove space after = if trying to assign a value (for empty string, use var='' ... ).
(SC1007)
[warning] 209-209: This assignment is only seen by the forked process.
(SC2097)
[warning] 209-209: Remove space after = if trying to assign a value (for empty string, use var='' ... ).
(SC1007)
[warning] 209-209: This expansion will not see the mentioned assignment.
(SC2098)
[warning] 214-214: This assignment is only seen by the forked process.
(SC2097)
[warning] 214-214: Remove space after = if trying to assign a value (for empty string, use var='' ... ).
(SC1007)
[warning] 214-214: This expansion will not see the mentioned assignment.
(SC2098)
[warning] 274-275: Remove space after = if trying to assign a value (for empty string, use var='' ... ).
(SC1007)
[warning] 316-316: Expanding an array without an index only gives the first element.
(SC2128)
🪛 actionlint (1.7.4)
.github/workflows/test.yml
31-31: the runner of "actions/setup-python@v4" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
.github/workflows/publish.yml
67-67: the runner of "softprops/action-gh-release@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🪛 Ruff (0.8.2)
src/mccann_hub/odoolib/model.py
102-103: Use a single if statement instead of nested if statements
(SIM102)
🔇 Additional comments (58)
src/mccann_hub/odoolib/dates.py (7)
9-29: Well-formatted license header improvements.These changes improve the license header formatting with consistent spacing and line structure, enhancing readability while maintaining the original license terms.
36-40: Good formatting of multi-line expression.The multi-line formatting for DEFAULT_SERVER_DATETIME_FORMAT is well-structured and follows proper indentation practices, improving code readability.
42-53: Improved docstring formatting in str_to_datetime function.The added blank line in the docstring improves readability by visually separating the function description from the implementation details.
54-64: Appropriate blank line spacing between functions.The addition of a blank line between functions follows PEP 8 guidelines and improves visual separation of code blocks.
72-75: Well-structured multi-line return statement.The reformatting of the return statement in str_to_time() spreads the expression across multiple lines with proper indentation, enhancing readability.
77-87: Improved docstring formatting in datetime_to_str function.The added blank line in the docstring improves readability by visually separating the function description from the implementation notes.
88-98: Consistent spacing between functions.The addition of blank lines between functions is applied consistently throughout the file, following good Python coding practices.
.gitignore (1)
1-175: Comprehensive .gitignore file looks goodThis is a well-structured .gitignore file that covers all common Python development artifacts, package files, environments, and tool-specific directories. It follows best practices for Python projects.
README.rst (3)
16-16: Package name change looks goodThe package name change from
odoo-client-libtomccann_hub-odoo_client_libis correctly reflected in the installation instructions.
20-20: Import statement properly updatedThe import statement has been correctly updated to reflect the new package structure.
65-70: Testing section is a helpful additionThe new testing section with Docker Compose instructions provides clear guidance for setting up a local test environment. This enhances the usability of the library.
src/mccann_hub/odoolib/connector/__init__.py (1)
6-12: Well-defined exports with allThe
__all__list properly defines which symbols should be exported from this module, which is a good practice for clarity and maintainability.src/mccann_hub/odoolib/connection/authentication_error.py (1)
34-39: Well-designed error classThe
AuthenticationErrorclass is well-designed with a clear docstring explaining its purpose. It follows good practices for custom exceptions.requirements.txt (1)
2-2:Details
✅ Verification successful
Verify future certificate expiration date
The
certifipackage has a version date of 2025.1.31, which is in the future. This may cause compatibility issues if attempting to install in environments that validate package versions.
🏁 Script executed:
#!/bin/bash # Check if this certifi version exists on PyPI curl -s https://pypi.org/pypi/certifi/json | jq '.releases | keys' | grep "2025.1.31" # Check the latest available version curl -s https://pypi.org/pypi/certifi/json | jq '.info.version'Length of output: 175
No action needed: certifi 2025.1.31 is a valid PyPI release
The pinned version (
certifi==2025.1.31) is confirmed as the latest release on PyPI, so there are no compatibility issues related to its date-based versioning.src/mccann_hub/odoolib/connector/json_rpcs.py (1)
41-51: Implementation looks goodThe PROTOCOL constant is correctly defined as "jsonrpcs" and the implementation properly modifies the URL to use HTTPS instead of HTTP.
src/mccann_hub/odoolib/connection/__init__.py (1)
1-4: Clean implementation with properly defined public APIThe module provides a clean interface by explicitly defining which symbols are exported through the
__all__list, following Python best practices.src/mccann_hub/odoolib/connector/xml_rpcs.py (2)
39-42: Implementation looks goodThe docstring correctly specifies that this connector uses the secured XMLRPC protocol, which appropriately differentiates it from the regular XmlRpcConnector.
51-58:Details
✅ Verification successful
Verify SSL certificate validation
When implementing secure connections, it's important to ensure proper certificate validation. The current implementation inherits behavior from XmlRpcConnector, passing the transport parameter to the parent, but it's worth verifying that the default behavior enforces certificate validation.
🏁 Script executed:
#!/bin/bash # Description: Search for certificate validation handling in XmlRpcConnector # or related classes to ensure secure connections validate certificates # Look for transport initialization code rg -A 10 "transport.*=|SafeTransport" --type python # Check for any certificate validation or context settings rg -A 5 -B 5 "ssl.*context|verify.*cert|cert.*verify" --type pythonLength of output: 185
🏁 Script executed:
#!/bin/bash # Show the beginning of xml_rpcs.py to inspect imports and class definitions sed -n '1,200p' src/mccann_hub/odoolib/connector/xml_rpcs.py # Locate the parent class XmlRpcConnector definition within the connector directory rg -n "class XmlRpcConnector" -R src/mccann_hub/odoolib/connector # Find where SafeTransport is imported or defined in the connector module rg -n "SafeTransport" -R src/mccann_hub/odoolib/connectorLength of output: 3386
🏁 Script executed:
#!/bin/bash # Locate and inspect xml_rpc.py to verify how XmlRpcConnector handles SSL transports # Show the path to xml_rpc.py fd xml_rpc.py # Print the first 200 lines of xml_rpc.py (class definitions and imports) sed -n '1,200p' src/mccann_hub/odoolib/connector/xml_rpc.py # Print lines 201–400 to see __init__ and make_connection implementations sed -n '201,400p' src/mccann_hub/odoolib/connector/xml_rpc.pyLength of output: 3489
🏁 Script executed:
#!/bin/bash # Inspect xmlrpc.client.SafeTransport implementation to confirm default SSL context behavior python - << 'EOF' import inspect, xmlrpc.client print("=== SafeTransport Source ===") print(inspect.getsource(xmlrpc.client.SafeTransport)) EOFLength of output: 1119
🏁 Script executed:
#!/bin/bash # Inspect http.client.HTTPSConnection to see how it handles SSL contexts and certificate validation python - << 'EOF' import inspect, http.client print("=== HTTPSConnection Source ===") print(inspect.getsource(http.client.HTTPSConnection)) EOFLength of output: 2860
SSL Certificate Validation Confirmed
The default
SafeTransport(with no customcontext) useshttp.client.HTTPSConnection, which creates a default SSL context viassl._create_default_https_context()—this enforcesCERT_REQUIREDand hostname checking. No changes are needed to ensure proper certificate validation.tests/test_xml_rpc.py (2)
52-66: Well-implemented test using mockingThe test effectively mocks the ServerProxy to verify the send method's behavior, checking both the correct URL construction and method invocation.
47-49:Details
❌ Incorrect review comment
Verify the logger name matches the module structure
The test expects the logger name to be "mccann_hub.odoolib.connector.xmlrpc", but based on the import path, it might be "mccann_hub.odoolib.connector.xml_rpc" instead. Verify this matches the actual implementation.
🏁 Script executed:
#!/bin/bash # Check the actual logger name in the XmlRpcConnector class rg -A 2 "_logger =" src/mccann_hub/odoolib/connector/xml_rpc.pyLength of output: 65
🏁 Script executed:
#!/bin/bash # List all Python files in the connector directory echo "Connector files:" fd src/mccann_hub/odoolib/connector -t f -e py # Show any logger initialization in xmlrpc.py echo -e "\nSearching for getLogger calls in xmlrpc.py:" rg -n "getLogger" src/mccann_hub/odoolib/connector/xmlrpc.py # Show any _logger assignments in xmlrpc.py echo -e "\nSearching for '_logger' in xmlrpc.py:" rg -n "_logger" src/mccann_hub/odoolib/connector/xmlrpc.pyLength of output: 1011
🏁 Script executed:
#!/bin/bash set -euo pipefail # Locate xml_rpc.py in the repository xml_file=$(find . -type f -name "xml_rpc.py" | head -n1) echo "Inspecting: $xml_file" # Show any logger initialization echo -e "\n[getLogger calls]" rg -n "getLogger" "$xml_file" || echo "No getLogger calls found in $xml_file" # Show any _logger assignments echo -e "\n[_logger assignments]" rg -n "_logger" "$xml_file" || echo "No _logger assignments found in $xml_file"Length of output: 806
🏁 Script executed:
#!/bin/bash set -euo pipefail # List all Python files in the connector directory echo "Python files in connector:" find src/mccann_hub/odoolib/connector -type f -name "*.py" # Search for a property or method defining _logger echo -e "\nSearching for '_logger' definitions:" rg -n "def _logger" -B1 -A3 src/mccann_hub/odoolib/connector # Search for direct assignments to _logger echo -e "\nSearching for '_logger =' across connector:" rg -n "_logger\s*=" -R src/mccann_hub/odoolib/connector # Search for getLogger usage echo -e "\nSearching for 'logging.getLogger' in connector:" rg -n "logging\.getLogger" -R src/mccann_hub/odoolib/connector # Search for import logging statements echo -e "\nSearching for 'import logging':" rg -n "import logging" -R src/mccann_hub/odoolib/connectorLength of output: 617
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect logger setup in the base connector class file="src/mccann_hub/odoolib/connector/_connector.py" echo "Inspecting: $file" echo -e "\n[getLogger calls]" rg -n "getLogger" "$file" || echo "No getLogger calls found" echo -e "\n[__name__ references]" rg -n "__name__" "$file" || echo "No __name__ references found" echo -e "\n[Entire file head for context]" head -n 200 "$file"Length of output: 3172
Ignore the logger name suggestion
TheConnectorbase class builds its logger name by joining__name__(minus the module’s last segment) withPROTOCOL. Inxml_rpc.py,PROTOCOL = "xmlrpc", so the resulting logger name is"mccann_hub.odoolib.connector.xmlrpc", exactly as asserted in the test. No changes are necessary.Likely an incorrect or invalid review comment.
src/mccann_hub/odoolib/connector/_sender.py (1)
44-51: 🛠️ Refactor suggestionAdd error handling for asyncio loop detection
The
async_sendmethod usesasyncio.get_running_loop()which will raise aRuntimeErrorif no event loop is running. Consider adding error handling to create a new loop if none exists.async def async_send(self, service_name: str, method: str, *args) -> Any: """ Async wrapper around `send` to allow non-blocking execution. """ - loop = asyncio.get_running_loop() + try: + loop = asyncio.get_running_loop() + except RuntimeError: + loop = asyncio.new_event_loop() + asyncio.set_event_loop(loop) return await loop.run_in_executor( None, lambda: self.send(service_name, method, *args) )Likely an incorrect or invalid review comment.
tests/test_json_rpc.py (2)
48-50: LGTM - Logger name assertion correctly matches expected pattern.The test correctly verifies that the logger name follows the expected pattern, which is derived from the package path and the connector's
PROTOCOLattribute.
52-78: Good test coverage for the JSON-RPC request/response flow.The test thoroughly validates the JSON-RPC request structure, including verifying the correct fields in the request payload, proper randomization of the ID field, and correct extraction of results from the response.
.github/workflows/test-publish.yml (1)
62-66: Good approach using multiple index URLs.Using both TestPyPI and PyPI as sources ensures dependencies that might not be available on TestPyPI can still be resolved from the main PyPI repository.
src/mccann_hub/odoolib/connector/_connector.py (1)
54-60: Good implementation of the service factory method.The
get_servicemethod provides a clean interface for obtaining aServiceinstance bound to this connector, promoting good separation of concerns.tests/test_service.py (1)
1-71: The Service test implementation looks complete and well-structuredThe test coverage for the Service class is thorough, testing initialization, synchronous method calls, method caching, and method reflection capabilities.
tests/test_json_rpcs.py (1)
1-92: The JsonRpcsConnector test implementation is comprehensiveThe test suite properly verifies initialization, successful requests, and error handling. Good job ensuring the JSON-RPC request structure is validated, including checking the proper format of parameters and handling of error responses.
tests/test.py (5)
40-40: LGTM: Updated import statement aligns with new package structure.The import statement has been properly updated to use the new namespace structure.
48-55: LGTM: Improved method formatting.The
_connmethod has been nicely reformatted with consistent indentation and parameter alignment, enhancing readability.
58-58: LGTM: Added helpful docstring.Adding a descriptive docstring to the
_get_protocolsmethod improves code documentation.
73-76: LGTM: Enhanced test assertions.The test_user_context method has been improved with more specific assertions that validate the presence of expected keys in the context dictionary.
89-89: LGTM: Consistent string quotes.Updated to use double quotes in the main guard, which is more consistent with Python style conventions.
src/mccann_hub/odoolib/service.py (2)
38-51: LGTM: Well-structured Service class initialization.The Service class is well-designed with proper constructor parameters, docstrings, and type hints. The logger initialization with a service-specific name is a good practice for debugging.
52-78: LGTM: Elegant implementation of dynamic method proxying.The
__getattr__method provides an elegant solution for dynamically creating proxy functions. The implementation:
- Supports both synchronous and asynchronous calls
- Includes detailed logging for debugging
- Attaches the async proxy to the sync proxy for a clean API
This design makes the service easy to use while maintaining flexibility.
test_containers/postgres/Dockerfile (3)
1-5: Warning: This is a generated file that should not be edited directly.The comments at the top indicate this Dockerfile is generated via "apply-templates.sh" and should not be edited directly. Any manual changes might be overwritten when the template is reapplied.
Consider documenting the template source and generation process in your project documentation to ensure future maintenance follows the correct workflow.
189-190: LGTM: Intentional VOLUME directive commenting.Good decision to comment out the VOLUME directive to enable database snapshots. This allows for more flexible container usage in testing environments.
196-219: LGTM: Well-documented signal handling.Excellent documentation explaining the choice of SIGINT as the STOPSIGNAL and providing references to PostgreSQL documentation. This helps future maintainers understand the rationale behind this configuration.
tests/test_connection.py (4)
37-46: LGTM: Well-structured test setup.The test class follows good practices with a proper setUp method that creates a mock connector and initializes the Connection with test parameters.
47-52: LGTM: Thorough initialization testing.The test_initialization method properly verifies that the Connection constructor correctly sets the instance properties.
53-63: LGTM: Effective context retrieval testing.The test_get_user_context method:
- Properly mocks the model return value
- Verifies the context is retrieved correctly
- Confirms the context is cached in the connection
- Validates that the correct method was called with the right parameters
This comprehensive testing ensures the get_user_context method works as expected.
64-68: LGTM: Good model instantiation testing.The test_get_model method validates that the get_model method returns the correct Model instance with the proper model name and connection reference.
test_containers/postgres/docker-ensure-initdb.sh (3)
1-15: Well-structured script header with clear documentation.The script header clearly explains the three main use cases for this script, which is excellent for maintainability and future developers.
33-53: Sound initialization logic with appropriate error checking.The script properly checks for an empty database directory before initializing, verifies minimum environment variables, checks directory permissions, and sets up PostgreSQL authentication correctly. The script also exports PGPASSWORD when needed for authentication.
54-71: Smart script self-identification and error handling.The script intelligently determines its own filename and changes behavior based on whether it's called as
docker-ensure-initdb.sh(no-op if database exists) ordocker-enforce-initdb.sh(error if database exists), which aligns well with the stated use cases in the header.tests/test_model.py (3)
35-43: Well-structured test setup with appropriate mocking.The test class properly sets up a mock connection with all necessary attributes required by the Model class. Good use of MagicMock for simulating the execute_kw method.
49-67: Good coverage of proxy method call scenarios.The tests comprehensively cover different proxy method call scenarios, including single ID, multiple IDs, and the case where an ID is not found in the result set.
69-78: Thorough testing of search_read functionality.The tests properly verify the search_read functionality, including the case where no results are found. Using MagicMock for the search and read methods is a good approach.
tests/test_async.py (4)
34-50: Well-structured async test class setup.The test class properly uses unittest.IsolatedAsyncioTestCase for async tests and provides helper methods for connection creation and protocol selection, which promotes code reuse across tests.
51-59: Good basic async read test.The test properly verifies async read functionality across different protocols and includes appropriate assertions to validate the result.
60-68: Improved user context test with proper assertions.The test for user context retrieval now includes assertions to verify both the structure of the context (it's a dictionary) and its expected content (contains 'lang' and 'tz' keys).
70-83: Comprehensive async search and count test.The test verifies that the async search and count methods work correctly by comparing the count result with the length of search results, providing good validation of these core async methods.
src/mccann_hub/odoolib/connector/xml_rpc.py (1)
39-65: Well-documented connector class with clear initialization.The XmlRpcConnector class is properly documented with clear parameter descriptions. The URL construction logic correctly handles both with and without version paths.
src/mccann_hub/odoolib/connection/_servicer.py (2)
41-77: Code looks good - well-organized class structure for authentication.The
Servicerclass is well-designed with clear separation of concerns. It properly handles authentication parameters and provides flexible login configuration.
131-139: Effective service proxy implementation.The
get_servicemethod properly delegates to the connector for creating service proxies while maintaining clean abstraction.setup.py (2)
42-44: LGTM - Good package organization using namespace packages.The use of
find_namespace_packageswith thewhere="src"parameter is a good modern practice for Python packaging.
52-61: Python version classifiers aligned with requirements.The classifiers correctly specify Python 3.9 through 3.13 compatibility, matching the
python_requires=">=3.9"requirement.test_containers/postgres/docker-entrypoint.sh (2)
1-25: Well-implemented file_env function for handling environment variables.The
file_envfunction properly handles reading environment variables from files, which is a good security practice for Docker containers, especially when working with secrets.
304-355: Main function is well-structured.The
_mainfunction follows a logical flow for initializing and configuring PostgreSQL within a Docker container. It properly handles different scenarios and delegates to specialized functions.🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 316-316: Expanding an array without an index only gives the first element.
(SC2128)
| retries: 5 | ||
|
|
||
| db: | ||
| image: ghcr.io/jabez007/odoo-client-lib |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Specify a specific version tag for database image
Using an image without a specific version tag can lead to unexpected changes when the image is updated. Consider using a specific version tag for better reproducibility.
- image: ghcr.io/jabez007/odoo-client-lib
+ image: ghcr.io/jabez007/odoo-client-lib:1.0.0📝 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.
| image: ghcr.io/jabez007/odoo-client-lib | |
| image: ghcr.io/jabez007/odoo-client-lib:1.0.0 |
| class Sender(object): | ||
| """ | ||
| The base abstract class for sending RPC requests | ||
| """ | ||
|
|
||
| def send(self, service_name: str, method: str, *args) -> Any: | ||
| """ | ||
| stub out method for children to override | ||
| """ | ||
| return {"service_name": service_name, "method": method, "args": args} | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve the base class implementation
The current send method returns a dictionary rather than raising NotImplementedError. Since this is meant to be an abstract method, it would be better to either:
- Make it a proper abstract method, or
- Raise
NotImplementedErrorto prevent accidental usage of the base implementation
from typing import Any
+from abc import ABC, abstractmethod
-class Sender(object):
+class Sender(ABC):
"""
The base abstract class for sending RPC requests
"""
+ @abstractmethod
def send(self, service_name: str, method: str, *args) -> Any:
"""
- stub out method for children to override
+ Send a request to the specified service and method with the given arguments.
+
+ :param service_name: The name of the service to call
+ :param method: The method name to call on the service
+ :param args: Additional arguments to pass to the method
+ :return: The result of the method call
"""
- return {"service_name": service_name, "method": method, "args": args}
+ raise NotImplementedError("Subclasses must implement send()")📝 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.
| class Sender(object): | |
| """ | |
| The base abstract class for sending RPC requests | |
| """ | |
| def send(self, service_name: str, method: str, *args) -> Any: | |
| """ | |
| stub out method for children to override | |
| """ | |
| return {"service_name": service_name, "method": method, "args": args} | |
| from typing import Any | |
| from abc import ABC, abstractmethod | |
| class Sender(ABC): | |
| """ | |
| The base abstract class for sending RPC requests | |
| """ | |
| @abstractmethod | |
| def send(self, service_name: str, method: str, *args) -> Any: | |
| """ | |
| Send a request to the specified service and method with the given arguments. | |
| :param service_name: The name of the service to call | |
| :param method: The method name to call on the service | |
| :param args: Additional arguments to pass to the method | |
| :return: The result of the method call | |
| """ | |
| raise NotImplementedError("Subclasses must implement send()") |
| # process initializer files, based on file extensions and permissions | ||
| docker_process_init_files() { | ||
| # psql here for backwards compatibility "${psql[@]}" | ||
| psql=( docker_process_sql ) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Unused variable warning.
The psql variable appears to be declared but not used directly (detected by Shellcheck). If it's used externally, consider adding a comment or exporting it.
🏁 Script executed:
#!/bin/bash
# Check if `psql` array is used elsewhere in the script
grep -n "psql\[" test_containers/postgres/docker-entrypoint.shLength of output: 122
Remove or properly expose the unused psql array
The assignment
psql=( docker_process_sql )in test_containers/postgres/docker-entrypoint.sh (lines 162–163) is never actually invoked—the only occurrence of psql[ is in a comment. Since Bash arrays aren’t exported, this declaration is effectively dead code and triggers a Shellcheck warning. You should either:
• Remove the psql assignment (and related comment) if it’s no longer needed.
• Or, if you really need it for backwards-compatibility, explicitly invoke ${psql[@]} where required or export the function instead (e.g. export -f docker_process_sql).
[test_containers/postgres/docker-entrypoint.sh:162–163]
🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 163-163: psql appears unused. Verify use (or export if used externally).
(SC2034)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
.github/workflows/test.yml (1)
29-33: Upgradeactions/setup-pythonto v5
The action version@v4is outdated and may not support the latest runners. Please bump to@v5:- - name: Setup Python - uses: actions/setup-python@v4 + - name: Setup Python + uses: actions/setup-python@v5 with: python-version: ${{ matrix.python_version }}🧰 Tools
🪛 actionlint (1.7.4)
30-30: the runner of "actions/setup-python@v4" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🧹 Nitpick comments (3)
.github/workflows/test.yml (3)
3-8: Expand workflow triggers to include pull requests
Currently the workflow only runs on direct pushes (excludingmain/master). It’s recommended to also trigger onpull_requestevents so CI runs on external contributor PRs:on: - push: - branches-ignore: - - main - - master + push: + branches-ignore: + - main + - master + pull_request: + branches-ignore: + - main + - master
25-28: Consider using GitHub Actionsservicesfor containers
Instead of manually invokingdocker compose, you can leverage theserviceskey under the job to spin up PostgreSQL and Odoo. This simplifies networking and readiness handling:jobs: run_tests: runs-on: ubuntu-latest services: postgres: image: postgres:13 ports: - 5432:5432 odoo: image: odoo:15 ports: - 8069:8069 steps: - name: Checkout code uses: actions/checkout@v4 # …omit manual docker compose step…
34-37: Cache pip dependencies to speed up CI
Useactions/cacheto reuse the pip cache across runs, reducing install time:- - name: Install requirements - run: | - pip install -r requirements.txt + - name: Cache pip + uses: actions/cache@v3 + with: + path: ~/.cache/pip + key: ${{ runner.os }}-pip-${{ hashFiles('requirements.txt') }} + - name: Install requirements + run: pip install -r requirements.txt
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.github/workflows/publish.yml(1 hunks).github/workflows/test.yml(1 hunks)pyproject.toml(1 hunks)setup.py(2 hunks)src/mccann_hub/odoolib/connector/json_rpc.py(1 hunks)src/mccann_hub/odoolib/main.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/mccann_hub/odoolib/main.py
🚧 Files skipped from review as they are similar to previous changes (4)
- .github/workflows/publish.yml
- src/mccann_hub/odoolib/connector/json_rpc.py
- pyproject.toml
- setup.py
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/test.yml
30-30: the runner of "actions/setup-python@v4" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🔇 Additional comments (1)
.github/workflows/test.yml (1)
38-41: Verify test discovery invocation
ThePYTHONPATH=src python -m unittest discover -s testsstep correctly locates and runs your unit tests. No changes needed here.
1 participant
Summary by CodeRabbit
New Features
Tests
Chores
pyproject.toml, enhancedsetup.py, and newrequirements.txt..gitignorecovering Python, packaging, testing, and development artifacts.