change case #108
change case #108
Conversation
|
|
📝 WalkthroughWalkthroughAdds DB-aware length helper properties and a suite of verbose tests to exercise collection/table name validation across boundary, character, position, and generation scenarios. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/unit_tests/test_collection_name_validation.py (1)
63-147: Remove excessive print statements from unit tests.The extensive print statements throughout all test methods create several issues:
- Performance: Print-heavy tests slow down test execution significantly.
- Maintainability: The verbose style makes tests harder to read and maintain.
- Output pollution: Test runners will be flooded with output, obscuring actual failures.
- Redundancy: pytest provides excellent assertion introspection and failure messages; manual print statements duplicate this functionality.
Unit tests should be concise, focusing on assertions rather than verbose logging. If detailed tracing is needed for debugging, consider using pytest's built-in features (
-v,-s,--log-cli-level) or logging framework instead.♻️ Example: Simplified test without print statements
def test_valid_names(self): """Names with allowed characters and length should pass.""" max_len = self._effective_max_length valid_names = [ "a", "A", "0", "collection_1", "MyCollection_123", "A" * max_len, ] - print(f"\n{'=' * 60}") - print(f"测试: test_valid_names - 测试有效名称") - print(f"{'=' * 60}") for name in valid_names: - print(f"\n输入: {repr(name)} (长度: {len(name)})") - try: - _validate_collection_name(name) - print(f"输出: ✅ 验证通过 - 名称有效") - except Exception as e: - print(f"输出: ❌ 验证失败 - {type(e).__name__}: {e}") - raise + _validate_collection_name(name)Apply this pattern to all test methods.
Also applies to: 149-455
🤖 Fix all issues with AI agents
In @tests/unit_tests/test_collection_name_validation.py:
- Around line 30-51: The test hardcodes a DB table name limit (property
_db_table_max_length returning 64) and mixes DB-level constraints into unit
tests; remove DB-aware validation from this unit test by deleting the
_db_table_max_length, _table_prefix_length and _effective_max_length_by_db_limit
properties and change assertions to only test client-level name rules (leave DB
table mapping and creation checks to integration tests), or if you must assert
DB limits here, replace the hardcoded value by importing the authoritative limit
from the implementation (e.g. import _DB_TABLE_MAX_LENGTH from the client code)
and/or update the client-side _validate_collection_name in client_base.py to
enforce that same limit and then assert that behavior in these unit tests;
ensure references to CollectionNames.prefix() remain correct when computing any
derived lengths.
- Around line 149-205: The _validate_collection_name function currently only
enforces the 512-char rule; update it to also enforce the DB table-name limit by
computing len(CollectionNames.prefix()) + len(name) and raising a ValueError if
that sum > 64; ensure the error message clearly states the prefix, name length,
and the 64-char DB limit so tests referencing effective_max_length_by_db_limit
and CollectionNames.table_name behave as expected.
🧹 Nitpick comments (2)
tests/unit_tests/test_collection_name_validation.py (2)
263-413: Good comprehensive coverage, but consider consolidating redundant tests.The new test methods provide thorough edge case coverage. However, some scenarios are already covered by existing tests:
test_underscore_positions,test_numeric_start,test_single_character_types, andtest_mixed_case_and_characterslargely duplicate coverage fromtest_valid_names.test_more_invalid_charactersexpands ontest_invalid_charactersbut tests the same validation rule.Consider consolidating these into the existing test methods or using parameterized tests to reduce code duplication.
♻️ Example: Parameterized test for consolidation
@pytest.mark.parametrize("name,description", [ ("_name", "underscore at start"), ("name_", "underscore at end"), ("0name", "digit at start"), ("123", "only digits"), # ... more cases ]) def test_valid_character_patterns(self, name, description): """Test various valid character patterns.""" _validate_collection_name(name)
64-455: Optional: Clean up f-string formatting issues.Static analysis identified multiple f-strings without placeholders that can be simplified to regular strings, and several
repr()calls that can use the!rconversion flag.While not critical, addressing these would align with Python best practices and satisfy linting rules.
Examples:
- Line 64:
print(f"测试: test_valid_names - 测试有效名称")→print("测试: test_valid_names - 测试有效名称")- Line 67:
print(f"\n输入: {repr(name)} ...")→print(f"\n输入: {name!r} ...")As per coding guidelines, consider running
ruff check --fixto automatically address these formatting issues.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/unit_tests/test_collection_name_validation.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-22T12:44:57.424Z
Learnt from: longbingljw
Repo: oceanbase/pyseekdb PR: 91
File: tests/integration_tests/test_collection_dml.py:36-36
Timestamp: 2025-12-22T12:44:57.424Z
Learning: In tests/integration_tests/test_collection_dml.py and similar test files, using `db_client._server._execute()` to run SQL commands directly (e.g., CREATE TABLE statements) is acceptable for integration tests that need custom table setup with specific vector index configurations.
Applied to files:
tests/unit_tests/test_collection_name_validation.py
🧬 Code graph analysis (1)
tests/unit_tests/test_collection_name_validation.py (3)
src/pyseekdb/client/meta_info.py (3)
CollectionNames(12-41)prefix(39-41)table_name(17-19)src/pyseekdb/client/collection.py (1)
name(63-65)src/pyseekdb/client/client_base.py (1)
_validate_collection_name(82-108)
🪛 Ruff (0.14.10)
tests/unit_tests/test_collection_name_validation.py
64-64: f-string without any placeholders
Remove extraneous f prefix
(F541)
67-67: Use explicit conversion flag
Replace with conversion flag
(RUF010)
70-70: f-string without any placeholders
Remove extraneous f prefix
(F541)
78-78: f-string without any placeholders
Remove extraneous f prefix
(F541)
83-83: Use explicit conversion flag
Replace with conversion flag
(RUF010)
84-84: f-string without any placeholders
Remove extraneous f prefix
(F541)
90-90: String contains ambiguous ， (FULLWIDTH COMMA). Did you mean , (COMMA)?
(RUF001)
96-96: f-string without any placeholders
Remove extraneous f prefix
(F541)
99-99: Use explicit conversion flag
Replace with conversion flag
(RUF010)
100-100: f-string without any placeholders
Remove extraneous f prefix
(F541)
100-100: String contains ambiguous ， (FULLWIDTH COMMA). Did you mean , (COMMA)?
(RUF001)
106-106: String contains ambiguous ， (FULLWIDTH COMMA). Did you mean , (COMMA)?
(RUF001)
114-114: f-string without any placeholders
Remove extraneous f prefix
(F541)
116-116: Use explicit conversion flag
Replace with conversion flag
(RUF010)
117-117: f-string without any placeholders
Remove extraneous f prefix
(F541)
117-117: String contains ambiguous ， (FULLWIDTH COMMA). Did you mean , (COMMA)?
(RUF001)
123-123: String contains ambiguous ， (FULLWIDTH COMMA). Did you mean , (COMMA)?
(RUF001)
136-136: f-string without any placeholders
Remove extraneous f prefix
(F541)
139-139: Use explicit conversion flag
Replace with conversion flag
(RUF010)
140-140: f-string without any placeholders
Remove extraneous f prefix
(F541)
140-140: String contains ambiguous ， (FULLWIDTH COMMA). Did you mean , (COMMA)?
(RUF001)
146-146: String contains ambiguous ， (FULLWIDTH COMMA). Did you mean , (COMMA)?
(RUF001)
152-152: f-string without any placeholders
Remove extraneous f prefix
(F541)
161-161: f-string without any placeholders
Remove extraneous f prefix
(F541)
162-162: Use explicit conversion flag
Replace with conversion flag
(RUF010)
167-167: Comment contains ambiguous （ (FULLWIDTH LEFT PARENTHESIS). Did you mean ( (LEFT PARENTHESIS)?
(RUF003)
167-167: Comment contains ambiguous ） (FULLWIDTH RIGHT PARENTHESIS). Did you mean ) (RIGHT PARENTHESIS)?
(RUF003)
170-170: f-string without any placeholders
Remove extraneous f prefix
(F541)
171-171: Use explicit conversion flag
Replace with conversion flag
(RUF010)
172-172: Use explicit conversion flag
Replace with conversion flag
(RUF010)
173-173: String contains ambiguous ， (FULLWIDTH COMMA). Did you mean , (COMMA)?
(RUF001)
176-176: f-string without any placeholders
Remove extraneous f prefix
(F541)
176-176: String contains ambiguous ， (FULLWIDTH COMMA). Did you mean , (COMMA)?
(RUF001)
181-181: Comment contains ambiguous （ (FULLWIDTH LEFT PARENTHESIS). Did you mean ( (LEFT PARENTHESIS)?
(RUF003)
181-181: Comment contains ambiguous ， (FULLWIDTH COMMA). Did you mean , (COMMA)?
(RUF003)
181-181: Comment contains ambiguous ） (FULLWIDTH RIGHT PARENTHESIS). Did you mean ) (RIGHT PARENTHESIS)?
(RUF003)
185-185: f-string without any placeholders
Remove extraneous f prefix
(F541)
185-185: String contains ambiguous （ (FULLWIDTH LEFT PARENTHESIS). Did you mean ( (LEFT PARENTHESIS)?
(RUF001)
185-185: String contains ambiguous ） (FULLWIDTH RIGHT PARENTHESIS). Did you mean ) (RIGHT PARENTHESIS)?
(RUF001)
186-186: Use explicit conversion flag
Replace with conversion flag
(RUF010)
187-187: Use explicit conversion flag
Replace with conversion flag
(RUF010)
189-189: String contains ambiguous ， (FULLWIDTH COMMA). Did you mean , (COMMA)?
(RUF001)
194-194: String contains ambiguous （ (FULLWIDTH LEFT PARENTHESIS). Did you mean ( (LEFT PARENTHESIS)?
(RUF001)
194-194: String contains ambiguous ） (FULLWIDTH RIGHT PARENTHESIS). Did you mean ) (RIGHT PARENTHESIS)?
(RUF001)
195-195: Do not catch blind exception: Exception
(BLE001)
201-201: f-string without any placeholders
Remove extraneous f prefix
(F541)
201-201: String contains ambiguous ， (FULLWIDTH COMMA). Did you mean , (COMMA)?
(RUF001)
202-202: f-string without any placeholders
Remove extraneous f prefix
(F541)
209-209: f-string without any placeholders
Remove extraneous f prefix
(F541)
218-218: f-string without any placeholders
Remove extraneous f prefix
(F541)
219-219: Use explicit conversion flag
Replace with conversion flag
(RUF010)
229-229: String contains ambiguous ， (FULLWIDTH COMMA). Did you mean , (COMMA)?
(RUF001)
230-230: String contains ambiguous ， (FULLWIDTH COMMA). Did you mean , (COMMA)?
(RUF001)
231-231: String contains ambiguous ， (FULLWIDTH COMMA). Did you mean , (COMMA)?
(RUF001)
240-240: Use explicit conversion flag
Replace with conversion flag
(RUF010)
241-241: Use explicit conversion flag
Replace with conversion flag
(RUF010)
244-244: String contains ambiguous （ (FULLWIDTH LEFT PARENTHESIS). Did you mean ( (LEFT PARENTHESIS)?
(RUF001)
244-244: String contains ambiguous ） (FULLWIDTH RIGHT PARENTHESIS). Did you mean ) (RIGHT PARENTHESIS)?
(RUF001)
249-249: f-string without any placeholders
Remove extraneous f prefix
(F541)
251-251: String contains ambiguous ， (FULLWIDTH COMMA). Did you mean , (COMMA)?
(RUF001)
252-252: String contains ambiguous ， (FULLWIDTH COMMA). Did you mean , (COMMA)?
(RUF001)
266-266: f-string without any placeholders
Remove extraneous f prefix
(F541)
283-283: Use explicit conversion flag
Replace with conversion flag
(RUF010)
284-284: f-string without any placeholders
Remove extraneous f prefix
(F541)
284-284: String contains ambiguous （ (FULLWIDTH LEFT PARENTHESIS). Did you mean ( (LEFT PARENTHESIS)?
(RUF001)
284-284: String contains ambiguous ） (FULLWIDTH RIGHT PARENTHESIS). Did you mean ) (RIGHT PARENTHESIS)?
(RUF001)
287-287: f-string without any placeholders
Remove extraneous f prefix
(F541)
295-295: f-string without any placeholders
Remove extraneous f prefix
(F541)
306-306: Use explicit conversion flag
Replace with conversion flag
(RUF010)
307-307: f-string without any placeholders
Remove extraneous f prefix
(F541)
307-307: String contains ambiguous （ (FULLWIDTH LEFT PARENTHESIS). Did you mean ( (LEFT PARENTHESIS)?
(RUF001)
307-307: String contains ambiguous ） (FULLWIDTH RIGHT PARENTHESIS). Did you mean ) (RIGHT PARENTHESIS)?
(RUF001)
310-310: f-string without any placeholders
Remove extraneous f prefix
(F541)
318-318: f-string without any placeholders
Remove extraneous f prefix
(F541)
334-334: Use explicit conversion flag
Replace with conversion flag
(RUF010)
335-335: f-string without any placeholders
Remove extraneous f prefix
(F541)
338-338: f-string without any placeholders
Remove extraneous f prefix
(F541)
346-346: f-string without any placeholders
Remove extraneous f prefix
(F541)
361-361: Use explicit conversion flag
Replace with conversion flag
(RUF010)
362-362: f-string without any placeholders
Remove extraneous f prefix
(F541)
365-365: f-string without any placeholders
Remove extraneous f prefix
(F541)
373-373: f-string without any placeholders
Remove extraneous f prefix
(F541)
404-404: Use explicit conversion flag
Replace with conversion flag
(RUF010)
405-405: f-string without any placeholders
Remove extraneous f prefix
(F541)
405-405: String contains ambiguous ， (FULLWIDTH COMMA). Did you mean , (COMMA)?
(RUF001)
411-411: String contains ambiguous ， (FULLWIDTH COMMA). Did you mean , (COMMA)?
(RUF001)
417-417: f-string without any placeholders
Remove extraneous f prefix
(F541)
424-424: f-string without any placeholders
Remove extraneous f prefix
(F541)
425-425: Use explicit conversion flag
Replace with conversion flag
(RUF010)
438-438: Use explicit conversion flag
Replace with conversion flag
(RUF010)
439-439: Use explicit conversion flag
Replace with conversion flag
(RUF010)
445-445: Do not catch blind exception: Exception
(BLE001)
| @property | ||
| def _db_table_max_length(self) -> int: | ||
| """ | ||
| Return the database table name maximum length (typically 64 for MySQL). | ||
| """ | ||
| return 64 | ||
|
|
||
| @property | ||
| def _table_prefix_length(self) -> int: | ||
| """ | ||
| Return the length of the collection table prefix (c$v1$). | ||
| """ | ||
| return len(CollectionNames.prefix()) | ||
|
|
||
| @property | ||
| def _effective_max_length_by_db_limit(self) -> int: | ||
| """ | ||
| Calculate effective max length based on DB table name limit minus prefix. | ||
| This is the actual constraint: prefix + name <= DB limit. | ||
| """ | ||
| return self._db_table_max_length - self._table_prefix_length | ||
|
|
There was a problem hiding this comment.
Reconsider hardcoded DB limit and unit test scope.
Two concerns:
- The DB table name limit of 64 is hardcoded. If the actual DB constraint differs (e.g., PostgreSQL allows 63), tests will be misleading.
- These properties introduce DB-level validation concerns into unit tests. Collection name validation at the client level and DB table name constraints are separate concerns that might be better validated in integration tests.
💡 Suggested approach
Consider one of these approaches:
Option 1: Remove DB-aware validation from unit tests and add integration tests that verify the complete flow (name → table name → actual DB creation).
Option 2: If DB limits must be checked at the client level, update _validate_collection_name in src/pyseekdb/client/client_base.py to enforce the DB limit, then test that behavior here.
Option 3: Source the DB limit from the actual implementation rather than hardcoding:
@property
def _db_table_max_length(self) -> int:
"""
Return the database table name maximum length.
This should match the actual DB being used.
"""
# Source from actual DB configuration or implementation
from pyseekdb.client.client_base import _DB_TABLE_MAX_LENGTH
return _DB_TABLE_MAX_LENGTH🤖 Prompt for AI Agents
In @tests/unit_tests/test_collection_name_validation.py around lines 30 - 51,
The test hardcodes a DB table name limit (property _db_table_max_length
returning 64) and mixes DB-level constraints into unit tests; remove DB-aware
validation from this unit test by deleting the _db_table_max_length,
_table_prefix_length and _effective_max_length_by_db_limit properties and change
assertions to only test client-level name rules (leave DB table mapping and
creation checks to integration tests), or if you must assert DB limits here,
replace the hardcoded value by importing the authoritative limit from the
implementation (e.g. import _DB_TABLE_MAX_LENGTH from the client code) and/or
update the client-side _validate_collection_name in client_base.py to enforce
that same limit and then assert that behavior in these unit tests; ensure
references to CollectionNames.prefix() remain correct when computing any derived
lengths.
| def test_effective_max_length_by_db_limit(self): | ||
| """Test that collection names respect DB table name limit when combined with prefix.""" | ||
| print(f"\n{'=' * 60}") | ||
| print(f"测试: test_effective_max_length_by_db_limit - 测试基于DB限制的有效最大长度") | ||
| print(f"{'=' * 60}") | ||
|
|
||
| prefix = CollectionNames.prefix() | ||
| prefix_len = len(prefix) | ||
| db_limit = self._db_table_max_length | ||
| effective_max = self._effective_max_length_by_db_limit | ||
| max_len = self._effective_max_length | ||
|
|
||
| print(f"\n配置信息:") | ||
| print(f" 前缀: {repr(prefix)} (长度: {prefix_len})") | ||
| print(f" DB表名最大长度: {db_limit}") | ||
| print(f" 有效最大长度: {effective_max} (计算: {db_limit} - {prefix_len})") | ||
| print(f" 当前验证函数的最大长度限制: {max_len}") | ||
|
|
||
| # 测试正好等于有效最大长度（应该通过） | ||
| exact_max_name = "a" * effective_max | ||
| table_name = CollectionNames.table_name(exact_max_name) | ||
| print(f"\n测试1: 正好等于有效最大长度") | ||
| print(f" 输入: {repr(exact_max_name)} (长度: {len(exact_max_name)})") | ||
| print(f" 生成的表名: {repr(table_name)} (长度: {len(table_name)})") | ||
| print(f" 预期: 验证通过，表名长度 {len(table_name)} <= {db_limit}") | ||
| try: | ||
| _validate_collection_name(exact_max_name) | ||
| print(f" 输出: ✅ 验证通过 - 名称有效，表名长度符合DB限制") | ||
| except Exception as e: | ||
| print(f" 输出: ❌ 验证失败 - {type(e).__name__}: {e}") | ||
| raise | ||
|
|
||
| # 测试超过有效最大长度（应该失败，因为表名会超过DB限制） | ||
| over_max_name = "a" * (effective_max + 1) | ||
| table_name_over = CollectionNames.table_name(over_max_name) | ||
| table_len_over = len(table_name_over) | ||
| print(f"\n测试2: 超过有效最大长度（基于DB限制）") | ||
| print(f" 输入名称: {repr(over_max_name[:50])}... (长度: {len(over_max_name)})") | ||
| print(f" 生成的表名: {repr(table_name_over[:50])}... (长度: {table_len_over})") | ||
| print(f" 表名长度计算: {prefix_len} + {len(over_max_name)} = {table_len_over}") | ||
| print(f" 预期: 表名长度 {table_len_over} > {db_limit}，应该被拒绝") | ||
|
|
||
| # 验证名称本身 | ||
| try: | ||
| _validate_collection_name(over_max_name) | ||
| print(f" 名称验证: ✅ 通过（当前验证函数只检查{max_len}的限制）") | ||
| except Exception as e: | ||
| print(f" 名称验证: ❌ 失败 - {type(e).__name__}: {e}") | ||
|
|
||
| # 检查表名长度 | ||
| if table_len_over > db_limit: | ||
| print(f" 表名长度验证: ❌ 失败 - 表名长度 {table_len_over} > DB限制 {db_limit}") | ||
| print(f" 输出: ⚠️ 表名长度超过DB限制，但当前验证函数可能不会检查此限制") | ||
| print(f" 提示: 建议在_validate_collection_name中添加基于DB限制的检查") | ||
| else: | ||
| print(f" 表名长度验证: ✅ 通过 - 表名长度 {table_len_over} <= DB限制 {db_limit}") | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for DB limit checks or table name validation in the codebase
echo "=== Searching for DB limit constants or checks ==="
rg -n -C3 --type=py 'table.*name.*length|TABLE.*MAX|_db.*limit|MAX.*TABLE|db_limit|table_max'
echo ""
echo "=== Searching for prefix length validation ==="
rg -n -C3 --type=py 'prefix.*len|CollectionNames.*prefix' | head -100
echo ""
echo "=== Looking for _validate_collection_name implementation ==="
rg -n -B2 -A15 'def _validate_collection_name' --type=pyRepository: oceanbase/pyseekdb
Length of output: 20248
🏁 Script executed:
# Get the rest of _validate_collection_name implementation
cat -n src/pyseekdb/client/client_base.py | sed -n '82,120p'Repository: oceanbase/pyseekdb
Length of output: 1780
🏁 Script executed:
# Check the _MAX_COLLECTION_NAME_LENGTH constant and CollectionNames.prefix()
rg -n '_MAX_COLLECTION_NAME_LENGTH|class CollectionNames' --type=py -A20 | head -80Repository: oceanbase/pyseekdb
Length of output: 5910
🏁 Script executed:
# Verify the actual prefix value and double-check the constants
cat -n src/pyseekdb/client/meta_info.py | sed -n '12,35p'Repository: oceanbase/pyseekdb
Length of output: 1087
🏁 Script executed:
# Check if there's a prefix() method
rg -n 'def prefix' src/pyseekdb/client/meta_info.pyRepository: oceanbase/pyseekdb
Length of output: 89
🏁 Script executed:
# Get the prefix() method implementation
cat -n src/pyseekdb/client/meta_info.py | sed -n '39,42p'Repository: oceanbase/pyseekdb
Length of output: 193
Update _validate_collection_name to enforce DB table name limits.
The validator currently allows collection names up to 512 characters but doesn't enforce the constraint that prefix + name ≤ 64 (the DB table name limit). This means names passing validation may fail when creating tables in the database.
With a 5-character prefix (c$v1$), the effective maximum should be 59 characters, not 512. Update the validator in src/pyseekdb/client/client_base.py to check:
if len(CollectionNames.prefix()) + len(name) > 64:
raise ValueError(...)The tests at lines 149–205, 206–261, and 414–455 document this gap and explicitly recommend adding DB limit checks to the validation function.
🧰 Tools
🪛 Ruff (0.14.10)
152-152: f-string without any placeholders
Remove extraneous f prefix
(F541)
161-161: f-string without any placeholders
Remove extraneous f prefix
(F541)
162-162: Use explicit conversion flag
Replace with conversion flag
(RUF010)
167-167: Comment contains ambiguous （ (FULLWIDTH LEFT PARENTHESIS). Did you mean ( (LEFT PARENTHESIS)?
(RUF003)
167-167: Comment contains ambiguous ） (FULLWIDTH RIGHT PARENTHESIS). Did you mean ) (RIGHT PARENTHESIS)?
(RUF003)
170-170: f-string without any placeholders
Remove extraneous f prefix
(F541)
171-171: Use explicit conversion flag
Replace with conversion flag
(RUF010)
172-172: Use explicit conversion flag
Replace with conversion flag
(RUF010)
173-173: String contains ambiguous ， (FULLWIDTH COMMA). Did you mean , (COMMA)?
(RUF001)
176-176: f-string without any placeholders
Remove extraneous f prefix
(F541)
176-176: String contains ambiguous ， (FULLWIDTH COMMA). Did you mean , (COMMA)?
(RUF001)
181-181: Comment contains ambiguous （ (FULLWIDTH LEFT PARENTHESIS). Did you mean ( (LEFT PARENTHESIS)?
(RUF003)
181-181: Comment contains ambiguous ， (FULLWIDTH COMMA). Did you mean , (COMMA)?
(RUF003)
181-181: Comment contains ambiguous ） (FULLWIDTH RIGHT PARENTHESIS). Did you mean ) (RIGHT PARENTHESIS)?
(RUF003)
185-185: f-string without any placeholders
Remove extraneous f prefix
(F541)
185-185: String contains ambiguous （ (FULLWIDTH LEFT PARENTHESIS). Did you mean ( (LEFT PARENTHESIS)?
(RUF001)
185-185: String contains ambiguous ） (FULLWIDTH RIGHT PARENTHESIS). Did you mean ) (RIGHT PARENTHESIS)?
(RUF001)
186-186: Use explicit conversion flag
Replace with conversion flag
(RUF010)
187-187: Use explicit conversion flag
Replace with conversion flag
(RUF010)
189-189: String contains ambiguous ， (FULLWIDTH COMMA). Did you mean , (COMMA)?
(RUF001)
194-194: String contains ambiguous （ (FULLWIDTH LEFT PARENTHESIS). Did you mean ( (LEFT PARENTHESIS)?
(RUF001)
194-194: String contains ambiguous ） (FULLWIDTH RIGHT PARENTHESIS). Did you mean ) (RIGHT PARENTHESIS)?
(RUF001)
195-195: Do not catch blind exception: Exception
(BLE001)
201-201: f-string without any placeholders
Remove extraneous f prefix
(F541)
201-201: String contains ambiguous ， (FULLWIDTH COMMA). Did you mean , (COMMA)?
(RUF001)
202-202: f-string without any placeholders
Remove extraneous f prefix
(F541)
🤖 Prompt for AI Agents
In @tests/unit_tests/test_collection_name_validation.py around lines 149 - 205,
The _validate_collection_name function currently only enforces the 512-char
rule; update it to also enforce the DB table-name limit by computing
len(CollectionNames.prefix()) + len(name) and raising a ValueError if that sum >
64; ensure the error message clearly states the prefix, name length, and the
64-char DB limit so tests referencing effective_max_length_by_db_limit and
CollectionNames.table_name behave as expected.
cms-cms
force-pushed
the
feature/wentian
branch
from
1755e74 to
64146ce
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @tests/unit_tests/test_collection_name_validation.py:
- Around line 63-73: Remove the noisy print statements from test_valid_names
(and the other similar tests referenced) so tests rely on assertions and pytest
reporting instead of runtime prints; locate the loops that call
_validate_collection_name and delete all print(...) and try/except wrappers,
leaving direct assertion/validation calls (or convert to pytest.mark.parametrize
with descriptive ids for each input) and, if temporary debug output is required
during development, switch to logging.getLogger(...) and configure via pytest
--log-cli-level rather than using print.
- Around line 149-262: The _validate_collection_name function is missing a check
that the prefixed table name (CollectionNames.table_name(name)) does not exceed
the DB table name length limit; update _validate_collection_name to (1)
import/use CollectionNames.table_name to compute the full table name, (2)
compare len(table_name) against a configurable DB limit (e.g.,
_DB_TABLE_NAME_MAX_LENGTH) and raise ValueError with a clear message if
exceeded, and (3) keep existing type/charset/_MAX_COLLECTION_NAME_LENGTH checks
but perform the prefixed-table-length check last so names that individually pass
cannot produce an oversized table name.
🧹 Nitpick comments (1)
tests/unit_tests/test_collection_name_validation.py (1)
414-456: Test duplication: table name generation logic already covered.The
test_table_name_generationmethod largely duplicates the validation logic already tested intest_effective_max_length_by_db_limitandtest_boundary_length_values(lines 149-262). All three tests verify the same concept: whether table names respect DB limits.Consider consolidating these tests or clearly differentiating their focus. For example:
- Keep
test_boundary_length_valuesfor edge cases- Remove or refocus
test_table_name_generationto test theCollectionNames.table_name()utility itself (e.g., correct prefix application)- Remove
test_effective_max_length_by_db_limitas its core logic is covered by boundary tests
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/unit_tests/test_collection_name_validation.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-22T12:44:57.424Z
Learnt from: longbingljw
Repo: oceanbase/pyseekdb PR: 91
File: tests/integration_tests/test_collection_dml.py:36-36
Timestamp: 2025-12-22T12:44:57.424Z
Learning: In tests/integration_tests/test_collection_dml.py and similar test files, using `db_client._server._execute()` to run SQL commands directly (e.g., CREATE TABLE statements) is acceptable for integration tests that need custom table setup with specific vector index configurations.
Applied to files:
tests/unit_tests/test_collection_name_validation.py
🧬 Code graph analysis (1)
tests/unit_tests/test_collection_name_validation.py (2)
src/pyseekdb/client/meta_info.py (3)
CollectionNames(12-41)prefix(39-41)table_name(17-19)src/pyseekdb/client/client_base.py (1)
_validate_collection_name(82-108)
🪛 Ruff (0.14.10)
tests/unit_tests/test_collection_name_validation.py
64-64: f-string without any placeholders
Remove extraneous f prefix
(F541)
67-67: Use explicit conversion flag
Replace with conversion flag
(RUF010)
70-70: f-string without any placeholders
Remove extraneous f prefix
(F541)
78-78: f-string without any placeholders
Remove extraneous f prefix
(F541)
83-83: Use explicit conversion flag
Replace with conversion flag
(RUF010)
84-84: f-string without any placeholders
Remove extraneous f prefix
(F541)
90-90: String contains ambiguous ， (FULLWIDTH COMMA). Did you mean , (COMMA)?
(RUF001)
96-96: f-string without any placeholders
Remove extraneous f prefix
(F541)
99-99: Use explicit conversion flag
Replace with conversion flag
(RUF010)
100-100: f-string without any placeholders
Remove extraneous f prefix
(F541)
100-100: String contains ambiguous ， (FULLWIDTH COMMA). Did you mean , (COMMA)?
(RUF001)
106-106: String contains ambiguous ， (FULLWIDTH COMMA). Did you mean , (COMMA)?
(RUF001)
114-114: f-string without any placeholders
Remove extraneous f prefix
(F541)
116-116: Use explicit conversion flag
Replace with conversion flag
(RUF010)
117-117: f-string without any placeholders
Remove extraneous f prefix
(F541)
117-117: String contains ambiguous ， (FULLWIDTH COMMA). Did you mean , (COMMA)?
(RUF001)
123-123: String contains ambiguous ， (FULLWIDTH COMMA). Did you mean , (COMMA)?
(RUF001)
136-136: f-string without any placeholders
Remove extraneous f prefix
(F541)
139-139: Use explicit conversion flag
Replace with conversion flag
(RUF010)
140-140: f-string without any placeholders
Remove extraneous f prefix
(F541)
140-140: String contains ambiguous ， (FULLWIDTH COMMA). Did you mean , (COMMA)?
(RUF001)
146-146: String contains ambiguous ， (FULLWIDTH COMMA). Did you mean , (COMMA)?
(RUF001)
152-152: f-string without any placeholders
Remove extraneous f prefix
(F541)
161-161: f-string without any placeholders
Remove extraneous f prefix
(F541)
162-162: Use explicit conversion flag
Replace with conversion flag
(RUF010)
167-167: Comment contains ambiguous （ (FULLWIDTH LEFT PARENTHESIS). Did you mean ( (LEFT PARENTHESIS)?
(RUF003)
167-167: Comment contains ambiguous ） (FULLWIDTH RIGHT PARENTHESIS). Did you mean ) (RIGHT PARENTHESIS)?
(RUF003)
170-170: f-string without any placeholders
Remove extraneous f prefix
(F541)
171-171: Use explicit conversion flag
Replace with conversion flag
(RUF010)
172-172: Use explicit conversion flag
Replace with conversion flag
(RUF010)
173-173: String contains ambiguous ， (FULLWIDTH COMMA). Did you mean , (COMMA)?
(RUF001)
176-176: f-string without any placeholders
Remove extraneous f prefix
(F541)
176-176: String contains ambiguous ， (FULLWIDTH COMMA). Did you mean , (COMMA)?
(RUF001)
181-181: Comment contains ambiguous （ (FULLWIDTH LEFT PARENTHESIS). Did you mean ( (LEFT PARENTHESIS)?
(RUF003)
181-181: Comment contains ambiguous ， (FULLWIDTH COMMA). Did you mean , (COMMA)?
(RUF003)
181-181: Comment contains ambiguous ） (FULLWIDTH RIGHT PARENTHESIS). Did you mean ) (RIGHT PARENTHESIS)?
(RUF003)
185-185: f-string without any placeholders
Remove extraneous f prefix
(F541)
185-185: String contains ambiguous （ (FULLWIDTH LEFT PARENTHESIS). Did you mean ( (LEFT PARENTHESIS)?
(RUF001)
185-185: String contains ambiguous ） (FULLWIDTH RIGHT PARENTHESIS). Did you mean ) (RIGHT PARENTHESIS)?
(RUF001)
186-186: Use explicit conversion flag
Replace with conversion flag
(RUF010)
187-187: Use explicit conversion flag
Replace with conversion flag
(RUF010)
189-189: String contains ambiguous ， (FULLWIDTH COMMA). Did you mean , (COMMA)?
(RUF001)
194-194: String contains ambiguous （ (FULLWIDTH LEFT PARENTHESIS). Did you mean ( (LEFT PARENTHESIS)?
(RUF001)
194-194: String contains ambiguous ） (FULLWIDTH RIGHT PARENTHESIS). Did you mean ) (RIGHT PARENTHESIS)?
(RUF001)
195-195: Do not catch blind exception: Exception
(BLE001)
201-201: f-string without any placeholders
Remove extraneous f prefix
(F541)
201-201: String contains ambiguous ， (FULLWIDTH COMMA). Did you mean , (COMMA)?
(RUF001)
202-202: f-string without any placeholders
Remove extraneous f prefix
(F541)
209-209: f-string without any placeholders
Remove extraneous f prefix
(F541)
218-218: f-string without any placeholders
Remove extraneous f prefix
(F541)
219-219: Use explicit conversion flag
Replace with conversion flag
(RUF010)
229-229: String contains ambiguous ， (FULLWIDTH COMMA). Did you mean , (COMMA)?
(RUF001)
230-230: String contains ambiguous ， (FULLWIDTH COMMA). Did you mean , (COMMA)?
(RUF001)
231-231: String contains ambiguous ， (FULLWIDTH COMMA). Did you mean , (COMMA)?
(RUF001)
240-240: Use explicit conversion flag
Replace with conversion flag
(RUF010)
241-241: Use explicit conversion flag
Replace with conversion flag
(RUF010)
244-244: String contains ambiguous （ (FULLWIDTH LEFT PARENTHESIS). Did you mean ( (LEFT PARENTHESIS)?
(RUF001)
244-244: String contains ambiguous ） (FULLWIDTH RIGHT PARENTHESIS). Did you mean ) (RIGHT PARENTHESIS)?
(RUF001)
249-249: f-string without any placeholders
Remove extraneous f prefix
(F541)
251-251: String contains ambiguous ， (FULLWIDTH COMMA). Did you mean , (COMMA)?
(RUF001)
252-252: String contains ambiguous ， (FULLWIDTH COMMA). Did you mean , (COMMA)?
(RUF001)
266-266: f-string without any placeholders
Remove extraneous f prefix
(F541)
283-283: Use explicit conversion flag
Replace with conversion flag
(RUF010)
284-284: f-string without any placeholders
Remove extraneous f prefix
(F541)
284-284: String contains ambiguous （ (FULLWIDTH LEFT PARENTHESIS). Did you mean ( (LEFT PARENTHESIS)?
(RUF001)
284-284: String contains ambiguous ） (FULLWIDTH RIGHT PARENTHESIS). Did you mean ) (RIGHT PARENTHESIS)?
(RUF001)
287-287: f-string without any placeholders
Remove extraneous f prefix
(F541)
295-295: f-string without any placeholders
Remove extraneous f prefix
(F541)
306-306: Use explicit conversion flag
Replace with conversion flag
(RUF010)
307-307: f-string without any placeholders
Remove extraneous f prefix
(F541)
307-307: String contains ambiguous （ (FULLWIDTH LEFT PARENTHESIS). Did you mean ( (LEFT PARENTHESIS)?
(RUF001)
307-307: String contains ambiguous ） (FULLWIDTH RIGHT PARENTHESIS). Did you mean ) (RIGHT PARENTHESIS)?
(RUF001)
310-310: f-string without any placeholders
Remove extraneous f prefix
(F541)
318-318: f-string without any placeholders
Remove extraneous f prefix
(F541)
334-334: Use explicit conversion flag
Replace with conversion flag
(RUF010)
335-335: f-string without any placeholders
Remove extraneous f prefix
(F541)
338-338: f-string without any placeholders
Remove extraneous f prefix
(F541)
346-346: f-string without any placeholders
Remove extraneous f prefix
(F541)
361-361: Use explicit conversion flag
Replace with conversion flag
(RUF010)
362-362: f-string without any placeholders
Remove extraneous f prefix
(F541)
365-365: f-string without any placeholders
Remove extraneous f prefix
(F541)
373-373: f-string without any placeholders
Remove extraneous f prefix
(F541)
404-404: Use explicit conversion flag
Replace with conversion flag
(RUF010)
405-405: f-string without any placeholders
Remove extraneous f prefix
(F541)
405-405: String contains ambiguous ， (FULLWIDTH COMMA). Did you mean , (COMMA)?
(RUF001)
411-411: String contains ambiguous ， (FULLWIDTH COMMA). Did you mean , (COMMA)?
(RUF001)
417-417: f-string without any placeholders
Remove extraneous f prefix
(F541)
424-424: f-string without any placeholders
Remove extraneous f prefix
(F541)
425-425: Use explicit conversion flag
Replace with conversion flag
(RUF010)
438-438: Use explicit conversion flag
Replace with conversion flag
(RUF010)
439-439: Use explicit conversion flag
Replace with conversion flag
(RUF010)
445-445: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (3)
tests/unit_tests/test_collection_name_validation.py (3)
263-369: Comprehensive character validation coverage.The new tests for underscore positions, numeric starts, single character types, and mixed cases provide excellent coverage of the allowed character set
[a-zA-Z0-9_]. These tests effectively validate that the validation logic correctly accepts all valid character combinations.
370-413: Thorough invalid character testing.The
test_more_invalid_charactersmethod comprehensively tests a wide range of special characters, whitespace, and control characters that should be rejected. This significantly strengthens the validation test suite.
30-51: The_db_table_max_lengthproperty correctly returns the MySQL/OceanBase table name limit of 64 characters. This is a database specification, not a configurable parameter, and using a hardcoded value in a test helper is appropriate. The property properly reflects the actual constraint that table names cannot exceed 64 characters across all supported databases. No changes needed.Likely an incorrect or invalid review comment.
| print(f"\n{'=' * 60}") | ||
| print(f"测试: test_valid_names - 测试有效名称") | ||
| print(f"{'=' * 60}") | ||
| for name in valid_names: | ||
| _validate_collection_name(name) | ||
| print(f"\n输入: {repr(name)} (长度: {len(name)})") | ||
| try: | ||
| _validate_collection_name(name) | ||
| print(f"输出: ✅ 验证通过 - 名称有效") | ||
| except Exception as e: | ||
| print(f"输出: ❌ 验证失败 - {type(e).__name__}: {e}") | ||
| raise |
There was a problem hiding this comment.
Excessive print statements degrade test output quality.
The addition of verbose print statements with Chinese text throughout the tests significantly pollutes test output and may cause rendering issues in CI/CD environments where UTF-8 support is limited. Unit tests should be self-documenting through clear assertions and test names rather than runtime output.
♻️ Recommended approach
If debugging output is necessary during development:
- Use Python's
loggingmodule instead ofprint - Set log level via pytest's
--log-cli-levelflag - Remove debug output before merging to production
Alternatively, if you need to trace test execution, consider using pytest's built-in verbose mode (-vv) and parameterized tests with descriptive IDs.
Example refactor for test_valid_names:
- print(f"\n{'=' * 60}")
- print(f"测试: test_valid_names - 测试有效名称")
- print(f"{'=' * 60}")
for name in valid_names:
- print(f"\n输入: {repr(name)} (长度: {len(name)})")
- try:
- _validate_collection_name(name)
- print(f"输出: ✅ 验证通过 - 名称有效")
- except Exception as e:
- print(f"输出: ❌ 验证失败 - {type(e).__name__}: {e}")
- raise
+ _validate_collection_name(name) # Should not raiseAlso applies to: 77-91, 95-107, 113-124, 135-147
🧰 Tools
🪛 Ruff (0.14.10)
64-64: f-string without any placeholders
Remove extraneous f prefix
(F541)
67-67: Use explicit conversion flag
Replace with conversion flag
(RUF010)
70-70: f-string without any placeholders
Remove extraneous f prefix
(F541)
🤖 Prompt for AI Agents
In @tests/unit_tests/test_collection_name_validation.py around lines 63 - 73,
Remove the noisy print statements from test_valid_names (and the other similar
tests referenced) so tests rely on assertions and pytest reporting instead of
runtime prints; locate the loops that call _validate_collection_name and delete
all print(...) and try/except wrappers, leaving direct assertion/validation
calls (or convert to pytest.mark.parametrize with descriptive ids for each
input) and, if temporary debug output is required during development, switch to
logging.getLogger(...) and configure via pytest --log-cli-level rather than
using print.
| def test_effective_max_length_by_db_limit(self): | ||
| """Test that collection names respect DB table name limit when combined with prefix.""" | ||
| print(f"\n{'=' * 60}") | ||
| print(f"测试: test_effective_max_length_by_db_limit - 测试基于DB限制的有效最大长度") | ||
| print(f"{'=' * 60}") | ||
|
|
||
| prefix = CollectionNames.prefix() | ||
| prefix_len = len(prefix) | ||
| db_limit = self._db_table_max_length | ||
| effective_max = self._effective_max_length_by_db_limit | ||
| max_len = self._effective_max_length | ||
|
|
||
| print(f"\n配置信息:") | ||
| print(f" 前缀: {repr(prefix)} (长度: {prefix_len})") | ||
| print(f" DB表名最大长度: {db_limit}") | ||
| print(f" 有效最大长度: {effective_max} (计算: {db_limit} - {prefix_len})") | ||
| print(f" 当前验证函数的最大长度限制: {max_len}") | ||
|
|
||
| # 测试正好等于有效最大长度（应该通过） | ||
| exact_max_name = "a" * effective_max | ||
| table_name = CollectionNames.table_name(exact_max_name) | ||
| print(f"\n测试1: 正好等于有效最大长度") | ||
| print(f" 输入: {repr(exact_max_name)} (长度: {len(exact_max_name)})") | ||
| print(f" 生成的表名: {repr(table_name)} (长度: {len(table_name)})") | ||
| print(f" 预期: 验证通过，表名长度 {len(table_name)} <= {db_limit}") | ||
| try: | ||
| _validate_collection_name(exact_max_name) | ||
| print(f" 输出: ✅ 验证通过 - 名称有效，表名长度符合DB限制") | ||
| except Exception as e: | ||
| print(f" 输出: ❌ 验证失败 - {type(e).__name__}: {e}") | ||
| raise | ||
|
|
||
| # 测试超过有效最大长度（应该失败，因为表名会超过DB限制） | ||
| over_max_name = "a" * (effective_max + 1) | ||
| table_name_over = CollectionNames.table_name(over_max_name) | ||
| table_len_over = len(table_name_over) | ||
| print(f"\n测试2: 超过有效最大长度（基于DB限制）") | ||
| print(f" 输入名称: {repr(over_max_name[:50])}... (长度: {len(over_max_name)})") | ||
| print(f" 生成的表名: {repr(table_name_over[:50])}... (长度: {table_len_over})") | ||
| print(f" 表名长度计算: {prefix_len} + {len(over_max_name)} = {table_len_over}") | ||
| print(f" 预期: 表名长度 {table_len_over} > {db_limit}，应该被拒绝") | ||
|
|
||
| # 验证名称本身 | ||
| try: | ||
| _validate_collection_name(over_max_name) | ||
| print(f" 名称验证: ✅ 通过（当前验证函数只检查{max_len}的限制）") | ||
| except Exception as e: | ||
| print(f" 名称验证: ❌ 失败 - {type(e).__name__}: {e}") | ||
|
|
||
| # 检查表名长度 | ||
| if table_len_over > db_limit: | ||
| print(f" 表名长度验证: ❌ 失败 - 表名长度 {table_len_over} > DB限制 {db_limit}") | ||
| print(f" 输出: ⚠️ 表名长度超过DB限制，但当前验证函数可能不会检查此限制") | ||
| print(f" 提示: 建议在_validate_collection_name中添加基于DB限制的检查") | ||
| else: | ||
| print(f" 表名长度验证: ✅ 通过 - 表名长度 {table_len_over} <= DB限制 {db_limit}") | ||
|
|
||
| def test_boundary_length_values(self): | ||
| """Test boundary values around the effective max length based on DB table name limit.""" | ||
| print(f"\n{'=' * 60}") | ||
| print(f"测试: test_boundary_length_values - 测试基于DB限制的边界长度值") | ||
| print(f"{'=' * 60}") | ||
|
|
||
| prefix = CollectionNames.prefix() | ||
| prefix_len = len(prefix) | ||
| db_limit = self._db_table_max_length | ||
| effective_max = self._effective_max_length_by_db_limit | ||
| max_len = self._effective_max_length | ||
|
|
||
| print(f"\n配置信息:") | ||
| print(f" 前缀: {repr(prefix)} (长度: {prefix_len})") | ||
| print(f" DB表名最大长度: {db_limit}") | ||
| print(f" 基于DB限制的有效最大长度: {effective_max} (计算: {db_limit} - {prefix_len})") | ||
| print(f" 当前验证函数的最大长度限制: {max_len}") | ||
| print(f" 测试将使用: {effective_max} (基于DB限制)") | ||
|
|
||
| # 使用基于DB限制的有效最大长度进行边界测试 | ||
| test_limit = effective_max | ||
|
|
||
| test_cases = [ | ||
| (test_limit - 1, True, f"比有效最大长度少1 ({test_limit - 1}字符)，应该通过"), | ||
| (test_limit, True, f"正好等于有效最大长度 ({test_limit}字符)，应该通过"), | ||
| (test_limit + 1, False, f"比有效最大长度多1 ({test_limit + 1}字符)，应该失败"), | ||
| ] | ||
|
|
||
| for length, should_pass, description in test_cases: | ||
| name = "a" * length | ||
| table_name = CollectionNames.table_name(name) | ||
| table_len = len(table_name) | ||
|
|
||
| print(f"\n测试: {description}") | ||
| print(f" 输入名称: {repr(name[:30])}... (长度: {len(name)})") | ||
| print(f" 生成的表名: {repr(table_name[:50])}... (长度: {table_len})") | ||
| print( | ||
| f" 表名长度验证: {prefix_len} + {len(name)} = {table_len} {'✅' if table_len <= db_limit else '❌ 超过DB限制'}") | ||
| print(f" 预期: {'通过' if should_pass else '失败（因为表名长度超过DB限制）'}") | ||
|
|
||
| try: | ||
| _validate_collection_name(name) | ||
| if should_pass: | ||
| print(f" 输出: ✅ 验证通过 - 符合预期") | ||
| else: | ||
| print(f" 输出: ⚠️ 验证通过 - 但表名长度 {table_len} > {db_limit}，超过了DB限制") | ||
| print(f" 提示: 当前_validate_collection_name只检查{max_len}的限制，未检查DB表名限制") | ||
| except ValueError as e: | ||
| if not should_pass: | ||
| print(f" 输出: ✅ 验证通过 - 正确拒绝: {e}") | ||
| else: | ||
| print(f" 输出: ❌ 验证失败 - 应该通过但被拒绝: {e}") | ||
| raise | ||
| except Exception as e: | ||
| print(f" 输出: ❌ 验证失败 - 意外异常: {type(e).__name__}: {e}") | ||
| raise | ||
|
|
There was a problem hiding this comment.
Critical validation gap: DB table name limit not enforced.
These tests reveal that _validate_collection_name doesn't verify whether prefix + collection_name exceeds the database table name limit. The tests acknowledge this gap (lines 201-203, 251-252) and explicitly suggest adding this check.
Without this validation, collection names that pass _validate_collection_name may still fail at table creation time when the prefixed table name exceeds the DB limit.
🔧 Proposed fix for validation function
In src/pyseekdb/client/client_base.py, update _validate_collection_name:
def _validate_collection_name(name: str) -> None:
"""
Validate collection name against allowed charset and length constraints.
Rules:
- Type must be str
- Length between 1 and _MAX_COLLECTION_NAME_LENGTH
- Only [a-zA-Z0-9_]
- Table name (prefix + name) must not exceed DB table name limit
Raises:
TypeError: If name is not a string.
ValueError: If name is empty, too long, or contains invalid characters.
"""
if not isinstance(name, str):
raise TypeError(f"Collection name must be a string, got {type(name).__name__}")
if not name:
raise ValueError("Collection name must not be empty")
if len(name) > _MAX_COLLECTION_NAME_LENGTH:
raise ValueError(
f"Collection name too long: {len(name)} characters; "
f"maximum allowed is {_MAX_COLLECTION_NAME_LENGTH}."
)
if _COLLECTION_NAME_PATTERN.match(name) is None:
raise ValueError(
"Collection name contains invalid characters. "
"Only letters, digits, and underscore are allowed: [a-zA-Z0-9_]"
)
# Check if prefixed table name exceeds DB limit
from pyseekdb.client.meta_info import CollectionNames
table_name = CollectionNames.table_name(name)
_DB_TABLE_NAME_MAX_LENGTH = 64 # MySQL default, should be configurable
if len(table_name) > _DB_TABLE_NAME_MAX_LENGTH:
raise ValueError(
f"Collection name too long: resulting table name would be "
f"{len(table_name)} characters (including prefix), which exceeds "
f"database limit of {_DB_TABLE_NAME_MAX_LENGTH}."
)🧰 Tools
🪛 Ruff (0.14.10)
152-152: f-string without any placeholders
Remove extraneous f prefix
(F541)
161-161: f-string without any placeholders
Remove extraneous f prefix
(F541)
162-162: Use explicit conversion flag
Replace with conversion flag
(RUF010)
167-167: Comment contains ambiguous （ (FULLWIDTH LEFT PARENTHESIS). Did you mean ( (LEFT PARENTHESIS)?
(RUF003)
167-167: Comment contains ambiguous ） (FULLWIDTH RIGHT PARENTHESIS). Did you mean ) (RIGHT PARENTHESIS)?
(RUF003)
170-170: f-string without any placeholders
Remove extraneous f prefix
(F541)
171-171: Use explicit conversion flag
Replace with conversion flag
(RUF010)
172-172: Use explicit conversion flag
Replace with conversion flag
(RUF010)
173-173: String contains ambiguous ， (FULLWIDTH COMMA). Did you mean , (COMMA)?
(RUF001)
176-176: f-string without any placeholders
Remove extraneous f prefix
(F541)
176-176: String contains ambiguous ， (FULLWIDTH COMMA). Did you mean , (COMMA)?
(RUF001)
181-181: Comment contains ambiguous （ (FULLWIDTH LEFT PARENTHESIS). Did you mean ( (LEFT PARENTHESIS)?
(RUF003)
181-181: Comment contains ambiguous ， (FULLWIDTH COMMA). Did you mean , (COMMA)?
(RUF003)
181-181: Comment contains ambiguous ） (FULLWIDTH RIGHT PARENTHESIS). Did you mean ) (RIGHT PARENTHESIS)?
(RUF003)
185-185: f-string without any placeholders
Remove extraneous f prefix
(F541)
185-185: String contains ambiguous （ (FULLWIDTH LEFT PARENTHESIS). Did you mean ( (LEFT PARENTHESIS)?
(RUF001)
185-185: String contains ambiguous ） (FULLWIDTH RIGHT PARENTHESIS). Did you mean ) (RIGHT PARENTHESIS)?
(RUF001)
186-186: Use explicit conversion flag
Replace with conversion flag
(RUF010)
187-187: Use explicit conversion flag
Replace with conversion flag
(RUF010)
189-189: String contains ambiguous ， (FULLWIDTH COMMA). Did you mean , (COMMA)?
(RUF001)
194-194: String contains ambiguous （ (FULLWIDTH LEFT PARENTHESIS). Did you mean ( (LEFT PARENTHESIS)?
(RUF001)
194-194: String contains ambiguous ） (FULLWIDTH RIGHT PARENTHESIS). Did you mean ) (RIGHT PARENTHESIS)?
(RUF001)
195-195: Do not catch blind exception: Exception
(BLE001)
201-201: f-string without any placeholders
Remove extraneous f prefix
(F541)
201-201: String contains ambiguous ， (FULLWIDTH COMMA). Did you mean , (COMMA)?
(RUF001)
202-202: f-string without any placeholders
Remove extraneous f prefix
(F541)
209-209: f-string without any placeholders
Remove extraneous f prefix
(F541)
218-218: f-string without any placeholders
Remove extraneous f prefix
(F541)
219-219: Use explicit conversion flag
Replace with conversion flag
(RUF010)
229-229: String contains ambiguous ， (FULLWIDTH COMMA). Did you mean , (COMMA)?
(RUF001)
230-230: String contains ambiguous ， (FULLWIDTH COMMA). Did you mean , (COMMA)?
(RUF001)
231-231: String contains ambiguous ， (FULLWIDTH COMMA). Did you mean , (COMMA)?
(RUF001)
240-240: Use explicit conversion flag
Replace with conversion flag
(RUF010)
241-241: Use explicit conversion flag
Replace with conversion flag
(RUF010)
244-244: String contains ambiguous （ (FULLWIDTH LEFT PARENTHESIS). Did you mean ( (LEFT PARENTHESIS)?
(RUF001)
244-244: String contains ambiguous ） (FULLWIDTH RIGHT PARENTHESIS). Did you mean ) (RIGHT PARENTHESIS)?
(RUF001)
249-249: f-string without any placeholders
Remove extraneous f prefix
(F541)
251-251: String contains ambiguous ， (FULLWIDTH COMMA). Did you mean , (COMMA)?
(RUF001)
252-252: String contains ambiguous ， (FULLWIDTH COMMA). Did you mean , (COMMA)?
(RUF001)
🤖 Prompt for AI Agents
In @tests/unit_tests/test_collection_name_validation.py around lines 149 - 262,
The _validate_collection_name function is missing a check that the prefixed
table name (CollectionNames.table_name(name)) does not exceed the DB table name
length limit; update _validate_collection_name to (1) import/use
CollectionNames.table_name to compute the full table name, (2) compare
len(table_name) against a configurable DB limit (e.g.,
_DB_TABLE_NAME_MAX_LENGTH) and raise ValueError with a clear message if
exceeded, and (3) keep existing type/charset/_MAX_COLLECTION_NAME_LENGTH checks
but perform the prefixed-table-length check last so names that individually pass
cannot produce an oversized table name.
2 participants
Summary
Solution Description
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.