From 751c5f4f731df67e72bbc25260e83f5283a822c4 Mon Sep 17 00:00:00 2001 From: Dusan Malusev Date: Tue, 11 Aug 2026 00:17:46 +0200 Subject: [PATCH] feat(cluster): add a ProtocolVersion enum for withProtocolVersion() `withProtocolVersion()` took a bare int, so a caller had to know that the native protocol version is the byte the protocol itself defines, and a typo produced a runtime error rather than a type error. Add `Cassandra\ProtocolVersion`, an int-backed enum whose cases carry that byte, so V1 through V5 match CASS_PROTOCOL_VERSION_V1 through _V5: ->withProtocolVersion(Cassandra\ProtocolVersion::V4) The method now accepts the enum or an int. The int path stays because the enum names only the five native versions, and the driver also understands versions it does not name, such as the DSE ones. It keeps its own range check. `Cluster\Builder`'s `protocolVersion` property now reports the matching enum case when the value maps to one, and falls back to the plain int when it does not. Code that read that property as an int and mapped a named version will see an enum instead. The descriptor generator did not understand enums: it emitted a create_object handler and an object-allocating registration for every class it saw. Teach it the enum kind, so an internal enum registers through zend_register_internal_enum() with no instance handlers, since its cases are immutable objects the engine builds. --- CHANGELOG.md | 6 + docs/Cassandra/Cluster/Builder.php | 2 +- include/php_scylladb_types.h | 1 + src/CMakeLists.txt | 3 + src/Cluster/Builder.c | 16 ++- src/Cluster/Builder.stub.php | 2 +- src/Cluster/BuilderHandlers.c | 15 ++- src/ProtocolVersion.c | 13 ++ src/ProtocolVersion.stub.php | 24 ++++ .../ClusterBuilderProtocolVersionTest.php | 70 ++++++++++ tests/Unit/IniConfigurationTest.php | 4 +- tools/gen_descriptor/gen_class_descriptor.php | 126 ++++++++++++++---- website/guide/connection-tuning.md | 6 +- 13 files changed, 250 insertions(+), 38 deletions(-) create mode 100644 src/ProtocolVersion.c create mode 100644 src/ProtocolVersion.stub.php create mode 100644 tests/Unit/ClusterBuilderProtocolVersionTest.php diff --git a/CHANGELOG.md b/CHANGELOG.md index 6c2503c85..823eff1e0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -45,6 +45,10 @@ carries no setting by default and `isIdempotent()` then returns `null`. A per-call `idempotent` option overrides the statement. For a batch the driver reads the flag off the batch, so a flag on a statement added to the batch has no effect. +- `Cassandra\ProtocolVersion`, an int-backed enum with the cases `V1` to `V5`. The backing value + is the protocol version byte, so it matches `CASS_PROTOCOL_VERSION_V1` to + `CASS_PROTOCOL_VERSION_V5`. `Cluster\Builder::withProtocolVersion()` accepts a case of this + enum, and still accepts a plain integer for a version the enum does not name. - `Rows::wasApplied(): bool`, which reads the `[applied]` column of a lightweight transaction result. A statement with no condition has no such column, and the method then returns `true`, so you can call it on any result. The `[applied]` column stays readable through `first()` and @@ -132,6 +136,8 @@ ### Changed +- Casting a `Cluster\Builder` to an array now gives a `Cassandra\ProtocolVersion` case for the + `protocolVersion` key, was an integer. A value that no case names stays an integer. - The `scylla-cpp` backend now builds the vendored driver in `third-party/cpp-driver` and links it into `cassandra.so`. Upstream no longer maintains the ScyllaDB cpp-driver, so it ships with the extension and gets fixed here. A checkout builds with no C/C++ driver installed. To link a diff --git a/docs/Cassandra/Cluster/Builder.php b/docs/Cassandra/Cluster/Builder.php index 42facfd52..d111ed105 100644 --- a/docs/Cassandra/Cluster/Builder.php +++ b/docs/Cassandra/Cluster/Builder.php @@ -210,7 +210,7 @@ public function withPersistentSessions($enabled) { } * * NOTE: Apache Cassandra 3.x supports protocol version 3 and 4 only * - * @param int $version The protocol version + * @param \Cassandra\ProtocolVersion|int $version The protocol version * * @return \Cassandra\Cluster\Builder self */ diff --git a/include/php_scylladb_types.h b/include/php_scylladb_types.h index 44cbd2c1e..8477eb369 100644 --- a/include/php_scylladb_types.h +++ b/include/php_scylladb_types.h @@ -812,6 +812,7 @@ extern PHP_SCYLLADB_API zend_class_entry *php_scylladb_core_ce; extern PHP_SCYLLADB_API zend_class_entry *php_scylladb_cluster_ce; extern PHP_SCYLLADB_API zend_class_entry *php_scylladb_default_cluster_ce; extern PHP_SCYLLADB_API zend_class_entry *php_scylladb_cluster_builder_ce; +extern PHP_SCYLLADB_API zend_class_entry *php_scylladb_protocol_version_ce; extern PHP_SCYLLADB_API zend_class_entry *php_scylladb_ssl_options_builder_ce; extern PHP_SCYLLADB_API zend_class_entry *php_scylladb_future_ce; extern PHP_SCYLLADB_API zend_class_entry *php_scylladb_future_prepared_statement_ce; diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index 5a391596a..53e7aa44f 100644 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -21,6 +21,7 @@ target_sources(src PRIVATE UuidGen.c DefaultSession.c PreparedStatement.c + ProtocolVersion.c Set.c SimpleStatement.c Tuple.c @@ -89,6 +90,7 @@ php_scylladb_generate_arginfo(src Blob.stub.php Map.stub.php Cassandra.stub.php + ProtocolVersion.stub.php PreparedStatement.stub.php Collection.stub.php Session.stub.php @@ -127,6 +129,7 @@ php_scylladb_generate_descriptor(src Tuple.stub.php UserTypeValue.stub.php Cassandra.stub.php + ProtocolVersion.stub.php Future.stub.php FuturePreparedStatement.stub.php FutureRows.stub.php diff --git a/src/Cluster/Builder.c b/src/Cluster/Builder.c index de007ba0b..4c557e940 100644 --- a/src/Cluster/Builder.c +++ b/src/Cluster/Builder.c @@ -19,6 +19,7 @@ #include #include +#include #include #include #include @@ -689,14 +690,21 @@ ZEND_METHOD(Cassandra_Cluster_Builder, withPersistentSessions) } ZEND_METHOD(Cassandra_Cluster_Builder, withProtocolVersion) { - zend_long version; + zend_object *versionCase = nullptr; + zend_long version = 0; ZEND_PARSE_PARAMETERS_START(1, 1) - Z_PARAM_LONG(version) + Z_PARAM_OBJ_OF_CLASS_OR_LONG(versionCase, php_scylladb_protocol_version_ce, version) ZEND_PARSE_PARAMETERS_END(); - if (version < 1) + if (versionCase != nullptr) { + version = Z_LVAL_P(zend_enum_fetch_case_value(versionCase)); + } + else if (version < 1) + { + /* An int still gets through for versions the enum does not name, such + * as the DSE ones — so it keeps its own range check. */ zval val; ZVAL_LONG(&val, version); throw_invalid_argument(&val, "version", "a positive number"); @@ -704,7 +712,7 @@ ZEND_METHOD(Cassandra_Cluster_Builder, withProtocolVersion) } auto self = PHP_SCYLLADB_GET_CLUSTER_BUILDER(getThis()); - self->protocol_version = version; + self->protocol_version = (uint32_t)version; RETURN_ZVAL(getThis(), 1, 0); } diff --git a/src/Cluster/Builder.stub.php b/src/Cluster/Builder.stub.php index 3672b3b30..4bd0844d6 100644 --- a/src/Cluster/Builder.stub.php +++ b/src/Cluster/Builder.stub.php @@ -96,7 +96,7 @@ public function withPersistentSessions(bool $enabled = true): static { } - public function withProtocolVersion(int $version): static + public function withProtocolVersion(\Cassandra\ProtocolVersion|int $version): static { } diff --git a/src/Cluster/BuilderHandlers.c b/src/Cluster/BuilderHandlers.c index 5d8a3ba7f..639366eca 100644 --- a/src/Cluster/BuilderHandlers.c +++ b/src/Cluster/BuilderHandlers.c @@ -1,4 +1,5 @@ #include +#include #include #include @@ -199,7 +200,19 @@ HashTable *php_scylladb_cluster_builder_properties(zend_object *object) } ZVAL_BOOL(&usePersistentSessions, self->persist); - ZVAL_LONG(&protocolVersion, self->protocol_version); + + zend_object *protocolVersionCase = nullptr; + if (zend_enum_get_case_by_value(&protocolVersionCase, php_scylladb_protocol_version_ce, + (zend_long)self->protocol_version, nullptr, true) == SUCCESS + && protocolVersionCase != nullptr) + { + ZVAL_OBJ_COPY(&protocolVersion, protocolVersionCase); + } + else + { + ZVAL_LONG(&protocolVersion, self->protocol_version); + } + ZVAL_LONG(&ioThreads, self->io_threads); ZVAL_LONG(&coreConnectionPerHost, self->core_connections_per_host); ZVAL_LONG(&maxConnectionsPerHost, self->max_connections_per_host); diff --git a/src/ProtocolVersion.c b/src/ProtocolVersion.c new file mode 100644 index 000000000..32d0dd035 --- /dev/null +++ b/src/ProtocolVersion.c @@ -0,0 +1,13 @@ +#include + +#include +#include + +/* gen_stub ignores docblocks on enum cases, so `@cvalue` cannot bind a case to + * a CASS_* macro the way it does for class constants. The cases carry literal + * values and these assertions hold them to the driver header instead. */ +static_assert(CASS_PROTOCOL_VERSION_V1 == 1, "Cassandra\\ProtocolVersion::V1 no longer matches CASS_PROTOCOL_VERSION_V1"); +static_assert(CASS_PROTOCOL_VERSION_V2 == 2, "Cassandra\\ProtocolVersion::V2 no longer matches CASS_PROTOCOL_VERSION_V2"); +static_assert(CASS_PROTOCOL_VERSION_V3 == 3, "Cassandra\\ProtocolVersion::V3 no longer matches CASS_PROTOCOL_VERSION_V3"); +static_assert(CASS_PROTOCOL_VERSION_V4 == 4, "Cassandra\\ProtocolVersion::V4 no longer matches CASS_PROTOCOL_VERSION_V4"); +static_assert(CASS_PROTOCOL_VERSION_V5 == 5, "Cassandra\\ProtocolVersion::V5 no longer matches CASS_PROTOCOL_VERSION_V5"); diff --git a/src/ProtocolVersion.stub.php b/src/ProtocolVersion.stub.php new file mode 100644 index 000000000..c05da7b61 --- /dev/null +++ b/src/ProtocolVersion.stub.php @@ -0,0 +1,24 @@ +value)->toBe(1); + expect(ProtocolVersion::V2->value)->toBe(2); + expect(ProtocolVersion::V3->value)->toBe(3); + expect(ProtocolVersion::V4->value)->toBe(4); + expect(ProtocolVersion::V5->value)->toBe(5); + }); + + it('exposes exactly the five native protocol versions', function () { + expect(array_column(ProtocolVersion::cases(), 'name')) + ->toBe(['V1', 'V2', 'V3', 'V4', 'V5']); + }); + + it('resolves a case from its backing value', function () { + expect(ProtocolVersion::from(4))->toBe(ProtocolVersion::V4); + expect(ProtocolVersion::tryFrom(9))->toBeNull(); + }); +}); + +describe('Cassandra\Cluster\Builder protocol version', function () { + + it('defaults to v4', function () { + $props = (array) Cassandra::cluster(); + + expect($props['protocolVersion'])->toBe(ProtocolVersion::V4); + }); + + it('stores the version passed as an enum case', function () { + $props = (array) Cassandra::cluster()->withProtocolVersion(ProtocolVersion::V5); + + expect($props['protocolVersion'])->toBe(ProtocolVersion::V5); + }); + + it('returns the builder so calls chain', function () { + $builder = Cassandra::cluster(); + + expect($builder->withProtocolVersion(ProtocolVersion::V3))->toBe($builder); + }); + + it('still accepts a plain integer', function () { + $props = (array) Cassandra::cluster()->withProtocolVersion(3); + + expect($props['protocolVersion'])->toBe(ProtocolVersion::V3); + }); + + it('keeps an integer the enum does not name', function () { + $props = (array) Cassandra::cluster()->withProtocolVersion(0x41); + + expect($props['protocolVersion'])->toBe(0x41); + }); + + it('rejects a non-positive integer', function () { + Cassandra::cluster()->withProtocolVersion(0); + })->throws(\Cassandra\Exception\InvalidArgumentException::class); + + it('rejects a value that is neither an int nor a ProtocolVersion', function () { + Cassandra::cluster()->withProtocolVersion('v4'); + })->throws(\TypeError::class); +}); diff --git a/tests/Unit/IniConfigurationTest.php b/tests/Unit/IniConfigurationTest.php index 2d41ecc61..cf25e300b 100644 --- a/tests/Unit/IniConfigurationTest.php +++ b/tests/Unit/IniConfigurationTest.php @@ -53,7 +53,7 @@ function builderProps(array $ini): ?array expect($props['contactPoints'])->toBe('10.0.0.1,10.0.0.2') ->and($props['defaultConsistency'])->toBe(\Cassandra::CONSISTENCY_LOCAL_QUORUM) ->and($props['defaultPageSize'])->toBe(250) - ->and($props['protocolVersion'])->toBe(3) + ->and($props['protocolVersion'])->toBe(\Cassandra\ProtocolVersion::V3) ->and($props['ioThreads'])->toBe(4) ->and($props['coreConnectionPerHost'])->toBe(2) ->and($props['maxConnectionsPerHost'])->toBe(8) @@ -77,7 +77,7 @@ function builderProps(array $ini): ?array expect($props['contactPoints'])->toBe('127.0.0.1') ->and($props['defaultConsistency'])->toBe(\Cassandra::CONSISTENCY_LOCAL_QUORUM) ->and($props['defaultPageSize'])->toBe(5000) - ->and($props['protocolVersion'])->toBe(4) + ->and($props['protocolVersion'])->toBe(\Cassandra\ProtocolVersion::V4) ->and($props['usePersistentSessions'])->toBeTrue(); }); diff --git a/tools/gen_descriptor/gen_class_descriptor.php b/tools/gen_descriptor/gen_class_descriptor.php index 95d49ce20..0d2c003d8 100644 --- a/tools/gen_descriptor/gen_class_descriptor.php +++ b/tools/gen_descriptor/gen_class_descriptor.php @@ -63,7 +63,7 @@ * Returns array of class records. Each record: * [ * 'fqn' => "Cassandra\\Foo\\Bar", // backslashes single, will be doubled at emit - * 'kind' => 'class' | 'interface', + * 'kind' => 'class' | 'interface' | 'enum', * 'is_final' => bool, * 'is_abstract'=> bool, * 'parents' => array of ['fqn' => '...', 'kind' => 'extends'|'implements'], @@ -115,8 +115,12 @@ function parse_stub(string $src): array continue; } - if ($id === T_CLASS || $id === T_INTERFACE) { - $kind = ($id === T_INTERFACE) ? 'interface' : 'class'; + if ($id === T_CLASS || $id === T_INTERFACE || $id === T_ENUM) { + $kind = match ($id) { + T_INTERFACE => 'interface', + T_ENUM => 'enum', + default => 'class', + }; // Walk back to find modifiers (final / abstract) and the doc comment. $isFinal = false; @@ -342,9 +346,8 @@ function emit_descriptor_file(array $classes, string $arginfoHeader, string $stu * PHP_SCYLLADB_VERSION, …) is visible to the arginfo register fn. */ #include "php_scylladb.h" #include -#include "$arginfoHeader" +$extraIncBlock#include "$arginfoHeader" #include -$extraIncBlock EOF . implode("\n", $bodies); @@ -380,6 +383,13 @@ function emit_class_descriptor(array $cls): array $handlersVar = "php_scylladb_{$snake}_handlers"; $registerFn = "php_scylladb_register_{$snake}"; + // Enums own no instances the extension allocates: cases are created by + // zend_register_internal_enum() and are immutable. No create_object, no + // handlers table, no offset — only the post_register hook stays. + if ($cls['kind'] === 'enum') { + return emit_enum_descriptor($cls, $snake, $ceVar, $registerFn, $fqnLit, $registerClassFn, $registryDeps, $registerArgs, $extraIncludes); + } + // Value-typed classes use a richer handlers struct that wraps the // standard zend_object_handlers with an extra `hash_value` callback. // Field access for standard handlers goes via `.std` when this is on. @@ -478,48 +488,108 @@ function emit_class_descriptor(array $cls): array $createObjWire = " if (&php_scylladb_{$snake}_new) ce->create_object = php_scylladb_{$snake}_new;\n"; $postReg = " if (&php_scylladb_{$snake}_post_register) php_scylladb_{$snake}_post_register(ce);\n"; - // Descriptor macro selection: single parent shortcut vs deps array. + $macro = emit_registry_macro($snake, $fqnLit, $ceVar, $registerFn, $registryDeps); + + $body = <<withProtocolVersion(4) // default 4 +->withProtocolVersion(Cassandra\ProtocolVersion::V4) // default V4 ``` +`Cassandra\ProtocolVersion` names the five native protocol versions, `V1` to `V5`. The method also +takes a plain integer, for a version the enum does not name. Reading `protocolVersion` back off the +builder gives an enum case when one matches the value, and an integer when none does. + Version 4 works with every supported server. Lower it only for an old cluster that rejects the handshake. The driver does not negotiate downward on its own.