fix: coerce empty port string to None in create_graph_engine#2486
Open
soichisumi wants to merge 2 commits intotopoteretes:mainfrom
Open
fix: coerce empty port string to None in create_graph_engine#2486soichisumi wants to merge 2 commits intotopoteretes:mainfrom
soichisumi wants to merge 2 commits intotopoteretes:mainfrom
Conversation
When GRAPH_DATABASE_PORT is unset or empty, get_graph_context_config() returns graph_database_port="" (empty string). This is passed directly to community adapters which may call int(port), causing: ValueError: invalid literal for int() with base 10: '' Coerce empty string and None to None before passing to adapters, so they can apply their own default port values.
Please make sure all the checkboxes are checked:
|
|
Hello @soichisumi, thank you for submitting a PR! We will respond as soon as possible. |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Walkthrough
Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cognee/infrastructure/databases/graph/get_graph_engine.py (1)
98-100: Normalizegraph_database_portbefore the cached call to avoid duplicate cache keys.This normalization happens inside
@lru_cachescope, so""andNonestill create separate cache entries even though both map toNonefor adapters.♻️ Proposed refactor
def create_graph_engine( @@ ): + normalized_port = ( + None + if graph_database_port is None + or (isinstance(graph_database_port, str) and graph_database_port.strip() == "") + else graph_database_port + ) return _create_graph_engine( graph_database_provider, graph_file_path, graph_database_url, graph_database_name, graph_database_username, graph_database_password, - graph_database_port, + normalized_port, graph_database_key, graph_dataset_database_handler, ) @@ - # Coerce empty/None port to None so adapters can apply their own defaults - port = graph_database_port if graph_database_port not in ("", None) else None - return adapter( graph_database_url=graph_database_url, graph_database_username=graph_database_username, graph_database_password=graph_database_password, - graph_database_port=port, + graph_database_port=graph_database_port, graph_database_key=graph_database_key, database_name=graph_database_name, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cognee/infrastructure/databases/graph/get_graph_engine.py` around lines 98 - 100, Normalize graph_database_port before the cached call to avoid duplicate lru_cache keys: ensure the variable graph_database_port is coerced (treat "" and None as None) prior to invoking the `@lru_cache-decorated` function or building the cache key so that both "" and None map to the same port value; update the code locations that compute or pass port (the port assignment currently using graph_database_port if graph_database_port not in ("", None) else None) so normalization occurs before the cached lookup in get_graph_engine (and any helper that calls the cached adapter resolution).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@cognee/infrastructure/databases/graph/get_graph_engine.py`:
- Around line 98-100: Normalize graph_database_port before the cached call to
avoid duplicate lru_cache keys: ensure the variable graph_database_port is
coerced (treat "" and None as None) prior to invoking the `@lru_cache-decorated`
function or building the cache key so that both "" and None map to the same port
value; update the code locations that compute or pass port (the port assignment
currently using graph_database_port if graph_database_port not in ("", None)
else None) so normalization occurs before the cached lookup in get_graph_engine
(and any helper that calls the cached adapter resolution).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 84cf6562-bf2f-45ce-a386-e31606b2041b
📒 Files selected for processing (1)
cognee/infrastructure/databases/graph/get_graph_engine.py
Address CodeRabbit feedback: normalize graph_database_port in the uncached create_graph_engine() wrapper instead of inside the @lru_cache-decorated _create_graph_engine(). This prevents "" and None from creating separate cache entries.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
When
GRAPH_DATABASE_PORTis unset or empty,get_graph_context_config()returnsgraph_database_port=""(empty string). This value is passed directly to community adapters (viasupported_databasesdict), which may callint(port), causing:This affects FalkorDB and potentially other community adapters that expect a numeric port.
Fix
Coerce empty string and
NonetoNonebefore passing to adapters in_create_graph_engine(). This lets each adapter apply its own default port value (e.g. FalkorDB defaults to 6379).Impact
supported_databasescode path (community adapters)Noneport gracefullySummary by CodeRabbit