[ 
https://issues.apache.org/jira/browse/PROTON-2308?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17461435#comment-17461435
 ] 

ASF GitHub Bot commented on PROTON-2308:
----------------------------------------

gemmellr commented on a change in pull request #346:
URL: https://github.com/apache/qpid-proton/pull/346#discussion_r771394080



##########
File path: cpp/src/link_test.cpp
##########
@@ -23,9 +23,114 @@
 #include <proton/sender_options.hpp>
 #include <proton/receiver_options.hpp>
 #include <proton/container.hpp>
+#include <proton/connection.hpp>
+#include <proton/connection_options.hpp>
+#include <proton/listen_handler.hpp>
+#include <proton/listener.hpp>
+#include <proton/messaging_handler.hpp>
+#include <proton/types.hpp>
+#include <proton/message.hpp>
+#include <proton/target_options.hpp>
+#include <proton/source_options.hpp>
+#include <proton/delivery.hpp>
 
 #include <iostream>
 
+#include <map>
+#include <condition_variable>
+#include <mutex>
+#include <thread>
+
+namespace {
+std::mutex m;
+std::condition_variable cv;
+bool listener_ready = false;
+int listener_port;
+} // namespace
+
+class test_recv : public proton::messaging_handler {
+  private:
+    class listener_ready_handler : public proton::listen_handler {
+        void on_open(proton::listener &l) override {
+            {
+                std::lock_guard<std::mutex> lk(m);
+                listener_port = l.port();
+                listener_ready = true;
+            }
+            cv.notify_one();
+        }
+    };
+
+    std::string url;
+    proton::listener listener;
+    listener_ready_handler listen_handler;
+
+  public:
+    test_recv(const std::string &s) : url(s) {}
+
+    void on_container_start(proton::container &c) override {
+        listener = c.listen(url, listen_handler);
+    }
+
+    void on_message(proton::delivery &d, proton::message &msg) override {
+        proton::symbol sym = "symbol";
+        proton::value val = "value";
+        std::map<proton::symbol, proton::value> props = 
d.receiver().target().dynamic_node_properties();
+
+        ASSERT(!props.empty());
+        for(auto it=props.begin(); it!=props.end(); it++) {
+            ASSERT_EQUAL(sym, it->first);
+            ASSERT_EQUAL(val, it->second);
+        }
+        d.receiver().close();
+        d.connection().close();
+        listener.stop();
+    }
+};
+
+class test_send : public proton::messaging_handler {
+  private:
+    std::string url;
+    proton::sender sender;
+
+  public:
+    test_send(const std::string &s) : url(s) {}
+
+    void on_container_start(proton::container &c) override {
+        proton::target_options opts;
+        std::map<proton::symbol,proton::value> m({{proton::symbol("symbol"), 
proton::value("value")}});
+        opts.dynamic_node_properties(m);
+        sender = c.open_sender(url, proton::sender_options().target(opts));
+    }
+
+    void on_sendable(proton::sender &s) override {
+        proton::message msg;
+        msg.body("message");
+        proton::tracker t = s.send(msg);
+        s.connection().close();
+    }
+};
+
+int test_dynamic_node_properties() {

Review comment:
       A dynamic consumer is by far the primary use case for dynamic, so if 
only having one test it would be that (essentially the opposite of this test). 
Having one test for a consumer, and another for a producer (from the client 
perspective) wouldnt be a bad idea, they are separate things, but dynamic 
senders are rare.
   
   The test doesnt actually do all the steps I'd say it should. Its possible 
another test is doing some of them somewhere, or that its just not tested for 
the cpp binding. It should use a 'dynamic' source/target and verify the 
behaviour in full, right now it isnt, but is instead attaching to a known 
address. The 'dynamic' handling involves the original peer not setting an 
address, and the other peer generating an address for the dynamic node and 
supplying its detail back to the original side in its reply attach, which the 
requesting peer can then read to determine the name of the dynamic node created 
for it. The dynamic-node-properties adds to that mechanism by letting the 
requester also ask for certain node details to be applied. The generating peer 
can always use that same field to indicate properties actually in effect for 
the dynamic node it created.
   
   The test only looks to verify that the node properties were sent [illegally 
due to not setting dynamic] from requestor to the other side. It doesnt check 
it can handle reading the details that come back, i.e to verify any dynamic 
address was given, or that any dynamic-node-properties arrived.
   
   As an example, here is a Java test verifying the dynamic flag and address 
propagation aspects (it doesnt test the dynamic node properties) for a dynamic 
consumer:
   
https://github.com/vert-x3/vertx-proton/blob/4.2.2/src/test/java/io/vertx/proton/ProtonClientTest.java#L468-L545
   
   I dont know if the 'built in' handshaking bits in proton-cpp that are 
opening the link in your test will inspect the dynamic flag and generate an 
address by themselves for the attach. If they do the test could just at least 
verify there is a generated value. If not, or better still, the test could set 
a generated address at the server side and check the client side can pick it up 
when its told the link opened. It could do the same for the dynamic node 
properties, e.g have the server apply something other than originally asked 
for, verify they are picked up too at the client once the link opens.




-- 
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: dev-unsubscr...@qpid.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


> [cpp] Add support for setting Dynamic Node Properties
> -----------------------------------------------------
>
>                 Key: PROTON-2308
>                 URL: https://issues.apache.org/jira/browse/PROTON-2308
>             Project: Qpid Proton
>          Issue Type: Improvement
>          Components: cpp-binding
>    Affects Versions: proton-c-0.33.0
>            Reporter: James Henry
>            Assignee: Rakhi Kumari
>            Priority: Major
>              Labels: api-addition
>
> Requesting support for setting the dynamic node properties be added to the 
> source and target options.
> This would allow the setting of termini node properties for senders and 
> receivers.
> Similar to the following request made for Python here: 
> https://issues.apache.org/jira/browse/PROTON-816



--
This message was sent by Atlassian Jira
(v8.20.1#820001)

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@qpid.apache.org
For additional commands, e-mail: dev-h...@qpid.apache.org

Reply via email to