Skip to content

Commit bc8ae33

Browse files
author
longbingljw
committed
spec bug fix and test fix
1 parent cae8cd1 commit bc8ae33

File tree

2 files changed

+140
-48
lines changed

2 files changed

+140
-48
lines changed

‎pyobvector/schema/reflection.py‎

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -143,10 +143,11 @@ def _parse_constraints(self, line):
143143
# do not handle partition
144144
return ret
145145
if tp == "fk_constraint":
146-
if len(spec["table"]) == 2 and spec["table"][0] == self.default_schema:
147-
spec["table"] = spec["table"][1:]
148-
if isinstance(spec, dict) and spec.get("onupdate", "").lower() == "restrict":
149-
spec["onupdate"] = None
150-
if isinstance(spec, dict) and spec.get("ondelete", "").lower() == "restrict":
151-
spec["ondelete"] = None
146+
table = spec.get("table", []) if isinstance(spec, dict) else []
147+
if isinstance(spec, dict) and isinstance(table, list) and len(table) == 2 and table[0] == self.default_schema:
148+
spec["table"] = table[1:]
149+
if isinstance(spec, dict) and (spec.get("onupdate") or "").lower() == "restrict":
150+
spec["onupdate"] = None
151+
if isinstance(spec, dict) and (spec.get("ondelete") or "").lower() == "restrict":
152+
spec["ondelete"] = None
152153
return ret

‎tests/test_reflection.py‎

Lines changed: 133 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,8 @@
33
import logging
44
from unittest.mock import Mock, patch
55
from pyobvector.schema.reflection import OceanBaseTableDefinitionParser
6+
from sqlalchemy.dialects.mysql.reflection import MySQLTableDefinitionParser
7+
import copy # Added for deepcopy
68

79
logger = logging.getLogger(__name__)
810

@@ -37,7 +39,7 @@ def test_dialect(self):
3739
self.engine = create_async_engine(connection_str)
3840

3941
def test_parse_constraints_with_string_spec(self):
40-
# test the logic directly
42+
"""Test that _parse_constraints handles string spec gracefully without crashing."""
4143
from pyobvector.schema.reflection import OceanBaseTableDefinitionParser
4244

4345
# Create a mock parser class to test our specific method
@@ -48,52 +50,141 @@ def __init__(self):
4850

4951
parser = MockParser()
5052

51-
# Test cases for different spec types
53+
# Test cases: we'll mock the parent method to return what we want to test
5254
test_cases = [
53-
# Case 1: spec is a string (this was causing the bug)
54-
("fk_constraint", "some_string_spec"),
55-
# Case 2: spec is a dict with onupdate/ondelete = "restrict"
56-
("fk_constraint", {
57-
"table": ["test", "other_table"],
58-
"onupdate": "restrict",
59-
"ondelete": "restrict"
60-
}),
61-
# Case 3: spec is a dict with onupdate/ondelete = "cascade"
62-
("fk_constraint", {
63-
"table": ["other_table"],
64-
"onupdate": "cascade",
65-
"ondelete": "cascade"
66-
}),
67-
# Case 4: spec is a dict without onupdate/ondelete
68-
("fk_constraint", {
69-
"table": ["other_table"],
70-
"name": "fk_test"
71-
}),
72-
# Case 5: spec is None (edge case)
73-
("fk_constraint", None),
55+
{
56+
"name": "String spec (the bug case)",
57+
"parent_return": ("fk_constraint", "some_string_spec"),
58+
"expected_result": ("fk_constraint", "some_string_spec") # Should remain unchanged
59+
},
60+
{
61+
"name": "Dict spec with restrict values",
62+
"parent_return": ("fk_constraint", {
63+
"table": ["test", "other_table"],
64+
"onupdate": "restrict",
65+
"ondelete": "restrict"
66+
}),
67+
"expected_result": ("fk_constraint", {
68+
"table": ["other_table"], # Should be trimmed
69+
"onupdate": None, # Should be None
70+
"ondelete": None # Should be None
71+
})
72+
},
73+
{
74+
"name": "Dict spec with cascade values",
75+
"parent_return": ("fk_constraint", {
76+
"table": ["other_table"],
77+
"onupdate": "cascade",
78+
"ondelete": "cascade"
79+
}),
80+
"expected_result": ("fk_constraint", {
81+
"table": ["other_table"],
82+
"onupdate": "cascade", # Should remain unchanged
83+
"ondelete": "cascade" # Should remain unchanged
84+
})
85+
},
86+
{
87+
"name": "Dict spec with None values",
88+
"parent_return": ("fk_constraint", {
89+
"table": ["other_table"],
90+
"onupdate": None,
91+
"ondelete": None
92+
}),
93+
"expected_result": ("fk_constraint", {
94+
"table": ["other_table"],
95+
"onupdate": None, # Should remain None
96+
"ondelete": None # Should remain None
97+
})
98+
},
99+
{
100+
"name": "Dict spec without table key",
101+
"parent_return": ("fk_constraint", {
102+
"onupdate": "restrict",
103+
"ondelete": "cascade"
104+
}),
105+
"expected_result": ("fk_constraint", {
106+
"onupdate": None, # Should be None
107+
"ondelete": "cascade" # Should remain unchanged
108+
})
109+
},
110+
{
111+
"name": "Dict spec with single table (no trimming)",
112+
"parent_return": ("fk_constraint", {
113+
"table": ["other_table"],
114+
"onupdate": "restrict"
115+
}),
116+
"expected_result": ("fk_constraint", {
117+
"table": ["other_table"], # Should remain unchanged (only 1 element)
118+
"onupdate": None # Should be None
119+
})
120+
},
121+
{
122+
"name": "Dict spec with empty dict",
123+
"parent_return": ("fk_constraint", {}),
124+
"expected_result": ("fk_constraint", {}) # Should remain unchanged
125+
},
126+
{
127+
"name": "Dict spec with None table",
128+
"parent_return": ("fk_constraint", {
129+
"table": None,
130+
"onupdate": "restrict"
131+
}),
132+
"expected_result": ("fk_constraint", {
133+
"table": None, # Should remain unchanged (not a list)
134+
"onupdate": None # Should be None
135+
})
136+
},
137+
{
138+
"name": "Dict spec with non-list table",
139+
"parent_return": ("fk_constraint", {
140+
"table": "not_a_list",
141+
"ondelete": "restrict"
142+
}),
143+
"expected_result": ("fk_constraint", {
144+
"table": "not_a_list", # Should remain unchanged (not a list)
145+
"ondelete": None # Should be None
146+
})
147+
},
148+
{
149+
"name": "None spec",
150+
"parent_return": ("fk_constraint", None),
151+
"expected_result": ("fk_constraint", None) # Should remain unchanged
152+
},
153+
{
154+
"name": "Non-fk constraint with string spec",
155+
"parent_return": ("unique", "string_spec"),
156+
"expected_result": ("unique", "string_spec") # Should remain unchanged
157+
}
74158
]
75159

76-
for tp, spec in test_cases:
77-
with self.subTest(tp=tp, spec=spec):
78-
# Mock the parent class method to return our test case
79-
with patch.object(parser.__class__.__bases__[0], '_parse_constraints', return_value=(tp, spec)):
80-
# This should not raise an exception
160+
for test_case in test_cases:
161+
with self.subTest(name=test_case["name"]):
162+
# Create a copy of the input to avoid mutation issues
163+
import copy
164+
parent_return = copy.deepcopy(test_case["parent_return"])
165+
expected_result = test_case["expected_result"]
166+
167+
# Mock the parent class method to return our test input
168+
with patch.object(MySQLTableDefinitionParser, '_parse_constraints', return_value=parent_return):
169+
# Call our method - this should apply our bugfix logic
81170
result = parser._parse_constraints("dummy line")
82171

83172
# Verify the result
84-
if result:
85-
result_tp, result_spec = result
86-
self.assertEqual(result_tp, tp)
87-
88-
# If spec was a dict with "restrict" values, they should be None now
89-
if isinstance(spec, dict):
90-
if spec.get("onupdate") == "restrict":
91-
self.assertIsNone(result_spec.get("onupdate"))
92-
if spec.get("ondelete") == "restrict":
93-
self.assertIsNone(result_spec.get("ondelete"))
94-
else:
95-
# If spec was not a dict, it should remain unchanged
96-
self.assertEqual(result_spec, spec)
173+
self.assertIsNotNone(result)
174+
result_tp, result_spec = result
175+
expected_tp, expected_spec = expected_result
176+
177+
self.assertEqual(result_tp, expected_tp)
178+
179+
# For detailed comparison
180+
if isinstance(expected_spec, dict) and isinstance(result_spec, dict):
181+
for key, expected_value in expected_spec.items():
182+
actual_value = result_spec.get(key)
183+
self.assertEqual(actual_value, expected_value,
184+
f"Test '{test_case['name']}': Key '{key}' expected {expected_value}, got {actual_value}")
185+
else:
186+
self.assertEqual(result_spec, expected_spec,
187+
f"Test '{test_case['name']}': Expected {expected_spec}, got {result_spec}")
97188

98189
def test_parse_constraints_string_spec_no_crash(self):
99190
"""Specific test to ensure string spec doesn't cause AttributeError."""
@@ -108,7 +199,7 @@ def __init__(self):
108199
parser = MockParser()
109200

110201
# Mock parent method to return string spec (the problematic case)
111-
with patch.object(parser.__class__.__bases__[0], '_parse_constraints', return_value=("fk_constraint", "string_spec")):
202+
with patch.object(MySQLTableDefinitionParser, '_parse_constraints', return_value=("fk_constraint", "string_spec")):
112203
# This should not raise AttributeError: 'str' object has no attribute 'get'
113204
try:
114205
result = parser._parse_constraints("dummy line")

0 commit comments

Comments
 (0)