Skip to content

CBG-5804 fix SyncGateway.get_document error cases - #519

Open
torcolvin wants to merge 1 commit into
mainfrom
CBG-5804
Open

CBG-5804 fix SyncGateway.get_document error cases#519
torcolvin wants to merge 1 commit into
mainfrom
CBG-5804

Conversation

@torcolvin

Copy link
Copy Markdown
Collaborator

CBG-5804 fix SyncGateway.get_document error cases

  • GET /ks/doc will return only the document body inline. If there was a key named "error", this function would raise an exception.
  • Document the actual behavior.
  • Remove None as a possible return since it can only ever return a RemoteDocument, remove unecesssary asserts on None
  • replace expected assertions with pytest.raises to make sure the expected exception is raised

- GET /ks/doc will return only the document body inline. If there was a
key named "error", this function would raise an exception.
- Document the actual behavior.
- Remove None as a possible return since it can only ever return a
  RemoteDocument, remove unecesssary asserts on None
- replace expected assertions with pytest.raises to make sure the
  expected exception is raised

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The updated get_document path can still fail for legitimate documents containing a top-level "error" field (via RemoteDocument), and one updated retry-based test risks flaking because async_retry_assert only retries on AssertionError, not CblSyncGatewayBadResponseError.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR updates the Python Sync Gateway client (cbltest.api.syncgateway.SyncGateway.get_document) to align with actual Sync Gateway GET-document behavior by always returning a RemoteDocument (or raising on HTTP errors), and then refactors many system tests to stop treating a missing document as None and to use exception-based assertions instead.

Changes:

  • Change SyncGateway.get_document to return RemoteDocument (no None return) and document that failures surface as CblSyncGatewayBadResponseError (e.g., 404 for missing/tombstoned docs).
  • Remove redundant is not None assertions throughout QE/dev_e2e tests and convert “expected missing” cases to pytest.raises.
  • Adjust a variety of tests to rely on “no exception == document exists” semantics when validating Sync Gateway state.
File summaries
File Description
tests/shared/upgrade_test_helpers.py Removes None assertions for SG docs now that get_document is non-optional.
tests/QE/test_xattrs.py Updates doc-exists checks to rely on get_document raising on missing rather than returning None.
tests/QE/test_ttl.py Converts expiry validation from try/except + None checks to pytest.raises(CblSyncGatewayBadResponseError).
tests/QE/test_server_setup.py Removes redundant None assertion when validating SG import.
tests/QE/test_replicator_encryption_hook.py Removes redundant SG None assertions; tests now fail via exceptions if doc is missing.
tests/QE/test_replication_upgrade_delta_sync.py Removes redundant None assertion before reading remote revision info.
tests/QE/test_replication_multiple_clients.py Removes redundant None assertion when validating SG docs after replication.
tests/QE/test_replication_functional.py Removes redundant None assertions when validating SG docs across replication scenarios.
tests/QE/test_no_conflicts.py Removes redundant None assertions for SG docs during conflict/no-conflict scenarios.
tests/QE/test_large_doc_workloads.py Converts “oversized doc must be missing” validation to pytest.raises and removes redundant None checks.
tests/QE/test_delta_sync.py Removes redundant None assertions for baseline SG docs used in delta-sync comparisons.
tests/QE/edge_server/test_system.py Removes redundant None assertions when validating SG docs during edge-server system scenarios.
tests/QE/edge_server/test_replication_sanity.py Removes redundant None assertions when validating SG docs during sanity replication checks.
tests/QE/edge_server/test_replication_edge_server.py Simplifies assertions to rely on get_document raising if missing.
tests/QE/edge_server/test_blobs.py Removes redundant None assertions when validating blob docs in SG.
tests/dev_e2e/test_replication_filter.py Removes redundant None assertions for SG docs used in filter tests.
tests/dev_e2e/test_replication_blob.py Removes redundant None assertion for SG blob test doc existence.
tests/dev_e2e/test_replication_behavior.py Removes None assertion inside a retry loop (needs follow-up to preserve retry semantics on exceptions).
tests/dev_e2e/test_replication_auto_purge.py Removes redundant None assertions prior to updating remote docs.
tests/dev_e2e/test_encrypted_properties.py Removes redundant None assertion for SG doc existence in encryption test.
tests/dev_e2e/test_custom_conflict.py Removes redundant None assertions when fetching SG docs for conflict setup/mutations.
client/src/cbltest/api/syncgateway.py Changes get_document return type/behavior and updates trace attributes + docstring accordingly.
Review details
  • Files reviewed: 22/22 changed files
  • Comments generated: 3
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment on lines 1453 to +1457
response = await self._send_request("get", f"/{db_name}.{scope}.{collection}/{doc_id}")
if not isinstance(response, dict):
raise ValueError("Inappropriate response from sync gateway get /doc (not JSON)")

cast_resp = cast(dict, response)
if "error" in cast_resp:
if cast_resp["reason"] == "missing" or cast_resp["reason"] == "deleted":
return None

raise CblSyncGatewayBadResponseError(
500,
f"Get doc from sync gateway had error '{cast_resp['reason']}'",
body=dumps(cast_resp),
)

return RemoteDocument(cast_resp)
return RemoteDocument(cast(dict, response))
Comment on lines 155 to 159
async def _confirm_resurrected_on_sg() -> None:
remote_doc = await sync_gateway.get_document("names", loc_deleted)
assert remote_doc is not None, f"{loc_deleted} not yet visible on Sync Gateway"
assert remote_doc.body.get("name") == resurrected_body["name"], (
f"{loc_deleted} on Sync Gateway does not reflect the resurrected content yet"
)
Comment on lines 1444 to 1451
with self._tracer.start_as_current_span(
"get_document",
attributes={
"cbl.database.name": db_name,
"cbl.scope.name": scope,
"cbl.collection.name": collection,
"cbl.document.id": doc_id,
"sg.database.name": db_name,
"sg.scope.name": scope,
"sg.collection.name": collection,
"sg.document.id": doc_id,
},
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants