Skip to content

Commit 3fad65c

Browse files
RobbyMeyersMariusWirtz
authored andcommitted
fix: address Copilot PR review feedback
- _odata_str_literal now percent-encodes URL-reserved characters (%, #, ?, &) by delegating to build_url_friendly_object_name. Previously a name_pattern like 'Sales & Marketing' would corrupt the URL query string at &$filter=. - get_elements_dataframe trio-filter path now forwards **kwargs to get_element_names so request-level options (timeout, cancel_at_timeout, async_requests_mode) reach the REST call, matching the rest of the method. Added unit tests covering both behaviors.
1 parent 2deb046 commit 3fad65c

2 files changed

Lines changed: 88 additions & 3 deletions

File tree

TM1py/Services/ElementService.py

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,7 @@
3636
CaseAndSpaceInsensitiveSet,
3737
CaseAndSpaceInsensitiveTuplesDict,
3838
build_element_unique_names,
39+
build_url_friendly_object_name,
3940
dimension_hierarchy_element_tuple_from_unique_name,
4041
format_url,
4142
require_data_admin,
@@ -463,6 +464,7 @@ def get_elements_dataframe(
463464
element_type=element_type,
464465
name_pattern=name_pattern,
465466
level=level,
467+
**kwargs,
466468
)
467469
if resolved:
468470
elements = (
@@ -1882,8 +1884,16 @@ def _coerce_element_types(value) -> List[int]:
18821884

18831885

18841886
def _odata_str_literal(s: str) -> str:
1885-
"""Wrap a string in single quotes, doubling embedded single quotes per OData spec."""
1886-
return "'" + s.replace("'", "''") + "'"
1887+
"""Wrap a string in single quotes for use inside an OData $filter clause that
1888+
will be appended raw to a URL query string.
1889+
1890+
Doubles embedded single quotes per the OData spec AND percent-encodes
1891+
URL-reserved characters ('%', '#', '?', '&') so that user-supplied values
1892+
containing those chars don't corrupt the query string. Delegates the escaping
1893+
to ``build_url_friendly_object_name`` to stay aligned with the rest of the
1894+
codebase's URL-construction convention.
1895+
"""
1896+
return "'" + build_url_friendly_object_name(s) + "'"
18871897

18881898

18891899
def _normalize_for_match(s: str) -> str:

Tests/ElementService_filtering_helpers_test.py

Lines changed: 76 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,13 @@
11
import unittest
2+
from unittest.mock import MagicMock, patch
23

34
from TM1py.Objects import Element
4-
from TM1py.Services.ElementService import _build_elements_filter, _coerce_element_types
5+
from TM1py.Services.ElementService import (
6+
ElementService,
7+
_build_elements_filter,
8+
_coerce_element_types,
9+
_odata_str_literal,
10+
)
511

612

713
class TestCoerceElementTypes(unittest.TestCase):
@@ -227,5 +233,74 @@ def test_level_bool_raises(self):
227233
_build_elements_filter(None, None, True)
228234

229235

236+
class TestOdataStrLiteralUrlSafety(unittest.TestCase):
237+
"""The filter clause is concatenated raw into the URL query string
238+
(url += '&$filter=' + clause), so any string literal inside it must
239+
percent-encode URL-reserved characters (&, %, #, ?) — not just escape
240+
OData single quotes. Otherwise an element name like 'Sales & Marketing'
241+
would prematurely terminate $filter and corrupt the query."""
242+
243+
NAME_EXPR = "tolower(replace(Name,' ',''))"
244+
245+
def test_literal_url_escapes_ampersand(self):
246+
self.assertEqual(_odata_str_literal("a&b"), "'a%26b'")
247+
248+
def test_literal_url_escapes_percent(self):
249+
self.assertEqual(_odata_str_literal("a%b"), "'a%25b'")
250+
251+
def test_literal_url_escapes_hash(self):
252+
self.assertEqual(_odata_str_literal("a#b"), "'a%23b'")
253+
254+
def test_literal_url_escapes_question_mark(self):
255+
self.assertEqual(_odata_str_literal("a?b"), "'a%3Fb'")
256+
257+
def test_literal_still_escapes_single_quote(self):
258+
# OData spec requires doubling embedded single quotes; the URL fix
259+
# must not regress this.
260+
self.assertEqual(_odata_str_literal("O'Brien"), "'O''Brien'")
261+
262+
def test_pattern_with_ampersand_produces_url_safe_filter(self):
263+
# The bug: 'Sales & Marketing' would emit a raw '&' that splits the
264+
# query string at &$filter= and creates a phantom '$filter' param.
265+
result = _build_elements_filter(None, "*Sales & Marketing*", None)
266+
self.assertNotIn("&", result)
267+
self.assertIn("%26", result)
268+
self.assertEqual(result, f"contains({self.NAME_EXPR},'sales%26marketing')")
269+
270+
271+
class TestGetElementsDataFrameForwardsKwargs(unittest.TestCase):
272+
"""Regression: in the trio-filter branch of get_elements_dataframe, the
273+
internal call to get_element_names previously dropped **kwargs, silently
274+
discarding request-level options (timeout, cancel_at_timeout,
275+
async_requests_mode) that the rest of the method forwards consistently."""
276+
277+
def test_trio_path_forwards_kwargs_to_get_element_names(self):
278+
service = ElementService.__new__(ElementService)
279+
service._rest = MagicMock()
280+
281+
sentinel = RuntimeError("__stop__")
282+
recorded = {}
283+
284+
def fake_get_element_names(*args, **kwargs):
285+
recorded["kwargs"] = kwargs
286+
raise sentinel
287+
288+
with patch.object(service, "get_element_names", side_effect=fake_get_element_names):
289+
with self.assertRaises(RuntimeError) as ctx:
290+
service.get_elements_dataframe(
291+
dimension_name="d",
292+
hierarchy_name="h",
293+
name_pattern="foo*",
294+
timeout=42,
295+
cancel_at_timeout=True,
296+
async_requests_mode=True,
297+
)
298+
self.assertIs(ctx.exception, sentinel)
299+
300+
self.assertEqual(recorded["kwargs"].get("timeout"), 42)
301+
self.assertEqual(recorded["kwargs"].get("cancel_at_timeout"), True)
302+
self.assertEqual(recorded["kwargs"].get("async_requests_mode"), True)
303+
304+
230305
if __name__ == "__main__":
231306
unittest.main()

0 commit comments

Comments
 (0)