-
Notifications
You must be signed in to change notification settings - Fork 466
solving issue #1978 neo4j max idle time #2174
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1 issue found across 4 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="cartography/cli.py">
<violation number="1" location="cartography/cli.py:997">
P2: Using `not config.neo4j_max_connection_idle_time` will incorrectly allow the environment variable to override an explicit CLI value of `0`. Use `is None` to properly check if the CLI argument was not provided.</violation>
</file>
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
cartography/cli.py
Outdated
| # TODO support parameter lookup in environment variables if not present on command line | ||
| config: argparse.Namespace = self.parser.parse_args(argv) | ||
|
|
||
| if not config.neo4j_max_connection_idle_time and os.environ.get( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P2: Using not config.neo4j_max_connection_idle_time will incorrectly allow the environment variable to override an explicit CLI value of 0. Use is None to properly check if the CLI argument was not provided.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At cartography/cli.py, line 997:
<comment>Using `not config.neo4j_max_connection_idle_time` will incorrectly allow the environment variable to override an explicit CLI value of `0`. Use `is None` to properly check if the CLI argument was not provided.</comment>
<file context>
@@ -984,6 +993,14 @@ def main(self, argv: str) -> int:
# TODO support parameter lookup in environment variables if not present on command line
config: argparse.Namespace = self.parser.parse_args(argv)
+
+ if not config.neo4j_max_connection_idle_time and os.environ.get(
+ "NEO4J_MAX_CONNECTION_IDLE_TIME"
+ ):
</file context>
| if not config.neo4j_max_connection_idle_time and os.environ.get( | |
| if config.neo4j_max_connection_idle_time is None and os.environ.get( |
✅ Addressed in bb8f9d4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
3 issues found across 2 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="tests/unit/cartography/test_neo4j_connection.py">
<violation number="1" location="tests/unit/cartography/test_neo4j_connection.py:23">
P2: Misleading assertion: the test name and config parameter reference 'idle_time' but the assertion checks for `max_connection_lifetime`. These are semantically different Neo4j driver parameters. If the intent is to use `max_connection_lifetime`, consider renaming the config parameter and test to reflect this (e.g., `neo4j_max_connection_lifetime`). If `max_connection_idle_time` is the intended parameter, the implementation in sync.py and this assertion should be updated.</violation>
</file>
<file name="cartography/sync.py">
<violation number="1" location="cartography/sync.py:254">
P1: Regression: `neo4j_max_connection_lifetime` config is no longer passed to the driver. The original code passed both `max_connection_lifetime` and `max_connection_idle_time` parameters. Consider also conditionally adding `max_connection_lifetime` from `config.neo4j_max_connection_lifetime` to preserve existing functionality.</violation>
<violation number="2" location="cartography/sync.py:256">
P0: Wrong Neo4j driver parameter name: `max_connection_lifetime` is used instead of `max_connection_idle_time`. The config variable is `neo4j_max_connection_idle_time` and the PR explicitly aims to add support for `max_connection_idle_time`, but this code incorrectly sets `max_connection_lifetime`.</violation>
</file>
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
| # Assert | ||
| mock_driver.assert_called_once() | ||
| args, kwargs = mock_driver.call_args | ||
| assert "max_connection_lifetime" in kwargs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P2: Misleading assertion: the test name and config parameter reference 'idle_time' but the assertion checks for max_connection_lifetime. These are semantically different Neo4j driver parameters. If the intent is to use max_connection_lifetime, consider renaming the config parameter and test to reflect this (e.g., neo4j_max_connection_lifetime). If max_connection_idle_time is the intended parameter, the implementation in sync.py and this assertion should be updated.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/unit/cartography/test_neo4j_connection.py, line 23:
<comment>Misleading assertion: the test name and config parameter reference 'idle_time' but the assertion checks for `max_connection_lifetime`. These are semantically different Neo4j driver parameters. If the intent is to use `max_connection_lifetime`, consider renaming the config parameter and test to reflect this (e.g., `neo4j_max_connection_lifetime`). If `max_connection_idle_time` is the intended parameter, the implementation in sync.py and this assertion should be updated.</comment>
<file context>
@@ -20,5 +20,5 @@ def test_neo4j_driver_init_with_idle_time(mock_driver):
args, kwargs = mock_driver.call_args
- assert "max_connection_idle_time" in kwargs
- assert kwargs["max_connection_idle_time"] == 120
+ assert "max_connection_lifetime" in kwargs
+ assert kwargs["max_connection_lifetime"] == 120
</file context>
| if config.neo4j_user or config.neo4j_password: | ||
| neo4j_auth = (config.neo4j_user, config.neo4j_password) | ||
| try: | ||
| args = {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P1: Regression: neo4j_max_connection_lifetime config is no longer passed to the driver. The original code passed both max_connection_lifetime and max_connection_idle_time parameters. Consider also conditionally adding max_connection_lifetime from config.neo4j_max_connection_lifetime to preserve existing functionality.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At cartography/sync.py, line 254:
<comment>Regression: `neo4j_max_connection_lifetime` config is no longer passed to the driver. The original code passed both `max_connection_lifetime` and `max_connection_idle_time` parameters. Consider also conditionally adding `max_connection_lifetime` from `config.neo4j_max_connection_lifetime` to preserve existing functionality.</comment>
<file context>
@@ -251,11 +251,14 @@ def run_with_config(sync: Sync, config: Union[Config, argparse.Namespace]) -> in
if config.neo4j_user or config.neo4j_password:
neo4j_auth = (config.neo4j_user, config.neo4j_password)
try:
+ args = {}
+ if config.neo4j_max_connection_idle_time:
+ args["max_connection_lifetime"] = config.neo4j_max_connection_idle_time
</file context>
✅ Addressed in 64716d0
| try: | ||
| args = {} | ||
| if config.neo4j_max_connection_idle_time: | ||
| args["max_connection_lifetime"] = config.neo4j_max_connection_idle_time |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P0: Wrong Neo4j driver parameter name: max_connection_lifetime is used instead of max_connection_idle_time. The config variable is neo4j_max_connection_idle_time and the PR explicitly aims to add support for max_connection_idle_time, but this code incorrectly sets max_connection_lifetime.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At cartography/sync.py, line 256:
<comment>Wrong Neo4j driver parameter name: `max_connection_lifetime` is used instead of `max_connection_idle_time`. The config variable is `neo4j_max_connection_idle_time` and the PR explicitly aims to add support for `max_connection_idle_time`, but this code incorrectly sets `max_connection_lifetime`.</comment>
<file context>
@@ -251,11 +251,14 @@ def run_with_config(sync: Sync, config: Union[Config, argparse.Namespace]) -> in
try:
+ args = {}
+ if config.neo4j_max_connection_idle_time:
+ args["max_connection_lifetime"] = config.neo4j_max_connection_idle_time
+
neo4j_driver = GraphDatabase.driver(
</file context>
| args["max_connection_lifetime"] = config.neo4j_max_connection_idle_time | |
| args["max_connection_idle_time"] = config.neo4j_max_connection_idle_time |
✅ Addressed in 64716d0
Signed-off-by: IlyaasK <[email protected]>
Signed-off-by: IlyaasK <[email protected]>
Signed-off-by: IlyaasK <[email protected]>
Summary
Related issues or links
max_connection_idle_timein Neo4j driver #1978Checklist
Provide proof that this works (this makes reviews move faster). Please perform one or more of the following:
Verification:
Added a new unit test tests/unit/cartography/test_neo4j_connection.py which mocks the
neo4j.GraphDatabase.driverand asserts that themax_connection_idle_timeparameter is correctly passed when run_with_config is executed.If you are changing a node or relationship:
If you are implementing a new intel module:
Detailed Changes
neo4j_max_connection_idle_timeas a documented field.Config.__init__to accept and store theneo4j_max_connection_idle_timeparameter.--neo4j-max-connection-idle-timeargument to theargparsedefinition.NEO4J_MAX_CONNECTION_IDLE_TIMEenvironment variable if the CLI argument is not provided.max_connection_idle_timeparameter to theGraphDatabase.driverinitialization.GraphDatabase.driveris initialized with the correctmax_connection_idle_timevalue when configured.