Cleanup and pull in recent Java SDK features #28

AnotherDaniel · 2024-01-30T08:21:34Z

clean up and update main readme
remove contributors list (not needed, have git logs)
pull in changes and additions from Java SDK PR UTransport::registerListener() using marker trait dispatch for message types + generic #60

Cargo.toml

PLeVasseur · 2024-01-30T19:01:21Z

src/proto/cloudevents/protocloudevent.rs

@@ -23,6 +23,58 @@ use crate::cloudevents::cloud_event::CloudEventAttributeValue;
 use crate::cloudevents::cloud_event::Data as CloudEventData;
 use crate::cloudevents::CloudEvent as CloudEventProto;

+use crate::uprotocol::umessage::UMessage;
+use crate::uri::serializer::{LongUriSerializer, UriSerializer};


IMHO it'd be nice if we're only using LongUriSerializer to only have to use that one, i.e. only have to:

use crate::uri::serializer::{LongUriSerializer};

I think we can achieve that by modifying this line:

use crate::uri::serializer::{SerializationError, UriSerializer};

to

pub use crate::uri::serializer::{SerializationError, UriSerializer};

Good catch!

Addendum - this doesn't really work for some reason, dependent code still won't compile without explicitly use-ing the Trait implementation. I guess I'll revert this change for now.

My bad! Looks like this is not possible in Rust. We have to bring the trait into scope explicitly with a use. Seems this is a conscious design decision to help when multiple traits define things with the same names and helps readability to understand where a function could be coming from.

PLeVasseur · 2024-01-30T19:17:18Z

src/proto/cloudevents/protocloudevent.rs

+
+impl From<UMessage> for cloudevents::Event {
+    fn from(source_event: UMessage) -> Self {
+        let source = source_event.source.get_or_default();


I noticed leaning on getting default and unwraps throughout.

I'm wondering if we should be doing any additional error checking and make it TryFrom instead to reflect that it may fail?

I suppose it depends on the context in which this will be used as well.

WDYT?

we have a try_from now - However, next I will have to spend some quality time revisiting the test setup for cloudevent.rs, stay tuned for another change in that area.

Tests redone, looking a bit more robust

PLeVasseur · 2024-01-30T19:18:54Z

src/rpc/rpcclient.rs

-        attributes: UAttributes,
+        method: UUri,
+        request: UPayload,
+        options: CallOptions,


PLeVasseur · 2024-01-30T19:46:03Z

src/rpc/rpcmapper.rs

-        let payload = response?; // Directly returns in case of error
-        Any::try_from(payload)
+        let message = response?; // Directly returns in case of error
+        Any::try_from(message.payload.unwrap())


Funnily enough, I was doing the same thing over in this PR 😆

We should probably be a little more careful here, similar to what I did on that PR s.t. we don't accidentally panic.

Pulled that in - and apologies for waltzing over your work there!

PLeVasseur · 2024-01-30T19:47:31Z

src/rpc/rpcmapper.rs

        let any = RpcMapper::pack_any(&status)?;
-        Ok(any.try_into().unwrap())
+        let payload =


Again, funny we both did this haha

Kai gave some good feedback on that PR on how to handle this nicely, perhaps you could reference that?

PLeVasseur · 2024-01-30T19:53:40Z

src/uri/validator/urivalidator.rs

+    /// # Returns
+    ///
+    /// Returns `true` if the `UAuthority` can be represented in micro format, `false` otherwise.
+    pub fn is_micro_form_authority(authority: &UAuthority) -> bool {


This one I would pause on tho. IMHO it makes more sense to merge the PR I've got going for this functionality. That PR goes into a lot more detail in checking also that the fields from the UUri are able to fit the specified number of bits & bytes available.

If the PR I have for checking micro form goes through, that can then be mirrored back to up-java.

PLeVasseur · 2024-01-30T19:56:43Z

Overall good to stay in sync with up-java, but I'd recommend removing this bit here for is_micro_form_authority() since this other PR I have does further checking into buffer size & available number of bytes.

I'll go close this other PR doing just the invoke_method() -> UMessage bit.

If you could remove that bit, I think we're good 👍

stevenhartley

Mapping error for priority that needs fixing

stevenhartley · 2024-01-30T12:35:19Z

src/proto/cloudevents/protocloudevent.rs

+        if let Some(token) = &attributes.token {
+            event.set_extension("token", ExtensionValue::String(token.to_string()));
+        }
+        if let Ok(priority) = attributes.priority.enum_value() {


CloudEvent priority needs to be "CS1", "CS2" i.e string. I'm adding the mapping in core-api now so you can pull from proto but it is not an integer. eclipse-uprotocol/up-core-api#98

I made the changes to achieve this - not yet based off the referenced PR, but manually. Will change that bit as soon as up-core-api#98 is merged.

stevenhartley · 2024-02-05T20:12:52Z

@dakrbo let me know when this is ready for merge (lint & test failed)

AnotherDaniel · 2024-02-05T20:45:49Z

Will take a couple of days yet - need to revisit some test cases at the very least, plus want to wait for up-core-api 1.5.6 Will let you know once this is ready!

AnotherDaniel · 2024-02-06T17:13:52Z

Ok; I did the test case rework that was necessary - now this needs a new/current tag in up-core-api, with the recent protobuf-options changes in. @stevenhartley are you planning to do a tag there soon?

stevenhartley · 2024-02-07T02:51:29Z

Yes I will do a release this week, not waiting for the other changes that I want to review at the generic uProtocol meeting on Thursday.

sophokles73 · 2024-02-07T15:33:31Z

src/transport/datamodel/utransport.rs

@@ -41,7 +51,7 @@ pub trait UTransport {
    ///
    /// # Returns
    /// Returns () on success, otherwise an Err(UStatus) with the appropriate failure information.
-    async fn send(
+    async fn send_parts(


FMPOV we do not need this function now that we use UMessage as the input parameter.
However, it seems that we are missing the Receive function required by the spec ...

Agree and fixed

sophokles73 · 2024-02-08T13:48:08Z

src/transport/datamodel/utransport.rs

@@ -47,7 +47,7 @@ pub trait UTransport {
    /// On success, returns a tuple containing the received `UPayload` and `UAttributes` wrapped in `Ok(_)`.
    /// If receiving the message fails, it returns `Err(UStatus)`,
    /// where `UStatus` contains a `UCode` indicating the specific error or failure reason.
-    async fn receive(&self, topic: UUri) -> Result<(UPayload, UAttributes), UStatus>;
+    async fn receive(&self, topic: UUri) -> Result<UMessage, UStatus>;


The doc comment still refers to the original types. Which also is a good example for why you should not mention the types of parameters in the doc comments at all (they are obvious from the generated documentation anyway) ;-)

sophokles73 · 2024-02-09T13:46:56Z

@AnotherDaniel There is no doubt that the changes in this PR are relevant and helpful 👍
However, the PR contains quite a few changes which do not necessarily seem to be related to each other. This makes it quite hard to review and rebase, because it touches on so many areas. For the future, I would suggest to try to keep the scope of a PR small and consistent and try to only change things that are in scope. That will make it much easier to get things reviewed and merged.

Oh, and before I forget: you should probably make sure that you squash your changes into a few consistent commits before having the PR merged ...

AnotherDaniel · 2024-02-09T14:08:22Z

I'm still mentally working my way down from the all-encompassing initial commit, but point taken :)

AnotherDaniel · 2024-02-14T08:43:07Z

@PLeVasseur , I had to do a number on the MicroUriSerializer code, to make it work with up-core-api v1.5.6 - sorry to get in the way of of your #18 there, probably you can update that one to the same up-core-api and then take over my changes?
@sophokles73 I'm on the fence about also removing the remaining UAuthority::get_name() method; but without that using the name field of UAuthority always feels ugly. WDYT?

sophokles73 · 2024-02-14T09:57:42Z

Why do you want to remove the getters from UAuthority in the first place? Aren't they useful anymore?

AnotherDaniel · 2024-02-14T10:16:07Z

They now come with the generated code...

sophokles73 · 2024-02-14T16:47:23Z

@AnotherDaniel can you rebase this PR?

…tocol#60

sophokles73

LGTM

AnotherDaniel · 2024-02-14T17:38:08Z

@stevenhartley the changes you requested wrt up-core-api 1.5.6 (umessage option strings) are in now

PLeVasseur reviewed Jan 30, 2024

View reviewed changes

Cargo.toml Show resolved Hide resolved

PLeVasseur reviewed Jan 30, 2024

View reviewed changes

src/rpc/rpcclient.rs

attributes: UAttributes,

method: UUri,

request: UPayload,

options: CallOptions,

Copy link

Contributor

PLeVasseur Jan 30, 2024

Choose a reason for hiding this comment

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

👍

PLeVasseur reviewed Jan 30, 2024

View reviewed changes

PLeVasseur mentioned this pull request Jan 30, 2024

Update RpcClient::invoke_method() to return a UMessage in support of symmetry / flexibility #27

Closed

stevenhartley requested changes Jan 30, 2024

View reviewed changes

sophokles73 reviewed Feb 7, 2024

View reviewed changes

sophokles73 reviewed Feb 8, 2024

View reviewed changes

PLeVasseur mentioned this pull request Feb 14, 2024

Increase strictness / correctness of URI micro form validation #18

Merged

Assorted changes and additions to follow Java-library PR eclipse-upro…

d1839ce

…tocol#60

AnotherDaniel force-pushed the update_and_sync_recent_changes branch from ef1df37 to d1839ce Compare February 14, 2024 17:33

AnotherDaniel requested a review from stevenhartley February 14, 2024 17:35

sophokles73 approved these changes Feb 14, 2024

View reviewed changes

stevenhartley merged commit 141a139 into eclipse-uprotocol:main Feb 14, 2024
3 checks passed

AnotherDaniel deleted the update_and_sync_recent_changes branch February 15, 2024 09:10

This was referenced Feb 19, 2024

Update uprotocol 1.5.6 eclipse-uprotocol/up-transport-zenoh-rust#4

Closed

Update up-client-zenoh-rust based on uprotocol 1.5.6 ZettaScaleLabs/up-client-zenoh-rust#1

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup and pull in recent Java SDK features #28

Cleanup and pull in recent Java SDK features #28

AnotherDaniel commented Jan 30, 2024

PLeVasseur Jan 30, 2024

AnotherDaniel Jan 31, 2024

AnotherDaniel Jan 31, 2024

PLeVasseur Feb 1, 2024

PLeVasseur Jan 30, 2024 •

edited

Loading

AnotherDaniel Feb 5, 2024

AnotherDaniel Feb 6, 2024

PLeVasseur Jan 30, 2024

PLeVasseur Jan 30, 2024

AnotherDaniel Jan 31, 2024

PLeVasseur Jan 30, 2024

PLeVasseur Jan 30, 2024

AnotherDaniel Jan 31, 2024

PLeVasseur commented Jan 30, 2024 •

edited

Loading

stevenhartley left a comment

stevenhartley Jan 30, 2024

AnotherDaniel Jan 31, 2024

stevenhartley commented Feb 5, 2024

AnotherDaniel commented Feb 5, 2024 via email •

edited

Loading

AnotherDaniel commented Feb 6, 2024

stevenhartley commented Feb 7, 2024

sophokles73 Feb 7, 2024

AnotherDaniel Feb 8, 2024

sophokles73 Feb 8, 2024

sophokles73 commented Feb 9, 2024 •

edited

Loading

AnotherDaniel commented Feb 9, 2024

AnotherDaniel commented Feb 14, 2024 •

edited

Loading

sophokles73 commented Feb 14, 2024

AnotherDaniel commented Feb 14, 2024

sophokles73 commented Feb 14, 2024

sophokles73 left a comment

AnotherDaniel commented Feb 14, 2024

Cleanup and pull in recent Java SDK features #28

Cleanup and pull in recent Java SDK features #28

Conversation

AnotherDaniel commented Jan 30, 2024

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

PLeVasseur Jan 30, 2024 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PLeVasseur commented Jan 30, 2024 • edited Loading

stevenhartley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevenhartley commented Feb 5, 2024

AnotherDaniel commented Feb 5, 2024 via email • edited Loading

AnotherDaniel commented Feb 6, 2024

stevenhartley commented Feb 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sophokles73 commented Feb 9, 2024 • edited Loading

AnotherDaniel commented Feb 9, 2024

AnotherDaniel commented Feb 14, 2024 • edited Loading

sophokles73 commented Feb 14, 2024

AnotherDaniel commented Feb 14, 2024

sophokles73 commented Feb 14, 2024

sophokles73 left a comment

Choose a reason for hiding this comment

AnotherDaniel commented Feb 14, 2024

PLeVasseur Jan 30, 2024 •

edited

Loading

PLeVasseur commented Jan 30, 2024 •

edited

Loading

AnotherDaniel commented Feb 5, 2024 via email •

edited

Loading

sophokles73 commented Feb 9, 2024 •

edited

Loading

AnotherDaniel commented Feb 14, 2024 •

edited

Loading