diff --git a/python/CHANGELOG.md b/python/CHANGELOG.md index 8decb8a361..21c8c60e30 100644 --- a/python/CHANGELOG.md +++ b/python/CHANGELOG.md @@ -3,6 +3,11 @@ All notable changes to this project will be documented in this file. This project adheres to [Semantic Versioning](http://semver.org/). +## [Unreleased] + +- Fix issue where a test-results log containing an update that named no fields could never finish importing. Re-run `import-test-result-log` to finish a log written by an earlier version. +- A key that is not a field of a create or update model now warns with `SiftIgnoredInputWarning`, and suggests a near match. + ## [v0.22.1] - September 29, 2026 - Fix streaming ingestion processes crashing with SIGSEGV or SIGABRT at interpreter exit. The `sift-stream-bindings` tokio runtime was never shut down, so a runtime thread that finished work while Python was finalizing re-entered the interpreter and killed the process after the program had completed. The bindings now stop the runtime from an `atexit` hook and expose `shutdown(timeout=5.0)` for callers that exit with `os._exit()`. If runtime threads outlive the timeout, `shutdown()` issues a `RuntimeWarning`. Requires `sift-stream-bindings` 0.5.2. ([#804](https://github.com/sift-stack/sift/pull/804)) diff --git a/python/lib/sift_client/_internal/low_level_wrappers/test_results.py b/python/lib/sift_client/_internal/low_level_wrappers/test_results.py index e0d33dec8c..91c035b389 100644 --- a/python/lib/sift_client/_internal/low_level_wrappers/test_results.py +++ b/python/lib/sift_client/_internal/low_level_wrappers/test_results.py @@ -2,6 +2,7 @@ import logging import uuid +import warnings from pathlib import Path from typing import TYPE_CHECKING, Any, NamedTuple, TypeVar, cast @@ -51,6 +52,8 @@ ) from sift_client._internal.low_level_wrappers.base import DEFAULT_PAGE_SIZE, LowLevelClientBase from sift_client._internal.pytest_plugin.audit_log import log_event +from sift_client._internal.util.util import caller_stacklevel +from sift_client.errors import SiftIgnoredInputWarning from sift_client.sift_types.test_report import ( TestMeasurement, TestMeasurementCreate, @@ -122,6 +125,36 @@ def _mark_simulated(instance: _EntityT) -> _EntityT: instance.__dict__["_simulated"] = True return instance + @classmethod + def _skip_empty_update( + cls, + entity_name: str, + existing: _EntityT | None, + simulated: _EntityT | None, + ) -> _EntityT: + """Short-circuit an update whose field mask is empty. + + Raises: + ValueError: On a live call with no ``existing`` entity. Nothing + changed server-side and there is no entity to return, so the + alternative is a fabricated one, which callers cannot tell from a + real read. + """ + warnings.warn( + f"Update to {entity_name} requested no field changes and was ignored.", + SiftIgnoredInputWarning, + stacklevel=caller_stacklevel(), + ) + if existing is not None: + return existing + if simulated is not None: + return cls._mark_simulated(simulated) + raise ValueError( + f"Update to {entity_name} named no fields to change. Pass the " + f"{entity_name} rather than its ID to get it back unchanged, or name " + "at least one field to update." + ) + @staticmethod def simulate_create_test_report_response( request: CreateTestReportRequest, @@ -534,7 +567,8 @@ async def update_test_report( simulate: If True, return a simulated response without making an API call. Returns: - The updated TestReport. + The updated TestReport, or the report unchanged when the update names + no fields (see ``_skip_empty_update``). """ if request is None: if update is None: @@ -542,7 +576,15 @@ async def update_test_report( test_report_proto, field_mask = update.to_proto_with_mask() request = UpdateTestReportRequest(test_report=test_report_proto, update_mask=field_mask) - if log_file is not None or simulate: + simulating = log_file is not None or simulate + if not request.update_mask.paths: + return self._skip_empty_update( + "TestReport", + existing, + self.simulate_update_test_report_response(request) if simulating else None, + ) + + if simulating: if log_file is not None: await log_request_to_file(log_file, "UpdateTestReport", request) return self._mark_simulated( @@ -689,7 +731,8 @@ async def update_test_step( simulate: If True, return a simulated response without making an API call. Returns: - The updated TestStep. + The updated TestStep, or the step unchanged when the update names no + fields (see ``_skip_empty_update``). """ if request is None: if update is None: @@ -700,7 +743,15 @@ async def update_test_step( field_mask.paths.append("error_info") request = UpdateTestStepRequest(test_step=test_step_proto, update_mask=field_mask) - if log_file is not None or simulate: + simulating = log_file is not None or simulate + if not request.update_mask.paths: + return self._skip_empty_update( + "TestStep", + existing, + self.simulate_update_test_step_response(request) if simulating else None, + ) + + if simulating: if log_file is not None: await log_request_to_file(log_file, "UpdateTestStep", request) return self._mark_simulated( @@ -892,7 +943,8 @@ async def update_test_measurement( simulate: If True, return a simulated response without making an API call. Returns: - The updated TestMeasurement. + The updated TestMeasurement, or the measurement unchanged when the + update names no fields (see ``_skip_empty_update``). """ if request is None: if update is None: @@ -902,7 +954,15 @@ async def update_test_measurement( test_measurement=test_measurement_proto, update_mask=field_mask ) - if log_file is not None or simulate: + simulating = log_file is not None or simulate + if not request.update_mask.paths: + return self._skip_empty_update( + "TestMeasurement", + existing, + self.simulate_update_test_measurement_response(request) if simulating else None, + ) + + if simulating: if log_file is not None: await log_request_to_file(log_file, "UpdateTestMeasurement", request) return self._mark_simulated( @@ -1226,6 +1286,12 @@ async def _replay_update_report( orig_report_id = request.test_report.test_report_id mapped_report_id = self._map_id(id_map, orig_report_id) request.test_report.test_report_id = mapped_report_id + # An empty mask asks the server to change nothing, and the API rejects it. + # Clients before the write-time guard logged such entries, and the cursor + # only advances past a line that succeeded, so one of them stopped every + # retry at the same place. Nothing to apply here, so count it skipped. + if not request.update_mask.paths: + return _EntryIds(orig_report_id or None, mapped_report_id or None, skipped=True) # Batch/simulate replays the whole log in order, so a missing report means # the log is malformed. Incremental replay may have created the report on an # earlier tick (its real ID lives in id_map), so state.report is legitimately @@ -1251,6 +1317,9 @@ async def _replay_update_step( orig_step_id = request.test_step.test_step_id mapped_step_id = self._map_id(id_map, orig_step_id) request.test_step.test_step_id = mapped_step_id + # No paths means nothing to apply; see _replay_update_report. + if not request.update_mask.paths: + return _EntryIds(orig_step_id or None, mapped_step_id or None, skipped=True) existing_step = state.steps_by_id.get(mapped_step_id) if simulate and existing_step is None: raise ValueError(f"UpdateTestStep for unknown step: {orig_step_id}") @@ -1275,6 +1344,9 @@ async def _replay_update_measurement( orig_meas_id = request.test_measurement.measurement_id mapped_meas_id = self._map_id(id_map, orig_meas_id) request.test_measurement.measurement_id = mapped_meas_id + # No paths means nothing to apply; see _replay_update_report. + if not request.update_mask.paths: + return _EntryIds(orig_meas_id or None, mapped_meas_id or None, skipped=True) existing_meas = state.measurements_by_id.get(mapped_meas_id) if simulate and existing_meas is None: raise ValueError(f"UpdateTestMeasurement for unknown measurement: {orig_meas_id}") diff --git a/python/lib/sift_client/_internal/util/util.py b/python/lib/sift_client/_internal/util/util.py index 28f69ef921..3d886556d5 100644 --- a/python/lib/sift_client/_internal/util/util.py +++ b/python/lib/sift_client/_internal/util/util.py @@ -1,16 +1,42 @@ from __future__ import annotations +import inspect +import os +from pathlib import Path from typing import TYPE_CHECKING, Any if TYPE_CHECKING: from collections.abc import Iterator +_PACKAGE_ROOT = str(Path(__file__).resolve().parent.parent.parent) +_PYDANTIC_PATH = f"{os.sep}pydantic{os.sep}" + def count_non_none(*args: Any) -> int: """Count the number of non-none arguments.""" return sum(1 for arg in args if arg is not None) +def caller_stacklevel() -> int: + """Return the ``warnings.warn`` stacklevel of the first frame outside the SDK. + + A fixed stacklevel points at SDK or pydantic internals, which tells the + caller nothing about which of their lines caused the warning. + + Call this from the function that calls ``warnings.warn``. + """ + frame = inspect.currentframe() + frame = frame.f_back if frame is not None else None # the function calling warn() + level = 1 + while frame is not None: + filename = frame.f_code.co_filename + if not filename.startswith(_PACKAGE_ROOT) and _PYDANTIC_PATH not in filename: + return level + frame = frame.f_back + level += 1 + return 2 + + def chunked(items: list[Any], size: int) -> Iterator[list[Any]]: """Yield successive chunks of at most ``size`` items.""" for i in range(0, len(items), size): diff --git a/python/lib/sift_client/_tests/_internal/low_level_wrappers/test_incremental_replay.py b/python/lib/sift_client/_tests/_internal/low_level_wrappers/test_incremental_replay.py index 63bbc85333..847f5d474a 100644 --- a/python/lib/sift_client/_tests/_internal/low_level_wrappers/test_incremental_replay.py +++ b/python/lib/sift_client/_tests/_internal/low_level_wrappers/test_incremental_replay.py @@ -24,6 +24,7 @@ # Aliased so pytest doesn't try to collect the `Test`-prefixed client as a suite. TestResultsLowLevelClient as ResultsLowLevelClient, ) +from sift_client.errors import SiftIgnoredInputWarning from sift_client.sift_types.test_report import ( TestMeasurement, TestMeasurementCreate, @@ -723,6 +724,57 @@ async def test_resume_propagates_errors_other_than_a_missing_report(tmp_path): await client.import_log_file(log_file) +@pytest.mark.asyncio +async def test_resume_skips_an_update_with_an_empty_mask(tmp_path): + """A logged update carrying no field paths is skipped, not sent.""" + log_file = tmp_path / "empty_mask.jsonl" + client = ResultsLowLevelClient(grpc_client=MagicMock()) + + report = await client.create_test_report(test_report=_report_create(), log_file=log_file) + # Written by hand: the guard in update_test_report now refuses to log this. + with log_file.open("a") as handle: + handle.write( + f'[UpdateTestReport] {{"testReport":{{"testReportId":"{report.id_}"}},' + '"updateMask":""}\n' + ) + update = TestReportUpdate(status=TestStatus.FAILED) + update.resource_id = report.id_ + await client.update_test_report(update=update, log_file=log_file) + + LogTracking(last_uploaded_line=1, id_map={report.id_: "real-report"}).save(log_file) + + client.update_test_report = AsyncMock(return_value=_make_report("real-report")) + + await client.import_log_file(log_file, incremental=True) + + # Only the real update was sent; the empty-mask line never reached the API. + client.update_test_report.assert_awaited_once() + sent = client.update_test_report.await_args.kwargs["request"] + assert sent.test_report.status == TestStatus.FAILED.value + # Both remaining lines are behind the cursor, so a later tick re-sends neither. + assert LogTracking.load(log_file).last_uploaded_line == 3 + + +@pytest.mark.asyncio +async def test_update_with_no_fields_is_not_logged(tmp_path): + """An update that changes nothing writes no log entry and returns the entity.""" + log_file = tmp_path / "no_fields.jsonl" + client = ResultsLowLevelClient(grpc_client=MagicMock()) + + report = await client.create_test_report(test_report=_report_create(), log_file=log_file) + lines_before = log_file.read_text().count("\n") + + update = TestReportUpdate(run_id=None) + update.resource_id = report.id_ + with pytest.warns(SiftIgnoredInputWarning, match="requested no field changes"): + returned = await client.update_test_report( + update=update, log_file=log_file, existing=report + ) + + assert returned is report + assert log_file.read_text().count("\n") == lines_before + + @pytest.mark.asyncio async def test_incremental_and_new_report_are_rejected_together(tmp_path): """The two flags contradict each other, so asking for both is an error. diff --git a/python/lib/sift_client/_tests/sift_types/test_base.py b/python/lib/sift_client/_tests/sift_types/test_base.py index a0c3cfc584..1562e70d60 100644 --- a/python/lib/sift_client/_tests/sift_types/test_base.py +++ b/python/lib/sift_client/_tests/sift_types/test_base.py @@ -2,11 +2,13 @@ from __future__ import annotations +import warnings from datetime import datetime, timezone from typing import ClassVar from unittest.mock import MagicMock import pytest +from pydantic import ConfigDict, ValidationError from sift.calculated_channels.v2.calculated_channels_pb2 import ( CalculatedChannel as CalculatedChannelProto, ) @@ -14,12 +16,14 @@ CreateCalculatedChannelRequest, ) +from sift_client.errors import SiftIgnoredInputWarning from sift_client.sift_types._base import ( BaseType, MappingHelper, ModelCreate, ModelUpdate, ) +from sift_client.sift_types.annotation import PhaseCreate class SimpleCreateModel(ModelCreate[CreateCalculatedChannelRequest]): @@ -226,6 +230,62 @@ def test_update_requires_resource_id(self): model.to_proto_with_mask() +class TestUnknownKeys: + """Tests for the warning on input keys that are not fields of the model.""" + + def test_unknown_key_warns_and_is_ignored(self): + """An unrecognized key contributes nothing to the field mask.""" + with pytest.warns( + SiftIgnoredInputWarning, match="Unknown field `tags` for SimpleUpdateModel" + ): + model = SimpleUpdateModel.model_validate({"tags": ["a"], "name": "new_name"}) + + model.resource_id = "test_id" + _, mask = model.to_proto_with_mask() + assert mask.paths == ["name"] + + def test_unknown_key_suggests_a_near_match(self): + """A misspelled or camelCase key is usually a typo for a real field.""" + with pytest.warns(SiftIgnoredInputWarning, match=r"did you mean `description`\?"): + SimpleUpdateModel.model_validate({"descriptionn": "x"}) + + def test_unknown_key_on_a_create_model_warns(self): + """A create drops the key too, which loses the value instead of no-opping.""" + with pytest.warns( + SiftIgnoredInputWarning, match="Unknown field `unit` for SimpleCreateModel" + ): + SimpleCreateModel.model_validate({"name": "n", "unit": "volts"}) + + def test_known_keys_do_not_warn(self): + """The common case stays quiet, including a field explicitly set to None.""" + with warnings.catch_warnings(): + warnings.simplefilter("error", SiftIgnoredInputWarning) + SimpleUpdateModel.model_validate({"name": "new_name", "description": None}) + + def test_extra_forbid_model_is_left_to_raise(self): + """A model that opts into extra="forbid" reports the key itself.""" + + class StrictUpdateModel(SimpleUpdateModel): + model_config = ConfigDict(extra="forbid") + + with warnings.catch_warnings(): + warnings.simplefilter("error", SiftIgnoredInputWarning) + with pytest.raises(ValidationError): + StrictUpdateModel.model_validate({"tags": ["a"]}) + + def test_extra_forbid_is_detected_through_inheritance(self): + """The real forbid model sets the config on a parent, not on itself. + + ``PhaseCreate`` inherits from ``AnnotationCreateBase``, so the skip relies + on pydantic merging parent config into the subclass. Asserting it against + a locally defined model would not exercise that merge. + """ + with warnings.catch_warnings(): + warnings.simplefilter("error", SiftIgnoredInputWarning) + with pytest.raises(ValidationError): + PhaseCreate.model_validate({"name": "p", "state": "open"}) + + class TestMappingHelper: """Tests for MappingHelper functionality.""" diff --git a/python/lib/sift_client/errors.py b/python/lib/sift_client/errors.py index 657c6a9488..b8f511e60e 100644 --- a/python/lib/sift_client/errors.py +++ b/python/lib/sift_client/errors.py @@ -11,6 +11,14 @@ class SiftExperimentalWarning(SiftWarning): """Warning for experimental features.""" +class SiftIgnoredInputWarning(SiftWarning): + """Warning for input the SDK ignored. + + Covers an unrecognized field on a Create/Update pydantic model, and an update + that names no fields to change. + """ + + class SiftCredentialsError(ValueError): """Raised when Sift credentials cannot be resolved. diff --git a/python/lib/sift_client/sift_types/_base.py b/python/lib/sift_client/sift_types/_base.py index 7af93087e6..7acec28abd 100644 --- a/python/lib/sift_client/sift_types/_base.py +++ b/python/lib/sift_client/sift_types/_base.py @@ -1,5 +1,7 @@ from __future__ import annotations +import difflib +import warnings from abc import ABC, abstractmethod from datetime import datetime from enum import Enum @@ -16,6 +18,9 @@ from google.protobuf import field_mask_pb2, message from pydantic import BaseModel, ConfigDict, Field, PrivateAttr, model_validator +from sift_client._internal.util.util import caller_stacklevel +from sift_client.errors import SiftIgnoredInputWarning + if TYPE_CHECKING: from sift_client.client import SiftClient @@ -113,6 +118,37 @@ class ModelCreateUpdateBase(BaseModel, ABC): def __init__(self, **data: Any): super().__init__(**data) + @model_validator(mode="before") + @classmethod + def _warn_on_unknown_keys(cls, data: Any) -> Any: + """Warn for input keys that are not fields of this model. + + Pydantic's default ``extra="ignore"`` drops an unrecognized key in + silence. A create then omits that field, and an update carries an empty + field mask, which the API rejects. + + Warn rather than raise, since a reporting mistake must not fail a test + run in progress. ``filterwarnings`` promotes it to an error. A model that + sets ``extra="forbid"`` raises on its own and is left alone; that is what + ``PhaseCreate`` does, and forbidding here instead would abort a live test + run over a dropped field. + """ + if not isinstance(data, dict) or cls.model_config.get("extra") == "forbid": + return data + unknown = [key for key in data if key not in cls.model_fields] + if not unknown: + return data + known = sorted(cls.model_fields) + for key in unknown: + close = difflib.get_close_matches(str(key), known, n=1, cutoff=0.6) + hint = f" (did you mean `{close[0]}`?)" if close else "" + warnings.warn( + f"Unknown field `{key}` for {cls.__name__}{hint}; ignored.", + SiftIgnoredInputWarning, + stacklevel=caller_stacklevel(), + ) + return data + @model_validator(mode="after") def _check_mapping_helpers(self): if self._to_proto_helpers: