Skip to content

[hugin] Port the 7.2.3 → 7.2.5 client surface - #573

Draft
redknightlois wants to merge 9 commits into
ravendb:v7.2from
redknightlois:draft-hugin.v7.2.5
Draft

[hugin] Port the 7.2.3 → 7.2.5 client surface#573
redknightlois wants to merge 9 commits into
ravendb:v7.2from
redknightlois:draft-hugin.v7.2.5

Conversation

@redknightlois

@redknightlois redknightlois commented Aug 17, 2026

Copy link
Copy Markdown
Member

This ports what the .NET client (src/Raven.Client) gained between 7.2.3 and 7.2.5 into the Node.js client, following this client's own conventions. Only the 7.2.3 to 7.2.5 changes are in scope: things the .NET client already had before 7.2.3 that were never ported are left alone, and anything the .NET client marks internal stays out of the public API.

One commit per feature, in dependency order; each one builds and passes its tests on its own:

  • CDC Sink tasks: AddCdcSinkOperation, UpdateCdcSinkOperation, the configuration and column mapping types, and the task wired into ongoing tasks, smuggler and the database record
  • Server-wide connection strings: GET, PUT and DELETE server operations and the ServerWideConnectionString type
  • Reading AI conversation messages: GetConversationMessagesOperation with paging, detail levels and a null result on 404; every conversation run now sends cancelPendingActionTools=false, as the .NET client does
  • Azure Service Bus in the Queue family: broker type, connection settings and sink source helpers
  • Hub and sink cursors on the pull replication sink task info
  • disableChecksumValidation on the S3 settings
  • SSO members on certificate metadata: usage, ssoServerPublicKeyPinningHashes, allowAnySsoServer, ssoIdentifiers
  • Certificate edit path: EditClientCertificateOperation takes disabled and always sends Disabled; the SSO fields are sent only when given
  • CLIENT_VERSION pinned to 7.2.5; the npm package version in package.json stays 7.2.6

Checks: tsc --noEmit and eslint are clean; request and response shapes were checked against a live 7.2.5 server and against the .NET types; there is one test file per feature under test/Ported, and the ones that need a server are gated on the test server.

This is a new series that replaces the first one after the review. The comment below lists what changed and why some things were left out.

Produced by hugin's reforge workflow; each commit carries a Reforge-Run trailer.

@M4xymm
M4xymm self-requested a review August 17, 2026 06:46

@M4xymm M4xymm left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Strong depth where it goes (full CdcSink Schema/ + Test/ surface, extra RunConversationOperation work, careful wire-fidelity comments), and everything compiles, lints, and passes export checks. However, there is one functional regression (dropping Disabled from the edit-certificate payload), the release plumbing is missing (no package.json/lock bump), and no Snowflake ETL, no Amazon SQS / Azure Queue Storage settings and no CdcSinkProcessState

@@ -11,4 +31,8 @@ export interface CertificateMetadata {
collectionSecondaryKeys?: string[];
collectionPrimaryKey?: string;
publicKeyPinningHash: string;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

disabled is also missing from the metadata interface, while C# CertificateMetadata has public bool Disabled and writes it in ToJson. Reading certificate metadata will drop this field

// case-sensitive permission matching (RDBC-1085).
const definition = {
const definition: any = {
Thumbprint: this._thumbprint,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

the definition payload omits Disabled, and the operation has no disabled parameter at all. The C# command always serializes Disabled (EditClientCertificateOperation.cs:119), so editing a disabled certificate through this client would silently re-enable it.

| "Olap"
| "ElasticSearch"
| "Queue"
| "Snowflake"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

"Snowflake" and "SnowflakeEtl" were added to the type unions, but there is no SnowflakeConnectionString class or SnowflakeEtlConfiguration anywhere in the patch. C# has both under ETL/Snowflake/.

@@ -0,0 +1,81 @@
import { StringUtil } from "../../../../Utility/StringUtil.js";

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

C# ETL/Queue/ also contains AmazonSqsConnectionSettings and AzureQueueStorageConnectionSettings; only Azure Service Bus was ported

return null;
}

const chunks: Buffer[] = [];

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Manual stream buffering (chunks + finishedAsync + stringToReadable) reimplements what this.defaultPipeline( => body = _) already does in every other command in this repo. Prefer the shared idiom unless the empty-body-on-404 case is proven unreachable through it

@redknightlois

Copy link
Copy Markdown
Member Author

This is a new series that replaces the first one. These are the rules it was held to:

  • Only the 7.2.3 to 7.2.5 changes of src/Raven.Client are in scope. Anything the .NET client already had at 7.2.3 that was never ported is a separate job, so it is neither added nor half added here.
  • Visibility follows the .NET client: what it marks internal, or leaves out of the payloads it sends, stays out of the public API.
  • When a request payload is touched, it is brought to parity: a field the .NET client always sends that this client drops is a bug in that file, however old.
  • New code is shaped like the existing code next to it in this repo.
  • CLIENT_VERSION tracks the server version; package.json is the npm release number and is bumped in the release commits.

How that applies to each review point:

  • Disabled missing from the edit payload: fixed. EditClientCertificateOperation takes a disabled parameter (default false) and always sends Disabled; the SSO fields are sent only when given, and Usage is never sent. Covered by CertificateSsoTest.
  • disabled missing from CertificateMetadata: not added in this series. The 7.2.3 to 7.2.5 change adds only the four SSO members to that class, and the read path converts the key casing of the whole response, so disabled already arrives at runtime; only the type does not name it. It is a one-line addition if you want it declared.
  • Snowflake and SnowflakeEtl in the type unions: removed. SnowflakeConnectionString predates 7.2.3 and this client never had it, so no union member without a class behind it. One thing to know: GetServerWideConnectionStringsOperation maps each entry by its type, as the .NET client does, and throws on a type this client does not know, so a server-wide Snowflake connection string would hit that.
  • AmazonSqsConnectionSettings and AzureQueueStorageConnectionSettings: not ported. Both already existed in the .NET client at 7.2.3; the 7.2.3 to 7.2.5 change only added Azure Service Bus. They are a separate port.
  • Manual stream buffering in GetConversationMessagesOperation: replaced with the shared this._defaultPipeline(_ => body = _), with a null check for the empty body on 404.
  • package.json and package-lock bump: left as is, matching how this repo does it (the "Update version to 7.2.x in client" commits are separate); CLIENT_VERSION moves to 7.2.5.
  • CdcSinkProcessState: added, and not exported, the same way QueueSinkProcessState is kept in this repo.
  • Dropped compared with the first series: the CDC sink schema and mapping test operations (internal classes in the .NET client), the cancelPendingActionTools constructor plumbing on AiConversation (internal in the .NET client; the query parameter is sent as false, which is all a public caller can produce there), CdcSinkTaskState (a server-side state document with no client operation) and the X-Forwarded-For constant (nothing in the .NET client uses it).
  • Tests: one file per feature under test/Ported, checking the request and response shapes where no server is needed.

@M4xymm

M4xymm commented Aug 27, 2026

Copy link
Copy Markdown
Member

Tests are failing.

CdcSinkCrudTests, ServerWideConnectionStringsTest tests should look like this:

((RavenTestContext.isRavenDbServerVersion("7.2") && !RavenTestContext.isPullRequest) ? describe : describe.skip)("CdcSinkCrudTest", function () {

Since the tests run during the PR's do not have a license, tests run manually on this branch will also verify the functionality of that pull request

reforge added 9 commits September 1, 2026 18:16
Port the CDC Sink surface the reference client gained in 7.2.5: the
Add/UpdateCdcSinkOperation maintenance operations on
/databases/{db}/admin/cdc-sink, the configuration DTOs, the CdcSink
ongoing-task result, and the database-record and smuggler integration.

The DTOs serialize through the command-payload serializer, so the
wire-required keys carry class defaults and lists default to []. toJSON
omits Type when it is "Default" and Postgres/OnDelete when null; the
reference writes those values explicitly, but the omission shape is what
the server normalizes to and keeps the internal TestMode property off the
wire. GetOngoingTaskInfoOperation gains the CdcSink nestedTypes
(lastBatchTime and lastActivityTime revive as dates) and returns null for
an unknown task instead of throwing. CdcSinkProcessState stays unexported,
matching the QueueSinkProcessState precedent.

The ported integration test mirrors the reference CdcSinkCrudTests and is
gated on a 7.2 server and a non-pull-request CI run, like the other
license-dependent suites.

Reforge-Run: 20260901T210548Z-1936468-reforge
Port the server-wide connection strings surface from the reference client:
ServerWideConnectionString with delegated name/type, and the Get, Put and
Remove operations on /admin/configuration/server-wide/connection-strings.

The PUT body is the flattened inner connection-string fields plus Type,
with ExcludedDatabases omitted when unset, matching what the server
accepts. GET revival restores the typed connection string per entry; an
entry whose Type is absent or not a string revives as a null element
(mirroring the reference), unknown types throw NotSupportedException, and
the server-computed UsedBy is ignored.

The version-gated integration test covers the round trip, propagation to
existing and new databases under the "Server Wide Connection String, {name}"
record key, update, delete, exclusions, and the constructor throws. It is
gated on a 7.2 server and a non-pull-request CI run, like the other
license-dependent suites.

Reforge-Run: 20260901T210548Z-1936468-reforge
Port the GetConversationMessages surface from the reference client: the
operation and its options on /databases/{db}/ai/agent/conversation/messages,
the message DTOs, and the two getConversationMessages overloads on
AiOperations. Every run-conversation request also carries
cancelPendingActionTools=false; the internal knob that could set it true is
not ported.

The command revives lastMessageAt and messages[].timestamp as dates, returns
null on a 404, reads Parameters from the raw body so parameter names stay
verbatim, and normalizes the null list keys the server always sends to
empty arrays.

The ported test under test/Ported/Server/Documents/AI/AiAgent covers the
constructor throws, synthetic-response revival, a seeded conversation
document read, the 404-null path, and the exclusive before bound; it is
gated on a 7.2 server.

Reforge-Run: 20260901T210548Z-1936468-reforge
Port the Azure Service Bus broker surface the reference client gained in
7.2.5: the QueueBrokerType member, the AzureServiceBusConnectionSettings /
EntraId / Passwordless DTOs with the public isValidConnection check, the
QueueConnectionString field, and the AzureServiceBusSinkSource queue and
subscription encoding helpers, plus their exports and tests.

Decisions a reader cannot recover from the diff:
- isValidConnection mirrors the reference exactly: presence-based
  auth-method counting (an EntraId/Passwordless object counts even with all
  fields empty, a connection string only when non-whitespace), then EntraId,
  then Passwordless, then the deliberately shallow case-insensitive sb://
  substring check; the Azure SDK produces the authoritative error at connect
  time.
- The ';' entry separator is collision-safe because Service Bus naming rules
  forbid it in queue, topic, and subscription names; the server validates
  entries at raft apply, so the client helpers only need to produce valid
  encodings.
- The integration test uses a fake sb:// connection string and a disabled
  sink: the authoritative connection check happens in the Azure SDK, so no
  real namespace is reachable in CI, and the server accepts the fake string.

Reforge-Run: 20260901T210548Z-1936468-reforge
Port RavenDB-26709: OngoingTaskPullReplicationAsSink gains hubCursor and
sinkCursor, the two fields the 7.2.5 server sends on every sink task-info
response (change-vector strings once the sink has replicated, null before).
The revival is generic, so no command, nestedTypes, or export change is
needed; the fields are non-optional strings per the family's result
interfaces, and a wire null revives as null. PinToMentorNode stays unported
with the rest of the OngoingTask base by convention. The reference's
IsEqualTo/AllowedPathsEqual helpers and the TCP replication protocol members
are internal or server-side and are not ported.

The unit test feeds the exact 7.2.5 sink-task wire body through
setResponseAsync (it runs on any environment); a gated live read-back
asserts non-empty cursors after replication, polling because the server
persists the cursors to cluster state asynchronously.

Reforge-Run: 20260901T210548Z-1936468-reforge
Adds the S3 checksum-validation toggle to both settings shapes:
non-optional on the backup S3Settings interface, optional on the
RemoteAttachmentsS3Settings class, mirroring their forcePathStyle siblings.
The flag reaches the wire through the existing command-payload serializer
with no command change; an unset field omits the key (the server defaults
false) and an explicit true or false is written under the PascalCase key.

The unit test exercises the periodic-backup payload, the remote-attachments
destination key, and the read-back revival through the client's own
commands.

Reforge-Run: 20260901T210548Z-1936468-reforge
The server-wide certificate read surface reports how a certificate is
used for single sign-on. CertificateMetadata (and CertificateDefinition,
which extends it) gains usage, ssoServerPublicKeyPinningHashes,
allowAnySsoServer and ssoIdentifiers, with SsoProvider, CertificateUsage
and SsoIdentifier as new modules next to their DatabaseAccess and
SecurityClearance siblings.

The members are optional: a 7.2.5 server always serializes all four (with
null, [] and false as the unset values), while an older server sends none
of them, and the generic response key-case transform revives whichever the
wire carries with no command or nested-type change. SsoIdentifier.domain is
optional for the same reason. `disabled` stays undeclared on the metadata
interface: the delta does not add it, even though the read-back carries it
at runtime.

Reforge-Run: 20260901T210548Z-1936468-reforge
EditClientCertificateParameters gains disabled plus the three SSO fields,
and the edit request always writes Disabled: false when the caller leaves
it out, matching what the reference client has sent on every edit. An
omitted key leaves the stored flag untouched, so writing the default is
what lets an edit clear it.

An SSO field reaches the wire only when the caller provides a non-null
value: the server treats an absent field as "leave the stored value alone"
and a present one as a replacement, so an explicitly-empty list is the way
to clear the stored hashes or identifiers. SsoIdentifier.domain is omitted
when it is null or empty. The body never carries Usage, which the server
derives from the existing certificate. The payload keeps the
casing-preserving serializer so permission keys (database names) still
travel verbatim.

The secured-server part of the test needs a certificate-capable server, so
it is gated on the server version like its siblings; the wire-shape and
revival parts need no server.

Reforge-Run: 20260901T210548Z-1936468-reforge
The reference client's version bump for this range moves its assembly
FullVersion to 7.2.5; the target's analogue is the CLIENT_VERSION constant
that RequestExecutor sends as the Raven-Client-Version header. The value
stays in the target's plain Major.Minor.Patch form, without the reference's
build suffix.

The npm release version in package.json and package-lock.json is a separate
number and stays 7.2.6.

Reforge-Run: 20260901T210548Z-1936468-reforge
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.

2 participants