hubcio commented on code in PR #3373:
URL: https://github.com/apache/iggy/pull/3373#discussion_r3324018552
##########
core/sdk/src/clients/client.rs:
##########
@@ -182,6 +184,31 @@ impl IggyClient {
pub async fn get_connection_info(&self) -> ConnectionInfo {
self.client.read().await.get_connection_info().await
}
+
+ /// Send a raw binary command (`code` + serialized `payload`) and return
the
+ /// raw response. Binary transports only; HTTP yields `FeatureUnavailable`.
+ pub async fn send_binary_request(&self, code: u32, payload: Bytes) ->
Result<Bytes, IggyError> {
Review Comment:
heads up for the `vsr`/server-ng path: this forwards any `code` straight
into `send_raw_with_response` with no validation. under `--features vsr`,
passing `LOGIN_REGISTER_CODE`/`LOGIN_REGISTER_WITH_PAT_CODE` on an
already-logged-in client hits the Register branch of `encode_request_header`,
which calls `register_request_id()` -> that asserts `!register_consumed` and
`!is_bound()`, both false after login, so it panics. the panic fires while
holding the `consensus_session` `std::sync::Mutex` (websocket encodes inline so
it unwinds the caller; tcp/quic encode inside `tokio::spawn` so it first
surfaces as `TcpError`/`QuicError`), which poisons the mutex - so every later
`reset_vsr_session`/`bind`/`disconnect` hits `.lock().expect("...poisoned")`
and re-panics, leaving the client unusable on all binary transports. classic
framing is unaffected (no consensus session there). raw `LOGOUT_USER_CODE` is
the same class: no panic, but it skips `reset_vsr_session`/state reset so the
sdk keeps
a dead session. simplest fix is to reject login/logout control codes here
(there's an `is_login_register_code` helper already), and/or have
`register_request_id`/`encode_request_header` return `Err` instead of asserting
now that this is a public caller-controlled entry point.
##########
core/sdk/src/clients/client.rs:
##########
@@ -182,6 +184,31 @@ impl IggyClient {
pub async fn get_connection_info(&self) -> ConnectionInfo {
self.client.read().await.get_connection_info().await
}
+
+ /// Send a raw binary command (`code` + serialized `payload`) and return
the
Review Comment:
the doc only mentions HTTP yielding `FeatureUnavailable`, but under
`--features vsr` this can also fail at encode time: a genuinely-custom/unknown
code is rejected by `operation_for_code` with `InvalidCommand` (a
known-but-replicated code with no mapping gets `UnknownReplicatedCommand`). so
the "send custom commands without modifying the sdk" goal only holds on the
classic protocol - under server-ng the encoder is closed-world and only carries
codes already in the dispatch table. worth spelling that restriction out here.
##########
core/common/src/types/http.rs:
##########
@@ -0,0 +1,40 @@
+/* Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+use strum::{Display, EnumString, IntoStaticStr};
+
+/// HTTP request method, kept dependency-free so `iggy_common` carries no
+/// concrete HTTP client type.
+#[derive(Clone, Copy, Debug, Default, Display, PartialEq, Eq, EnumString,
IntoStaticStr)]
Review Comment:
`Display`, `EnumString` (FromStr) and `Default` aren't used anywhere in-repo
right now - only `IntoStaticStr` via `<&str>::from` is. fine to keep them since
this is prelude-exported public api, just flagging they're currently
unexercised. `#[default] Get` only matters to downstream callers; a one-line
"defaults to GET" doc would remove any surprise.
##########
core/common/src/types/http.rs:
##########
@@ -0,0 +1,40 @@
+/* Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+use strum::{Display, EnumString, IntoStaticStr};
+
+/// HTTP request method, kept dependency-free so `iggy_common` carries no
+/// concrete HTTP client type.
+#[derive(Clone, Copy, Debug, Default, Display, PartialEq, Eq, EnumString,
IntoStaticStr)]
+pub enum HttpMethod {
Review Comment:
follow-up, not for this pr: there's already an `HttpMethod` in
`core/connectors/sinks/http_sink/src/lib.rs` (serde-based, `#[default] Post`,
no `Options`). two enums now coexist in different crates. not trivially
mergeable (this one is intentionally dependency-free, that one is serde
config), but worth a dedup later if `iggy_common` ever gains serde.
##########
core/integration/tests/sdk/raw.rs:
##########
@@ -0,0 +1,90 @@
+/* Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+use iggy::prelude::*;
+use iggy_binary_protocol::WireEncode;
+use iggy_binary_protocol::codes::{GET_STATS_CODE, PING_CODE};
+use iggy_binary_protocol::requests::system::{GetStatsRequest, PingRequest};
+use integration::iggy_harness;
+
+#[cfg(not(feature = "vsr"))]
+#[iggy_harness(test_client_transport = [Tcp, Quic, Http, WebSocket])]
+async fn given_authenticated_client_when_sending_raw_request_should_round_trip(
+ harness: &TestHarness,
+) {
+ let client = harness.root_client().await.unwrap();
+ assert_raw_round_trip(&client).await;
+}
+
+#[cfg(feature = "vsr")]
+#[iggy_harness(test_client_transport = [Tcp, WebSocket])]
Review Comment:
the `vsr` arm only ever sends `PING_CODE` and `GET_STATS_CODE`, both
non-replicated, so it never exercises a replicated/custom code or the
login/logout-code path under vsr. a case that sends a login/register code on a
bound client (should return a clean `Err`, not panic) plus an unknown-code
reject would actually cover the raw path on server-ng.
##########
core/sdk/src/clients/client.rs:
##########
@@ -182,6 +184,31 @@ impl IggyClient {
pub async fn get_connection_info(&self) -> ConnectionInfo {
self.client.read().await.get_connection_info().await
}
+
+ /// Send a raw binary command (`code` + serialized `payload`) and return
the
+ /// raw response. Binary transports only; HTTP yields `FeatureUnavailable`.
+ pub async fn send_binary_request(&self, code: u32, payload: Bytes) ->
Result<Bytes, IggyError> {
+ match &*self.client.read().await {
+ ClientWrapper::Tcp(client) => client.send_raw_with_response(code,
payload).await,
+ ClientWrapper::Quic(client) => client.send_raw_with_response(code,
payload).await,
+ ClientWrapper::WebSocket(client) =>
client.send_raw_with_response(code, payload).await,
+ ClientWrapper::Http(_) | ClientWrapper::Iggy(_) =>
Err(IggyError::FeatureUnavailable),
Review Comment:
minor consistency: `Iggy(_)` is lumped with `Http(_)` into
`FeatureUnavailable`, but `ClientWrapper::Iggy` wraps a nested `IggyClient`
that has these same inherent methods, so it could just recurse (`Iggy(client)
=> client.send_binary_request(...).await`). in practice `Iggy` is never
constructed today (the builder and `from_connection_string` only produce
concrete transports), so this is dead-path tidiness rather than a live bug.
note `send_http_request` hides `Iggy` under the `_` arm just below, so a fix
would need an explicit arm there too.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
