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

refactor: Simplify SmartREST publish topics #3292

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

albinsuresh
Copy link
Contributor

@albinsuresh albinsuresh commented Dec 12, 2024

Proposed changes

While publishing to a nested child device, publish directly to the SmartREST topic
of that device (c8y/s/us/<xid>), without specifying all of all its ancestors.

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Improvement (general improvements like code refactoring that doesn't explicitly fix a bug or add any new functionality)
  • Documentation Update (if none of the other choices apply)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Paste Link to the issue


Checklist

  • I have read the CONTRIBUTING doc
  • I have signed the CLA (in all commits with git commit -s)
  • I ran cargo fmt as mentioned in CODING_GUIDELINES
  • I used cargo clippy as mentioned in CODING_GUIDELINES
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

Further comments

Copy link

codecov bot commented Dec 12, 2024

Codecov Report

Attention: Patch coverage is 87.65432% with 10 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
crates/extensions/c8y_mapper_ext/src/converter.rs 75.00% 0 Missing and 5 partials ⚠️
crates/core/tedge_mapper/src/c8y/mapper.rs 0.00% 2 Missing ⚠️
...ates/extensions/tedge_mqtt_ext/src/test_helpers.rs 0.00% 2 Missing ⚠️
crates/core/tedge_api/src/entity_store.rs 93.75% 0 Missing and 1 partial ⚠️
Additional details and impacted files

📢 Thoughts on this report? Let us know!

Copy link
Contributor

github-actions bot commented Dec 12, 2024

Robot Results

✅ Passed ❌ Failed ⏭️ Skipped Total Pass % ⏱️ Duration
549 0 2 549 100 1h21m46.009959999s

@reubenmiller reubenmiller added refactoring Developer value theme:c8y Theme: Cumulocity related topics labels Dec 12, 2024
Copy link
Contributor

@didier-wenzek didier-wenzek left a comment

Choose a reason for hiding this comment

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

Approved - with 2 suggestions.

// Skipping the last ancestor as it is the main device represented by the root topic itself
target_topic.push('/');
target_topic.push_str(ancestor.as_ref());
pub fn publish_topic_from_parent(parent: Option<&str>, prefix: &TopicPrefix) -> Topic {
Copy link
Contributor

Choose a reason for hiding this comment

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

A doc comment would be helpful, at least some examples.

Suggested change
pub fn publish_topic_from_parent(parent: Option<&str>, prefix: &TopicPrefix) -> Topic {
/// (None, prefix) -> ???
/// (Some(parent), prefix) -> ???
pub fn publish_topic_from_parent(parent: Option<&str>, prefix: &TopicPrefix) -> Topic {

Ok(topic)
}

pub fn parent_xid_if_not_main_device(
Copy link
Contributor

Choose a reason for hiding this comment

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

A function name doesn't have to tell the whole story. The signature is enough to understand that this will return None for a device with no parent.

Suggested change
pub fn parent_xid_if_not_main_device(
pub fn parent_xid(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh yeah, I was kinda expecting some comment on this API, as I really struggled to name it properly. By bad, for not adding the rustdocs for it and hoping that a longer/detailed name would convey it properly.

So, I didn't name it just parent_xid because what's returned is not quite the parent_xid in all cases. For any child devices at level 2 and beyond, this function returns Some(<parent-xid>) as expected. But for the immediate child devices of the main device, where the parent xid is the device id of the main device, this function must return None instead of returning Some(<main-device-id>). We're returning None in that case, because all the existing SmartREST APIs like child_device_creation_message, service_creation_message_payload etc expects None when the message must be published to s/us directly and Some(<parent-xid>) when the message must be published to s/us/<parent-xid>. That's the reason why this function impl has this additional filtering step: filter(|tid| *tid != &self.device_topic_id) to switch it to None when the parent is the main device.

The alternatives were the following:

  1. Implement a simple parent_xid function as suggested and call the additional filtering at the places where the SmartREST APIs are used.
  2. Update those SmartREST APIs to take the main device xid as an additional parameter and do the filtering there

I implemented it this way it is now, to just have all that logic at one place and reuse it everywhere. But, if that API looks weird, then I can implement one of the above approaches.

PS: Writing so much to explain the API contract, I kinda feel that it was the wrong choice. Option. 2 above may have been a better choice.

Copy link
Contributor

@didier-wenzek didier-wenzek Dec 12, 2024

Choose a reason for hiding this comment

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

I got it.

Option 2 sounds the correct choice (because moving away the c8y details under c8y_api).

However, it would be better to first stabilize the main PR and add such improvements only in a second step.

For now, simply add this note as a doc comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resolved by 21c694f

While publishing to a nested child device, publish directly to the SmartREST topic
of that device (`c8y/s/us/<xid>`), without specifying all of all its ancestors.
@albinsuresh albinsuresh force-pushed the refactor/simplify-smartrest-publish-topics branch from 21c694f to 3c37347 Compare December 16, 2024 09:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactoring Developer value theme:c8y Theme: Cumulocity related topics
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants