refactor: Extract operation implementation details from `CumulocityConverter` #3260

Bravo555 · 2024-11-22T09:43:54Z

TODO

cleanup

Proposed changes

This PR introduces an SupportedOperations struct that is used by CumulocityConverter to load and save operations.

It achieves following things:

Extract all fs operations and path building from CumulocityConverter into new operations module: The main CumulocityConverter doesn't create any files or symlinks, or doesn't need to create paths, the logic of mapping supported operations to the filesystem is contained in its own module
No main device/child device branching in CumulocityConverter: instead of having separate operations: Operations and children: HashMap<Child, Operations>, SupportedOperation handles this distinction, the converter just passes an external id
Use BTreeMap for to have only one operation under a given name and to ensure consistent ordering

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

refactor: Operations struct refactor #3256

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

codecov · 2024-11-22T09:54:33Z

Codecov Report

Attention: Patch coverage is 83.45324% with 46 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
crates/core/c8y_api/src/smartrest/operations.rs	86.63%	17 Missing and 12 partials ⚠️
crates/extensions/c8y_mapper_ext/src/converter.rs	72.13%	6 Missing and 11 partials ⚠️

Additional details and impacted files

📢 Thoughts on this report? Let us know!

Using it multiple times will create operations with the same names, but its impossible to have multiple operation implementations for the same operation name Signed-off-by: Marcel Guzik <[email protected]>

Signed-off-by: Marcel Guzik <[email protected]>

github-actions · 2024-11-27T12:25:14Z

Robot Results

✅ Passed	❌ Failed	⏭️ Skipped	Total	Pass %	⏱️ Duration
527	0	2	527	100	1h27m28.699284s

Signed-off-by: Marcel Guzik <[email protected]>

didier-wenzek · 2024-11-28T10:41:10Z

crates/extensions/c8y_mapper_ext/src/converter.rs

-                let prev_operation = current_operations.remove_operation(&operation.name);
+                let prev_operation = current_operations.insert_operation(operation);


This change seems weird at first ... but is correct!

Previously any operation with the same name was removed before adding a new version of the same operation.

didier-wenzek · 2024-11-28T10:47:18Z

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

+    /// Required because when given an external id of the main device when creating an operation, we want to create it
+    /// in a main directory, instead of a subdirectory.


Okay. It makes sense for this PR focus on code refactoring without any user-visible changes, but this asymmetry between the main device and the child devices makes things more complicated than they should.

I don't fully get what you mean. Do you mean this PR specifically, or the codebase more generally?

One maybe unnecessary change that I did was instead of having "operations for the main device" in one field and "operations for child devices" in a hashmap, i put it all in a single hashmap, so that CumulocityConverter only passes along the external id and doesn't have to care if a device is a child device or a main device. In retrospect the functions can still take an external id but the operations for the main device can be made a normal field instead of a hashmap entry, since "operations for the main device" is something that always exists and putting it in a hashmap where it can be removed introduces an extra failure mode.

My point is in no case to disagree with this PR which is a real improvement reducing the accidental complexity of the c8y mapper.

My comment is coming from the fact that you (correctly) feel the need to add a comment on why we need the main device external id here (while we don't for the child devices).

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

didier-wenzek · 2024-11-28T11:02:14Z

crates/extensions/c8y_mapper_ext/src/converter.rs

-            EntityType::MainDevice => Ok(self.operations.filter_by_topic(topic, prefix)),
-            EntityType::ChildDevice => {
-                if let Some(operations) = self.children.get(device_xid) {
-                    Ok(operations.filter_by_topic(topic, prefix))
-                } else {
-                    Ok(Vec::new())
-                }
-            }
+            EntityType::MainDevice | EntityType::ChildDevice => Ok(self
+                .supported_operations
+                .get_operation_handlers(device_xid, topic, prefix)),


This is one of the benefit of this PR, pushing away the artificial difference that has been introduced between the main and the child devices (see also: #3260 (comment))

didier-wenzek

Approved. This is nice clarification of the code base.

refactor: Extract operation implementation details from `CumulocityConverter`

Bravo555 requested a deployment to Test Pull Request November 22, 2024 09:43 — with GitHub Actions Waiting

Bravo555 changed the title ~~refactor: Use a BTreeMap for Operations storage~~ refactor: Extract operation implementation details from CumulocityConverter Nov 27, 2024

Bravo555 added 2 commits November 27, 2024 12:57

Remove Operation::new with hardcoded name

1d30873

Using it multiple times will create operations with the same names, but its impossible to have multiple operation implementations for the same operation name Signed-off-by: Marcel Guzik <[email protected]>

Changed Operations::operations container to BTreeMap

0121168

Signed-off-by: Marcel Guzik <[email protected]>

Bravo555 force-pushed the refactor/operations-loading branch from 51774d9 to 7efcbfc Compare November 27, 2024 12:02

Bravo555 temporarily deployed to Test Pull Request November 27, 2024 12:02 — with GitHub Actions Inactive

Bravo555 temporarily deployed to Test Auto November 27, 2024 12:08 — with GitHub Actions Inactive

supported operations: remove parent/child branching from converter

2f0c534

Signed-off-by: Marcel Guzik <[email protected]>

Bravo555 force-pushed the refactor/operations-loading branch from 7efcbfc to 2f0c534 Compare November 28, 2024 09:07

Bravo555 temporarily deployed to Test Pull Request November 28, 2024 09:07 — with GitHub Actions Inactive

Bravo555 marked this pull request as ready for review November 28, 2024 09:07

Bravo555 requested review from didier-wenzek, albinsuresh, jarhodes314 and rina23q as code owners November 28, 2024 09:07

Bravo555 temporarily deployed to Test Auto November 28, 2024 09:13 — with GitHub Actions Inactive

didier-wenzek reviewed Nov 28, 2024

View reviewed changes

didier-wenzek approved these changes Nov 28, 2024

View reviewed changes

Bravo555 added this pull request to the merge queue Nov 28, 2024

Bravo555 removed this pull request from the merge queue due to a manual request Nov 28, 2024

github-merge-queue bot pushed a commit that referenced this pull request Nov 28, 2024

Merge pull request #3260 from Bravo555/refactor/operations-loading

3beeb44

refactor: Extract operation implementation details from `CumulocityConverter`

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: Extract operation implementation details from `CumulocityConverter` #3260

refactor: Extract operation implementation details from `CumulocityConverter` #3260

Bravo555 commented Nov 22, 2024 •

edited

Loading

codecov bot commented Nov 22, 2024 •

edited

Loading

github-actions bot commented Nov 27, 2024 •

edited

Loading

didier-wenzek Nov 28, 2024

didier-wenzek Nov 28, 2024

Bravo555 Nov 28, 2024

didier-wenzek Nov 28, 2024

didier-wenzek Nov 28, 2024

didier-wenzek left a comment

		let prev_operation = current_operations.remove_operation(&operation.name);
		let prev_operation = current_operations.insert_operation(operation);

		/// Required because when given an external id of the main device when creating an operation, we want to create it
		/// in a main directory, instead of a subdirectory.

refactor: Extract operation implementation details from CumulocityConverter #3260

Are you sure you want to change the base?

refactor: Extract operation implementation details from CumulocityConverter #3260

Conversation

Bravo555 commented Nov 22, 2024 • edited Loading

TODO

Proposed changes

Types of changes

Paste Link to the issue

Checklist

Further comments

codecov bot commented Nov 22, 2024 • edited Loading

Codecov Report

github-actions bot commented Nov 27, 2024 • edited Loading

Robot Results

didier-wenzek Nov 28, 2024

Choose a reason for hiding this comment

didier-wenzek Nov 28, 2024

Choose a reason for hiding this comment

Bravo555 Nov 28, 2024

Choose a reason for hiding this comment

didier-wenzek Nov 28, 2024

Choose a reason for hiding this comment

didier-wenzek Nov 28, 2024

Choose a reason for hiding this comment

didier-wenzek left a comment

Choose a reason for hiding this comment

refactor: Extract operation implementation details from `CumulocityConverter` #3260

refactor: Extract operation implementation details from `CumulocityConverter` #3260

Bravo555 commented Nov 22, 2024 •

edited

Loading

codecov bot commented Nov 22, 2024 •

edited

Loading

github-actions bot commented Nov 27, 2024 •

edited

Loading