Refactor: Simplify entity auto-registration by didier-wenzek · Pull Request #3139 · thin-edge/thin-edge.io

didier-wenzek · 2024-09-25T14:40:10Z

Proposed changes

Introduce a new function mqtt_topic::default_topic_schema::parse() taking an entity topic id and returning the registration messages matching the auto-registration behavior.

Using this parse function simplifies the auto-registration logic which has no more to parse again and again the entity topic id to extract registration info.

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

didier-wenzek · 2024-09-25T14:42:25Z

crates/core/tedge_api/src/entity_store.rs

-                    parent: None,
+                    parent: Some(EntityTopicId::from_str("device/main//").unwrap()),


I was surprised by this. Why the parent of child device was expected to be None? Because no registration is required for the main device?

EntityRegistrationMessage is supposed to be a subtype of MqttMessage, but with required fields like type or external_id present. Because it was supposed to be only created by converting from MqttMessage, it was to have only the same information that MqttMessage had, and actual filling of fallbacks and defaults was to be done in entity store when adding the entity (because only the entity store knows what's the actual name of main device and has a function to generate the entity's external id).

So in this case MqttMessage payload that contained no parent field would be converted to EntityRegistrationMessage that didn't contain it as well, and the parent would be resolved by the entity store when registering the child.

So I think it'd be better to leave it as None.

and the parent would be resolved by the entity store when registering the child.

This is what I would like to fix with this PR. Indeed, for auto registration, the entity store starts with a topic matching the default topic scheme (starting with a validation) and then extracts sub data (type, names, relationship) using misc functions assuming each the default topic scheme.

With this PR, the entity store can parse an entity topic id into all the info required for auto-registration is applicable.

This also addresses what you noted : currently it seems it has a very large surface with lots of methods that are ultimately a bit shallow

So I think it'd be better to leave it as None.

However, I will opt for the other way, i.e. setting the parent. Indeed, the point of the parse(entity_topic_id)
is to provide an explicit representation of all what is implied by an entity topic of the default topic scheme.

codecov · 2024-09-25T14:51:58Z

Codecov Report

Attention: Patch coverage is 98.21429% with 1 line in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
crates/core/tedge_api/src/entity_store.rs	95.65%	0 Missing and 1 partial ⚠️

Additional details and impacted files

📢 Thoughts on this report? Let us know!

github-actions · 2024-09-25T15:14:13Z

Robot Results

✅ Passed	❌ Failed	⏭️ Skipped	Total	Pass %	⏱️ Duration
506	0	2	506	100	1h30m52.977191s

Bravo555

Seems a small simplification with no functional changes, approving with a small comment regarding the changed unit test.

But I'm reminded how entity store and entity topic id logic could be made simpler, as currently it seems it has a very large surface with lots of methods that are ultimately a bit shallow.

Bravo555 · 2024-09-25T16:30:05Z

crates/core/tedge_api/src/mqtt_topics.rs

+            ["device", child, "service", service] if !child.is_empty() && !service.is_empty() => {
+                // The device of a service has to be registered fist
+                let child_topic_id = EntityTopicId::default_child_device(child).unwrap();
+                let mut registrations = parse(&child_topic_id);
+
+                // Then the service can be registered
+                registrations.push(EntityRegistrationMessage {
+                    topic_id: topic_id.clone(),
+                    external_id: None,
+                    r#type: EntityType::Service,
+                    parent: Some(EntityTopicId::default_child_device(child).unwrap()),
+                    other: json!({ "name": service }).as_object().unwrap().to_owned(),
+                });
+                registrations
+            }


suggestion: this branch also covers the case when child = "main", so service for the main device, perhaps rename child to device to make it more clear

Or even a dedicated arm for the main device service case, as the parent (main device) need not be registered in that case.

Fixed by 1d3f707

Bravo555 · 2024-09-25T16:46:29Z

crates/core/tedge_api/src/entity_store.rs

+            if auto_entity.r#type == EntityType::Service {
+                auto_entity
+                    .other
+                    .insert("type".to_string(), self.default_service_type.clone().into());
            }


thought: Bit awkward we still need this branch here because c8y needs to have a service type, which I reckon doesn't have much use in a local entity store, but we only have access to fallback default service type in the entity store.

In the future it'd be good to move it either up to c8y mapper (I think it was there before, but can't remember the reason why I pulled it into the entity store, perhaps I was wrong to do so) or down somewhere, but in any case it feels kind of out of place in here.

Bravo555 · 2024-09-25T16:53:56Z

crates/core/tedge_api/src/entity_store.rs

-                    parent: None,
+                    parent: Some(EntityTopicId::from_str("device/main//").unwrap()),


EntityRegistrationMessage is supposed to be a subtype of MqttMessage, but with required fields like type or external_id present. Because it was supposed to be only created by converting from MqttMessage, it was to have only the same information that MqttMessage had, and actual filling of fallbacks and defaults was to be done in entity store when adding the entity (because only the entity store knows what's the actual name of main device and has a function to generate the entity's external id).

So in this case MqttMessage payload that contained no parent field would be converted to EntityRegistrationMessage that didn't contain it as well, and the parent would be resolved by the entity store when registering the child.

So I think it'd be better to leave it as None.

albinsuresh

The refactoring looks fine to me. Yet there are some test failures.

didier-wenzek · 2024-09-26T08:54:28Z

crates/core/tedge_api/src/entity_store.rs

-                .default_source_device_identifier()
-                .expect("device id must be present as the topic id follows the default scheme");
-
-            if !parent_device_id.is_default_main_device() && self.get(&parent_device_id).is_none() {


Oops, I removed this check that the device should not already be registered.

self.get(&parent_device_id).is_none()

This explains why the system tests are failing.

For instance for :

Before, only the service where registered:

c8y/s/us/foo_child1 102,foo:device:foo_child1:service:app1,service,app1,up

But this check missing service is also registered, while already known by c8y:

c8y/s/us 101,foo:device:foo_child1,foo_child1,thin-edge.io-child c8y/s/us/foo_child1 102,foo:device:foo_child1:service:app1,service,app1,up

Fixed by eaf5a8f (with a following simplification: 1d3f707)

albinsuresh

LGTM.

Introduce a new function `mqtt_topic::default_topic_schema::parse()` taking an entity topic id and returning the registration messages matching the auto-registration behavior. Using this parse function simplifies the auto-registration logic which has no more to parse again and again the entity topic id to extract registration info. Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>

Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>

didier-wenzek requested review from albinsuresh, jarhodes314 and rina23q as code owners September 25, 2024 14:40

didier-wenzek temporarily deployed to Test Pull Request September 25, 2024 14:40 — with GitHub Actions Inactive

didier-wenzek commented Sep 25, 2024

View reviewed changes

didier-wenzek had a problem deploying to Test Auto September 25, 2024 14:56 — with GitHub Actions Failure

didier-wenzek force-pushed the refactor/default-entity-scheme branch from 879ede2 to c1e76fa Compare September 25, 2024 15:48

didier-wenzek had a problem deploying to Test Pull Request September 25, 2024 15:48 — with GitHub Actions Failure

didier-wenzek temporarily deployed to Test Pull Request September 25, 2024 16:57 — with GitHub Actions Inactive

didier-wenzek had a problem deploying to Test Auto September 25, 2024 17:03 — with GitHub Actions Failure

Bravo555 approved these changes Sep 25, 2024

View reviewed changes

albinsuresh reviewed Sep 26, 2024

View reviewed changes

didier-wenzek commented Sep 26, 2024

View reviewed changes

didier-wenzek temporarily deployed to Test Pull Request September 26, 2024 09:01 — with GitHub Actions Inactive

didier-wenzek temporarily deployed to Test Auto September 26, 2024 09:06 — with GitHub Actions Inactive

didier-wenzek had a problem deploying to Test Pull Request September 26, 2024 09:58 — with GitHub Actions Failure

didier-wenzek temporarily deployed to Test Pull Request September 26, 2024 10:04 — with GitHub Actions Inactive

didier-wenzek temporarily deployed to Test Auto September 26, 2024 10:10 — with GitHub Actions Inactive

didier-wenzek requested review from Bravo555 and albinsuresh September 26, 2024 13:28

albinsuresh approved these changes Sep 27, 2024

View reviewed changes

didier-wenzek added 2 commits September 27, 2024 09:17

Remove unused methods

7b71d09

Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>

didier-wenzek force-pushed the refactor/default-entity-scheme branch from cde04b7 to 7b71d09 Compare September 27, 2024 07:17

didier-wenzek temporarily deployed to Test Pull Request September 27, 2024 07:18 — with GitHub Actions Inactive

didier-wenzek enabled auto-merge September 27, 2024 07:19

didier-wenzek temporarily deployed to Test Auto September 27, 2024 07:23 — with GitHub Actions Inactive

didier-wenzek added this pull request to the merge queue Sep 27, 2024

Merged via the queue into thin-edge:main with commit 084fa38 Sep 27, 2024

didier-wenzek added refactoring Developer value theme:registration Theme: Device registration and device certificate related topics labels Sep 27, 2024

didier-wenzek deleted the refactor/default-entity-scheme branch September 27, 2024 08:26

		parent: None,
		parent: Some(EntityTopicId::from_str("device/main//").unwrap()),

Conversation

didier-wenzek commented Sep 25, 2024

Proposed changes

Types of changes

Paste Link to the issue

Checklist

Further comments

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Sep 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

github-actions bot commented Sep 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Robot Results

Uh oh!

Bravo555 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

albinsuresh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

albinsuresh left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented Sep 25, 2024 •

edited

Loading

github-actions bot commented Sep 25, 2024 •

edited

Loading