-
Notifications
You must be signed in to change notification settings - Fork 14
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
uProtocol up-L1 1.5.8 #111
Conversation
src/main/java/org/eclipse/uprotocol/client/UMessageBuilder.java
Outdated
Show resolved
Hide resolved
src/main/java/org/eclipse/uprotocol/uri/validator/UriValidator.java
Outdated
Show resolved
Hide resolved
src/main/java/org/eclipse/uprotocol/uri/validator/UriValidator.java
Outdated
Show resolved
Hide resolved
src/main/java/org/eclipse/uprotocol/transport/builder/UMessageBuilder.java
Show resolved
Hide resolved
src/main/java/org/eclipse/uprotocol/uri/factory/UriFactory.java
Outdated
Show resolved
Hide resolved
src/main/java/org/eclipse/uprotocol/cloudevent/factory/CloudEventFactory.java
Outdated
Show resolved
Hide resolved
src/main/java/org/eclipse/uprotocol/cloudevent/factory/UCloudEvent.java
Outdated
Show resolved
Hide resolved
src/main/java/org/eclipse/uprotocol/cloudevent/factory/UCloudEvent.java
Outdated
Show resolved
Hide resolved
22959af
to
fc822fc
Compare
fyi: I rebased that PR to trigger the codeql scan from the workflow and it seems to have been working fine. |
src/main/java/org/eclipse/uprotocol/cloudevent/validate/CloudEventValidator.java
Outdated
Show resolved
Hide resolved
src/main/java/org/eclipse/uprotocol/cloudevent/validate/CloudEventValidator.java
Outdated
Show resolved
Hide resolved
src/main/java/org/eclipse/uprotocol/transport/validate/UAttributesValidator.java
Outdated
Show resolved
Hide resolved
src/main/java/org/eclipse/uprotocol/uuid/serializer/UuidSerializer.java
Outdated
Show resolved
Hide resolved
src/main/java/org/eclipse/uprotocol/uri/serializer/UriSerializer.java
Outdated
Show resolved
Hide resolved
src/main/java/org/eclipse/uprotocol/transport/validate/UAttributesValidator.java
Outdated
Show resolved
Hide resolved
*/ | ||
static UStatus publish(UTransport transport, UUri topic, Message message) { | ||
Objects.requireNonNull(transport, UTransport.TRANSPORT_NULL_ERROR); | ||
Objects.requireNonNull(topic, "Publish topic missing"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be good to define a constant instead of duplicating this literal "Publish topic missing"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe next commit :-)
src/main/java/org/eclipse/uprotocol/transport/builder/UMessageBuilder.java
Outdated
Show resolved
Hide resolved
src/main/java/org/eclipse/uprotocol/cloudevent/factory/CloudEventFactory.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not done - to be continued
src/main/java/org/eclipse/uprotocol/cloudevent/factory/CloudEventFactory.java
Outdated
Show resolved
Hide resolved
src/main/java/org/eclipse/uprotocol/cloudevent/factory/CloudEventFactory.java
Outdated
Show resolved
Hide resolved
* @param destination The destination to send the notification to. | ||
* @return Returns the {@link UStatus} with the status of the notification. | ||
*/ | ||
static UStatus notify(UTransport transport, UUri topic, UUri destination) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
where is the notification payload?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Message that is passed in the other static method. You could send notification with or without a payload
src/main/java/org/eclipse/uprotocol/communication/Notifier.java
Outdated
Show resolved
Hide resolved
*/ | ||
static UStatus unregisterNotificationListener(UTransport transport, UUri topic, UListener listener) { | ||
Objects.requireNonNull(transport, UTransport.TRANSPORT_NULL_ERROR); | ||
return transport.unregisterListener(topic, Optional.of(transport.getSource()), listener); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same comment as before. functions that change mutate their parameters should not be needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
L2 are wrappers for the transport, they are not meant to re-implement the transport
return UStatus.newBuilder().setCode(UCode.INTERNAL).build(); | ||
} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
subscribe and unsubscribe are not supporting the SUBSCRIBE_PENDING scenario
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hum... how do you see it should handle the SUBSCRIBE_PENDING scenario? This API only sends the subscription request and sets up the listener to receive the published messages, the application can then decide what it wants to do if the response is pending.
} | ||
try { | ||
switch (format) { | ||
case UPAYLOAD_FORMAT_UNSPECIFIED: // Default is WRAPPED_IN_ANY |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about JSON and Base64String representations?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function only supports unpacking protobuf serialized messages right now
/** | ||
* The unchecked exception which carries uProtocol error model. | ||
*/ | ||
public class UStatusException extends RuntimeException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
peronally am not a fan of exceptions and definitely not checked exceptions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hum... what should change here then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we can fully avoid exceptions and it is nice to have one which may carry status. CompletableFuture may be completed exceptionally and this can be used to propagate status...
* | ||
* @return UUri containing the source address | ||
*/ | ||
UUri getSource(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why does the uTransport interface have a getSource() method?
seems a little strange to couple this with something that is very technology specific
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When the transport is instantiated, we tell it our source address (who we are), this is then pulled by the L2 wrapper interfaces, otherwise we have to pass this to each static method as well as the transport instance.
/** | ||
* Builder for easy construction of the UAttributes object. | ||
*/ | ||
public class UMessageBuilder { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is replacing the protobuf UMessage?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No I'm still using uprotocol.v1.UMessage but this builds teh right type of UMessage (request, response, notification, publish).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now this may just be a vibes thing, but it feels like these unit tests are a bit too large... Each test seems to be doing a lot at once. Take this one for example:
@Test
public void test_to_from_message_from_response_cloudevent() {
// additional attributes
final UCloudEventAttributes uCloudEventAttributes = new UCloudEventAttributes.UCloudEventAttributesBuilder()
.withPriority(UPriority.UPRIORITY_CS2)
.withTtl(3)
.build();
//cloudevent
final CloudEvent cloudEvent = CloudEventFactory.response(buildSourceForTest(),"//bo.cloud/2/1/0",
UuidSerializer.serialize(UuidFactory.Factories.UPROTOCOL.factory().create()),
Optional.of( buildProtoPayloadForTest()),
uCloudEventAttributes);
UMessage result = UCloudEvent.toMessage(cloudEvent);
assertNotNull(result);
assertTrue(UCloudEvent.getRequestId(cloudEvent).isPresent());
assertEquals(UCloudEvent.getRequestId(cloudEvent).get(),
UuidSerializer.serialize(result.getAttributes().getReqid()));
assertTrue(UCloudEvent.getTtl(cloudEvent).isPresent());
assertEquals(UCloudEvent.getTtl(cloudEvent).get(), result.getAttributes().getTtl());
assertTrue(UCloudEvent.getSink(cloudEvent).isPresent());
assertEquals(UCloudEvent.getSink(cloudEvent).get(),
UriSerializer.serialize(result.getAttributes().getSink()));
assertEquals(UCloudEvent.getPayload(cloudEvent).toByteString(),result.getPayload());
assertEquals(UCloudEvent.getSource(cloudEvent),UriSerializer.serialize(result.getAttributes().getSource()));
assertTrue(UCloudEvent.getPriority(cloudEvent).isPresent());
assertEquals(UCloudEvent.getPriority(cloudEvent).get(), UCloudEvent.getCePriority(result.getAttributes().getPriority()));
final CloudEvent cloudEvent1 = UCloudEvent.fromMessage(result);
assertEquals(cloudEvent,cloudEvent1);
}
Now, I understand what this test is doing, since I work with this library regularly. But, someone else would have trouble IMO, considering its size. For this test, I think it could be divided into 3 tests:
- Check if the fields are present
- Check if the decoded fields are equal
- Check if the fields are equal after the decoding.
Here's what I'm thinking of doing in up-python:
def build_response_cloudevent(self):
# additional attributes
u_cloud_event_attributes = (
UCloudEventAttributesBuilder()
.with_ttl(3)
.with_priority(UPriority.UPRIORITY_CS2)
.build()
)
cloud_event = CloudEventFactory.response(
build_uri_for_test(),
"//bo.cloud/12345/1/531",
CloudEventFactory.generate_cloud_event_id(),
build_proto_payload_for_test(),
u_cloud_event_attributes,
)
return cloud_event
def test_message_presence_response_cloudevent(self):
cloud_event = self.build_response_cloudevent()
result = UCloudEvent.toMessage(cloud_event)
self.assertIsNotNone(result)
self.assertIsNotNone(UCloudEvent.get_request_id(cloud_event))
self.assertIsNotNone(UCloudEvent.get_ttl(cloud_event))
self.assertIsNotNone(UCloudEvent.get_sink(cloud_event))
self.assertIsNotNone(UCloudEvent.get_payload(cloud_event))
self.assertIsNotNone(UCloudEvent.get_source(cloud_event))
self.assertIsNotNone(UCloudEvent.get_priority(cloud_event))
def test_message_equality_response_cloudevent(self):
cloud_event = self.build_response_cloudevent()
result = UCloudEvent.toMessage(cloud_event)
self.assertEqual(UCloudEvent.get_request_id(cloud_event), UuidSerializer.serialize(result.attributes.reqid))
self.assertEqual(UCloudEvent.get_ttl(cloud_event), result.attributes.ttl)
self.assertEqual(UCloudEvent.get_sink(cloud_event), UriSerializer.serialize(result.attributes.sink))
self.assertEqual(UCloudEvent.get_payload(cloud_event).SerializeToString(), result.payload)
self.assertEqual(UCloudEvent.get_source(cloud_event), UriSerializer().serialize(result.attributes.source))
self.assertEqual(UCloudEvent.get_priority(cloud_event), UCloudEvent.get_ce_priority(result.attributes.priority))
def test_message_reconversion_response_cloudevent(self):
cloud_event = self.build_response_cloudevent()
result = UCloudEvent.toMessage(cloud_event)
cloud_event1 = UCloudEvent.fromMessage(result)
self.assertEqual(cloud_event, cloud_event1)
It's certainly not something that needs to be "fixed" for this PR to get merged, but I think it may be good to take up some cleanup on the test cases in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@matthewd0123 I plan to reboot these methods once this PR is merged so I can properly implement 1.3.x <->1.5.x uProtocol translation functions and I plan to extract that from this UCloudEvent and will split the test cases up so they are easier to read
ace19f8
to
385e1cf
Compare
Code coverage report is ready! 📈
|
|
||
CompletionStage<UPayload> responsePayload = new CompletableFuture<UPayload>(); | ||
|
||
transport.registerListener(methodUri, transport.getSource(), new UListener() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For every call register a new instance of a listener against topic? It should be linked by request id instead, if you make more then one invoke of the same API how you know which listener to trigger upon receiving of responses?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mishap4 , OK I addressed this in the latest change by filtering in the listener by the reqid. This allows me to not have to introduce state and/or instance in L2. If this turns out to be a performance issue (because someone invokes the same
message for a thousand times in a row) then we will reevaluate the situation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
L2 changes have been pushed out to #117
/** | ||
* The unchecked exception which carries uProtocol error model. | ||
*/ | ||
public class UStatusException extends RuntimeException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we can fully avoid exceptions and it is nice to have one which may carry status. CompletableFuture may be completed exceptionally and this can be used to propagate status...
UCode.OK); | ||
|
||
// Unregister the listener so we no longer receive notifications | ||
assertEquals(Notifier.unregisterNotificationListener(transport, topic, listener), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where do you send notification to receive in this test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Example code only shows how to use the API
I cannot build on this branch local state:
build
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@stevenhartley, I am also getting the same exception as @r-vanooyen is facing. I suspect our build issues on Windows are due to case sensitivity conflicts with class names. Since Windows is case-insensitive by default, unlike Unix-based systems, this is causing problems. Specifically, the SubscriberTest class has two inner classes named UnhappyUnSubscribeUTransport and UnHappyUnSubscribeUTransport. Please rename one of these classes to resolve the issue.
* with the failure if the subscription was not successful. The API will also register the listener to be | ||
* called when messages are received. | ||
* | ||
* @param topic The topic to subscribe to. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing transport param
* The subscriber no longer wishes to be subscribed to said topic so we issue a unsubscribe | ||
* request to the USubscription service. | ||
* | ||
* @param topic The topic to unsubscribe to. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing transport param
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
* <b>IMPORTANT NOTE:</b> If {@link UPayloadFormat} is not {@link UPAYLOAD_FORMAT_PROTOBUF_WRAPPED_IN_ANY}, | ||
* there is no guarantee that the parsing to T is correct as we do not have the data schema. | ||
* | ||
* @param payload the payload to unpack |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
params here don't match
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
Code coverage report is ready! 📈
|
Hum, @r-vanooyen can you try again? I don't seem to have any build issues on my side. |
Code coverage report is ready! 📈
|
ad113ed
to
5e01c3c
Compare
Code coverage report is ready! 📈
|
Code coverage report is ready! 📈
|
2bfe8d9
to
b9b4765
Compare
@neelam-kushwah & @r-vanooyen , I fixed the issue, thanks for the hint @neelam-kushwah! |
Code coverage report is ready! 📈
|
Code coverage report is ready! 📈
|
Code coverage report is ready! 📈
|
} | ||
CallOptions that = (CallOptions) obj; | ||
return mTimeout == that.mTimeout && mPriority == that.mPriority && mToken.equals(that.mToken); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also need to override hashcode() when override equals()
In 1.5.8 we have made the following changes: - URI: Condense definitions to a single form removing long and micro forms. Simplified the predicates to be easier to use - Updates to uTransport API to support pattern matching for all message types - Updates to UAttributes data model and removed UPayload placing the format in the header and the payload data directly in UMessage - Full 100% code coverage and test - Numerous linting and style fixes and corrections - Switch to Java 17 from Java 11 for CI builds - Refactoring uProtocol custom options into uoptions.proto to add SOME/IP serialization support and explicit topic declaration - Removed Optional variables passing to methods #116
0be98c3
to
d4e73a3
Compare
Code coverage report is ready! 📈
|
The following includes all the L1 changes for 1.5.8, the new L2 APIs will be in a separate PR
#116