mirror of
https://github.com/Significant-Gravitas/AutoGPT.git
synced 2026-04-08 03:00:28 -04:00
fix(store): address reviewer feedback — name fallback, MAX_LEN constant, and new tests
- content_handlers.py: fallback metadata["name"] to block_id when both
display_name and block.name are falsy/None, ensuring the field is
always a non-empty string
- text_utils.py: add _MAX_CAMELCASE_INPUT_LEN = 500 named constant
(with comment) and apply it as a safety cap in split_camelcase()
- content_handlers_test.py: add three new test cases requested by reviewer:
* test_get_enabled_blocks_cached — verifies lru_cache makes get_blocks()
called only once across multiple _get_enabled_blocks() calls
* test_get_enabled_blocks_returns_immutable_mapping — verifies the
MappingProxyType wrapper raises TypeError on mutation
* test_block_handler_get_missing_items_batch_size_zero — verifies
batch_size=0 returns an empty list
Update test_block_handler_handles_none_name assertion to match the new
block_id fallback behaviour (was None, now "none-name-block").
This commit is contained in:
@@ -282,7 +282,7 @@ class BlockHandler(ContentHandler):
|
||||
content_type=ContentType.BLOCK,
|
||||
searchable_text=searchable_text,
|
||||
metadata={
|
||||
"name": display_name or block.name,
|
||||
"name": display_name or block.name or block_id,
|
||||
"categories": categories_list,
|
||||
"providers": provider_names,
|
||||
"has_llm_model_field": has_llm_model_field,
|
||||
|
||||
@@ -88,6 +88,32 @@ def test_get_enabled_blocks_skips_broken():
|
||||
assert list(result.keys()) == ["good"]
|
||||
|
||||
|
||||
def test_get_enabled_blocks_cached():
|
||||
"""_get_enabled_blocks() calls get_blocks() only once across multiple calls."""
|
||||
blocks = {"b1": _make_block_class(name="B1")}
|
||||
with patch(
|
||||
"backend.api.features.store.content_handlers.get_blocks", return_value=blocks
|
||||
) as mock_get_blocks:
|
||||
result1 = _get_enabled_blocks()
|
||||
result2 = _get_enabled_blocks()
|
||||
assert result1 is result2
|
||||
mock_get_blocks.assert_called_once()
|
||||
|
||||
|
||||
def test_get_enabled_blocks_returns_immutable_mapping():
|
||||
"""The returned mapping is a MappingProxyType — mutation raises TypeError."""
|
||||
import types
|
||||
|
||||
blocks = {"b1": _make_block_class(name="B1")}
|
||||
with patch(
|
||||
"backend.api.features.store.content_handlers.get_blocks", return_value=blocks
|
||||
):
|
||||
result = _get_enabled_blocks()
|
||||
assert isinstance(result, types.MappingProxyType)
|
||||
with pytest.raises(TypeError):
|
||||
result["new_key"] = object() # type: ignore[index]
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# StoreAgentHandler
|
||||
# ---------------------------------------------------------------------------
|
||||
@@ -204,6 +230,27 @@ async def test_block_handler_get_missing_items_splits_camelcase():
|
||||
assert "AI Text Generator Block" in items[0].searchable_text
|
||||
|
||||
|
||||
@pytest.mark.asyncio(loop_scope="session")
|
||||
async def test_block_handler_get_missing_items_batch_size_zero():
|
||||
"""batch_size=0 returns an empty list without querying the database."""
|
||||
handler = BlockHandler()
|
||||
|
||||
blocks = {"b1": _make_block_class(name="B1")}
|
||||
|
||||
with patch(
|
||||
"backend.api.features.store.content_handlers.get_blocks", return_value=blocks
|
||||
):
|
||||
with patch(
|
||||
"backend.api.features.store.content_handlers.query_raw_with_schema",
|
||||
return_value=[],
|
||||
) as mock_query:
|
||||
items = await handler.get_missing_items(batch_size=0)
|
||||
assert items == []
|
||||
# DB query is still issued to learn which blocks lack embeddings;
|
||||
# the empty result comes from itertools.islice limiting to 0 items.
|
||||
mock_query.assert_called_once()
|
||||
|
||||
|
||||
@pytest.mark.asyncio(loop_scope="session")
|
||||
async def test_block_handler_disabled_dont_exhaust_batch():
|
||||
"""Disabled blocks don't consume batch budget, so enabled blocks get indexed."""
|
||||
@@ -310,9 +357,9 @@ async def test_block_handler_handles_none_name():
|
||||
# display_name should be "" because block.name is None
|
||||
# searchable_text should still contain the description
|
||||
assert "A block with no name" in items[0].searchable_text
|
||||
# metadata["name"] falls back to block.name (None) when
|
||||
# display_name is empty — the ``or`` in ``display_name or block.name``
|
||||
assert items[0].metadata["name"] is None
|
||||
# metadata["name"] falls back to block_id when both display_name
|
||||
# and block.name are falsy, ensuring it is always a non-empty string.
|
||||
assert items[0].metadata["name"] == "none-name-block"
|
||||
|
||||
|
||||
@pytest.mark.asyncio(loop_scope="session")
|
||||
|
||||
@@ -1,5 +1,8 @@
|
||||
"""Small text helpers shared across store search modules."""
|
||||
|
||||
# Safety cap; block names are typically < 50 chars
|
||||
_MAX_CAMELCASE_INPUT_LEN = 500
|
||||
|
||||
|
||||
def split_camelcase(text: str) -> str:
|
||||
"""Split CamelCase into separate words.
|
||||
@@ -24,6 +27,10 @@ def split_camelcase(text: str) -> str:
|
||||
>>> split_camelcase("OAuth2Block")
|
||||
'OAuth2 Block'
|
||||
"""
|
||||
# Truncate to safety cap before processing
|
||||
if len(text) > _MAX_CAMELCASE_INPUT_LEN:
|
||||
text = text[:_MAX_CAMELCASE_INPUT_LEN]
|
||||
|
||||
if len(text) <= 1:
|
||||
return text
|
||||
|
||||
|
||||
Reference in New Issue
Block a user