Skip to content

Commit 866bb5c

Browse files
thoraxethoraxe
committed
more coderabbit changes
1 parent d6e1475 commit 866bb5c

File tree

2 files changed

+169
-15
lines changed

2 files changed

+169
-15
lines changed

‎src/app/endpoints/query.py‎

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,7 @@
4343
router = APIRouter(tags=["query"])
4444
auth_dependency = get_auth_dependency()
4545

46-
METADATA_PATTERN = re.compile(r"^Metadata:\s*(\{.*?\})\s*$", re.MULTILINE)
46+
METADATA_PATTERN = re.compile(r"^\s*Metadata:\s*(\{.*?\})\s*$", re.MULTILINE)
4747

4848

4949
def _process_knowledge_search_content(
@@ -59,10 +59,11 @@ def _process_knowledge_search_content(
5959
meta = ast.literal_eval(match)
6060
if "document_id" in meta:
6161
metadata_map[meta["document_id"]] = meta
62-
except Exception: # pylint: disable=broad-except
62+
except (SyntaxError, ValueError): # only expected from literal_eval
6363
logger.debug(
6464
"An exception was thrown in processing %s",
6565
match,
66+
exc_info=True,
6667
)
6768

6869

@@ -121,7 +122,7 @@ def extract_referenced_documents_from_steps(steps: list) -> list[dict[str, str]]
121122
"description": "User is not authorized",
122123
"model": ForbiddenResponse,
123124
},
124-
503: {
125+
500: {
125126
"detail": {
126127
"response": "Unable to connect to Llama Stack",
127128
"cause": "Connection error.",
@@ -487,8 +488,10 @@ async def retrieve_response( # pylint: disable=too-many-locals
487488

488489
# When stream=False, response should have output_message attribute
489490
response_obj = cast(Any, response)
491+
content = getattr(getattr(response_obj, "output_message", None), "content", "")
492+
content_str = "" if content is None else str(content)
490493
return (
491-
str(response_obj.output_message.content),
494+
content_str,
492495
conversation_id,
493496
referenced_documents,
494497
)

‎tests/unit/app/endpoints/test_query.py‎

Lines changed: 162 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,11 @@
4444
Content: ABC
4545
Metadata: {'docs_url': 'https://example.com/doc2', 'title': 'Doc2', 'document_id': 'doc-2', \
4646
'source': None}
47+
""",
48+
"""Result 2b
49+
Content: ABC
50+
Metadata: {'docs_url': 'https://example.com/doc2b', 'title': 'Doc2b', 'document_id': 'doc-2b', \
51+
'source': None}
4752
""",
4853
"""END of knowledge_search tool results.
4954
""",
@@ -101,10 +106,6 @@ def setup_configuration_fixture():
101106
async def test_query_endpoint_handler_configuration_not_loaded(mocker):
102107
"""Test the query endpoint handler if configuration is not loaded."""
103108
# simulate state when no configuration is loaded
104-
mocker.patch(
105-
"app.endpoints.query.configuration",
106-
return_value=mocker.Mock(),
107-
)
108109
mocker.patch("app.endpoints.query.configuration", None)
109110

110111
query = "What is OpenStack?"
@@ -272,12 +273,11 @@ async def test_query_endpoint_handler_with_referenced_documents(mocker):
272273
def test_select_model_and_provider_id_from_request(mocker):
273274
"""Test the select_model_and_provider_id function."""
274275
mocker.patch(
275-
"metrics.utils.configuration.inference.default_provider",
276+
"app.endpoints.query.configuration.inference.default_provider",
276277
"default_provider",
277278
)
278279
mocker.patch(
279-
"metrics.utils.configuration.inference.default_model",
280-
"default_model",
280+
"app.endpoints.query.configuration.inference.default_model", "default_model"
281281
)
282282

283283
model_list = [
@@ -312,12 +312,11 @@ def test_select_model_and_provider_id_from_request(mocker):
312312
def test_select_model_and_provider_id_from_configuration(mocker):
313313
"""Test the select_model_and_provider_id function."""
314314
mocker.patch(
315-
"metrics.utils.configuration.inference.default_provider",
315+
"app.endpoints.query.configuration.inference.default_provider",
316316
"default_provider",
317317
)
318318
mocker.patch(
319-
"metrics.utils.configuration.inference.default_model",
320-
"default_model",
319+
"app.endpoints.query.configuration.inference.default_model", "default_model"
321320
)
322321

323322
model_list = [
@@ -557,11 +556,13 @@ async def test_retrieve_response_with_knowledge_search_extracts_referenced_docum
557556
assert conversation_id == "fake_conversation_id"
558557

559558
# Assert referenced documents were extracted correctly
560-
assert len(referenced_documents) == 2
559+
assert len(referenced_documents) == 3
561560
assert referenced_documents[0]["doc_url"] == "https://example.com/doc1"
562561
assert referenced_documents[0]["doc_title"] == "Doc1"
563562
assert referenced_documents[1]["doc_url"] == "https://example.com/doc2"
564563
assert referenced_documents[1]["doc_title"] == "Doc2"
564+
assert referenced_documents[2]["doc_url"] == "https://example.com/doc2b"
565+
assert referenced_documents[2]["doc_title"] == "Doc2b"
565566

566567
# Doc3 should not be included because it has "Title" instead of "title"
567568
doc_titles = [doc["doc_title"] for doc in referenced_documents]
@@ -1722,3 +1723,153 @@ def test_process_knowledge_search_content_with_no_text_attribute(mocker):
17221723

17231724
# Verify metadata_map remains empty since text attribute is missing
17241725
assert len(metadata_map) == 0
1726+
1727+
1728+
@pytest.mark.asyncio
1729+
async def test_retrieve_response_with_none_content(prepare_agent_mocks, mocker):
1730+
"""Test retrieve_response handles None content gracefully."""
1731+
mock_client, mock_agent = prepare_agent_mocks
1732+
1733+
# Mock response with None content
1734+
mock_response = mocker.Mock()
1735+
mock_response.output_message.content = None
1736+
mock_response.steps = []
1737+
mock_agent.create_turn.return_value = mock_response
1738+
1739+
mock_client.shields.list.return_value = []
1740+
mock_client.vector_dbs.list.return_value = []
1741+
1742+
# Mock configuration with empty MCP servers
1743+
mock_config = mocker.Mock()
1744+
mock_config.mcp_servers = []
1745+
mocker.patch("app.endpoints.query.configuration", mock_config)
1746+
mocker.patch(
1747+
"app.endpoints.query.get_agent",
1748+
return_value=(mock_agent, "fake_conversation_id", "fake_session_id"),
1749+
)
1750+
1751+
query_request = QueryRequest(query="What is OpenStack?")
1752+
model_id = "fake_model_id"
1753+
access_token = "test_token"
1754+
1755+
response, conversation_id, _ = await retrieve_response(
1756+
mock_client, model_id, query_request, access_token
1757+
)
1758+
1759+
# Should return empty string instead of "None"
1760+
assert response == ""
1761+
assert conversation_id == "fake_conversation_id"
1762+
1763+
1764+
@pytest.mark.asyncio
1765+
async def test_retrieve_response_with_missing_output_message(
1766+
prepare_agent_mocks, mocker
1767+
):
1768+
"""Test retrieve_response handles missing output_message gracefully."""
1769+
mock_client, mock_agent = prepare_agent_mocks
1770+
1771+
# Mock response without output_message attribute
1772+
mock_response = mocker.Mock(spec=["steps"]) # Only has steps attribute
1773+
mock_response.steps = []
1774+
mock_agent.create_turn.return_value = mock_response
1775+
1776+
mock_client.shields.list.return_value = []
1777+
mock_client.vector_dbs.list.return_value = []
1778+
1779+
# Mock configuration with empty MCP servers
1780+
mock_config = mocker.Mock()
1781+
mock_config.mcp_servers = []
1782+
mocker.patch("app.endpoints.query.configuration", mock_config)
1783+
mocker.patch(
1784+
"app.endpoints.query.get_agent",
1785+
return_value=(mock_agent, "fake_conversation_id", "fake_session_id"),
1786+
)
1787+
1788+
query_request = QueryRequest(query="What is OpenStack?")
1789+
model_id = "fake_model_id"
1790+
access_token = "test_token"
1791+
1792+
response, conversation_id, _ = await retrieve_response(
1793+
mock_client, model_id, query_request, access_token
1794+
)
1795+
1796+
# Should return empty string when output_message is missing
1797+
assert response == ""
1798+
assert conversation_id == "fake_conversation_id"
1799+
1800+
1801+
@pytest.mark.asyncio
1802+
async def test_retrieve_response_with_missing_content_attribute(
1803+
prepare_agent_mocks, mocker
1804+
):
1805+
"""Test retrieve_response handles missing content attribute gracefully."""
1806+
mock_client, mock_agent = prepare_agent_mocks
1807+
1808+
# Mock response with output_message but no content attribute
1809+
mock_response = mocker.Mock()
1810+
mock_response.output_message = mocker.Mock(spec=[]) # No content attribute
1811+
mock_response.steps = []
1812+
mock_agent.create_turn.return_value = mock_response
1813+
1814+
mock_client.shields.list.return_value = []
1815+
mock_client.vector_dbs.list.return_value = []
1816+
1817+
# Mock configuration with empty MCP servers
1818+
mock_config = mocker.Mock()
1819+
mock_config.mcp_servers = []
1820+
mocker.patch("app.endpoints.query.configuration", mock_config)
1821+
mocker.patch(
1822+
"app.endpoints.query.get_agent",
1823+
return_value=(mock_agent, "fake_conversation_id", "fake_session_id"),
1824+
)
1825+
1826+
query_request = QueryRequest(query="What is OpenStack?")
1827+
model_id = "fake_model_id"
1828+
access_token = "test_token"
1829+
1830+
response, conversation_id, _ = await retrieve_response(
1831+
mock_client, model_id, query_request, access_token
1832+
)
1833+
1834+
# Should return empty string when content attribute is missing
1835+
assert response == ""
1836+
assert conversation_id == "fake_conversation_id"
1837+
1838+
1839+
@pytest.mark.asyncio
1840+
async def test_retrieve_response_with_structured_content_object(
1841+
prepare_agent_mocks, mocker
1842+
):
1843+
"""Test retrieve_response handles structured content objects properly."""
1844+
mock_client, mock_agent = prepare_agent_mocks
1845+
1846+
# Mock response with a structured content object
1847+
structured_content = {"type": "text", "value": "This is structured content"}
1848+
mock_response = mocker.Mock()
1849+
mock_response.output_message.content = structured_content
1850+
mock_response.steps = []
1851+
mock_agent.create_turn.return_value = mock_response
1852+
1853+
mock_client.shields.list.return_value = []
1854+
mock_client.vector_dbs.list.return_value = []
1855+
1856+
# Mock configuration with empty MCP servers
1857+
mock_config = mocker.Mock()
1858+
mock_config.mcp_servers = []
1859+
mocker.patch("app.endpoints.query.configuration", mock_config)
1860+
mocker.patch(
1861+
"app.endpoints.query.get_agent",
1862+
return_value=(mock_agent, "fake_conversation_id", "fake_session_id"),
1863+
)
1864+
1865+
query_request = QueryRequest(query="What is OpenStack?")
1866+
model_id = "fake_model_id"
1867+
access_token = "test_token"
1868+
1869+
response, conversation_id, _ = await retrieve_response(
1870+
mock_client, model_id, query_request, access_token
1871+
)
1872+
1873+
# Should convert the structured object to string representation
1874+
assert response == str(structured_content)
1875+
assert conversation_id == "fake_conversation_id"

0 commit comments

Comments
 (0)