fix #2383: Use "service" if service type missing #2385

Bravo555 · 2023-10-30T12:06:54Z

Proposed changes

Some additions:

A C8y converter test was added to make sure that the emitted service creation message is valid if service.type is empty.
c8y_api::inventory functions which created C8y smartrest messages were changed to return Result. Currently we only check if values marked as mandatory in SmartREST reference documentation are not empty, but we could do more validation in the future.
Added ConversionError::UnexpectedError variant, which can be constructed from an anyhow::Error.

Unexpected part is supposed to denote that the error was not related due to invalid input, or the user doing something wrong, but some unexpected conditions occured that makes it so that the request cannot be completed. Like a panic!, it can be used when there is a logic error on part of the programmer, but unlike panic! it doesn't crash the entire program. More about the idea here.

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

empty service.type value leads to invalid c8y smart rest message being sent #2383

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

rina23q

It looks good as it is now although there can be improvements.

rina23q · 2023-10-30T12:35:30Z

crates/extensions/c8y_mapper_ext/src/converter.rs

+        let service_type = if config.service_type.is_empty() {
+            "service".to_string()
+        } else {
+            config.service_type.clone()
+        };


This implementation is defenitely fine as of now, especially with the restricted time limit for the new release.

However, I've got the feeling that the ideal solution is to change the tedge_config API; If user tries to set an empty string, it works the same as "unset". The "service" is already used as the default value of service.type. It's a little pity that we need to hard-code the same value here.

rina23q · 2023-10-30T12:50:54Z

crates/core/c8y_api/src/smartrest/inventory.rs

-) -> Message {
-    Message::new(
+) -> Result<Message, InvalidValueError> {
+    // TODO: most of this noise can be eliminated by implementing `Serialize`/`Deserialize` for smartrest format


As you said here, why not implementing the representation struct for SmartREST 101 and 102? We already have them for other message IDs, for operations.
https://github.com/thin-edge/thin-edge.io/blob/main/crates/core/c8y_api/src/smartrest/smartrest_serializer.rs

Sorry forgot to check compilation result.

didier-wenzek · 2023-10-30T14:56:31Z

crates/extensions/c8y_mapper_ext/src/error.rs

+
+    #[error("Unexpected error")]
+    UnexpectedError(#[from] anyhow::Error),


This error case is unused.

In all cases, I don't see what's your intent when you write Unexpected part is supposed to denote that the error was not related due to invalid input, or the user doing something wrong, but some unexpected conditions occured that makes it so that the request cannot be completed. Like a panic!, it can be used when there is a logic error on part of the programmer, but unlike panic! it doesn't crash the entire program.

ConversionError is already a ball of mug and I don't see how adding such an open case can help.

It is used in crates/extensions/c8y_mapper_ext/src/converter.rs:290 and crates/extensions/c8y_mapper_ext/src/converter.rs:308 via ?.

The motivation was the same as behind #2353 (comment): by adding a variant that has #[from] anyhow:Error, if in the future we have to handle another type of error, then we can just use .context() to transform it into an anyhow::Error and then ? to convert it into ConversionError::UnexpectedError.

Additional enum variants are only useful as long as the calling code can reasonably match on them. In this case, UnexpectedError doesn't fix ConversionError much, but hopefully it introduces an idea of an opaque error, that we can start using more in the future to stop adding more variants, and also perhaps turn some of the variants we're not matching on into opaque errors.

albinsuresh

LGTM

albinsuresh · 2023-10-31T06:30:09Z

crates/core/c8y_api/src/smartrest/inventory.rs

-) -> Message {
-    Message::new(
+) -> Result<Message, InvalidValueError> {
+    if child_id.is_empty() {


I understand that the scope of this PR is limited to services only, but since you added this check here, why not extend it to device_name and device_type as well?

albinsuresh · 2023-10-31T06:36:53Z

crates/extensions/c8y_mapper_ext/src/converter.rs

+        let service_creation_message = output
+            .into_iter()
+            .find(|m| m.topic.name == SMARTREST_PUBLISH_TOPIC)
+            .expect("service creation message should be present");
+
+        let mut smartrest_fields = service_creation_message.payload_str().unwrap().split(',');
+
+        assert_eq!(smartrest_fields.next().unwrap(), "102");
+        assert_eq!(
+            smartrest_fields.next().unwrap(),
+            format!("{}:device:main:service:service1", converter.device_name)
+        );
+        assert_eq!(smartrest_fields.next().unwrap(), "service");
+        assert_eq!(smartrest_fields.next().unwrap(), "service1");
+        assert_eq!(smartrest_fields.next().unwrap(), "up");


You could use that helper.

Suggested change

let service_creation_message = output

.into_iter()

.find(|m| m.topic.name == SMARTREST_PUBLISH_TOPIC)

.expect("service creation message should be present");

let mut smartrest_fields = service_creation_message.payload_str().unwrap().split(',');

assert_eq!(smartrest_fields.next().unwrap(), "102");

assert_eq!(

smartrest_fields.next().unwrap(),

format!("{}:device:main:service:service1", converter.device_name)

);

assert_eq!(smartrest_fields.next().unwrap(), "service");

assert_eq!(smartrest_fields.next().unwrap(), "service1");

assert_eq!(smartrest_fields.next().unwrap(), "up");

assert_messages_matching(

&messages,

[

(

SMARTREST_PUBLISH_TOPIC,

"102,test-device:device:main:service:service1,service,service1,up".into(),

),

],

);

Just noticed all the compilation failures. But the planned code changes look fine.

codecov · 2023-10-31T14:22:38Z

Codecov Report

Merging #2385 (65bc1ff) into main (edba325) will increase coverage by 0.0%.
Report is 15 commits behind head on main.
The diff coverage is 67.5%.

Additional details and impacted files

Files	Coverage Δ
crates/extensions/c8y_mapper_ext/src/error.rs	`0.0% <ø> (ø)`
crates/extensions/c8y_mapper_ext/src/converter.rs	`80.6% <98.0%> (+0.2%)`	⬆️
crates/extensions/tedge_health_ext/src/lib.rs	`94.2% <75.0%> (-0.9%)`	⬇️
crates/core/tedge_mapper/src/c8y/mapper.rs	`0.0% <0.0%> (ø)`
crates/core/c8y_api/src/smartrest/inventory.rs	`69.1% <44.2%> (-30.9%)`	⬇️

... and 13 files with indirect coverage changes

github-actions · 2023-10-31T14:32:59Z

Robot Results

✅ Passed	❌ Failed	⏭️ Skipped	Total	Pass %	⏱️ Duration
345	0	3	345	100	1h1m18.924999999s

reubenmiller · 2023-10-31T15:09:36Z

@Bravo555 Looking at the system test report the error looks to be legitimate. The following error was found in the log output of the test:

Test Case: Test if all c8y services using default service type when service type configured as empty

Oct 31 14:27:58 dcd30d07c97a tedge-mapper[1264]: Error: Field `service_type` contains invalid value: ""

Bravo555 · 2023-10-31T15:12:41Z

@reubenmiller Yes, I missed one place where empty service type was passed to another actor. It seems to be that handling it on the tedge_config level is the only resilient solution, so it's not possible for service.type to be empty in the first place. Will add this change to this PR.

Some additions: 1. A C8y converter test was added to make sure that the emitted service creation message is valid if `service.type` is empty. 2. `c8y_api::inventory` functions which created C8y smartrest messages were changed to return `Result`. Currently we only check if values marked as mandatory in [SmartREST reference documentation][1] are not empty, but we could do more validation in the future. 3. Added `ConversionError::UnexpectedError` variant, which can be constructed from an `anyhow::Error`. _Unexpected_ part is supposed to denote that the error was not related due to invalid input, or the user doing something wrong, but some unexpected conditions occured that makes it so that the request cannot be completed. Like a `panic!`, it can be used when there is a logic error on part of the programmer, but unlike `panic!` it doesn't crash the entire program. More about the idea [here][2]. [1]: https://cumulocity.com/guides/reference/smartrest-two/#102 [2]: https://www.lpalmieri.com/posts/error-handling-rust/#avoid-ball-of-mud-error-enums

didier-wenzek

Approved, to include this fix in the upcoming release.

However, this will have to be addressed at the config level as suggested by @rina23q and acknowledged by @Bravo555 as a less fragile solution

Bravo555 requested review from albinsuresh and reubenmiller October 30, 2023 12:06

rina23q previously approved these changes Oct 30, 2023

View reviewed changes

didier-wenzek reviewed Oct 30, 2023

View reviewed changes

albinsuresh previously approved these changes Oct 31, 2023

View reviewed changes

Bravo555 force-pushed the fix/2383/empty-service-type branch from 74c1208 to 7708405 Compare October 31, 2023 14:02

Bravo555 had a problem deploying to Test Pull Request October 31, 2023 14:14 — with GitHub Actions Failure

Bravo555 force-pushed the fix/2383/empty-service-type branch from 7708405 to 3d44472 Compare October 31, 2023 16:33

Bravo555 had a problem deploying to Test Pull Request October 31, 2023 16:45 — with GitHub Actions Failure

Bravo555 force-pushed the fix/2383/empty-service-type branch from 3d44472 to 65bc1ff Compare October 31, 2023 16:49

Bravo555 temporarily deployed to Test Pull Request October 31, 2023 17:03 — with GitHub Actions Inactive

didier-wenzek approved these changes Oct 31, 2023

View reviewed changes

Bravo555 merged commit 420901e into thin-edge:main Oct 31, 2023

Bravo555 deleted the fix/2383/empty-service-type branch October 31, 2023 18:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix #2383: Use "service" if service type missing #2385

fix #2383: Use "service" if service type missing #2385

Bravo555 commented Oct 30, 2023

rina23q left a comment

rina23q Oct 30, 2023

rina23q Oct 30, 2023

didier-wenzek Oct 30, 2023

Bravo555 Oct 30, 2023

albinsuresh left a comment

albinsuresh Oct 31, 2023

albinsuresh Oct 31, 2023

codecov bot commented Oct 31, 2023 •

edited

Loading

github-actions bot commented Oct 31, 2023 •

edited

Loading

reubenmiller commented Oct 31, 2023 •

edited

Loading

Bravo555 commented Oct 31, 2023

didier-wenzek left a comment


		#[error("Unexpected error")]
		UnexpectedError(#[from] anyhow::Error),

fix #2383: Use "service" if service type missing #2385

fix #2383: Use "service" if service type missing #2385

Conversation

Bravo555 commented Oct 30, 2023

Proposed changes

Types of changes

Paste Link to the issue

Checklist

Further comments

rina23q left a comment

Choose a reason for hiding this comment

rina23q Oct 30, 2023

Choose a reason for hiding this comment

rina23q Oct 30, 2023

Choose a reason for hiding this comment

didier-wenzek Oct 30, 2023

Choose a reason for hiding this comment

Bravo555 Oct 30, 2023

Choose a reason for hiding this comment

albinsuresh left a comment

Choose a reason for hiding this comment

albinsuresh Oct 31, 2023

Choose a reason for hiding this comment

albinsuresh Oct 31, 2023

Choose a reason for hiding this comment

codecov bot commented Oct 31, 2023 • edited Loading

Codecov Report

github-actions bot commented Oct 31, 2023 • edited Loading

Robot Results

reubenmiller commented Oct 31, 2023 • edited Loading

Bravo555 commented Oct 31, 2023

didier-wenzek left a comment

Choose a reason for hiding this comment

codecov bot commented Oct 31, 2023 •

edited

Loading

github-actions bot commented Oct 31, 2023 •

edited

Loading

reubenmiller commented Oct 31, 2023 •

edited

Loading