Fix duplicate topic name checking in Node

[iche033]

🦟 Bug fix

Summary

Currently when duplicate topic names are advertised from the same node, we get a discovery service error:

Node::Advertise(): Error advertising topic [/foo]. Did you forget to start the discovery service?

The msg leads the user to think that it's a discovery issue instead of duplicate topic names. Turns out that there's actually logic for checking duplicate names already but there's a bug. The code checks the fully qualified name of the input topic against a set of non-fully qualified topic names so it never finds duplicates. This PR fixes the check, and a more relevant error msg should now be printed:

Topic [/foo] already advertised. You cannot advertise the same topic twice on the same node. If you want to advertise the same topic with different types, use separate nodes

To test, run the AdvertiseTwoEqualTopic test in Node_TEST, e.g.

./build/gz-transport14/bin/UNIT_Node_TEST --gtest_filter="*AdvertiseTwoEqualTopic*"

Checklist

• Signed all commits for DCO
• Added tests
• Updated documentation (as needed)
• Updated migration guide (as needed)
• Consider updating Python bindings (if the library has them)
• codecheck passed (See contributing)
• All tests passed (See test coverage)
• While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

[caguero]

Ignore this comment.

[caguero]

I wonder if we should remove this whole block:

https://github.com/gazebosim/gz-transport/blob/gz-transport14/src/Node.cc#L1019-L1029

The topic duplication seems to be checked inside the call if (!this->Shared()->dataPtr->msgDiscovery->Advertise(publisher))

And I don't remember a good reason for restricting a node to advertise the same topic with multiple types. Something like this.

[iche033]

Something like this.

yeah that looks good. Maybe add an error msg about not being able to add publisher?

err_msg.diff

diff --git a/include/gz/transport/Discovery.hh b/include/gz/transport/Discovery.hh
index b5c296be..2fd4831e 100644
--- a/include/gz/transport/Discovery.hh
+++ b/include/gz/transport/Discovery.hh
@@ -316,7 +316,12 @@ namespace gz
 
           // Add the addressing information (local publisher).
           if (!this->info.AddPublisher(_publisher))
+          {
+            std::cerr << "Discovery::Advertise(): Error adding publisher. "
+                      << "Publisher of the topic " << _publisher.Topic()
+                      << " already exists " << std::endl;
             return false;
+          }
 
           cb = this->connectionCb;

Lets close this one and create a new PR from your branch?

[iche033]

Lets close this one and create a new PR from your branch?

alright closing this PR