Create a ClientTrait for RustSDK #142

lukasmittag · 2025-02-26T09:57:11Z

For handling the Rust SDK and preparing to make it an own crate this introduces a ClientTrait. With this the maintenance of the soon-to-be Rust SDK will be easy and manageable.

codecov · 2025-02-26T10:32:55Z

Codecov Report

Attention: Patch coverage is 19.16667% with 194 lines in your changes missing coverage. Please review.

Project coverage is 64.09%. Comparing base (d8ccffe) to head (851a688).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
databroker-cli/src/kuksa_cli.rs	0.00%	90 Missing ⚠️
lib/kuksa/src/lib.rs	39.39%	60 Missing ⚠️
lib/sdv/src/lib.rs	0.00%	34 Missing ⚠️
lib/common/src/types.rs	0.00%	9 Missing ⚠️
databroker-cli/src/sdv_cli.rs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #142      +/-   ##
==========================================
+ Coverage   59.97%   64.09%   +4.12%     
==========================================
  Files          34       36       +2     
  Lines       15391    17550    +2159     
==========================================
+ Hits         9230    11248    +2018     
- Misses       6161     6302     +141

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

wba2hi

no functional test has been done yet.

lib/kuksa/src/lib.rs

lib/common/src/lib.rs

wba2hi · 2025-02-26T13:33:23Z

databroker-cli/src/kuksa_cli.rs

-            None
-        }
-    };
+    let datapoint_entries = handle_get_metadata(vec![path], client).await.unwrap();


Wouldn't simply unwrapping the result here crash the application in case of an err? might be a rare case though as this only happens when cli::print_error(...) or prin_resp_err_fm fails - might be neglectable.

I think it is neglectable because the error will be printed. If there is an error databroker-cli most problaly should panic because something went really wrong. Which will happen with unwrap()

databroker-cli/src/kuksa_cli.rs

lib/kuksa/src/lib.rs

lib/common/src/lib.rs

wba2hi

No functional test has been done yet

lib/common/src/conversion.rs

wba2hi · 2025-03-17T14:20:21Z

lib/common/src/lib.rs

@@ -41,6 +44,195 @@ pub enum ClientError {
    Function(Vec<Error>),
 }

+#[async_trait]
+pub trait SDVClientTraitV1 {


So if I understand correctly, then we now have the following interfaces

ClientTrait SDVClientTrait => SDVClientTraitV1 KuksaClientTrait => KuksaClientTraitV1 => KuksaClientTraitV2

I guess we only need to expose / use SdvClientTrait and KuksaClientTrait for our own purposes (e.g. easy extension and use in databroker-cli?)

If we now add a new protocol, e.g. kuksa.val.v3 we would need to add a new KuksaClientTraitV3, and adapt the conversions thingy to convert the proto specific models to the alias classes, right?

lib/common/src/lib.rs

wba2hi · 2025-03-18T07:41:26Z

lib/common/src/types.rs

+pub type SensorUpdateSDVTypeV1 = HashMap<String, SDVprotoV1::Datapoint>;
+pub type UpdateActuationSDVTypeV1 = HashMap<String, SDVprotoV1::Datapoint>;
+pub type PathSDVTypeV1 = Vec<String>;
+pub type SubscribeSDVTypeV1 = String;
+pub type PublishResponseSDVTypeV1 = SDVprotoV1::UpdateDatapointsReply;
+pub type GetResponseSDVTypeV1 = HashMap<String, SDVprotoV1::Datapoint>;
+pub type SubscribeResponseSDVTypeV1 = Streaming<SDVprotoV1::SubscribeReply>;
+pub type ProvideResponseSDVTypeV1 = Streaming<SDVprotoV1::SubscribeReply>;
+pub type ActuateResponseSDVTypeV1 = SDVprotoV1::SetDatapointsReply;
+pub type MetadataResponseSDVTypeV1 = Vec<SDVprotoV1::Metadata>;
+
+// Type aliases V1
+pub type SensorUpdateTypeV1 = HashMap<String, protoV1::Datapoint>;
+pub type UpdateActuationTypeV1 = HashMap<String, protoV1::Datapoint>;
+pub type PathTypeV1 = Vec<String>;
+pub type SubscribeTypeV1 = PathTypeV1;
+pub type PublishResponseTypeV1 = ();
+pub type GetResponseTypeV1 = Vec<protoV1::DataEntry>;
+pub type SubscribeResponseTypeV1 = Streaming<protoV1::SubscribeResponse>;
+pub type ProvideResponseTypeV1 = Streaming<protoV1::SubscribeResponse>;
+pub type ActuateResponseTypeV1 = ();
+pub type MetadataResponseTypeV1 = GetResponseTypeV1;
+
+// Type aliases V2
+pub type SensorUpdateTypeV2 = protoV2::Value;
+pub type UpdateActuationTypeV2 = SensorUpdateTypeV2;
+pub type UpdateActuationsTypeV2 = HashMap<PathTypeV2, UpdateActuationTypeV2>;
+pub type PathTypeV2 = String;
+pub type PathsTypeV2 = Vec<PathTypeV2>;
+pub type IdsTypeV2 = Vec<i32>;
+pub type SubscribeTypeV2 = PathsTypeV2;
+pub type SubscribeByIdTypeV2 = IdsTypeV2;
+pub type PublishResponseTypeV2 = ();
+pub type GetResponseTypeV2 = protoV2::Datapoint;
+pub type MultipleGetResponseTypeV2 = Vec<GetResponseTypeV2>;
+pub type SubscribeResponseTypeV2 = tonic::Streaming<protoV2::SubscribeResponse>;
+pub type SubscribeByIdResponseTypeV2 = tonic::Streaming<protoV2::SubscribeByIdResponse>;
+pub type ProvideResponseTypeV2 = ();
+pub type ActuateResponseTypeV2 = ();
+pub type OpenProviderStreamResponseTypeV2 = OpenProviderStream;
+pub type MetadataTypeV2 = (PathTypeV2, PathTypeV2);
+pub type MetadataResponseTypeV2 = Vec<protoV2::Metadata>;
+pub type ServerInfoTypeV2 = ServerInfo;


Still feels pretty arbitrary. It's probably best to add some comments to understand the overall strategy here to simplify on how to decide which types to assign? Any granularity that helps me to decide which types to use when we add a new protocol and therefore a new Trait is sufficient.

hmm, think of it more to handle the conversions and now what you want to do there. i know it feels arbitrary. One could argue you can just use the same naming as in the API's. Thats what I thought about but was not sure this does not suggest then something different. So for example publish_value have a PublishValueRequestType and PublishValueResponseType same for the other functions - XRequestType XResponseType. Would that make more sense to you? I introduced PathType because I felt like every underlying function should handle the input path as the same in ONE ClientTrait however it could differ BETWEEN Traits.

lib/common/src/conversion.rs

lib/kuksa_val_v2/src/lib.rs

wba2hi · 2025-03-18T09:42:55Z

lib/kuksa_val_v2/src/lib.rs

-        vss_paths: Vec<&str>,
+        vss_paths: Vec<String>,


Could you elaborate on this change? Is it better to use a "String" over a "&str"?

That's what I'm currently working on. The problem with references in rust and traits seems to be that lifetime handling is difficult. So I'm currently trying to solve this by using clones and hence no references. But I'm extending my horizon and am experimenting how it would be done with lifetime handling.

I experimented a little bit with references and lifetimes. Could not bring it in a solid state yet https://github.com/boschglobal/kuksa-databroker/tree/experiment/client_trait_refs_lifetimes so will continue fixing review findings due to the lack of time.

wba2hi · 2025-03-24T07:13:05Z

lib/kuksa_val_v2/src/lib.rs

+        Ok(ClientTraitV2::subscribe(self, paths.convert_to_v2(), None)
+            .await
+            .unwrap()
+            .convert_to_v1())


wba2hi · 2025-03-24T07:13:24Z

lib/kuksa_val_v2/src/lib.rs

+        Ok(ClientTraitV2::subscribe(self, paths.convert_to_v2(), None)
+            .await
+            .unwrap()
+            .convert_to_v1())


wba2hi · 2025-03-24T07:21:59Z

lib/kuksa/src/lib.rs

-
-        Ok(metadata_result)
+#[async_trait]
+impl kuksa_common::SDVClientTraitV1 for KuksaClient {


Asking myself here, should the KuksaClient (val.v1) really implement the SDVClientTrait? If there are similar methods I would probably re-declare them in the corresponding interface instead of "implementing all SDV stuff" where we potentially inherit a lot of unimplemented / unsupported / incompatible stuff.

The more unimplemented / incompatible stuff is exposed on a general level, the less simple and useful is the abstraction.

I agree, that was just the first attempt to keep backwards compatibility. If the interface is deprecated this would be changed to a deprecation message.

lib/common/src/types.rs

wba2hi · 2025-03-24T07:49:06Z

lib/common/src/conversion.rs

+
+impl ConvertToV1<SubscribeResponseTypeV1> for SubscribeResponseTypeV2 {
+    fn convert_to_v1(self) -> SubscribeResponseTypeV1 {
+        unimplemented!("Not possible to convert stream objects!")


well, it actually is possible isn't it? read the messages from stream, convert them to a v1 object and then emitting them to a new stream. compare to databroker/kuksa_val_v1/val.rs -> convert_to_proto_stream but yeah I admit, that whatever relates to streaming stuff feels really overcomplicated in Rust.

wba2hi

Functional Test:

could not test kuksa.val.v2, because it's not yet supported by databroker-cli
kuksa.val.v1 worked
~~sdv.databroker.v1 returned unimplemented for everything, but this issue already seems to persist on main?~~ works. I forgot to put the --enable-databroker-v1 flag

lib/common/src/conversion.rs

lukasmittag · 2025-03-25T12:54:25Z

with regard to functional testing. For kuksa-val-v2.rs functions there are (integration/unit) tests making sure of the right behaviour.

wba2hi

LGTM

lukasmittag force-pushed the feature/rust_sdk_client_trait branch 2 times, most recently from ddbc538 to bae6f53 Compare February 26, 2025 10:27

wba2hi requested changes Feb 26, 2025

View reviewed changes

rafaeling reviewed Feb 28, 2025

View reviewed changes

lib/common/src/lib.rs Show resolved Hide resolved

lukasmittag added 11 commits March 17, 2025 13:12

Create a ClientTrait for RustSDK

04a577e

Fix clippy finding

dc73f97

Use ClientTrait in integration tests for databroker

6a20b9d

Fix comments

e3c9725

Rename types for Actuation and Sensor providing

9d99ca5

Types, conversion and ClientTraits for different versions

33e305c

Implement SDVClientTrait for sdv.databroker.v1 client

36a47c0

Implement SDVClientTrait and ClientTraitV1 for kuksa.val.v1 client

0788461

Implement ClientTraitV1 and ClientTraitV2 for kuksa.val.v2 client

8b79c7b

Adapt cli to use ClientTraits correctly

8e0985a

Adapt databroker tests to use ClientTraits correctly

efb84d4

lukasmittag force-pushed the feature/rust_sdk_client_trait branch from df25af6 to efb84d4 Compare March 17, 2025 12:13

lukasmittag added 2 commits March 18, 2025 10:06

Fix types of kuksa.val.v2 client

0393283

Convert &str to String and use clone

e99185f

wba2hi requested changes Mar 18, 2025

View reviewed changes

lukasmittag added 2 commits March 20, 2025 14:01

Formatting

94039a2

Review findings

82bfa41

wba2hi requested changes Mar 24, 2025

View reviewed changes

lukasmittag added 7 commits March 24, 2025 15:40

Add unit tests for conversion functions

fb73023

Remove unit type comparisons

32fbae1

Merge branch 'main' into feature/rust_sdk_client_trait

0767fd1

Adapt to new metadata structure in kuksa.val.v2

734411f

Align naming

3db8572

Fix logger initialization

d6763a6

Formatting

c13cf45

Update lock files

41ffc53

wba2hi requested changes Mar 25, 2025

View reviewed changes

Remove code duplicates

851a688

wba2hi approved these changes Mar 25, 2025

View reviewed changes

lukasmittag merged commit 3be386a into eclipse-kuksa:main Mar 25, 2025
36 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create a ClientTrait for RustSDK #142

Create a ClientTrait for RustSDK #142

lukasmittag commented Feb 26, 2025

codecov bot commented Feb 26, 2025 •

edited

Loading

wba2hi left a comment

wba2hi Feb 26, 2025

lukasmittag Mar 20, 2025

wba2hi left a comment

wba2hi Mar 17, 2025

wba2hi Mar 18, 2025

wba2hi Mar 18, 2025

lukasmittag Mar 18, 2025

wba2hi Mar 18, 2025

lukasmittag Mar 18, 2025 •

edited

Loading

lukasmittag Mar 20, 2025

wba2hi Mar 24, 2025

wba2hi Mar 24, 2025

wba2hi Mar 24, 2025

lukasmittag Mar 24, 2025

wba2hi Mar 24, 2025

wba2hi left a comment •

edited

Loading

lukasmittag commented Mar 25, 2025

wba2hi left a comment

Create a ClientTrait for RustSDK #142

Create a ClientTrait for RustSDK #142

Conversation

lukasmittag commented Feb 26, 2025

codecov bot commented Feb 26, 2025 • edited Loading

Codecov Report

wba2hi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wba2hi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukasmittag Mar 18, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wba2hi left a comment • edited Loading

Choose a reason for hiding this comment

lukasmittag commented Mar 25, 2025

wba2hi left a comment

Choose a reason for hiding this comment

codecov bot commented Feb 26, 2025 •

edited

Loading

lukasmittag Mar 18, 2025 •

edited

Loading

wba2hi left a comment •

edited

Loading