Skip to content

fix(falkordb): guard clone() against empty graph names#1652

Open
kramerica-inc-dev wants to merge 1 commit into
getzep:mainfrom
kramerica-inc-dev:fix/falkordb-empty-graph-name
Open

fix(falkordb): guard clone() against empty graph names#1652
kramerica-inc-dev wants to merge 1 commit into
getzep:mainfrom
kramerica-inc-dev:fix/falkordb-empty-graph-name

Conversation

@kramerica-inc-dev

Copy link
Copy Markdown

Summary

Fixes #1650: an empty group_id crashes the FalkorDB driver with the misleading TypeError Expected a string parameter, but received <class 'str'>.

Mechanism: validate_group_id() deliberately lets '' through — the empty string is the default case for other providers — so the guard belongs in the FalkorDB driver, not in the validator. The empty string flows into FalkorDriver.clone(database=''), which builds a driver bound to an empty graph name. Because select_graph() is lazy, nothing fails at clone time; the crash only surfaces on the first query on the clone, when falkordb-py's select_graph('') raises the TypeError above (misleading, since the value is a string — reported by us as FalkorDB/falkordb-py#244).

The fix is minimal and local to falkordb_driver.py:

Production context: in our deployment this was triggered by a replayed queue entry without a group_id; the delayed crash (first query, not clone time) made it hard to trace back to the empty group_id.

Relation to #1572: complementary, not conflicting. That PR memoizes clone(); this PR normalizes the database argument before the existing branch chain. If both land, the falsy guard simply runs before (or inside) the memoized lookup.

We are the reporters of both #1650 and FalkorDB/falkordb-py#244.

Type of Change

  • Bug fix
  • New feature
  • Performance improvement
  • Documentation/Tests

Objective

N/A (bug fix).

Testing

  • Unit tests added/updated
  • Integration tests added/updated
  • All existing tests pass

New tests in tests/driver/test_falkordb_driver.py (existing mock pattern, skipped when falkordb is not installed):

  • test_clone_with_empty_database_falls_back_to_current_databaseclone('') returns the source driver with its database intact.
  • test_clone_with_empty_database_never_selects_empty_graph — a query on a clone('') driver never calls select_graph('') (asserted on the mock client).
  • test_clone_with_database_creates_new_driverclone('group') behavior unchanged.
  • test_clone_with_default_group_id_uses_default_database — the default_group_id ('_') branch stays intact.
  • test_get_graph_with_empty_name_defaults_to_default_database_get_graph('') falls back to the default database.

ruff format --check / ruff check pass; pytest tests/driver/test_falkordb_driver.py -m "not integration": 30 passed, 1 skipped. The three empty-name tests fail on unpatched main, confirming they cover the bug.

Breaking Changes

  • This PR contains breaking changes

No. clone('') previously produced a driver that was guaranteed to crash on first use, so no working behavior changes.

Checklist

  • Code follows project style guidelines (make lint passes)
  • Self-review completed
  • Documentation updated where necessary
  • No secrets or sensitive information committed

Related Issues

Closes #1650

An empty group_id passes validate_group_id() (empty strings are allowed
as the default case for other providers) and flows into
FalkorDriver.clone(database=''), which builds a driver bound to an empty
graph name. The failure only surfaces on the first query, when
falkordb-py's select_graph('') raises the misleading TypeError
"Expected a string parameter, but received <class 'str'>".

Treat a falsy database in clone() as "use the current database", so
clone('') returns the driver itself, and apply the same falsy guard in
_get_graph() as defense in depth.

Closes getzep#1650
@zep-cla-assistant

Copy link
Copy Markdown
Contributor


Thank you for your submission, we really appreciate it. Like many open-source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution. For privacy information, see our Privacy Notice. You can sign the CLA by just posting a Pull Request Comment same as the below format.


I have read the CLA Document and I hereby sign the CLA behalf on myself, e-mail: example@example.com

or

I have read the CLA Document and I hereby sign the CLA behalf of my company, e-mail: example@example.com

Signature is valid for 6 months.


This bot will be retriggered when the Contributor License Agreement comment has been provided. Posted by the CLA Assistant Lite bot.

@Naseem77

Copy link
Copy Markdown
Contributor

@kramerica-inc-dev Reproduced the confusing TypeError from select_graph('') on both clone('') and _get_graph(''). With your guard they fall back to the default graph and the full driver test suite passes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

FalkorDriver: empty group_id crashes with misleading TypeError "Expected a string parameter, but received <class 'str'>"

2 participants