[SYNPY-1912] Fix upsert_rows misreporting Table update responses

[andrewelamb]

Problem:

JIRA: SYNPY-1912

Every successful Table.upsert_rows() call logs a false failure claim. Reported by a user on 4.12, still present on develop at 4.13.0:

[syn76890550:demo-table]: Found 5 rows to update and 2 rows to insert. 5 rows could not be updated.

All 5 updates and both inserts were applied. The two halves of the message contradict each other by construction: the failure count always equals the full number of updated rows, so it is never a real partial count.

Reproduction: store 5 rows in a Table, then upsert_rows() those 5 keys with new values plus 2 new keys. A follow-up query confirms all 7 rows are correct, while the client has already logged "5 rows could not be updated."

Root cause

TableUpdateTransaction.fill_from_dict() in synapseclient/models/table_components.py recognised only one response shape. It looked for an updateResults key and collected entityId values that carry no failureCode or failureMessage.

updateResults is a view-only concept. A Table upsert sends an AppendableRowSetRequest for the update half and an UploadToTableRequest for the insert half. The server answers with RowReferenceSetResults and UploadToTableResult respectively. Neither carries updateResults, and neither carries entityId, because table rows are not entities. So entities_with_changes_applied stayed None for every table upsert.

The message logic in synapseclient/models/mixins/table_components.py then drew the wrong conclusion:

• The count of actually-updated rows stayed 0.
• The printed count used a total_row_count_actually_updated or total_row_count_to_update fallback, so 0 was falsy and the correct count (5) was printed.
• A separate comparison of 0 against 5 found a shortfall and appended "5 rows could not be updated."

The three defects masked each other, which is why the message was self-contradictory rather than simply wrong. All three had to change together, or the message would print 0.

The raw response recorded from production Synapse for the update half (syn76890550):

{
  "concreteType": "org.sagebionetworks.repo.model.table.RowReferenceSetResults",
  "rowReferenceSet": {
    "tableId": "syn76890550",
    "etag": "5aac0c05-c0dc-4119-b284-4c394a6044aa",
    "rows": [
      {"rowId": 1, "versionNumber": 2},
      {"rowId": 2, "versionNumber": 2},
      {"rowId": 3, "versionNumber": 2},
      {"rowId": 4, "versionNumber": 2},
      {"rowId": 5, "versionNumber": 2}
    ]
  }
}

Five row references, no failureCode and no failureMessage anywhere. The insert half returned UploadToTableResult with rowsProcessed 2.

Secondary defects in the same code

1. row_update_results was assigned rather than accumulated inside the per-query-chunk loop. With rows_per_query defaulting to 50000, any upsert over 50k rows discarded all but the last chunk's results, so the count was wrong even on views, where the parsing did work.
2. failureCode and failureMessage were read and then discarded. A genuine failure gave the user a bare count and no diagnostic detail.
3. With wait_for_eventually_consistent_view on, a non-empty tracking dict, and a final chunk that pushed nothing, _wait_for_eventually_consistent_changes iterated None and raised TypeError.

Solution:

Nothing about how the upsert stores data changes. Only how the client accounts for what the server reported.

Model every TableUpdateResponse type

Rather than adding shape-sniffing branches inline, every response type Synapse can return is now a dataclass, so the count comes off a typed attribute instead of a dict key. In synapseclient/models/table_components.py:

• TableUpdateResponse — abstract base holding concrete_type, an abstract fill_from_dict classmethod, and a rows_changed property returning None by default.
• EntityUpdateResults — update_results: list[EntityUpdateResult]; rows_changed counts entities with no reported failure.
• RowReferenceSetResults — row_reference_set; rows_changed is the row count. This is the table update half.
• UploadToTableResult — rows_processed, etag; rows_changed is rows_processed. This is the table insert half.
• TableSchemaChangeResponse and TableSearchChangeResponse — rows_changed stays None, since neither applies rows.
• UnknownTableUpdateResponse — holds the raw response in data: dict. Returned when a response cannot be identified, so a response type added to Synapse after this release neither raises nor is miscounted.
• Supporting types RowReference, RowReferenceSet, EntityUpdateResult (with a succeeded property), and the EntityUpdateFailureCode enum, which coerces an unrecognised code string to UNKNOWN rather than raising.
• table_update_response_from_dict() dispatches on concreteType, falls back to a distinguishing key (rowReferenceSet, rowsProcessed, updateResults, schema, searchEnabled), then to UnknownTableUpdateResponse.

TableUpdateTransaction now derives three aggregates from results on each access and stores none of them:

• total_rows_changed — the sum of rows_changed over every response that reports one, so table row updates, table inserts, and view entity updates all contribute. None before the transaction is sent, 0 when nothing changed.
• failed_entity_updates — the failures of every EntityUpdateResults in results, with their retained entity_id, failure_code, and failure_message.
• entities_with_changes_applied — unchanged in meaning, now computed from the modelled responses instead of a second hand-rolled walk of the raw array. It is still consumed as dictionary keys for the eventually-consistent view wait, so it must hold entity IDs and must stay None rather than [] when nothing succeeded.

fill_from_dict also had a latent defect: a response carrying "results": null raised TypeError. The guard is now if synapse_response.get("results", None).

Fix the reporting

In synapseclient/models/mixins/table_components.py:

• row_update_results is now a list that is extended per query chunk instead of overwritten, so upserts over rows_per_query report correct totals. This also removes the latent TypeError above.
• The 58-line message block was extracted out of _upsert_rows_async into a new module-level _log_upsert_summary(). _upsert_rows_async now reads as query, update, report, insert.
• The reported update count is the sum of total_rows_changed over the accumulated transactions. The or total_row_count_to_update fallback is gone, replaced by an explicit branch on whether anything was pushed, so a dry_run (or a live run where nothing matched) still reports the planned count, while a confirmed count of 0 now prints as 0.
• The failure clause is built only from failures Synapse actually reported, formatted as . 2 rows could not be updated: syn123 (NOT_FOUND); syn124 (UNKNOWN: detail). A count alone was not actionable.
• A shortfall with no reported failure now logs at debug level. It is a client accounting gap, most likely an unmodelled response shape, not a user-facing failure. Promoting it to the info message would reintroduce the original defect for the next response type Synapse adds.
• The wording Found {n} rows to update and {m} rows to insert is byte-for-byte unchanged.

For a Table, failed_entity_updates is always empty, which is correct rather than a gap: a rejected table update fails the async job and raises before the reporting runs. For a Table the honest report is a confirmed count, never a silent partial.

Testing:

Unit tests

tests/unit/synapseclient/mixins/unit_test_table_components.py — seven new classes, 95 tests. Full unit module: 287 tests pass.

• TestTableUpdateResponseFromDict — dispatch: one case per known concrete type, one per distinguishing-key fallback, an unrecognised concrete type that still carries a known key, an unidentifiable response, an empty dict, and that TableUpdateResponse() cannot be instantiated.
• TestTableUpdateResponseRowsChanged — a 12-case parametrized table over rows_changed, plus the non-count fields of each subclass. The cases pin 0 against None: rows: [] and rowsProcessed: 0 give 0, while an absent rowReferenceSet and an absent rowsProcessed give None.
• TestEntityUpdateResult — succeeded across all four code/message combinations, coercion of every documented failure code and of an unrecognised one, message retention, the split into successful_entity_ids and failed_entity_updates, a success with no entityId, and update_results of None.
• TestTableUpdateTransactionFillFromDict — total_rows_changed for table and view responses and summed across three response types in one transaction, an unmodelled response contributing nothing, the failure detail per failure, failures flattened across responses, None versus 0 versus [] for each aggregate, and snapshot_version_number filled alongside the modelled responses.
• TestTableUpdateRequest — a parametrized check that all four request classes are a TableUpdateRequest, which guards against a fifth type being added without the base; that the base cannot be instantiated; the search change payload including search_enabled=False being sent as False rather than dropped; UploadToTableRequest.entity_id reporting table_id; a parametrized table over the table_id/entity_id aliasing on UploadToTableRequest — either alone or both with the same value fill the other field, while neither given or two different values raise ValueError; and one transaction carrying all four changes in the order given.
• TestUpsertRowsResultReporting — 10 tests driving _upsert_rows_async directly with a minimal Table and View entity, patching _push_row_updates_to_synapse and asserting on client.logger. This is the first credential-free coverage of the reported defect: the byte-for-byte success message with no failure clause, accumulation across three query chunks, dry_run reporting the planned count, a confirmed count of 0 printing as 0 with the gap logged at debug level, and a 5-case parametrized table over the failure-clause format.
• TestLogUpsertSummary — 12 tests on the extracted helper, using real RowReferenceSetResults and EntityUpdateResults payloads rather than mocks, so the total_rows_changed and failed_entity_updates properties are actually exercised. Covers the dry-run path, summing across transactions, responses carrying no row count, a 4-case parametrized failure-clause table, failures flattened across transactions suppressing the debug gap, a shortfall with no failure logging the gap, and more rows confirmed than sent logging no gap (the < boundary).

Integration tests

tests/integration/synapseclient/models/async/test_table_async.py

• capture_client_logs() — module-level helper that attaches a handler directly to syn.logger. Needed because the integration syn fixture uses SILENT_LOGGER_NAME, which has propagate: False, so caplog sees nothing.
• TestUpsertRows.test_upsert_reports_accurate_row_counts — stores 5 rows, upserts those 5 keys with new values plus 2 new keys, asserts the table holds all 7 correct rows, then asserts the logged message contains Found 5 rows to update and 2 rows to insert and does not contain could not be updated.
• Parametrized on rows_per_query: 50000 (single query chunk) and 2 (four query chunks). The response modelling fixes the first case; the accumulation fix is what fixes the second.