Skip to content
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

Simplify c8y_api::smartrest::topic module avoiding all redundant code to create topics #2316

Open
albinsuresh opened this issue Oct 5, 2023 · 1 comment
Labels
refactoring Developer value

Comments

@albinsuresh
Copy link
Contributor

Is your refactoring request related to a problem? Please describe.

The c8y_api::smartrest::topic has various mechanisms to generate SmartREST topics for the main device, immediate child device, nested child devices and services. The recently introduced publish_topic_from_ancestors API already covers most of these cases and the rest are just redundant, especially the ones like C8yTopic::ChildSmartRestResponse, dedicated for immediate child devices only.

Describe the solution you'd like

Refactor the c8y_api::smartrest::topic module with a smaller set of helper functions to create topics for all kinds of targets: the main device, immediate child device, nested child devices and services.

@Bravo555
Copy link
Contributor

Bravo555 commented Jul 31, 2024

There also seems to be an important difference in behaviour between publish_topic_from_ancestors and C8yTopic::to_topic for ChildSmartRestResponse(String) variant:

  • publish_topic_from_ancestors uses entity's ancestors, so when a child is nested, it creates a topic that contains all devices in the hierarchy:

    /// # Examples
    ///
    /// - `["main"]` -> `c8y/s/us`
    /// - `["child1", "main"]` -> `c8y/s/us/child1`
    /// - `["child2", "child1", "main"]` -> `c8y/s/us/child1/child2`

  • C8yTopic::to_topic doesn't take nesting into account, so smartrest publish topics it generates for nested child devices may be wrong

    impl C8yTopic {
    /// Return the c8y SmartRest response topic for the given entity
    pub fn smartrest_response_topic(
    entity: &EntityMetadata,
    prefix: &TopicPrefix,
    ) -> Option<Topic> {
    match entity.r#type {
    EntityType::MainDevice => Some(C8yTopic::upstream_topic(prefix)),
    EntityType::ChildDevice | EntityType::Service => {
    Self::ChildSmartRestResponse(entity.external_id.clone().into())
    .to_topic(prefix)
    .ok()
    }
    }
    }
    pub fn to_topic(&self, prefix: &TopicPrefix) -> Result<Topic, MqttError> {
    Topic::new(self.with_prefix(prefix).as_str())
    }
    pub fn upstream_topic(prefix: &TopicPrefix) -> Topic {
    Topic::new_unchecked(&Self::SmartRestResponse.with_prefix(prefix))
    }
    pub fn downstream_topic(prefix: &TopicPrefix) -> Topic {
    Topic::new_unchecked(&Self::SmartRestRequest.with_prefix(prefix))
    }
    pub fn with_prefix(&self, prefix: &TopicPrefix) -> String {
    match self {
    Self::SmartRestRequest => format!("{prefix}/{SMARTREST_SUBSCRIBE_TOPIC}"),
    Self::SmartRestResponse => format!("{prefix}/{SMARTREST_PUBLISH_TOPIC}"),
    Self::ChildSmartRestResponse(child_id) => {
    format!("{prefix}/{SMARTREST_PUBLISH_TOPIC}/{child_id}")
    }
    }
    }

C8yTopic::to_topic is used by the availability actor, so is it possible it may not work correctly for nested child devices?

EDIT: There seem to be some checks specifically for nested child devices (58b1ebb), in which case perhaps there's no problems but it's still a bit confusing why using a "wrong" smartrest publish topic is fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactoring Developer value
Projects
None yet
Development

No branches or pull requests

2 participants