[hugin] Port the 7.2.3 → 7.2.5 client surface - #305
Conversation
Add the CdcSinkConfiguration family and the add/update operations, and wire the task into ongoing-task info, the DatabaseRecord, and the public exports. Serialization matches the reference client: CdcColumnMapping omits Type when it is Default, CdcSinkTaskState keeps a case-insensitive Tables dict whose keys keep their stored casing on the wire, and CdcSinkTableLoadState writes null lists instead of empty arrays. The add/update commands are MaintenanceOperation/RavenCommand pairs with the RaftCommand marker and a null-response throw, so server rejections surface as RavenException through ExceptionDispatcher; no client-side validation runs before the request is sent. Reforge-Run: 20260819T014403Z-2668162-reforge
Bump RequestExecutor.CLIENT_VERSION to 7.2.5 so the Raven-Client-Version header carries the new version. Add DisableChecksumValidation to the S3 backup and remote-attachment settings with value equality and hashability that include the flag, matching the reference Equals/GetHashCode overrides. Expose UsedBy on every connection-string class through a typed ConnectionStringUsage object parsed from the GET response; to_json never writes it, so a GET -> from_json -> to_json round-trip does not leak the server-computed metadata. Append HubCursor and SinkCursor to the pull-replication-as-sink task info after AllowedSinkToHubPaths. Reforge-Run: 20260819T014403Z-2668162-reforge
Add ServerWideConnectionString wrapping any concrete connection string plus excluded databases, and put/get/remove operations against /admin/configuration/server-wide/connection-strings. The wrapper serializes the inner connection string with exactly one Type key carrying the enum NAME string and ExcludedDatabases (null when unset); UsedBy is server-computed metadata and is never written. from_json dispatches on Type, yields None when Type is missing (the server rejects such bodies), and parses the usages including their DatabaseName. The GET query carries name then type, each only when set, with the type value as the enum NAME, never the Python repr. Put and remove implement the RaftCommand marker and raise on a null response like the reference commands. Reforge-Run: 20260819T014403Z-2668162-reforge
Add the AzureServiceBus broker type and the connection-settings classes,
which validate that exactly one authentication method is set: a
connection string containing sb://, a fully populated EntraId, or a
Passwordless with a namespace. get_service_bus_url extracts the sb://
endpoint from the connection string preserving input case, or builds
sb://{namespace}/ from the auth classes. to_json writes only the set
fields, to_audit_json masks the secrets, and all three classes implement
equality with a consistent hash.
AzureServiceBusSinkSource encodes a plain queue name as a queue and
'topic;subscription' as a topic subscription; entry validation reports
errors naming the script and the entry, with the reference error
messages.
Reforge-Run: 20260819T014403Z-2668162-reforge
Add GetConversationMessagesOperation with its options and typed results for reading agent conversation messages, reachable from the store through the maintenance executor. The command sends GET to /ai/agent/conversation/messages with the raven 7-digit timestamp form for before/after and the detail-level enum name; a 404 (null response) leaves the result None, matching the reference SetResponse. Parameters keep their native JSON types. RunConversationOperation always appends cancelPendingActionTools to the URL immediately after the debug parameter, and AiConversation resets the flag after a successful run. The streaming loop observes a caller cancellation signal between lines so a mid-stream cancel stops the read promptly. Reforge-Run: 20260819T014403Z-2668162-reforge
Add usage, sso_server_public_key_pinning_hashes, allow_any_sso_server, and sso_identifiers to CertificateMetadata, with the CertificateUsage and SsoProvider enums and the SsoIdentifier class. CertificateDefinition.to_json writes the SSO keys after Disabled; from_json defaults a missing Usage to None, missing lists to [], and missing AllowAnySsoServer to False. EditClientCertificateOperation.Parameters gains the three nullable SSO fields. The edit body is written manually with conditional SSO keys: a field is written only when provided (an empty list clears the stored value), and each SsoIdentifier writes Domain only when non-empty. The body is never built from CertificateDefinition.to_json, whose unconditional SSO defaults would wipe a stored SSO configuration on a plain permission edit. Reforge-Run: 20260819T014403Z-2668162-reforge
Capture the Database-Cluster-Tx-Id response header into
SessionInfo.cluster_transaction_id in the request-executor success path,
guarded on the header's presence, before process_response. The session
needs the cluster id for the change-vector fix, mirroring the reference
client's SessionInfo chain.
UpdateEntityDocumentInfo splits a document's change vector on '|' when
the session has a cluster-transaction id: more than two parts throw with
the document id and vector in the message, otherwise the etag for the
cluster id is read from the LAST part only and
last_cluster_transaction_index advances to the max of the current value
and the etag. A null change vector is a no-op. ClientChangeVectorUtils
carries the separator and the GetEtagById logic (etag between the last
':' and the '-{id}' marker, 0 when the id is absent).
Reforge-Run: 20260819T014403Z-2668162-reforge
751c85e to
940f296
Compare
| @@ -0,0 +1,244 @@ | |||
| """Tests for the server-wide connection strings surface: | |||
There was a problem hiding this comment.
Blocker: this directory has no __init__.py, so none of these tests ever run.
unittest discover skips directories that are not importable packages, and CI runs python -m unittest discover. Measured on this branch:
loader.discover("ravendb/tests") -> 272 modules
modules under serverwide_tests -> 0
All three files here (test_certificates_sso.py, test_server_wide_connection_strings.py, test_server_wide_connection_strings_live.py, about 510 lines) are dead in CI. They pass when loaded by name directly, which is why this is invisible locally.
cdc_sink_tests/ and queue_tests/ both got an __init__.py, this one was missed. Adding an empty ravendb/tests/serverwide_tests/__init__.py fixes it.
Separately: there is already a ravendb/tests/jvm_migrated_tests/serverwide_tests/, so we now have two test packages with the same name. Worth picking a different name, or following TESTING.md and putting these under ravendb/tests/operations_tests/.
| from typing import Any, Dict, Iterator, List, Optional, Tuple | ||
|
|
||
|
|
||
| class CdcSinkTablesDict(dict): |
There was a problem hiding this comment.
Blocker (bug) plus a design question.
The bug: clear(), popitem(), copy() and fromkeys() are not overridden, so they operate on the underlying storage and leave _original_keys stale. to_json() below iterates self.tables.keys() and then looks each key up, so after a clear() it raises:
state = CdcSinkTaskState(configuration_name="x")
state.tables["A"] = CdcSinkTableLoadState()
state.tables.clear()
len(state.tables) # 0
state.tables.keys() # ['A']
state.to_json() # KeyError: 'a'The design question: is this class needed at all? In C# Tables is a plain Dictionary<string, CdcSinkTableLoadState> with an OrdinalIgnoreCase comparer, and CdcSinkTaskState is the @cdc-states state document that the server writes and reads. Nothing in this client looks a table up by name, case-insensitively or otherwise. 60 lines of comparer emulation are being carried for a behavior we never exercise.
There is also a cost beyond the bug: subclassing dict while returning lists from keys() / items() / values() breaks the mapping contract those methods are supposed to satisfy (they return views), so anything that treats this as a dict can behave unexpectedly.
Suggestion: make tables a plain Dict[str, CdcSinkTableLoadState]. If a case-insensitive lookup turns out to be needed later, a small get_table(name) helper on CdcSinkTaskState is enough, without overriding the type.
| def test_request_executor_captures_header_in_success_path(self): | ||
| from ravendb.http.request_executor import RequestExecutor | ||
|
|
||
| source = open("ravendb/http/request_executor.py").read() |
There was a problem hiding this comment.
Blocker: this asserts on the text of a source file, not on behavior.
source = open("ravendb/http/request_executor.py").read()
success_index = source.index("command.process_response(self._cache, response, url)")
header_index = source.index("DATABASE_CLUSTER_TRANSACTION_ID in response.headers")
self.assertLess(header_index, success_index)Two problems. It depends on the process CWD, so it errors as soon as the suite runs from anywhere but the repo root (verified: FileNotFoundError: 'ravendb/http/request_executor.py'). And it pins the formatting of an unrelated file rather than the behavior we care about, so a harmless reordering or rename breaks it while a real regression in the capture would still slip through.
The behavior worth testing is "given a response carrying Database-Cluster-Tx-Id, the session info ends up with that value". That is a stub of RequestExecutor.execute or, more simply, the integration test that already exists in test_cluster_transaction_change_vector_integration.py. I would drop this one.
Also, the from ravendb.http.request_executor import RequestExecutor two lines up is unused.
| self.entra_id = entra_id | ||
| self.passwordless = passwordless | ||
|
|
||
| def is_valid_connection(self) -> bool: |
There was a problem hiding this comment.
This whole block (is_valid_connection, _is_only_one_connection_provided, get_service_bus_url, _try_extract_endpoint, _get_namespace, plus to_audit_json on all three classes and the __eq__ / __hash__ triples) has no caller anywhere in the client. The only references are in the new tests.
Compare with the siblings in this same package, which are the convention we settled on for queue connection settings:
KafkaConnectionSettings:to_json,from_jsonRabbitMqConnectionSettings:to_json,from_jsonAmazonSqsConnectionSettings+AmazonSqsCredentials:to_json,from_jsonAzureQueueStorageConnectionSettings+EntraId+Passwordless:to_json,from_json
All of those have the same IsValid / GetUrl / ToAuditJson / Equals surface on the C# side, and we deliberately did not port it, because validation is the server's job and the client just gets a RavenException back. to_audit_json in particular is a server audit-log concern with no meaning in a client SDK.
I would cut this down to to_json / from_json on all three classes and let the server reject bad configurations, the way the other four brokers already do. If we want client-side validation for Service Bus, that is a fine conversation, but then it should be added for every broker in one go rather than only for the newest one.
| from typing import List, Optional, Tuple | ||
|
|
||
|
|
||
| class AzureServiceBusSinkSource: |
There was a problem hiding this comment.
Same point as on the connection settings: this file has no caller in the client. queue(), subscription(), validate_entry(), validate_script() and try_parse_subscription() are only reached from test_azure_service_bus.py, and the class is not exported from ravendb/__init__.py either, so a user cannot reach it as from ravendb import AzureServiceBusSinkSource.
validate_script() in particular is the server's queue-sink validation, which the client never runs.
Two smaller things if this does stay:
if not queue_name or (isinstance(queue_name, str) and queue_name.isspace())is repeated five times. The parameter is annotatedstr, sonot queue_name or queue_name.isspace()already coversNone,""and whitespace. Theisinstanceguard only exists because the tests passNoneinto astrparameter.- A class holding only
@staticmethods is a C# static-class shape. Module-level functions are the Python equivalent and read better here.
| response = FakeResponse() | ||
| with self.assertRaises(Exception) as ctx: | ||
| command.process_response(HttpCache(), response, "http://x") | ||
| self.assertIsInstance(ctx.exception, Exception) |
There was a problem hiding this comment.
assertIsInstance(ctx.exception, Exception) inside with self.assertRaises(Exception) cannot fail, assertRaises(Exception) has already established it. So the only real assertion in this test is assertLess(len(lines_read), 100), and the exception type goes unchecked.
Given that we are deliberately choosing which exception cancellation raises, that is the thing worth pinning:
with self.assertRaises(OperationCancelledException):
command.process_response(HttpCache(), response, "http://x")
self.assertLess(len(lines_read), 100, "the stream must not be fully consumed")(OperationCancelledException assuming we switch to the existing cancellation primitive, see my comment on run_conversation_operation.py.)
Also from ravendb.http.misc import ResponseDisposeHandling at the top of this test is unused.
| # Server rejected the task (e.g. community server without a CDC Sink | ||
| # license): the rejection must surface as a RavenException whose message | ||
| # embeds the server's error text, never the Message field alone. | ||
| self.assertIsInstance(e, RavenException) |
There was a problem hiding this comment.
self.assertIsInstance(e, RavenException) inside except RavenException as e: is always true.
More importantly, the except branch ends in return, so on any server without a CDC Sink license this test passes without having tested anything. That is a reasonable thing to want, but a skip states it honestly while a silent return reports green coverage we do not have. Since the class is already gated on RAVENDB_LICENSE, either the gate is enough and the try/except can go, or the branch should be self.skipTest("CDC sink not licensed on this server").
Same pattern at line 115.
The except Exception: pass pairs in tearDown will also hide real cleanup failures. self.addCleanup(...) gives per-resource cleanup without swallowing.
| self.assertEqual("cdc-1", task.configuration.name) | ||
|
|
||
| def test_nullable_fields_stay_none_when_absent(self): | ||
| task = OngoingTaskCdcSink.from_json(self._task_dict()) |
There was a problem hiding this comment.
This line is dead, the very next statement rebinds task. Looks like an editing leftover, safe to delete.
| self.assertEqual("7.2.5", RequestExecutor.CLIENT_VERSION) | ||
|
|
||
| def test_wire_header_carries_client_version(self): | ||
| executor = object.__new__(RequestExecutor) |
There was a problem hiding this comment.
object.__new__(RequestExecutor) plus manually setting four private attributes to reach _set_request_headers is very tightly coupled to the current internals. It breaks on any refactor of the constructor or those fields, and when it does the failure points at the client-version header rather than at what actually changed.
test_client_version_is_7_2_5 right above already pins the constant, which is the part this PR changes, and the header plumbing is exercised by every integration test that talks to a server. I would drop this one.
Minor: OngoingTaskType is imported at line 20 but never used in this module.
| entra_id=AzureServiceBusEntraId(namespace="ns", tenant_id="t", client_id="c", client_secret="s"), | ||
| ) | ||
| result = settings.to_json() | ||
| self.assertEqual({"Endpoint=sb://x/"}, {result["ConnectionString"]}) |
There was a problem hiding this comment.
self.assertEqual({"Endpoint=sb://x/"}, {result["ConnectionString"]}) wraps both sides in single-element sets. Plain equality says the same thing and produces a readable diff on failure:
self.assertEqual("Endpoint=sb://x/", result["ConnectionString"])Similar in test_cdc_sink_configuration.py: self.assertTrue(out["Tables"]["products"]["InitialLoadCompleted"] is False) is assertFalse(...).
OverallThanks for driving this through. The coverage of the 7.2.3 to 7.2.5 delta looks complete, and splitting one commit per feature area made this reviewable. I checked the riskiest ports against the C# v7.2 sources and they hold up:
My main concern is not correctness. It is that large parts of this read as C# transcribed into Python rather than Python written against the same behavior. The skill we use for these syncs says it explicitly: port the behavior, not the syntax. Four themes: 1. C# ceremony carried over literally. 2. Server-side logic ported into client DTOs. 3. New mechanisms next to ones we already have. The client already has 4. Tests that document the porting session rather than the client. Roughly 2550 lines of tests against 2027 lines of code, but a large share asserts things that cannot regress: JSON key order (14 assertions, versus about 6 in the entire existing suite), the absence of fields that never existed, and in one case the literal text of a source file. Two Blockers
Details are in the 22 inline comments on the review above. |
2 participants
Ports everything the reference client (
src/Raven.Client, 7.2.3 → 7.2.5) gained into the Python client — every observable change in that delta: new public surface, wire changes, behavior changes, and bug fixes a caller or the server can see, in the client's own conventions. Nothing outside the delta is touched.One commit per feature area:
CLIENT_VERSIONbump,DisableChecksumValidationon both S3 settings classes,usedBysurfaced across the connection-string family, pull-replication sink cursorsGetConversationMessagesOperationwith paging and detail levels, 404-as-None, pluscancelPendingActionToolsand cancellation-aware streaming readsThis PR was produced by hugin's reforge workflow; each commit carries its provenance trailer.