From 827dd84884eaaacee433f4bd197bb61baf9870a7 Mon Sep 17 00:00:00 2001 From: Abel Milash Date: Wed, 25 Mar 2026 00:02:55 -0700 Subject: [PATCH 1/5] Batch picklist type-check with CRM.In to reduce API calls --- examples/advanced/walkthrough.py | 8 + src/PowerPlatform/Dataverse/data/_odata.py | 165 +++-- tests/unit/data/test_odata_internal.py | 809 ++++++++++++++++++++- 3 files changed, 909 insertions(+), 73 deletions(-) diff --git a/examples/advanced/walkthrough.py b/examples/advanced/walkthrough.py index ef633d00..a50b86dd 100644 --- a/examples/advanced/walkthrough.py +++ b/examples/advanced/walkthrough.py @@ -444,6 +444,14 @@ def _run_walkthrough(client): print(f" new_Priority stored as integer: {retrieved.get('new_priority')}") print(f" new_Priority@FormattedValue: {retrieved.get('new_priority@OData.Community.Display.V1.FormattedValue')}") + # Update with a string label + log_call(f"client.records.update('{table_name}', label_id, {{'new_Priority': 'Low'}})") + backoff(lambda: client.records.update(table_name, label_id, {"new_Priority": "Low"})) + updated_label = backoff(lambda: client.records.get(table_name, label_id)) + print(f"[OK] Updated record with string label 'Low' for new_Priority") + print(f" new_Priority stored as integer: {updated_label.get('new_priority')}") + print(f" new_Priority@FormattedValue: {updated_label.get('new_priority@OData.Community.Display.V1.FormattedValue')}") + # ============================================================================ # 11. COLUMN MANAGEMENT # ============================================================================ diff --git a/src/PowerPlatform/Dataverse/data/_odata.py b/src/PowerPlatform/Dataverse/data/_odata.py index 78ce4091..cdfeb406 100644 --- a/src/PowerPlatform/Dataverse/data/_odata.py +++ b/src/PowerPlatform/Dataverse/data/_odata.py @@ -1216,18 +1216,75 @@ def _normalize_picklist_label(self, label: str) -> str: norm = re.sub(r"\s+", " ", norm).strip().lower() return norm + def _request_metadata_with_retry(self, method: str, url: str, **kwargs): + """Fetch metadata with retries on transient 404 responses.""" + max_attempts = 5 + backoff_seconds = 0.4 + for attempt in range(1, max_attempts + 1): + try: + return self._request(method, url, **kwargs) + except HttpError as err: + if getattr(err, "status_code", None) == 404: + if attempt < max_attempts: + time.sleep(backoff_seconds * (2 ** (attempt - 1))) + continue + raise RuntimeError(f"Metadata request failed after {max_attempts} retries (404): {url}") from err + raise + raise RuntimeError(f"Metadata request failed: {url}") + + def _check_attribute_types(self, table_schema_name: str, attr_logicals: List[str]) -> None: + """Batch-check AttributeType for multiple attributes in one API call. + + Uses an OData ``or`` filter to check all given attributes at once. + Non-picklist attributes (and attributes not found) are cached with an + empty map. Picklist attributes are cached with ``{"type": "Picklist"}`` + so that ``_optionset_map`` knows to fetch their options. + """ + if not attr_logicals: + return + table_esc = self._escape_odata_quotes(table_schema_name.lower()) + quoted_values = ",".join( + f'"{self._escape_odata_quotes(a.lower())}"' for a in attr_logicals + ) + attr_filter = ( + f"Microsoft.Dynamics.CRM.In(PropertyName='LogicalName',PropertyValues=[{quoted_values}])" + ) + url = ( + f"{self.api}/EntityDefinitions(LogicalName='{table_esc}')/Attributes" + f"?$filter={attr_filter}&$select=LogicalName,AttributeType" + ) + r = self._request_metadata_with_retry("get", url) + body = r.json() + items = body.get("value", []) if isinstance(body, dict) else [] + now = time.time() + found: set[str] = set() + for item in items: + if not isinstance(item, dict): + continue + ln = item.get("LogicalName", "").lower() + atype = item.get("AttributeType", "") + found.add(ln) + if atype not in ("Picklist", "PickList"): + cache_key = (self._normalize_cache_key(table_schema_name), ln) + self._picklist_label_cache[cache_key] = {"map": {}, "ts": now} + else: + cache_key = (self._normalize_cache_key(table_schema_name), ln) + self._picklist_label_cache[cache_key] = {"type": "Picklist", "ts": now} + # Attributes not in the response don't exist on the table -- cache them too + for a in attr_logicals: + if a.lower() not in found: + cache_key = (self._normalize_cache_key(table_schema_name), a.lower()) + self._picklist_label_cache[cache_key] = {"map": {}, "ts": now} + def _optionset_map(self, table_schema_name: str, attr_logical: str) -> Optional[Dict[str, int]]: """Build or return cached mapping of normalized label -> value for a picklist attribute. Returns empty dict if attribute is not a picklist or has no options. Returns None only for invalid inputs or unexpected metadata parse failures. - - Notes - ----- - - This method calls the Web API twice per attribute so it could have perf impact when there are lots of columns on the entity. """ if not table_schema_name or not attr_logical: return None + # Normalize cache key for case-insensitive lookups cache_key = (self._normalize_cache_key(table_schema_name), self._normalize_cache_key(attr_logical)) now = time.time() @@ -1239,64 +1296,12 @@ def _optionset_map(self, table_schema_name: str, attr_logical: str) -> Optional[ attr_esc = self._escape_odata_quotes(attr_logical.lower()) table_schema_name_esc = self._escape_odata_quotes(table_schema_name.lower()) - # Step 1: lightweight fetch (no expand) to determine attribute type - url_type = ( - f"{self.api}/EntityDefinitions(LogicalName='{table_schema_name_esc}')/Attributes" - f"?$filter=LogicalName eq '{attr_esc}'&$select=LogicalName,AttributeType" - ) - # Retry on 404 (metadata not yet published) before surfacing the error. - r_type = None - max_attempts = 5 - backoff_seconds = 0.4 - for attempt in range(1, max_attempts + 1): - try: - r_type = self._request("get", url_type) - break - except HttpError as err: - if getattr(err, "status_code", None) == 404: - if attempt < max_attempts: - # Exponential backoff: 0.4s, 0.8s, 1.6s, 3.2s - time.sleep(backoff_seconds * (2 ** (attempt - 1))) - continue - raise RuntimeError( - f"Picklist attribute metadata not found after retries: entity='{table_schema_name}' attribute='{attr_logical}' (404)" - ) from err - raise - if r_type is None: - raise RuntimeError("Failed to retrieve attribute metadata due to repeated request failures.") - - body_type = r_type.json() - items = body_type.get("value", []) if isinstance(body_type, dict) else [] - if not items: - return None - attr_md = items[0] - if attr_md.get("AttributeType") not in ("Picklist", "PickList"): - self._picklist_label_cache[cache_key] = {"map": {}, "ts": now} - return {} - - # Step 2: fetch with expand only now that we know it's a picklist - # Need to cast to the derived PicklistAttributeMetadata type; OptionSet is not a nav on base AttributeMetadata. + # Fetch OptionSet values for this picklist attribute cast_url = ( f"{self.api}/EntityDefinitions(LogicalName='{table_schema_name_esc}')/Attributes(LogicalName='{attr_esc}')/" "Microsoft.Dynamics.CRM.PicklistAttributeMetadata?$select=LogicalName&$expand=OptionSet($select=Options)" ) - # Step 2 fetch with retries: expanded OptionSet (cast form first) - r_opts = None - for attempt in range(1, max_attempts + 1): - try: - r_opts = self._request("get", cast_url) - break - except HttpError as err: - if getattr(err, "status_code", None) == 404: - if attempt < max_attempts: - time.sleep(backoff_seconds * (2 ** (attempt - 1))) - continue - raise RuntimeError( - f"Picklist OptionSet metadata not found after retries: entity='{table_schema_name}' attribute='{attr_logical}' (404)" - ) from err - raise - if r_opts is None: - raise RuntimeError("Failed to retrieve picklist OptionSet metadata due to repeated request failures.") + r_opts = self._request_metadata_with_retry("get", cast_url) attr_full = {} try: @@ -1335,22 +1340,54 @@ def _convert_labels_to_ints(self, table_schema_name: str, record: Dict[str, Any] Heuristic: For each string value, attempt to resolve against picklist metadata. If attribute isn't a picklist or label not found, value left unchanged. + + Performance: collects all string-valued fields with a cold cache and + issues a single batch type-check using the OData ``in`` operator + before resolving individual picklist options. """ - out = record.copy() - for k, v in list(out.items()): + resolved_record = record.copy() + # Collect candidate string-valued attribute names + candidates: List[str] = [] + for k, v in resolved_record.items(): if not isinstance(v, str) or not v.strip(): continue - # Skip OData annotations — they are not attribute names if isinstance(k, str) and "@odata." in k: continue + candidates.append(k) + if not candidates: + return resolved_record + + # Determine which candidates need a type-check (not yet cached) + now = time.time() + uncached_attrs: List[str] = [] + for attr in candidates: + cache_key = (self._normalize_cache_key(table_schema_name), self._normalize_cache_key(attr)) + entry = self._picklist_label_cache.get(cache_key) + if not ( + isinstance(entry, dict) + and "map" in entry + and (now - entry.get("ts", 0)) < self._picklist_cache_ttl_seconds + ): + uncached_attrs.append(attr) + + # Batch type-check uncached attributes in one API call + if uncached_attrs: + self._check_attribute_types(table_schema_name, uncached_attrs) + + # Only call _optionset_map for picklists + for k in candidates: + cache_key = (self._normalize_cache_key(table_schema_name), self._normalize_cache_key(k)) + entry = self._picklist_label_cache.get(cache_key) + if not (isinstance(entry, dict) and (entry.get("type") == "Picklist" or entry.get("map"))): + continue mapping = self._optionset_map(table_schema_name, k) if not mapping: continue - norm = self._normalize_picklist_label(v) + norm = self._normalize_picklist_label(resolved_record[k]) val = mapping.get(norm) if val is not None: - out[k] = val - return out + resolved_record[k] = val + return resolved_record def _attribute_payload( self, column_schema_name: str, dtype: Any, *, is_primary_name: bool = False diff --git a/tests/unit/data/test_odata_internal.py b/tests/unit/data/test_odata_internal.py index bea5a3d6..c0e8c6d3 100644 --- a/tests/unit/data/test_odata_internal.py +++ b/tests/unit/data/test_odata_internal.py @@ -506,23 +506,23 @@ def test_odata_bind_keys_preserve_case(self): def test_convert_labels_skips_odata_keys(self): """_convert_labels_to_ints should skip @odata.bind keys (no metadata lookup).""" - # Patch _optionset_map to track calls - calls = [] - original = self.od._optionset_map + # Patch _check_attribute_types to track which attrs are type-checked + checked_attrs = [] + original_check = self.od._check_attribute_types - def tracking_optionset_map(table, attr): - calls.append(attr) - return original(table, attr) + def tracking_check(table, attrs): + checked_attrs.extend(attrs) + return original_check(table, attrs) - self.od._optionset_map = tracking_optionset_map + self.od._check_attribute_types = tracking_check record = { "name": "Contoso", "new_CustomerId@odata.bind": "/contacts(00000000-0000-0000-0000-000000000001)", "@odata.type": "Microsoft.Dynamics.CRM.account", } self.od._convert_labels_to_ints("account", record) - # Only "name" should be checked, not the @odata keys - self.assertEqual(calls, ["name"]) + # Only "name" should be type-checked, not the @odata keys + self.assertEqual(checked_attrs, ["name"]) def test_returns_none(self): """_upsert always returns None.""" @@ -530,5 +530,796 @@ def test_returns_none(self): self.assertIsNone(result) +class TestPicklistLabelResolution(unittest.TestCase): + """Tests for picklist label-to-integer resolution. + + Covers _check_attribute_types, _optionset_map, _request_metadata_with_retry, + _convert_labels_to_ints, and their integration through _create / _update / _upsert. + """ + + def setUp(self): + self.od = _make_odata_client() + + # ---- _check_attribute_types ---- + + def test_check_caches_non_picklist_as_empty_map(self): + """Non-picklist attributes are cached as {"map": {}}.""" + mock_resp = MagicMock() + mock_resp.json.return_value = {"value": [{"LogicalName": "name", "AttributeType": "String"}]} + self.od._request.return_value = mock_resp + + self.od._check_attribute_types("account", ["name"]) + + entry = self.od._picklist_label_cache.get(("account", "name")) + self.assertIsNotNone(entry) + self.assertEqual(entry["map"], {}) + self.assertIn("ts", entry) + + def test_check_caches_picklist_with_type_marker(self): + """Picklist attributes are cached as {"type": "Picklist"}.""" + mock_resp = MagicMock() + mock_resp.json.return_value = {"value": [{"LogicalName": "industrycode", "AttributeType": "Picklist"}]} + self.od._request.return_value = mock_resp + + self.od._check_attribute_types("account", ["industrycode"]) + + entry = self.od._picklist_label_cache.get(("account", "industrycode")) + self.assertIsNotNone(entry) + self.assertEqual(entry.get("type"), "Picklist") + self.assertNotIn("map", entry) + + def test_check_caches_missing_attrs_as_empty_map(self): + """Attributes not in the API response are cached as non-picklist.""" + mock_resp = MagicMock() + mock_resp.json.return_value = {"value": []} + self.od._request.return_value = mock_resp + + self.od._check_attribute_types("account", ["nonexistent"]) + + entry = self.od._picklist_label_cache.get(("account", "nonexistent")) + self.assertIsNotNone(entry) + self.assertEqual(entry["map"], {}) + + def test_check_handles_mixed_types(self): + """Batch with both picklist and non-picklist attributes caches correctly.""" + mock_resp = MagicMock() + mock_resp.json.return_value = { + "value": [ + {"LogicalName": "name", "AttributeType": "String"}, + {"LogicalName": "industrycode", "AttributeType": "Picklist"}, + {"LogicalName": "telephone1", "AttributeType": "String"}, + ] + } + self.od._request.return_value = mock_resp + + self.od._check_attribute_types("account", ["name", "industrycode", "telephone1"]) + + self.assertEqual(self.od._picklist_label_cache[("account", "name")]["map"], {}) + self.assertEqual( + self.od._picklist_label_cache[("account", "industrycode")].get("type"), + "Picklist", + ) + self.assertEqual(self.od._picklist_label_cache[("account", "telephone1")]["map"], {}) + + def test_check_empty_list_does_nothing(self): + """Empty attr list should not make any API call.""" + self.od._check_attribute_types("account", []) + self.od._request.assert_not_called() + + def test_check_case_insensitive_cache_keys(self): + """Cache keys are normalized to lowercase.""" + mock_resp = MagicMock() + mock_resp.json.return_value = {"value": [{"LogicalName": "industrycode", "AttributeType": "Picklist"}]} + self.od._request.return_value = mock_resp + + self.od._check_attribute_types("Account", ["IndustryCode"]) + + self.assertIn(("account", "industrycode"), self.od._picklist_label_cache) + + def test_check_makes_single_api_call(self): + """Batch should result in exactly one API call regardless of attr count.""" + mock_resp = MagicMock() + mock_resp.json.return_value = { + "value": [ + {"LogicalName": "a", "AttributeType": "String"}, + {"LogicalName": "b", "AttributeType": "Picklist"}, + {"LogicalName": "c", "AttributeType": "Integer"}, + ] + } + self.od._request.return_value = mock_resp + + self.od._check_attribute_types("account", ["a", "b", "c"]) + + self.assertEqual(self.od._request.call_count, 1) + call_url = self.od._request.call_args.args[1] + self.assertIn("Microsoft.Dynamics.CRM.In(", call_url) + + def test_check_uses_crm_in_function_in_url(self): + """Batch URL uses Microsoft.Dynamics.CRM.In function with quoted values.""" + mock_resp = MagicMock() + mock_resp.json.return_value = {"value": []} + self.od._request.return_value = mock_resp + + self.od._check_attribute_types("account", ["name", "industrycode"]) + + call_url = self.od._request.call_args.args[1] + self.assertIn("Microsoft.Dynamics.CRM.In(PropertyName='LogicalName'", call_url) + self.assertIn('"name"', call_url) + self.assertIn('"industrycode"', call_url) + + # ---- _optionset_map ---- + + def test_optionset_returns_none_for_empty_table(self): + """_optionset_map returns None for empty table name.""" + self.assertIsNone(self.od._optionset_map("", "industrycode")) + + def test_optionset_returns_none_for_empty_attr(self): + """_optionset_map returns None for empty attribute name.""" + self.assertIsNone(self.od._optionset_map("account", "")) + + def test_optionset_returns_cached_map(self): + """Warm cache hit returns the map without API calls.""" + import time + + self.od._picklist_label_cache[("account", "industrycode")] = { + "map": {"technology": 6}, + "ts": time.time(), + } + + result = self.od._optionset_map("account", "industrycode") + self.assertEqual(result, {"technology": 6}) + self.od._request.assert_not_called() + + def test_optionset_type_marker_triggers_fetch(self): + """type=Picklist entry does not count as a cache hit -- should fetch options.""" + import time + + self.od._picklist_label_cache[("account", "industrycode")] = { + "type": "Picklist", + "ts": time.time(), + } + + mock_resp = MagicMock() + mock_resp.text = "{}" + mock_resp.json.return_value = { + "OptionSet": {"Options": [{"Value": 6, "Label": {"LocalizedLabels": [{"Label": "Tech"}]}}]} + } + self.od._request.return_value = mock_resp + + result = self.od._optionset_map("account", "industrycode") + self.assertEqual(result, {"tech": 6}) + self.od._request.assert_called_once() + + def test_optionset_fetches_via_picklist_cast_url(self): + """API call uses the PicklistAttributeMetadata cast segment.""" + mock_resp = MagicMock() + mock_resp.text = "{}" + mock_resp.json.return_value = {"OptionSet": {"Options": []}} + self.od._request.return_value = mock_resp + + self.od._optionset_map("account", "industrycode") + + call_url = self.od._request.call_args.args[1] + self.assertIn("PicklistAttributeMetadata", call_url) + self.assertIn("industrycode", call_url) + + def test_optionset_multiple_options_parsed(self): + """Multiple options are all captured in the returned mapping.""" + mock_resp = MagicMock() + mock_resp.text = "{}" + mock_resp.json.return_value = { + "OptionSet": { + "Options": [ + {"Value": 1, "Label": {"LocalizedLabels": [{"Label": "Active"}]}}, + {"Value": 2, "Label": {"LocalizedLabels": [{"Label": "Inactive"}]}}, + {"Value": 3, "Label": {"LocalizedLabels": [{"Label": "Suspended"}]}}, + ] + } + } + self.od._request.return_value = mock_resp + + result = self.od._optionset_map("account", "statuscode") + self.assertEqual(result, {"active": 1, "inactive": 2, "suspended": 3}) + + def test_optionset_caches_resolved_map_with_ts(self): + """After fetching, the resolved map is stored in cache with a timestamp.""" + import time + + mock_resp = MagicMock() + mock_resp.text = "{}" + mock_resp.json.return_value = { + "OptionSet": {"Options": [{"Value": 6, "Label": {"LocalizedLabels": [{"Label": "Tech"}]}}]} + } + self.od._request.return_value = mock_resp + + before = time.time() + self.od._optionset_map("account", "industrycode") + after = time.time() + + entry = self.od._picklist_label_cache[("account", "industrycode")] + self.assertEqual(entry["map"], {"tech": 6}) + self.assertGreaterEqual(entry["ts"], before) + self.assertLessEqual(entry["ts"], after) + + def test_optionset_returns_none_on_malformed_json(self): + """_optionset_map returns None when response JSON is unparseable.""" + mock_resp = MagicMock() + mock_resp.text = "not json" + mock_resp.json.side_effect = ValueError("No JSON") + self.od._request.return_value = mock_resp + + result = self.od._optionset_map("account", "industrycode") + self.assertIsNone(result) + + def test_optionset_returns_none_when_options_not_list(self): + """_optionset_map returns None when OptionSet.Options is not a list.""" + mock_resp = MagicMock() + mock_resp.text = "{}" + mock_resp.json.return_value = {"OptionSet": {"Options": "not-a-list"}} + self.od._request.return_value = mock_resp + + result = self.od._optionset_map("account", "industrycode") + self.assertIsNone(result) + + def test_optionset_returns_empty_dict_when_no_options(self): + """_optionset_map returns {} when Options list is empty.""" + mock_resp = MagicMock() + mock_resp.text = "{}" + mock_resp.json.return_value = {"OptionSet": {"Options": []}} + self.od._request.return_value = mock_resp + + result = self.od._optionset_map("account", "industrycode") + self.assertEqual(result, {}) + + # ---- _request_metadata_with_retry ---- + + def test_retry_succeeds_on_first_try(self): + """No retry needed when first call succeeds.""" + mock_resp = MagicMock() + self.od._request.return_value = mock_resp + + result = self.od._request_metadata_with_retry("get", "https://example.com/test") + self.assertIs(result, mock_resp) + self.assertEqual(self.od._request.call_count, 1) + + def test_retry_retries_on_404(self): + """Should retry on 404 and succeed on later attempt.""" + from PowerPlatform.Dataverse.core.errors import HttpError + + err_404 = HttpError("Not Found", status_code=404) + mock_resp = MagicMock() + self.od._request.side_effect = [err_404, mock_resp] + + result = self.od._request_metadata_with_retry("get", "https://example.com/test") + self.assertIs(result, mock_resp) + self.assertEqual(self.od._request.call_count, 2) + + def test_retry_raises_after_max_attempts(self): + """Should raise RuntimeError after all retries exhausted.""" + from PowerPlatform.Dataverse.core.errors import HttpError + + err_404 = HttpError("Not Found", status_code=404) + self.od._request.side_effect = err_404 + + with self.assertRaises(RuntimeError) as ctx: + self.od._request_metadata_with_retry("get", "https://example.com/test") + self.assertIn("404", str(ctx.exception)) + + def test_retry_does_not_retry_non_404(self): + """Non-404 errors should be raised immediately without retry.""" + from PowerPlatform.Dataverse.core.errors import HttpError + + err_500 = HttpError("Server Error", status_code=500) + self.od._request.side_effect = err_500 + + with self.assertRaises(HttpError): + self.od._request_metadata_with_retry("get", "https://example.com/test") + self.assertEqual(self.od._request.call_count, 1) + + # ---- _convert_labels_to_ints ---- + + def test_convert_no_string_values_skips_batch(self): + """Record with no string values should not trigger any API call.""" + record = {"quantity": 5, "amount": 99.99, "completed": False} + result = self.od._convert_labels_to_ints("account", record) + self.assertEqual(result, record) + self.od._request.assert_not_called() + + def test_convert_empty_record_returns_copy(self): + """Empty record returns empty dict without API calls.""" + result = self.od._convert_labels_to_ints("account", {}) + self.assertEqual(result, {}) + self.od._request.assert_not_called() + + def test_convert_whitespace_only_string_skipped(self): + """String values that are only whitespace should not be candidates.""" + record = {"name": " ", "description": ""} + result = self.od._convert_labels_to_ints("account", record) + self.assertEqual(result, record) + self.od._request.assert_not_called() + + def test_convert_odata_keys_skipped(self): + """@odata.bind keys must not be type-checked or resolved.""" + checked = [] + orig = self.od._check_attribute_types + + def track(table, attrs): + checked.extend(attrs) + return orig(table, attrs) + + self.od._check_attribute_types = track + record = { + "name": "Contoso", + "new_CustomerId@odata.bind": "/contacts(guid)", + "@odata.type": "Microsoft.Dynamics.CRM.account", + } + self.od._convert_labels_to_ints("account", record) + self.assertEqual(checked, ["name"]) + + def test_convert_warm_cache_no_api_calls(self): + """Second call with same fields should make zero API calls.""" + import time + + now = time.time() + self.od._picklist_label_cache[("account", "name")] = {"map": {}, "ts": now} + self.od._picklist_label_cache[("account", "industrycode")] = { + "map": {"technology": 6}, + "ts": now, + } + + record = {"name": "Contoso", "industrycode": "Technology"} + result = self.od._convert_labels_to_ints("account", record) + + self.assertEqual(result["industrycode"], 6) + self.assertEqual(result["name"], "Contoso") + self.od._request.assert_not_called() + + def test_convert_resolves_picklist_label_to_int(self): + """Full flow: batch identifies picklist, optionset_map fetches options, label resolved.""" + batch_resp = MagicMock() + batch_resp.json.return_value = { + "value": [ + {"LogicalName": "name", "AttributeType": "String"}, + {"LogicalName": "industrycode", "AttributeType": "Picklist"}, + ] + } + options_resp = MagicMock() + options_resp.text = '{"OptionSet": ...}' + options_resp.json.return_value = { + "OptionSet": { + "Options": [ + { + "Value": 6, + "Label": {"LocalizedLabels": [{"Label": "Technology", "LanguageCode": 1033}]}, + } + ] + } + } + self.od._request.side_effect = [batch_resp, options_resp] + + record = {"name": "Contoso", "industrycode": "Technology"} + result = self.od._convert_labels_to_ints("account", record) + + self.assertEqual(result["industrycode"], 6) + self.assertEqual(result["name"], "Contoso") + self.assertEqual(self.od._request.call_count, 2) + + def test_convert_non_picklist_skips_optionset_map(self): + """Non-picklist fields should not trigger _optionset_map calls.""" + batch_resp = MagicMock() + batch_resp.json.return_value = { + "value": [ + {"LogicalName": "name", "AttributeType": "String"}, + {"LogicalName": "telephone1", "AttributeType": "String"}, + ] + } + self.od._request.return_value = batch_resp + + optionset_calls = [] + orig_map = self.od._optionset_map + + def tracking_map(table, attr): + optionset_calls.append(attr) + return orig_map(table, attr) + + self.od._optionset_map = tracking_map + + record = {"name": "Contoso", "telephone1": "555-0100"} + self.od._convert_labels_to_ints("account", record) + + self.assertEqual(optionset_calls, []) + + def test_convert_unmatched_label_left_unchanged(self): + """If a picklist label doesn't match any option, value stays as string.""" + import time + + self.od._picklist_label_cache[("account", "industrycode")] = { + "map": {"technology": 6, "consulting": 12}, + "ts": time.time(), + } + + record = {"industrycode": "UnknownIndustry"} + result = self.od._convert_labels_to_ints("account", record) + self.assertEqual(result["industrycode"], "UnknownIndustry") + + def test_convert_does_not_mutate_original_record(self): + """_convert_labels_to_ints must return a copy, not mutate the input.""" + import time + + self.od._picklist_label_cache[("account", "industrycode")] = { + "map": {"technology": 6}, + "ts": time.time(), + } + + original = {"industrycode": "Technology"} + result = self.od._convert_labels_to_ints("account", original) + + self.assertEqual(result["industrycode"], 6) + self.assertEqual(original["industrycode"], "Technology") + + def test_convert_only_batches_uncached_attrs(self): + """Warm-cached attrs should not be included in the batch call.""" + import time + + self.od._picklist_label_cache[("account", "name")] = { + "map": {}, + "ts": time.time(), + } + + batch_resp = MagicMock() + batch_resp.json.return_value = {"value": [{"LogicalName": "industrycode", "AttributeType": "Picklist"}]} + options_resp = MagicMock() + options_resp.text = "{}" + options_resp.json.return_value = { + "OptionSet": {"Options": [{"Value": 6, "Label": {"LocalizedLabels": [{"Label": "Tech"}]}}]} + } + self.od._request.side_effect = [batch_resp, options_resp] + + record = {"name": "Contoso", "industrycode": "Tech"} + result = self.od._convert_labels_to_ints("account", record) + + batch_call_url = self.od._request.call_args_list[0].args[1] + self.assertIn("industrycode", batch_call_url) + self.assertNotIn("'name'", batch_call_url) + self.assertEqual(result["industrycode"], 6) + + def test_convert_multiple_picklists_in_one_record(self): + """Multiple picklist fields in the same record are all resolved.""" + batch_resp = MagicMock() + batch_resp.json.return_value = { + "value": [ + {"LogicalName": "industrycode", "AttributeType": "Picklist"}, + {"LogicalName": "statuscode", "AttributeType": "Picklist"}, + ] + } + industry_resp = MagicMock() + industry_resp.text = "{}" + industry_resp.json.return_value = { + "OptionSet": {"Options": [{"Value": 6, "Label": {"LocalizedLabels": [{"Label": "Tech"}]}}]} + } + status_resp = MagicMock() + status_resp.text = "{}" + status_resp.json.return_value = { + "OptionSet": {"Options": [{"Value": 1, "Label": {"LocalizedLabels": [{"Label": "Active"}]}}]} + } + self.od._request.side_effect = [batch_resp, industry_resp, status_resp] + + record = {"industrycode": "Tech", "statuscode": "Active"} + result = self.od._convert_labels_to_ints("account", record) + + self.assertEqual(result["industrycode"], 6) + self.assertEqual(result["statuscode"], 1) + # 1 batch + 2 optionset fetches + self.assertEqual(self.od._request.call_count, 3) + + def test_convert_mixed_picklists_and_non_picklists(self): + """2 picklists + 2 non-picklist strings: 1 batch + 2 optionset = 3 calls.""" + batch_resp = MagicMock() + batch_resp.json.return_value = { + "value": [ + {"LogicalName": "name", "AttributeType": "String"}, + {"LogicalName": "industrycode", "AttributeType": "Picklist"}, + {"LogicalName": "description", "AttributeType": "Memo"}, + {"LogicalName": "statuscode", "AttributeType": "Picklist"}, + ] + } + industry_resp = MagicMock() + industry_resp.text = "{}" + industry_resp.json.return_value = { + "OptionSet": {"Options": [{"Value": 6, "Label": {"LocalizedLabels": [{"Label": "Tech"}]}}]} + } + status_resp = MagicMock() + status_resp.text = "{}" + status_resp.json.return_value = { + "OptionSet": {"Options": [{"Value": 1, "Label": {"LocalizedLabels": [{"Label": "Active"}]}}]} + } + self.od._request.side_effect = [batch_resp, industry_resp, status_resp] + + record = { + "name": "Contoso", + "industrycode": "Tech", + "description": "A company", + "statuscode": "Active", + } + result = self.od._convert_labels_to_ints("account", record) + + self.assertEqual(result["industrycode"], 6) + self.assertEqual(result["statuscode"], 1) + self.assertEqual(result["name"], "Contoso") + self.assertEqual(result["description"], "A company") + # 1 batch + 2 optionset fetches (non-picklists don't trigger optionset) + self.assertEqual(self.od._request.call_count, 3) + + def test_convert_all_non_picklist_makes_one_api_call(self): + """All non-picklist string fields: 1 batch call, 0 optionset = 1 total.""" + batch_resp = MagicMock() + batch_resp.json.return_value = { + "value": [ + {"LogicalName": "name", "AttributeType": "String"}, + {"LogicalName": "description", "AttributeType": "Memo"}, + {"LogicalName": "telephone1", "AttributeType": "String"}, + ] + } + self.od._request.return_value = batch_resp + + record = {"name": "Contoso", "description": "A company", "telephone1": "555-0100"} + self.od._convert_labels_to_ints("account", record) + + # Only the batch type-check, no optionset fetches + self.assertEqual(self.od._request.call_count, 1) + + def test_convert_no_string_values_makes_zero_api_calls(self): + """All non-string values: 0 API calls total.""" + record = {"revenue": 1000000, "quantity": 5, "active": True} + self.od._convert_labels_to_ints("account", record) + + self.assertEqual(self.od._request.call_count, 0) + + def test_convert_partial_cache_only_batches_uncached(self): + """1 cached non-picklist + 1 uncached picklist: 1 batch + 1 optionset = 2 calls.""" + import time + + # Pre-cache "name" as non-picklist + self.od._picklist_label_cache[("account", "name")] = {"map": {}, "ts": time.time()} + + batch_resp = MagicMock() + batch_resp.json.return_value = { + "value": [{"LogicalName": "industrycode", "AttributeType": "Picklist"}] + } + options_resp = MagicMock() + options_resp.text = "{}" + options_resp.json.return_value = { + "OptionSet": {"Options": [{"Value": 6, "Label": {"LocalizedLabels": [{"Label": "Tech"}]}}]} + } + self.od._request.side_effect = [batch_resp, options_resp] + + record = {"name": "Contoso", "industrycode": "Tech"} + result = self.od._convert_labels_to_ints("account", record) + + self.assertEqual(result["industrycode"], 6) + self.assertEqual(result["name"], "Contoso") + # 1 batch (only industrycode) + 1 optionset fetch + self.assertEqual(self.od._request.call_count, 2) + + def test_convert_single_picklist_makes_two_api_calls(self): + """Single picklist field (cold cache): 1 batch + 1 optionset = 2 total.""" + batch_resp = MagicMock() + batch_resp.json.return_value = { + "value": [{"LogicalName": "industrycode", "AttributeType": "Picklist"}] + } + options_resp = MagicMock() + options_resp.text = "{}" + options_resp.json.return_value = { + "OptionSet": {"Options": [{"Value": 6, "Label": {"LocalizedLabels": [{"Label": "Tech"}]}}]} + } + self.od._request.side_effect = [batch_resp, options_resp] + + record = {"industrycode": "Tech"} + result = self.od._convert_labels_to_ints("account", record) + + self.assertEqual(result["industrycode"], 6) + self.assertEqual(self.od._request.call_count, 2) + + def test_convert_integer_values_passed_through(self): + """Integer values (already resolved) are left unchanged.""" + import time + + self.od._picklist_label_cache[("account", "industrycode")] = { + "map": {"technology": 6}, + "ts": time.time(), + } + + record = {"industrycode": 6, "name": "Contoso"} + result = self.od._convert_labels_to_ints("account", record) + self.assertEqual(result["industrycode"], 6) + + def test_convert_case_insensitive_label_matching(self): + """Picklist label matching is case-insensitive.""" + import time + + self.od._picklist_label_cache[("account", "industrycode")] = { + "map": {"technology": 6}, + "ts": time.time(), + } + + record = {"industrycode": "TECHNOLOGY"} + result = self.od._convert_labels_to_ints("account", record) + self.assertEqual(result["industrycode"], 6) + + # ---- Integration: through _create ---- + + def test_create_resolves_picklist_in_payload(self): + """_create resolves a picklist label to its integer in the POST payload.""" + batch_resp = MagicMock() + batch_resp.json.return_value = { + "value": [ + {"LogicalName": "name", "AttributeType": "String"}, + {"LogicalName": "industrycode", "AttributeType": "Picklist"}, + ] + } + options_resp = MagicMock() + options_resp.text = "{}" + options_resp.json.return_value = { + "OptionSet": { + "Options": [ + { + "Value": 6, + "Label": {"LocalizedLabels": [{"Label": "Technology", "LanguageCode": 1033}]}, + } + ] + } + } + post_resp = MagicMock() + post_resp.headers = { + "OData-EntityId": "https://example.crm.dynamics.com/api/data/v9.2/accounts(00000000-0000-0000-0000-000000000001)" + } + self.od._request.side_effect = [batch_resp, options_resp, post_resp] + + result = self.od._create("accounts", "account", {"name": "Contoso", "industrycode": "Technology"}) + self.assertEqual(result, "00000000-0000-0000-0000-000000000001") + post_calls = [c for c in self.od._request.call_args_list if c.args[0] == "post"] + payload = post_calls[0].kwargs["json"] + self.assertEqual(payload["industrycode"], 6) + self.assertEqual(payload["name"], "Contoso") + + def test_create_warm_cache_skips_batch(self): + """_create with warm cache makes only the POST call.""" + import time + + now = time.time() + self.od._picklist_label_cache[("account", "industrycode")] = { + "map": {"technology": 6}, + "ts": now, + } + self.od._picklist_label_cache[("account", "name")] = {"map": {}, "ts": now} + + post_resp = MagicMock() + post_resp.headers = { + "OData-EntityId": "https://example.crm.dynamics.com/api/data/v9.2/accounts(00000000-0000-0000-0000-000000000001)" + } + self.od._request.return_value = post_resp + + result = self.od._create("accounts", "account", {"name": "Contoso", "industrycode": "Technology"}) + self.assertEqual(result, "00000000-0000-0000-0000-000000000001") + self.assertEqual(self.od._request.call_count, 1) + payload = self.od._request.call_args.kwargs["json"] + self.assertEqual(payload["industrycode"], 6) + + # ---- Integration: through _update ---- + + def test_update_resolves_picklist_in_payload(self): + """_update resolves a picklist label to its integer in the PATCH payload.""" + self.od._entity_set_from_schema_name = MagicMock(return_value="new_tickets") + + batch_resp = MagicMock() + batch_resp.json.return_value = { + "value": [ + {"LogicalName": "new_status", "AttributeType": "Picklist"}, + ] + } + options_resp = MagicMock() + options_resp.text = "{}" + options_resp.json.return_value = { + "OptionSet": { + "Options": [ + { + "Value": 100000001, + "Label": {"LocalizedLabels": [{"Label": "In Progress", "LanguageCode": 1033}]}, + } + ] + } + } + patch_resp = MagicMock() + self.od._request.side_effect = [batch_resp, options_resp, patch_resp] + + self.od._update( + "new_ticket", + "00000000-0000-0000-0000-000000000001", + {"new_status": "In Progress"}, + ) + patch_calls = [c for c in self.od._request.call_args_list if c.args[0] == "patch"] + payload = patch_calls[0].kwargs["json"] + self.assertEqual(payload["new_status"], 100000001) + + def test_update_warm_cache_skips_batch(self): + """_update with warm cache makes only the PATCH call.""" + import time + + self.od._entity_set_from_schema_name = MagicMock(return_value="new_tickets") + self.od._picklist_label_cache[("new_ticket", "new_status")] = { + "map": {"in progress": 100000001}, + "ts": time.time(), + } + + self.od._update( + "new_ticket", + "00000000-0000-0000-0000-000000000001", + {"new_status": "In Progress"}, + ) + self.assertEqual(self.od._request.call_count, 1) + self.assertEqual(self.od._request.call_args.args[0], "patch") + payload = self.od._request.call_args.kwargs["json"] + self.assertEqual(payload["new_status"], 100000001) + + # ---- Integration: through _upsert ---- + + def test_upsert_resolves_picklist_in_payload(self): + """_upsert resolves a picklist label to its integer in the PATCH payload.""" + batch_resp = MagicMock() + batch_resp.json.return_value = { + "value": [ + {"LogicalName": "industrycode", "AttributeType": "Picklist"}, + {"LogicalName": "name", "AttributeType": "String"}, + ] + } + options_resp = MagicMock() + options_resp.text = "{}" + options_resp.json.return_value = { + "OptionSet": { + "Options": [ + { + "Value": 6, + "Label": {"LocalizedLabels": [{"Label": "Technology", "LanguageCode": 1033}]}, + } + ] + } + } + patch_resp = MagicMock() + self.od._request.side_effect = [batch_resp, options_resp, patch_resp] + + self.od._upsert( + "accounts", + "account", + {"accountnumber": "ACC-001"}, + {"name": "Contoso", "industrycode": "Technology"}, + ) + patch_calls = [c for c in self.od._request.call_args_list if c.args[0] == "patch"] + payload = patch_calls[0].kwargs["json"] + self.assertEqual(payload["industrycode"], 6) + self.assertEqual(payload["name"], "Contoso") + + def test_upsert_warm_cache_skips_batch(self): + """_upsert with warm cache makes only the PATCH call.""" + import time + + now = time.time() + self.od._picklist_label_cache[("account", "name")] = {"map": {}, "ts": now} + self.od._picklist_label_cache[("account", "industrycode")] = { + "map": {"technology": 6}, + "ts": now, + } + + self.od._upsert( + "accounts", + "account", + {"accountnumber": "ACC-001"}, + {"name": "Contoso", "industrycode": "Technology"}, + ) + self.assertEqual(self.od._request.call_count, 1) + patch_calls = [c for c in self.od._request.call_args_list if c.args[0] == "patch"] + payload = patch_calls[0].kwargs["json"] + self.assertEqual(payload["industrycode"], 6) + + if __name__ == "__main__": unittest.main() From 821d4a0f2460e366614901e0d0b432d46e2f03b9 Mon Sep 17 00:00:00 2001 From: Abel Milash Date: Wed, 25 Mar 2026 00:08:02 -0700 Subject: [PATCH 2/5] Fix stale docstrings and apply black formatting --- examples/advanced/walkthrough.py | 4 +++- src/PowerPlatform/Dataverse/data/_odata.py | 12 ++++-------- tests/unit/data/test_odata_internal.py | 8 ++------ 3 files changed, 9 insertions(+), 15 deletions(-) diff --git a/examples/advanced/walkthrough.py b/examples/advanced/walkthrough.py index a50b86dd..e1eed5cd 100644 --- a/examples/advanced/walkthrough.py +++ b/examples/advanced/walkthrough.py @@ -450,7 +450,9 @@ def _run_walkthrough(client): updated_label = backoff(lambda: client.records.get(table_name, label_id)) print(f"[OK] Updated record with string label 'Low' for new_Priority") print(f" new_Priority stored as integer: {updated_label.get('new_priority')}") - print(f" new_Priority@FormattedValue: {updated_label.get('new_priority@OData.Community.Display.V1.FormattedValue')}") + print( + f" new_Priority@FormattedValue: {updated_label.get('new_priority@OData.Community.Display.V1.FormattedValue')}" + ) # ============================================================================ # 11. COLUMN MANAGEMENT diff --git a/src/PowerPlatform/Dataverse/data/_odata.py b/src/PowerPlatform/Dataverse/data/_odata.py index cdfeb406..8cb54087 100644 --- a/src/PowerPlatform/Dataverse/data/_odata.py +++ b/src/PowerPlatform/Dataverse/data/_odata.py @@ -1235,7 +1235,7 @@ def _request_metadata_with_retry(self, method: str, url: str, **kwargs): def _check_attribute_types(self, table_schema_name: str, attr_logicals: List[str]) -> None: """Batch-check AttributeType for multiple attributes in one API call. - Uses an OData ``or`` filter to check all given attributes at once. + Uses ``Microsoft.Dynamics.CRM.In`` to check all given attributes at once. Non-picklist attributes (and attributes not found) are cached with an empty map. Picklist attributes are cached with ``{"type": "Picklist"}`` so that ``_optionset_map`` knows to fetch their options. @@ -1243,12 +1243,8 @@ def _check_attribute_types(self, table_schema_name: str, attr_logicals: List[str if not attr_logicals: return table_esc = self._escape_odata_quotes(table_schema_name.lower()) - quoted_values = ",".join( - f'"{self._escape_odata_quotes(a.lower())}"' for a in attr_logicals - ) - attr_filter = ( - f"Microsoft.Dynamics.CRM.In(PropertyName='LogicalName',PropertyValues=[{quoted_values}])" - ) + quoted_values = ",".join(f'"{self._escape_odata_quotes(a.lower())}"' for a in attr_logicals) + attr_filter = f"Microsoft.Dynamics.CRM.In(PropertyName='LogicalName',PropertyValues=[{quoted_values}])" url = ( f"{self.api}/EntityDefinitions(LogicalName='{table_esc}')/Attributes" f"?$filter={attr_filter}&$select=LogicalName,AttributeType" @@ -1342,7 +1338,7 @@ def _convert_labels_to_ints(self, table_schema_name: str, record: Dict[str, Any] If attribute isn't a picklist or label not found, value left unchanged. Performance: collects all string-valued fields with a cold cache and - issues a single batch type-check using the OData ``in`` operator + issues a single batch type-check using ``Microsoft.Dynamics.CRM.In`` before resolving individual picklist options. """ resolved_record = record.copy() diff --git a/tests/unit/data/test_odata_internal.py b/tests/unit/data/test_odata_internal.py index c0e8c6d3..273197e6 100644 --- a/tests/unit/data/test_odata_internal.py +++ b/tests/unit/data/test_odata_internal.py @@ -1083,9 +1083,7 @@ def test_convert_partial_cache_only_batches_uncached(self): self.od._picklist_label_cache[("account", "name")] = {"map": {}, "ts": time.time()} batch_resp = MagicMock() - batch_resp.json.return_value = { - "value": [{"LogicalName": "industrycode", "AttributeType": "Picklist"}] - } + batch_resp.json.return_value = {"value": [{"LogicalName": "industrycode", "AttributeType": "Picklist"}]} options_resp = MagicMock() options_resp.text = "{}" options_resp.json.return_value = { @@ -1104,9 +1102,7 @@ def test_convert_partial_cache_only_batches_uncached(self): def test_convert_single_picklist_makes_two_api_calls(self): """Single picklist field (cold cache): 1 batch + 1 optionset = 2 total.""" batch_resp = MagicMock() - batch_resp.json.return_value = { - "value": [{"LogicalName": "industrycode", "AttributeType": "Picklist"}] - } + batch_resp.json.return_value = {"value": [{"LogicalName": "industrycode", "AttributeType": "Picklist"}]} options_resp = MagicMock() options_resp.text = "{}" options_resp.json.return_value = { From 203af1ec8f0525f161c3936a1d803fe905f97390 Mon Sep 17 00:00:00 2001 From: Abel Milash Date: Wed, 25 Mar 2026 00:15:11 -0700 Subject: [PATCH 3/5] Add edge case tests for batch failure propagation and missing AttributeType --- tests/unit/data/test_odata_internal.py | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/tests/unit/data/test_odata_internal.py b/tests/unit/data/test_odata_internal.py index 273197e6..a646b064 100644 --- a/tests/unit/data/test_odata_internal.py +++ b/tests/unit/data/test_odata_internal.py @@ -647,6 +647,18 @@ def test_check_uses_crm_in_function_in_url(self): self.assertIn('"name"', call_url) self.assertIn('"industrycode"', call_url) + def test_check_missing_attribute_type_cached_as_non_picklist(self): + """Attribute with missing AttributeType in response is cached as non-picklist.""" + mock_resp = MagicMock() + mock_resp.json.return_value = {"value": [{"LogicalName": "customfield"}]} # no AttributeType key + self.od._request.return_value = mock_resp + + self.od._check_attribute_types("account", ["customfield"]) + + entry = self.od._picklist_label_cache.get(("account", "customfield")) + self.assertIsNotNone(entry) + self.assertEqual(entry["map"], {}) + # ---- _optionset_map ---- def test_optionset_returns_none_for_empty_table(self): @@ -1099,6 +1111,15 @@ def test_convert_partial_cache_only_batches_uncached(self): # 1 batch (only industrycode) + 1 optionset fetch self.assertEqual(self.od._request.call_count, 2) + def test_convert_batch_failure_propagates(self): + """Server error during batch type-check propagates to caller.""" + from PowerPlatform.Dataverse.core.errors import HttpError + + self.od._request.side_effect = HttpError("Server Error", status_code=500) + + with self.assertRaises(HttpError): + self.od._convert_labels_to_ints("account", {"name": "Contoso"}) + def test_convert_single_picklist_makes_two_api_calls(self): """Single picklist field (cold cache): 1 batch + 1 optionset = 2 total.""" batch_resp = MagicMock() From c1f46e6d73349889d39ae03df78c165ca330e3a8 Mon Sep 17 00:00:00 2001 From: Abel Milash Date: Thu, 26 Mar 2026 12:29:00 -0700 Subject: [PATCH 4/5] Option C: bulk fetch all picklists in single API call --- src/PowerPlatform/Dataverse/data/_odata.py | 191 ++--- tests/unit/data/test_odata_internal.py | 880 ++++++++++----------- tests/unit/test_context_manager.py | 2 +- 3 files changed, 473 insertions(+), 600 deletions(-) diff --git a/src/PowerPlatform/Dataverse/data/_odata.py b/src/PowerPlatform/Dataverse/data/_odata.py index 8cb54087..8d22e098 100644 --- a/src/PowerPlatform/Dataverse/data/_odata.py +++ b/src/PowerPlatform/Dataverse/data/_odata.py @@ -160,8 +160,7 @@ def __init__( self._logical_to_entityset_cache: dict[str, str] = {} # Cache: normalized table_schema_name (lowercase) -> primary id attribute (e.g. accountid) self._logical_primaryid_cache: dict[str, str] = {} - # Picklist label cache: (normalized_table_schema_name, normalized_attribute) -> {'map': {...}, 'ts': epoch_seconds} - self._picklist_label_cache = {} + self._picklist_label_cache: dict[str, dict] = {} self._picklist_cache_ttl_seconds = 3600 # 1 hour TTL @contextmanager @@ -1217,7 +1216,7 @@ def _normalize_picklist_label(self, label: str) -> str: return norm def _request_metadata_with_retry(self, method: str, url: str, **kwargs): - """Fetch metadata with retries on transient 404 responses.""" + """Fetch metadata with retries on transient errors.""" max_attempts = 5 backoff_seconds = 0.4 for attempt in range(1, max_attempts + 1): @@ -1232,104 +1231,58 @@ def _request_metadata_with_retry(self, method: str, url: str, **kwargs): raise raise RuntimeError(f"Metadata request failed: {url}") - def _check_attribute_types(self, table_schema_name: str, attr_logicals: List[str]) -> None: - """Batch-check AttributeType for multiple attributes in one API call. + def _bulk_fetch_picklists(self, table_schema_name: str) -> None: + """Fetch all picklist attributes and their options for a table in one API call. - Uses ``Microsoft.Dynamics.CRM.In`` to check all given attributes at once. - Non-picklist attributes (and attributes not found) are cached with an - empty map. Picklist attributes are cached with ``{"type": "Picklist"}`` - so that ``_optionset_map`` knows to fetch their options. + Uses collection-level PicklistAttributeMetadata cast to retrieve every picklist + attribute on the table, including its OptionSet options. Populates the nested + cache so that ``_convert_labels_to_ints`` resolves labels without further API calls. """ - if not attr_logicals: + table_key = self._normalize_cache_key(table_schema_name) + now = time.time() + table_entry = self._picklist_label_cache.get(table_key) + if isinstance(table_entry, dict) and (now - table_entry.get("ts", 0)) < self._picklist_cache_ttl_seconds: return + table_esc = self._escape_odata_quotes(table_schema_name.lower()) - quoted_values = ",".join(f'"{self._escape_odata_quotes(a.lower())}"' for a in attr_logicals) - attr_filter = f"Microsoft.Dynamics.CRM.In(PropertyName='LogicalName',PropertyValues=[{quoted_values}])" url = ( - f"{self.api}/EntityDefinitions(LogicalName='{table_esc}')/Attributes" - f"?$filter={attr_filter}&$select=LogicalName,AttributeType" + f"{self.api}/EntityDefinitions(LogicalName='{table_esc}')" + f"/Attributes/Microsoft.Dynamics.CRM.PicklistAttributeMetadata" + f"?$select=LogicalName&$expand=OptionSet($select=Options)" ) - r = self._request_metadata_with_retry("get", url) - body = r.json() + response = self._request_metadata_with_retry("get", url) + body = response.json() items = body.get("value", []) if isinstance(body, dict) else [] - now = time.time() - found: set[str] = set() + + picklists: Dict[str, Dict[str, int]] = {} for item in items: if not isinstance(item, dict): continue ln = item.get("LogicalName", "").lower() - atype = item.get("AttributeType", "") - found.add(ln) - if atype not in ("Picklist", "PickList"): - cache_key = (self._normalize_cache_key(table_schema_name), ln) - self._picklist_label_cache[cache_key] = {"map": {}, "ts": now} - else: - cache_key = (self._normalize_cache_key(table_schema_name), ln) - self._picklist_label_cache[cache_key] = {"type": "Picklist", "ts": now} - # Attributes not in the response don't exist on the table -- cache them too - for a in attr_logicals: - if a.lower() not in found: - cache_key = (self._normalize_cache_key(table_schema_name), a.lower()) - self._picklist_label_cache[cache_key] = {"map": {}, "ts": now} - - def _optionset_map(self, table_schema_name: str, attr_logical: str) -> Optional[Dict[str, int]]: - """Build or return cached mapping of normalized label -> value for a picklist attribute. - - Returns empty dict if attribute is not a picklist or has no options. Returns None only - for invalid inputs or unexpected metadata parse failures. - """ - if not table_schema_name or not attr_logical: - return None - - # Normalize cache key for case-insensitive lookups - cache_key = (self._normalize_cache_key(table_schema_name), self._normalize_cache_key(attr_logical)) - now = time.time() - entry = self._picklist_label_cache.get(cache_key) - if isinstance(entry, dict) and "map" in entry and (now - entry.get("ts", 0)) < self._picklist_cache_ttl_seconds: - return entry["map"] - - # LogicalNames in Dataverse are stored in lowercase, so we need to lowercase for filters - attr_esc = self._escape_odata_quotes(attr_logical.lower()) - table_schema_name_esc = self._escape_odata_quotes(table_schema_name.lower()) - - # Fetch OptionSet values for this picklist attribute - cast_url = ( - f"{self.api}/EntityDefinitions(LogicalName='{table_schema_name_esc}')/Attributes(LogicalName='{attr_esc}')/" - "Microsoft.Dynamics.CRM.PicklistAttributeMetadata?$select=LogicalName&$expand=OptionSet($select=Options)" - ) - r_opts = self._request_metadata_with_retry("get", cast_url) - - attr_full = {} - try: - attr_full = r_opts.json() if r_opts.text else {} - except ValueError: - return None - option_set = attr_full.get("OptionSet") or {} - options = option_set.get("Options") if isinstance(option_set, dict) else None - if not isinstance(options, list): - return None - mapping: Dict[str, int] = {} - for opt in options: - if not isinstance(opt, dict): + if not ln: continue - val = opt.get("Value") - if not isinstance(val, int): - continue - label_def = opt.get("Label") or {} - locs = label_def.get("LocalizedLabels") - if isinstance(locs, list): - for loc in locs: - if isinstance(loc, dict): - lab = loc.get("Label") - if isinstance(lab, str) and lab.strip(): - normalized = self._normalize_picklist_label(lab) - mapping.setdefault(normalized, val) - if mapping: - self._picklist_label_cache[cache_key] = {"map": mapping, "ts": now} - return mapping - # No options available - self._picklist_label_cache[cache_key] = {"map": {}, "ts": now} - return {} + option_set = item.get("OptionSet") or {} + options = option_set.get("Options") if isinstance(option_set, dict) else None + mapping: Dict[str, int] = {} + if isinstance(options, list): + for opt in options: + if not isinstance(opt, dict): + continue + val = opt.get("Value") + if not isinstance(val, int): + continue + label_def = opt.get("Label") or {} + locs = label_def.get("LocalizedLabels") + if isinstance(locs, list): + for loc in locs: + if isinstance(loc, dict): + lab = loc.get("Label") + if isinstance(lab, str) and lab.strip(): + normalized = self._normalize_picklist_label(lab) + mapping.setdefault(normalized, val) + picklists[ln] = mapping + + self._picklist_label_cache[table_key] = {"ts": now, "picklists": picklists} def _convert_labels_to_ints(self, table_schema_name: str, record: Dict[str, Any]) -> Dict[str, Any]: """Return a copy of record with any labels converted to option ints. @@ -1337,49 +1290,39 @@ def _convert_labels_to_ints(self, table_schema_name: str, record: Dict[str, Any] Heuristic: For each string value, attempt to resolve against picklist metadata. If attribute isn't a picklist or label not found, value left unchanged. - Performance: collects all string-valued fields with a cold cache and - issues a single batch type-check using ``Microsoft.Dynamics.CRM.In`` - before resolving individual picklist options. + On first encounter of a table, bulk-fetches all picklist attributes and + their options in a single API call, then resolves labels from the warm cache. """ resolved_record = record.copy() - # Collect candidate string-valued attribute names - candidates: List[str] = [] + + # Check if there are any string-valued candidates worth resolving + has_candidates = any( + isinstance(v, str) and v.strip() and isinstance(k, str) and "@odata." not in k + for k, v in resolved_record.items() + ) + if not has_candidates: + return resolved_record + + # Bulk-fetch all picklists for this table (1 API call, cached for TTL) + self._bulk_fetch_picklists(table_schema_name) + + # Resolve labels from the nested cache + table_key = self._normalize_cache_key(table_schema_name) + table_entry = self._picklist_label_cache.get(table_key) + if not isinstance(table_entry, dict): + return resolved_record + picklists = table_entry.get("picklists", {}) + for k, v in resolved_record.items(): if not isinstance(v, str) or not v.strip(): continue if isinstance(k, str) and "@odata." in k: continue - candidates.append(k) - if not candidates: - return resolved_record - - # Determine which candidates need a type-check (not yet cached) - now = time.time() - uncached_attrs: List[str] = [] - for attr in candidates: - cache_key = (self._normalize_cache_key(table_schema_name), self._normalize_cache_key(attr)) - entry = self._picklist_label_cache.get(cache_key) - if not ( - isinstance(entry, dict) - and "map" in entry - and (now - entry.get("ts", 0)) < self._picklist_cache_ttl_seconds - ): - uncached_attrs.append(attr) - - # Batch type-check uncached attributes in one API call - if uncached_attrs: - self._check_attribute_types(table_schema_name, uncached_attrs) - - # Only call _optionset_map for picklists - for k in candidates: - cache_key = (self._normalize_cache_key(table_schema_name), self._normalize_cache_key(k)) - entry = self._picklist_label_cache.get(cache_key) - if not (isinstance(entry, dict) and (entry.get("type") == "Picklist" or entry.get("map"))): - continue - mapping = self._optionset_map(table_schema_name, k) - if not mapping: + attr_key = self._normalize_cache_key(k) + mapping = picklists.get(attr_key) + if not isinstance(mapping, dict) or not mapping: continue - norm = self._normalize_picklist_label(resolved_record[k]) + norm = self._normalize_picklist_label(v) val = mapping.get(norm) if val is not None: resolved_record[k] = val diff --git a/tests/unit/data/test_odata_internal.py b/tests/unit/data/test_odata_internal.py index a646b064..4384f326 100644 --- a/tests/unit/data/test_odata_internal.py +++ b/tests/unit/data/test_odata_internal.py @@ -2,7 +2,7 @@ # Licensed under the MIT license. import unittest -from unittest.mock import MagicMock +from unittest.mock import MagicMock, patch from PowerPlatform.Dataverse.data._odata import _ODataClient @@ -506,23 +506,24 @@ def test_odata_bind_keys_preserve_case(self): def test_convert_labels_skips_odata_keys(self): """_convert_labels_to_ints should skip @odata.bind keys (no metadata lookup).""" - # Patch _check_attribute_types to track which attrs are type-checked - checked_attrs = [] - original_check = self.od._check_attribute_types + import time - def tracking_check(table, attrs): - checked_attrs.extend(attrs) - return original_check(table, attrs) + # Pre-populate cache so no API call needed + self.od._picklist_label_cache["account"] = { + "ts": time.time(), + "picklists": {}, + } - self.od._check_attribute_types = tracking_check record = { "name": "Contoso", "new_CustomerId@odata.bind": "/contacts(00000000-0000-0000-0000-000000000001)", "@odata.type": "Microsoft.Dynamics.CRM.account", } - self.od._convert_labels_to_ints("account", record) - # Only "name" should be type-checked, not the @odata keys - self.assertEqual(checked_attrs, ["name"]) + result = self.od._convert_labels_to_ints("account", record) + # @odata keys must be left unchanged + self.assertEqual(result["new_CustomerId@odata.bind"], "/contacts(00000000-0000-0000-0000-000000000001)") + self.assertEqual(result["@odata.type"], "Microsoft.Dynamics.CRM.account") + self.assertEqual(result["name"], "Contoso") def test_returns_none(self): """_upsert always returns None.""" @@ -533,255 +534,141 @@ def test_returns_none(self): class TestPicklistLabelResolution(unittest.TestCase): """Tests for picklist label-to-integer resolution. - Covers _check_attribute_types, _optionset_map, _request_metadata_with_retry, + Covers _bulk_fetch_picklists, _request_metadata_with_retry, _convert_labels_to_ints, and their integration through _create / _update / _upsert. + + Cache structure (nested): + _picklist_label_cache = { + "table_key": {"ts": epoch, "picklists": {"attr": {norm_label: int}}} + } """ def setUp(self): self.od = _make_odata_client() - # ---- _check_attribute_types ---- + # ---- Helper to build a bulk-fetch API response ---- + @staticmethod + def _bulk_response(*picklists): + """Build a mock response for _bulk_fetch_picklists. + + Each picklist is (logical_name, [(value, label), ...]). + """ + items = [] + for ln, options in picklists: + opts = [{"Value": val, "Label": {"LocalizedLabels": [{"Label": lab}]}} for val, lab in options] + items.append({"LogicalName": ln, "OptionSet": {"Options": opts}}) + resp = MagicMock() + resp.json.return_value = {"value": items} + return resp + + # ---- _bulk_fetch_picklists ---- + + def test_bulk_fetch_populates_nested_cache(self): + """Bulk fetch stores picklists in nested {table: {ts, picklists: {...}}} format.""" + import time - def test_check_caches_non_picklist_as_empty_map(self): - """Non-picklist attributes are cached as {"map": {}}.""" - mock_resp = MagicMock() - mock_resp.json.return_value = {"value": [{"LogicalName": "name", "AttributeType": "String"}]} - self.od._request.return_value = mock_resp + resp = self._bulk_response( + ("industrycode", [(6, "Technology"), (12, "Consulting")]), + ) + self.od._request.return_value = resp - self.od._check_attribute_types("account", ["name"]) + self.od._bulk_fetch_picklists("account") - entry = self.od._picklist_label_cache.get(("account", "name")) + entry = self.od._picklist_label_cache.get("account") self.assertIsNotNone(entry) - self.assertEqual(entry["map"], {}) self.assertIn("ts", entry) - - def test_check_caches_picklist_with_type_marker(self): - """Picklist attributes are cached as {"type": "Picklist"}.""" - mock_resp = MagicMock() - mock_resp.json.return_value = {"value": [{"LogicalName": "industrycode", "AttributeType": "Picklist"}]} - self.od._request.return_value = mock_resp - - self.od._check_attribute_types("account", ["industrycode"]) - - entry = self.od._picklist_label_cache.get(("account", "industrycode")) - self.assertIsNotNone(entry) - self.assertEqual(entry.get("type"), "Picklist") - self.assertNotIn("map", entry) - - def test_check_caches_missing_attrs_as_empty_map(self): - """Attributes not in the API response are cached as non-picklist.""" - mock_resp = MagicMock() - mock_resp.json.return_value = {"value": []} - self.od._request.return_value = mock_resp - - self.od._check_attribute_types("account", ["nonexistent"]) - - entry = self.od._picklist_label_cache.get(("account", "nonexistent")) - self.assertIsNotNone(entry) - self.assertEqual(entry["map"], {}) - - def test_check_handles_mixed_types(self): - """Batch with both picklist and non-picklist attributes caches correctly.""" - mock_resp = MagicMock() - mock_resp.json.return_value = { - "value": [ - {"LogicalName": "name", "AttributeType": "String"}, - {"LogicalName": "industrycode", "AttributeType": "Picklist"}, - {"LogicalName": "telephone1", "AttributeType": "String"}, - ] - } - self.od._request.return_value = mock_resp - - self.od._check_attribute_types("account", ["name", "industrycode", "telephone1"]) - - self.assertEqual(self.od._picklist_label_cache[("account", "name")]["map"], {}) - self.assertEqual( - self.od._picklist_label_cache[("account", "industrycode")].get("type"), - "Picklist", + self.assertIn("picklists", entry) + self.assertEqual(entry["picklists"]["industrycode"], {"technology": 6, "consulting": 12}) + + def test_bulk_fetch_multiple_picklists(self): + """Multiple picklist attributes are all stored under the same table entry.""" + resp = self._bulk_response( + ("industrycode", [(6, "Technology")]), + ("statuscode", [(1, "Active"), (2, "Inactive")]), ) - self.assertEqual(self.od._picklist_label_cache[("account", "telephone1")]["map"], {}) + self.od._request.return_value = resp - def test_check_empty_list_does_nothing(self): - """Empty attr list should not make any API call.""" - self.od._check_attribute_types("account", []) - self.od._request.assert_not_called() - - def test_check_case_insensitive_cache_keys(self): - """Cache keys are normalized to lowercase.""" - mock_resp = MagicMock() - mock_resp.json.return_value = {"value": [{"LogicalName": "industrycode", "AttributeType": "Picklist"}]} - self.od._request.return_value = mock_resp - - self.od._check_attribute_types("Account", ["IndustryCode"]) - - self.assertIn(("account", "industrycode"), self.od._picklist_label_cache) - - def test_check_makes_single_api_call(self): - """Batch should result in exactly one API call regardless of attr count.""" - mock_resp = MagicMock() - mock_resp.json.return_value = { - "value": [ - {"LogicalName": "a", "AttributeType": "String"}, - {"LogicalName": "b", "AttributeType": "Picklist"}, - {"LogicalName": "c", "AttributeType": "Integer"}, - ] - } - self.od._request.return_value = mock_resp - - self.od._check_attribute_types("account", ["a", "b", "c"]) - - self.assertEqual(self.od._request.call_count, 1) - call_url = self.od._request.call_args.args[1] - self.assertIn("Microsoft.Dynamics.CRM.In(", call_url) + self.od._bulk_fetch_picklists("account") - def test_check_uses_crm_in_function_in_url(self): - """Batch URL uses Microsoft.Dynamics.CRM.In function with quoted values.""" - mock_resp = MagicMock() - mock_resp.json.return_value = {"value": []} - self.od._request.return_value = mock_resp - - self.od._check_attribute_types("account", ["name", "industrycode"]) - - call_url = self.od._request.call_args.args[1] - self.assertIn("Microsoft.Dynamics.CRM.In(PropertyName='LogicalName'", call_url) - self.assertIn('"name"', call_url) - self.assertIn('"industrycode"', call_url) + picklists = self.od._picklist_label_cache["account"]["picklists"] + self.assertEqual(picklists["industrycode"], {"technology": 6}) + self.assertEqual(picklists["statuscode"], {"active": 1, "inactive": 2}) - def test_check_missing_attribute_type_cached_as_non_picklist(self): - """Attribute with missing AttributeType in response is cached as non-picklist.""" - mock_resp = MagicMock() - mock_resp.json.return_value = {"value": [{"LogicalName": "customfield"}]} # no AttributeType key - self.od._request.return_value = mock_resp + def test_bulk_fetch_no_picklists_caches_empty(self): + """Table with no picklist attributes gets cached with empty picklists dict.""" + resp = MagicMock() + resp.json.return_value = {"value": []} + self.od._request.return_value = resp - self.od._check_attribute_types("account", ["customfield"]) + self.od._bulk_fetch_picklists("account") - entry = self.od._picklist_label_cache.get(("account", "customfield")) + entry = self.od._picklist_label_cache.get("account") self.assertIsNotNone(entry) - self.assertEqual(entry["map"], {}) - - # ---- _optionset_map ---- + self.assertEqual(entry["picklists"], {}) - def test_optionset_returns_none_for_empty_table(self): - """_optionset_map returns None for empty table name.""" - self.assertIsNone(self.od._optionset_map("", "industrycode")) - - def test_optionset_returns_none_for_empty_attr(self): - """_optionset_map returns None for empty attribute name.""" - self.assertIsNone(self.od._optionset_map("account", "")) - - def test_optionset_returns_cached_map(self): - """Warm cache hit returns the map without API calls.""" + def test_bulk_fetch_skips_when_cache_fresh(self): + """Warm cache within TTL should skip the API call.""" import time - self.od._picklist_label_cache[("account", "industrycode")] = { - "map": {"technology": 6}, + self.od._picklist_label_cache["account"] = { "ts": time.time(), + "picklists": {"industrycode": {"technology": 6}}, } - result = self.od._optionset_map("account", "industrycode") - self.assertEqual(result, {"technology": 6}) + self.od._bulk_fetch_picklists("account") self.od._request.assert_not_called() - def test_optionset_type_marker_triggers_fetch(self): - """type=Picklist entry does not count as a cache hit -- should fetch options.""" + def test_bulk_fetch_refreshes_when_cache_expired(self): + """Expired cache should trigger a new API call.""" import time - self.od._picklist_label_cache[("account", "industrycode")] = { - "type": "Picklist", - "ts": time.time(), + self.od._picklist_label_cache["account"] = { + "ts": time.time() - 7200, # 2 hours ago, beyond 1h TTL + "picklists": {"industrycode": {"technology": 6}}, } - mock_resp = MagicMock() - mock_resp.text = "{}" - mock_resp.json.return_value = { - "OptionSet": {"Options": [{"Value": 6, "Label": {"LocalizedLabels": [{"Label": "Tech"}]}}]} - } - self.od._request.return_value = mock_resp + resp = self._bulk_response(("industrycode", [(6, "Tech"), (12, "Consulting")])) + self.od._request.return_value = resp - result = self.od._optionset_map("account", "industrycode") - self.assertEqual(result, {"tech": 6}) + self.od._bulk_fetch_picklists("account") self.od._request.assert_called_once() + self.assertEqual( + self.od._picklist_label_cache["account"]["picklists"]["industrycode"], + {"tech": 6, "consulting": 12}, + ) - def test_optionset_fetches_via_picklist_cast_url(self): - """API call uses the PicklistAttributeMetadata cast segment.""" - mock_resp = MagicMock() - mock_resp.text = "{}" - mock_resp.json.return_value = {"OptionSet": {"Options": []}} - self.od._request.return_value = mock_resp - - self.od._optionset_map("account", "industrycode") - - call_url = self.od._request.call_args.args[1] - self.assertIn("PicklistAttributeMetadata", call_url) - self.assertIn("industrycode", call_url) - - def test_optionset_multiple_options_parsed(self): - """Multiple options are all captured in the returned mapping.""" - mock_resp = MagicMock() - mock_resp.text = "{}" - mock_resp.json.return_value = { - "OptionSet": { - "Options": [ - {"Value": 1, "Label": {"LocalizedLabels": [{"Label": "Active"}]}}, - {"Value": 2, "Label": {"LocalizedLabels": [{"Label": "Inactive"}]}}, - {"Value": 3, "Label": {"LocalizedLabels": [{"Label": "Suspended"}]}}, - ] - } - } - self.od._request.return_value = mock_resp - - result = self.od._optionset_map("account", "statuscode") - self.assertEqual(result, {"active": 1, "inactive": 2, "suspended": 3}) - - def test_optionset_caches_resolved_map_with_ts(self): - """After fetching, the resolved map is stored in cache with a timestamp.""" - import time - - mock_resp = MagicMock() - mock_resp.text = "{}" - mock_resp.json.return_value = { - "OptionSet": {"Options": [{"Value": 6, "Label": {"LocalizedLabels": [{"Label": "Tech"}]}}]} - } - self.od._request.return_value = mock_resp - - before = time.time() - self.od._optionset_map("account", "industrycode") - after = time.time() - - entry = self.od._picklist_label_cache[("account", "industrycode")] - self.assertEqual(entry["map"], {"tech": 6}) - self.assertGreaterEqual(entry["ts"], before) - self.assertLessEqual(entry["ts"], after) + def test_bulk_fetch_case_insensitive_table_key(self): + """Table key is normalized to lowercase.""" + resp = self._bulk_response(("industrycode", [(6, "Tech")])) + self.od._request.return_value = resp - def test_optionset_returns_none_on_malformed_json(self): - """_optionset_map returns None when response JSON is unparseable.""" - mock_resp = MagicMock() - mock_resp.text = "not json" - mock_resp.json.side_effect = ValueError("No JSON") - self.od._request.return_value = mock_resp + self.od._bulk_fetch_picklists("Account") - result = self.od._optionset_map("account", "industrycode") - self.assertIsNone(result) + self.assertIn("account", self.od._picklist_label_cache) + self.assertNotIn("Account", self.od._picklist_label_cache) - def test_optionset_returns_none_when_options_not_list(self): - """_optionset_map returns None when OptionSet.Options is not a list.""" - mock_resp = MagicMock() - mock_resp.text = "{}" - mock_resp.json.return_value = {"OptionSet": {"Options": "not-a-list"}} - self.od._request.return_value = mock_resp + def test_bulk_fetch_uses_picklist_cast_url(self): + """API call uses PicklistAttributeMetadata cast segment.""" + resp = self._bulk_response() + self.od._request.return_value = resp - result = self.od._optionset_map("account", "industrycode") - self.assertIsNone(result) + self.od._bulk_fetch_picklists("account") - def test_optionset_returns_empty_dict_when_no_options(self): - """_optionset_map returns {} when Options list is empty.""" - mock_resp = MagicMock() - mock_resp.text = "{}" - mock_resp.json.return_value = {"OptionSet": {"Options": []}} - self.od._request.return_value = mock_resp + call_url = self.od._request.call_args.args[1] + self.assertIn("PicklistAttributeMetadata", call_url) + self.assertIn("OptionSet", call_url) + + def test_bulk_fetch_makes_single_api_call(self): + """Bulk fetch uses exactly one API call regardless of picklist count.""" + resp = self._bulk_response( + ("a", [(1, "X")]), + ("b", [(2, "Y")]), + ("c", [(3, "Z")]), + ) + self.od._request.return_value = resp - result = self.od._optionset_map("account", "industrycode") - self.assertEqual(result, {}) + self.od._bulk_fetch_picklists("account") + self.assertEqual(self.od._request.call_count, 1) # ---- _request_metadata_with_retry ---- @@ -794,7 +681,8 @@ def test_retry_succeeds_on_first_try(self): self.assertIs(result, mock_resp) self.assertEqual(self.od._request.call_count, 1) - def test_retry_retries_on_404(self): + @patch("PowerPlatform.Dataverse.data._odata.time.sleep") + def test_retry_retries_on_404(self, mock_sleep): """Should retry on 404 and succeed on later attempt.""" from PowerPlatform.Dataverse.core.errors import HttpError @@ -805,8 +693,10 @@ def test_retry_retries_on_404(self): result = self.od._request_metadata_with_retry("get", "https://example.com/test") self.assertIs(result, mock_resp) self.assertEqual(self.od._request.call_count, 2) + mock_sleep.assert_called_once() - def test_retry_raises_after_max_attempts(self): + @patch("PowerPlatform.Dataverse.data._odata.time.sleep") + def test_retry_raises_after_max_attempts(self, mock_sleep): """Should raise RuntimeError after all retries exhausted.""" from PowerPlatform.Dataverse.core.errors import HttpError @@ -816,6 +706,7 @@ def test_retry_raises_after_max_attempts(self): with self.assertRaises(RuntimeError) as ctx: self.od._request_metadata_with_retry("get", "https://example.com/test") self.assertIn("404", str(ctx.exception)) + self.assertTrue(mock_sleep.called) def test_retry_does_not_retry_non_404(self): """Non-404 errors should be raised immediately without retry.""" @@ -830,7 +721,7 @@ def test_retry_does_not_retry_non_404(self): # ---- _convert_labels_to_ints ---- - def test_convert_no_string_values_skips_batch(self): + def test_convert_no_string_values_skips_fetch(self): """Record with no string values should not trigger any API call.""" record = {"quantity": 5, "amount": 99.99, "completed": False} result = self.od._convert_labels_to_ints("account", record) @@ -851,32 +742,34 @@ def test_convert_whitespace_only_string_skipped(self): self.od._request.assert_not_called() def test_convert_odata_keys_skipped(self): - """@odata.bind keys must not be type-checked or resolved.""" - checked = [] - orig = self.od._check_attribute_types + """@odata.bind keys must not be resolved.""" + import time - def track(table, attrs): - checked.extend(attrs) - return orig(table, attrs) + self.od._picklist_label_cache["account"] = { + "ts": time.time(), + "picklists": {}, + } - self.od._check_attribute_types = track record = { "name": "Contoso", "new_CustomerId@odata.bind": "/contacts(guid)", "@odata.type": "Microsoft.Dynamics.CRM.account", } - self.od._convert_labels_to_ints("account", record) - self.assertEqual(checked, ["name"]) + result = self.od._convert_labels_to_ints("account", record) + # @odata keys left unchanged + self.assertEqual(result["new_CustomerId@odata.bind"], "/contacts(guid)") + self.assertEqual(result["@odata.type"], "Microsoft.Dynamics.CRM.account") def test_convert_warm_cache_no_api_calls(self): - """Second call with same fields should make zero API calls.""" + """Warm cache should resolve labels without any API calls.""" import time now = time.time() - self.od._picklist_label_cache[("account", "name")] = {"map": {}, "ts": now} - self.od._picklist_label_cache[("account", "industrycode")] = { - "map": {"technology": 6}, + self.od._picklist_label_cache["account"] = { "ts": now, + "picklists": { + "industrycode": {"technology": 6}, + }, } record = {"name": "Contoso", "industrycode": "Technology"} @@ -887,67 +780,40 @@ def test_convert_warm_cache_no_api_calls(self): self.od._request.assert_not_called() def test_convert_resolves_picklist_label_to_int(self): - """Full flow: batch identifies picklist, optionset_map fetches options, label resolved.""" - batch_resp = MagicMock() - batch_resp.json.return_value = { - "value": [ - {"LogicalName": "name", "AttributeType": "String"}, - {"LogicalName": "industrycode", "AttributeType": "Picklist"}, - ] - } - options_resp = MagicMock() - options_resp.text = '{"OptionSet": ...}' - options_resp.json.return_value = { - "OptionSet": { - "Options": [ - { - "Value": 6, - "Label": {"LocalizedLabels": [{"Label": "Technology", "LanguageCode": 1033}]}, - } - ] - } - } - self.od._request.side_effect = [batch_resp, options_resp] + """Full flow: bulk fetch returns picklists, label resolved to int.""" + resp = self._bulk_response( + ("industrycode", [(6, "Technology")]), + ) + self.od._request.return_value = resp record = {"name": "Contoso", "industrycode": "Technology"} result = self.od._convert_labels_to_ints("account", record) self.assertEqual(result["industrycode"], 6) self.assertEqual(result["name"], "Contoso") - self.assertEqual(self.od._request.call_count, 2) - - def test_convert_non_picklist_skips_optionset_map(self): - """Non-picklist fields should not trigger _optionset_map calls.""" - batch_resp = MagicMock() - batch_resp.json.return_value = { - "value": [ - {"LogicalName": "name", "AttributeType": "String"}, - {"LogicalName": "telephone1", "AttributeType": "String"}, - ] - } - self.od._request.return_value = batch_resp - - optionset_calls = [] - orig_map = self.od._optionset_map - - def tracking_map(table, attr): - optionset_calls.append(attr) - return orig_map(table, attr) + # Single bulk fetch call + self.assertEqual(self.od._request.call_count, 1) - self.od._optionset_map = tracking_map + def test_convert_non_picklist_leaves_string_unchanged(self): + """Non-picklist string fields are left as strings (no picklist entry in cache).""" + resp = self._bulk_response() # no picklists on table + self.od._request.return_value = resp record = {"name": "Contoso", "telephone1": "555-0100"} - self.od._convert_labels_to_ints("account", record) + result = self.od._convert_labels_to_ints("account", record) - self.assertEqual(optionset_calls, []) + self.assertEqual(result["name"], "Contoso") + self.assertEqual(result["telephone1"], "555-0100") def test_convert_unmatched_label_left_unchanged(self): """If a picklist label doesn't match any option, value stays as string.""" import time - self.od._picklist_label_cache[("account", "industrycode")] = { - "map": {"technology": 6, "consulting": 12}, + self.od._picklist_label_cache["account"] = { "ts": time.time(), + "picklists": { + "industrycode": {"technology": 6, "consulting": 12}, + }, } record = {"industrycode": "UnknownIndustry"} @@ -958,9 +824,9 @@ def test_convert_does_not_mutate_original_record(self): """_convert_labels_to_ints must return a copy, not mutate the input.""" import time - self.od._picklist_label_cache[("account", "industrycode")] = { - "map": {"technology": 6}, + self.od._picklist_label_cache["account"] = { "ts": time.time(), + "picklists": {"industrycode": {"technology": 6}}, } original = {"industrycode": "Technology"} @@ -969,83 +835,29 @@ def test_convert_does_not_mutate_original_record(self): self.assertEqual(result["industrycode"], 6) self.assertEqual(original["industrycode"], "Technology") - def test_convert_only_batches_uncached_attrs(self): - """Warm-cached attrs should not be included in the batch call.""" - import time - - self.od._picklist_label_cache[("account", "name")] = { - "map": {}, - "ts": time.time(), - } - - batch_resp = MagicMock() - batch_resp.json.return_value = {"value": [{"LogicalName": "industrycode", "AttributeType": "Picklist"}]} - options_resp = MagicMock() - options_resp.text = "{}" - options_resp.json.return_value = { - "OptionSet": {"Options": [{"Value": 6, "Label": {"LocalizedLabels": [{"Label": "Tech"}]}}]} - } - self.od._request.side_effect = [batch_resp, options_resp] - - record = {"name": "Contoso", "industrycode": "Tech"} - result = self.od._convert_labels_to_ints("account", record) - - batch_call_url = self.od._request.call_args_list[0].args[1] - self.assertIn("industrycode", batch_call_url) - self.assertNotIn("'name'", batch_call_url) - self.assertEqual(result["industrycode"], 6) - def test_convert_multiple_picklists_in_one_record(self): """Multiple picklist fields in the same record are all resolved.""" - batch_resp = MagicMock() - batch_resp.json.return_value = { - "value": [ - {"LogicalName": "industrycode", "AttributeType": "Picklist"}, - {"LogicalName": "statuscode", "AttributeType": "Picklist"}, - ] - } - industry_resp = MagicMock() - industry_resp.text = "{}" - industry_resp.json.return_value = { - "OptionSet": {"Options": [{"Value": 6, "Label": {"LocalizedLabels": [{"Label": "Tech"}]}}]} - } - status_resp = MagicMock() - status_resp.text = "{}" - status_resp.json.return_value = { - "OptionSet": {"Options": [{"Value": 1, "Label": {"LocalizedLabels": [{"Label": "Active"}]}}]} - } - self.od._request.side_effect = [batch_resp, industry_resp, status_resp] + resp = self._bulk_response( + ("industrycode", [(6, "Tech")]), + ("statuscode", [(1, "Active")]), + ) + self.od._request.return_value = resp record = {"industrycode": "Tech", "statuscode": "Active"} result = self.od._convert_labels_to_ints("account", record) self.assertEqual(result["industrycode"], 6) self.assertEqual(result["statuscode"], 1) - # 1 batch + 2 optionset fetches - self.assertEqual(self.od._request.call_count, 3) + # Single bulk fetch call + self.assertEqual(self.od._request.call_count, 1) def test_convert_mixed_picklists_and_non_picklists(self): - """2 picklists + 2 non-picklist strings: 1 batch + 2 optionset = 3 calls.""" - batch_resp = MagicMock() - batch_resp.json.return_value = { - "value": [ - {"LogicalName": "name", "AttributeType": "String"}, - {"LogicalName": "industrycode", "AttributeType": "Picklist"}, - {"LogicalName": "description", "AttributeType": "Memo"}, - {"LogicalName": "statuscode", "AttributeType": "Picklist"}, - ] - } - industry_resp = MagicMock() - industry_resp.text = "{}" - industry_resp.json.return_value = { - "OptionSet": {"Options": [{"Value": 6, "Label": {"LocalizedLabels": [{"Label": "Tech"}]}}]} - } - status_resp = MagicMock() - status_resp.text = "{}" - status_resp.json.return_value = { - "OptionSet": {"Options": [{"Value": 1, "Label": {"LocalizedLabels": [{"Label": "Active"}]}}]} - } - self.od._request.side_effect = [batch_resp, industry_resp, status_resp] + """Picklists resolved, non-picklist strings left unchanged, 1 API call.""" + resp = self._bulk_response( + ("industrycode", [(6, "Tech")]), + ("statuscode", [(1, "Active")]), + ) + self.od._request.return_value = resp record = { "name": "Contoso", @@ -1059,26 +871,18 @@ def test_convert_mixed_picklists_and_non_picklists(self): self.assertEqual(result["statuscode"], 1) self.assertEqual(result["name"], "Contoso") self.assertEqual(result["description"], "A company") - # 1 batch + 2 optionset fetches (non-picklists don't trigger optionset) - self.assertEqual(self.od._request.call_count, 3) + self.assertEqual(self.od._request.call_count, 1) def test_convert_all_non_picklist_makes_one_api_call(self): - """All non-picklist string fields: 1 batch call, 0 optionset = 1 total.""" - batch_resp = MagicMock() - batch_resp.json.return_value = { - "value": [ - {"LogicalName": "name", "AttributeType": "String"}, - {"LogicalName": "description", "AttributeType": "Memo"}, - {"LogicalName": "telephone1", "AttributeType": "String"}, - ] - } - self.od._request.return_value = batch_resp + """All non-picklist string fields: 1 bulk fetch call, labels unchanged.""" + resp = self._bulk_response() # no picklists + self.od._request.return_value = resp record = {"name": "Contoso", "description": "A company", "telephone1": "555-0100"} - self.od._convert_labels_to_ints("account", record) + result = self.od._convert_labels_to_ints("account", record) - # Only the batch type-check, no optionset fetches self.assertEqual(self.od._request.call_count, 1) + self.assertEqual(result["name"], "Contoso") def test_convert_no_string_values_makes_zero_api_calls(self): """All non-string values: 0 API calls total.""" @@ -1087,32 +891,8 @@ def test_convert_no_string_values_makes_zero_api_calls(self): self.assertEqual(self.od._request.call_count, 0) - def test_convert_partial_cache_only_batches_uncached(self): - """1 cached non-picklist + 1 uncached picklist: 1 batch + 1 optionset = 2 calls.""" - import time - - # Pre-cache "name" as non-picklist - self.od._picklist_label_cache[("account", "name")] = {"map": {}, "ts": time.time()} - - batch_resp = MagicMock() - batch_resp.json.return_value = {"value": [{"LogicalName": "industrycode", "AttributeType": "Picklist"}]} - options_resp = MagicMock() - options_resp.text = "{}" - options_resp.json.return_value = { - "OptionSet": {"Options": [{"Value": 6, "Label": {"LocalizedLabels": [{"Label": "Tech"}]}}]} - } - self.od._request.side_effect = [batch_resp, options_resp] - - record = {"name": "Contoso", "industrycode": "Tech"} - result = self.od._convert_labels_to_ints("account", record) - - self.assertEqual(result["industrycode"], 6) - self.assertEqual(result["name"], "Contoso") - # 1 batch (only industrycode) + 1 optionset fetch - self.assertEqual(self.od._request.call_count, 2) - - def test_convert_batch_failure_propagates(self): - """Server error during batch type-check propagates to caller.""" + def test_convert_bulk_fetch_failure_propagates(self): + """Server error during bulk fetch propagates to caller.""" from PowerPlatform.Dataverse.core.errors import HttpError self.od._request.side_effect = HttpError("Server Error", status_code=500) @@ -1120,30 +900,24 @@ def test_convert_batch_failure_propagates(self): with self.assertRaises(HttpError): self.od._convert_labels_to_ints("account", {"name": "Contoso"}) - def test_convert_single_picklist_makes_two_api_calls(self): - """Single picklist field (cold cache): 1 batch + 1 optionset = 2 total.""" - batch_resp = MagicMock() - batch_resp.json.return_value = {"value": [{"LogicalName": "industrycode", "AttributeType": "Picklist"}]} - options_resp = MagicMock() - options_resp.text = "{}" - options_resp.json.return_value = { - "OptionSet": {"Options": [{"Value": 6, "Label": {"LocalizedLabels": [{"Label": "Tech"}]}}]} - } - self.od._request.side_effect = [batch_resp, options_resp] + def test_convert_single_picklist_makes_one_api_call(self): + """Single picklist field (cold cache): 1 bulk fetch total.""" + resp = self._bulk_response(("industrycode", [(6, "Tech")])) + self.od._request.return_value = resp record = {"industrycode": "Tech"} result = self.od._convert_labels_to_ints("account", record) self.assertEqual(result["industrycode"], 6) - self.assertEqual(self.od._request.call_count, 2) + self.assertEqual(self.od._request.call_count, 1) def test_convert_integer_values_passed_through(self): """Integer values (already resolved) are left unchanged.""" import time - self.od._picklist_label_cache[("account", "industrycode")] = { - "map": {"technology": 6}, + self.od._picklist_label_cache["account"] = { "ts": time.time(), + "picklists": {"industrycode": {"technology": 6}}, } record = {"industrycode": 6, "name": "Contoso"} @@ -1154,43 +928,232 @@ def test_convert_case_insensitive_label_matching(self): """Picklist label matching is case-insensitive.""" import time - self.od._picklist_label_cache[("account", "industrycode")] = { - "map": {"technology": 6}, + self.od._picklist_label_cache["account"] = { "ts": time.time(), + "picklists": {"industrycode": {"technology": 6}}, } record = {"industrycode": "TECHNOLOGY"} result = self.od._convert_labels_to_ints("account", record) self.assertEqual(result["industrycode"], 6) - # ---- Integration: through _create ---- + def test_convert_second_call_same_table_no_api(self): + """Second convert call for same table uses cached bulk fetch, no API call.""" + resp = self._bulk_response(("industrycode", [(6, "Tech")])) + self.od._request.return_value = resp - def test_create_resolves_picklist_in_payload(self): - """_create resolves a picklist label to its integer in the POST payload.""" - batch_resp = MagicMock() - batch_resp.json.return_value = { + self.od._convert_labels_to_ints("account", {"industrycode": "Tech"}) + self.assertEqual(self.od._request.call_count, 1) + + # Second call -- cache warm + self.od._request.reset_mock() + result = self.od._convert_labels_to_ints("account", {"industrycode": "Tech"}) + self.assertEqual(result["industrycode"], 6) + self.od._request.assert_not_called() + + def test_convert_different_tables_separate_fetches(self): + """Different tables each get their own bulk fetch.""" + resp1 = self._bulk_response(("industrycode", [(6, "Tech")])) + resp2 = self._bulk_response(("new_status", [(100, "Open")])) + self.od._request.side_effect = [resp1, resp2] + + r1 = self.od._convert_labels_to_ints("account", {"industrycode": "Tech"}) + r2 = self.od._convert_labels_to_ints("new_ticket", {"new_status": "Open"}) + + self.assertEqual(r1["industrycode"], 6) + self.assertEqual(r2["new_status"], 100) + self.assertEqual(self.od._request.call_count, 2) + + def test_convert_only_odata_and_non_strings_skips_fetch(self): + """Record with only @odata keys and non-string values should skip fetch entirely.""" + record = { + "@odata.type": "Microsoft.Dynamics.CRM.account", + "new_CustomerId@odata.bind": "/contacts(guid)", + "quantity": 5, + "active": True, + } + result = self.od._convert_labels_to_ints("account", record) + self.assertEqual(result, record) + self.od._request.assert_not_called() + + def test_convert_partial_picklist_match(self): + """Some picklists match, some don't -- matched ones resolved, unmatched left as string.""" + import time + + self.od._picklist_label_cache["account"] = { + "ts": time.time(), + "picklists": { + "industrycode": {"technology": 6, "consulting": 12}, + "statuscode": {"active": 1, "inactive": 2}, + }, + } + + record = {"industrycode": "Technology", "statuscode": "UnknownStatus"} + result = self.od._convert_labels_to_ints("account", record) + + self.assertEqual(result["industrycode"], 6) + self.assertEqual(result["statuscode"], "UnknownStatus") + + def test_convert_mixed_int_and_label_in_same_record(self): + """One picklist already int, another is a label string -- only label resolved.""" + import time + + self.od._picklist_label_cache["account"] = { + "ts": time.time(), + "picklists": { + "industrycode": {"technology": 6}, + "statuscode": {"active": 1}, + }, + } + + record = {"industrycode": 6, "statuscode": "Active"} + result = self.od._convert_labels_to_ints("account", record) + + self.assertEqual(result["industrycode"], 6) + self.assertEqual(result["statuscode"], 1) + + def test_convert_same_label_different_picklists(self): + """Same label text in two different picklist columns resolves to different values.""" + import time + + self.od._picklist_label_cache["new_ticket"] = { + "ts": time.time(), + "picklists": { + "new_priority": {"high": 3}, + "new_severity": {"high": 100}, + }, + } + + record = {"new_priority": "High", "new_severity": "High"} + result = self.od._convert_labels_to_ints("new_ticket", record) + + self.assertEqual(result["new_priority"], 3) + self.assertEqual(result["new_severity"], 100) + + def test_convert_picklist_with_empty_options(self): + """Picklist attribute with zero defined options: label stays as string.""" + import time + + self.od._picklist_label_cache["account"] = { + "ts": time.time(), + "picklists": { + "customcode": {}, # picklist exists but has no options + }, + } + + record = {"customcode": "SomeValue"} + result = self.od._convert_labels_to_ints("account", record) + self.assertEqual(result["customcode"], "SomeValue") + + def test_convert_full_realistic_record(self): + """Realistic record: mix of strings, ints, bools, @odata keys, and picklists.""" + resp = self._bulk_response( + ("industrycode", [(6, "Technology"), (12, "Consulting")]), + ("statuscode", [(1, "Active"), (2, "Inactive")]), + ) + self.od._request.return_value = resp + + record = { + "name": "Contoso Ltd", + "industrycode": "Technology", + "statuscode": "Active", + "revenue": 1000000, + "telephone1": "555-0100", + "emailaddress1": "info@contoso.com", + "new_completed": True, + "new_quantity": 42, + "description": "A technology company", + "@odata.type": "Microsoft.Dynamics.CRM.account", + "new_CustomerId@odata.bind": "/contacts(00000000-0000-0000-0000-000000000001)", + } + result = self.od._convert_labels_to_ints("account", record) + + # Picklists resolved + self.assertEqual(result["industrycode"], 6) + self.assertEqual(result["statuscode"], 1) + # Non-picklist strings unchanged + self.assertEqual(result["name"], "Contoso Ltd") + self.assertEqual(result["telephone1"], "555-0100") + self.assertEqual(result["emailaddress1"], "info@contoso.com") + self.assertEqual(result["description"], "A technology company") + # Non-strings unchanged + self.assertEqual(result["revenue"], 1000000) + self.assertEqual(result["new_completed"], True) + self.assertEqual(result["new_quantity"], 42) + # @odata keys unchanged + self.assertEqual(result["@odata.type"], "Microsoft.Dynamics.CRM.account") + self.assertEqual( + result["new_CustomerId@odata.bind"], + "/contacts(00000000-0000-0000-0000-000000000001)", + ) + self.assertEqual(self.od._request.call_count, 1) + + def test_bulk_fetch_skips_malformed_items(self): + """Bulk fetch ignores items that aren't dicts or lack LogicalName.""" + resp = MagicMock() + resp.json.return_value = { "value": [ - {"LogicalName": "name", "AttributeType": "String"}, - {"LogicalName": "industrycode", "AttributeType": "Picklist"}, + "not-a-dict", + {"LogicalName": "", "OptionSet": {"Options": []}}, + { + "LogicalName": "industrycode", + "OptionSet": {"Options": [{"Value": 6, "Label": {"LocalizedLabels": [{"Label": "Tech"}]}}]}, + }, + {"no_logical_name_key": True}, ] } - options_resp = MagicMock() - options_resp.text = "{}" - options_resp.json.return_value = { - "OptionSet": { - "Options": [ - { - "Value": 6, - "Label": {"LocalizedLabels": [{"Label": "Technology", "LanguageCode": 1033}]}, - } - ] - } + self.od._request.return_value = resp + + self.od._bulk_fetch_picklists("account") + + picklists = self.od._picklist_label_cache["account"]["picklists"] + self.assertEqual(len(picklists), 1) + self.assertEqual(picklists["industrycode"], {"tech": 6}) + + def test_bulk_fetch_first_label_wins_for_same_value(self): + """When multiple localized labels exist, first label wins via setdefault.""" + resp = MagicMock() + resp.json.return_value = { + "value": [ + { + "LogicalName": "industrycode", + "OptionSet": { + "Options": [ + { + "Value": 6, + "Label": { + "LocalizedLabels": [ + {"Label": "Technology"}, + {"Label": "Technologie"}, + ] + }, + } + ] + }, + } + ] } + self.od._request.return_value = resp + + self.od._bulk_fetch_picklists("account") + + picklists = self.od._picklist_label_cache["account"]["picklists"] + # Both labels should be present, mapping to the same value + self.assertEqual(picklists["industrycode"]["technology"], 6) + self.assertEqual(picklists["industrycode"]["technologie"], 6) + + # ---- Integration: through _create ---- + + def test_create_resolves_picklist_in_payload(self): + """_create resolves a picklist label to its integer in the POST payload.""" + bulk_resp = self._bulk_response( + ("industrycode", [(6, "Technology")]), + ) post_resp = MagicMock() post_resp.headers = { "OData-EntityId": "https://example.crm.dynamics.com/api/data/v9.2/accounts(00000000-0000-0000-0000-000000000001)" } - self.od._request.side_effect = [batch_resp, options_resp, post_resp] + self.od._request.side_effect = [bulk_resp, post_resp] result = self.od._create("accounts", "account", {"name": "Contoso", "industrycode": "Technology"}) self.assertEqual(result, "00000000-0000-0000-0000-000000000001") @@ -1199,16 +1162,15 @@ def test_create_resolves_picklist_in_payload(self): self.assertEqual(payload["industrycode"], 6) self.assertEqual(payload["name"], "Contoso") - def test_create_warm_cache_skips_batch(self): + def test_create_warm_cache_skips_fetch(self): """_create with warm cache makes only the POST call.""" import time now = time.time() - self.od._picklist_label_cache[("account", "industrycode")] = { - "map": {"technology": 6}, + self.od._picklist_label_cache["account"] = { "ts": now, + "picklists": {"industrycode": {"technology": 6}}, } - self.od._picklist_label_cache[("account", "name")] = {"map": {}, "ts": now} post_resp = MagicMock() post_resp.headers = { @@ -1228,26 +1190,11 @@ def test_update_resolves_picklist_in_payload(self): """_update resolves a picklist label to its integer in the PATCH payload.""" self.od._entity_set_from_schema_name = MagicMock(return_value="new_tickets") - batch_resp = MagicMock() - batch_resp.json.return_value = { - "value": [ - {"LogicalName": "new_status", "AttributeType": "Picklist"}, - ] - } - options_resp = MagicMock() - options_resp.text = "{}" - options_resp.json.return_value = { - "OptionSet": { - "Options": [ - { - "Value": 100000001, - "Label": {"LocalizedLabels": [{"Label": "In Progress", "LanguageCode": 1033}]}, - } - ] - } - } + bulk_resp = self._bulk_response( + ("new_status", [(100000001, "In Progress")]), + ) patch_resp = MagicMock() - self.od._request.side_effect = [batch_resp, options_resp, patch_resp] + self.od._request.side_effect = [bulk_resp, patch_resp] self.od._update( "new_ticket", @@ -1258,14 +1205,14 @@ def test_update_resolves_picklist_in_payload(self): payload = patch_calls[0].kwargs["json"] self.assertEqual(payload["new_status"], 100000001) - def test_update_warm_cache_skips_batch(self): + def test_update_warm_cache_skips_fetch(self): """_update with warm cache makes only the PATCH call.""" import time self.od._entity_set_from_schema_name = MagicMock(return_value="new_tickets") - self.od._picklist_label_cache[("new_ticket", "new_status")] = { - "map": {"in progress": 100000001}, + self.od._picklist_label_cache["new_ticket"] = { "ts": time.time(), + "picklists": {"new_status": {"in progress": 100000001}}, } self.od._update( @@ -1282,27 +1229,11 @@ def test_update_warm_cache_skips_batch(self): def test_upsert_resolves_picklist_in_payload(self): """_upsert resolves a picklist label to its integer in the PATCH payload.""" - batch_resp = MagicMock() - batch_resp.json.return_value = { - "value": [ - {"LogicalName": "industrycode", "AttributeType": "Picklist"}, - {"LogicalName": "name", "AttributeType": "String"}, - ] - } - options_resp = MagicMock() - options_resp.text = "{}" - options_resp.json.return_value = { - "OptionSet": { - "Options": [ - { - "Value": 6, - "Label": {"LocalizedLabels": [{"Label": "Technology", "LanguageCode": 1033}]}, - } - ] - } - } + bulk_resp = self._bulk_response( + ("industrycode", [(6, "Technology")]), + ) patch_resp = MagicMock() - self.od._request.side_effect = [batch_resp, options_resp, patch_resp] + self.od._request.side_effect = [bulk_resp, patch_resp] self.od._upsert( "accounts", @@ -1315,15 +1246,14 @@ def test_upsert_resolves_picklist_in_payload(self): self.assertEqual(payload["industrycode"], 6) self.assertEqual(payload["name"], "Contoso") - def test_upsert_warm_cache_skips_batch(self): + def test_upsert_warm_cache_skips_fetch(self): """_upsert with warm cache makes only the PATCH call.""" import time now = time.time() - self.od._picklist_label_cache[("account", "name")] = {"map": {}, "ts": now} - self.od._picklist_label_cache[("account", "industrycode")] = { - "map": {"technology": 6}, + self.od._picklist_label_cache["account"] = { "ts": now, + "picklists": {"industrycode": {"technology": 6}}, } self.od._upsert( diff --git a/tests/unit/test_context_manager.py b/tests/unit/test_context_manager.py index df916583..2f1aab9c 100644 --- a/tests/unit/test_context_manager.py +++ b/tests/unit/test_context_manager.py @@ -158,7 +158,7 @@ def test_close_clears_odata_caches(self): odata = client._get_odata() odata._logical_to_entityset_cache["test"] = "value" odata._logical_primaryid_cache["test"] = "value" - odata._picklist_label_cache[("test", "attr")] = {"map": {}, "ts": 0} + odata._picklist_label_cache["test"] = {"ts": 0, "picklists": {"attr": {}}} client.close() From f4be98b56f441926c4db3bc89761b18b7ddc91df Mon Sep 17 00:00:00 2001 From: Abel Milash Date: Wed, 8 Apr 2026 11:57:19 -0700 Subject: [PATCH 5/5] Update based on comments --- src/PowerPlatform/Dataverse/data/_odata.py | 2 +- tests/unit/data/test_odata_internal.py | 15 +++++++++++++++ 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/src/PowerPlatform/Dataverse/data/_odata.py b/src/PowerPlatform/Dataverse/data/_odata.py index 8d22e098..6707152c 100644 --- a/src/PowerPlatform/Dataverse/data/_odata.py +++ b/src/PowerPlatform/Dataverse/data/_odata.py @@ -1229,7 +1229,6 @@ def _request_metadata_with_retry(self, method: str, url: str, **kwargs): continue raise RuntimeError(f"Metadata request failed after {max_attempts} retries (404): {url}") from err raise - raise RuntimeError(f"Metadata request failed: {url}") def _bulk_fetch_picklists(self, table_schema_name: str) -> None: """Fetch all picklist attributes and their options for a table in one API call. @@ -1237,6 +1236,7 @@ def _bulk_fetch_picklists(self, table_schema_name: str) -> None: Uses collection-level PicklistAttributeMetadata cast to retrieve every picklist attribute on the table, including its OptionSet options. Populates the nested cache so that ``_convert_labels_to_ints`` resolves labels without further API calls. + The Dataverse metadata API does not page results. """ table_key = self._normalize_cache_key(table_schema_name) now = time.time() diff --git a/tests/unit/data/test_odata_internal.py b/tests/unit/data/test_odata_internal.py index 4384f326..74a5b8bd 100644 --- a/tests/unit/data/test_odata_internal.py +++ b/tests/unit/data/test_odata_internal.py @@ -670,6 +670,21 @@ def test_bulk_fetch_makes_single_api_call(self): self.od._bulk_fetch_picklists("account") self.assertEqual(self.od._request.call_count, 1) + def test_bulk_fetch_stress_large_workload(self): + """Bulk fetch correctly parses a response with a large number of picklist attributes.""" + num_picklists = 5000 + picklists = [(f"new_pick{i}", [(100000000 + j, f"Option {j}") for j in range(4)]) for i in range(num_picklists)] + resp = self._bulk_response(*picklists) + self.od._request.return_value = resp + + self.od._bulk_fetch_picklists("account") + + self.assertEqual(self.od._request.call_count, 1) + cached = self.od._picklist_label_cache["account"]["picklists"] + self.assertEqual(len(cached), num_picklists) + self.assertEqual(cached["new_pick0"]["option 0"], 100000000) + self.assertEqual(cached[f"new_pick{num_picklists - 1}"]["option 3"], 100000003) + # ---- _request_metadata_with_retry ---- def test_retry_succeeds_on_first_try(self):