Improve validation of SmartREST payloads

[Bravo555]

Proposed changes

NOTE: there are some .expect("TODO: ...") calls where it's unclear what should be done on certain corner cases, e.g. when operation name is so pathologically long that it increases the size of the payload over the limit. These TODOs will have to be resolved before merging.

This PR introduces a SmartrestPayload type, meant to encode assumptions about payload of SmartREST messages, i.e. that:

• it is a "CSV-like" format
• it has a size limit

This type should be used in places where we previously just used a plain String or constructed smartrest messages directly with format!, to ensure that above invariants always hold, and as the diff shows, in some places these invariants were being ignored. Most of them are corner cases, i.e. where it is technically possible for a device id or template id to increase the payload size over the limit, which should not normally happen.

One more advantage is also that we can more easily see where smartrest payloads are being constructed or used by looking up references to SmartrestPayload type in an IDE or rust-analyzer.

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

Next steps

This PR only introduces the type and introduces it in some functions that construct SmartREST messages that are later sent, but there remain some more functions in other modules that sanitize smartrest payloads. These functions read and return strings, which is maybe why they weren't eagerly used (hard to know if you need to call them). The next step would be to integrate these sanitizing functions into the SmartrestPayload type.

[codecov]

Codecov Report

Attention: Patch coverage is 91.08280% with 14 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
crates/core/c8y_api/src/smartrest/payload.rs	86.84%	3 Missing and 2 partials ⚠️
crates/core/tedge_mapper/src/c8y/mapper.rs	0.00%	4 Missing ⚠️
...core/c8y_api/src/smartrest/smartrest_serializer.rs	95.65%	3 Missing ⚠️
crates/core/c8y_api/src/smartrest/inventory.rs	95.65%	0 Missing and 1 partial ⚠️
crates/extensions/c8y_mapper_ext/src/converter.rs	75.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

📢 Thoughts on this report? Let us know!

[github-actions]

Robot Results

✅ Passed	❌ Failed	⏭️ Skipped	Total	Pass %	⏱️ Duration
510	0	2	510	100	1h27m2.01017s

[didier-wenzek]

I would be conservative here, and simply keep String as before.

[Bravo555]

Indeed, also csv::Writer should never output invalid utf-8 so this branch should be unreachable anyway.

[didier-wenzek]

The simpler is to have a pub(crate) fn new(...) -> Self constructor instead of a pub(crate) field.

Better, but more involved, is to move here the core of the serializer::succeed_operation implementation into something along the following lines:

impl SmartrestPayload {
    /// Trim the last fields, if the resulting payload is above size limit.
    pub fn trim_serialize<S: Serialize>(fields: impl IntoIterator<Item = S>) -> Result<Self, SmartrestPayloadError> {
    }

[Bravo555]

I wanted this type to eventually be used for all Smartrest messages, and for this it shouldn't make assumptions about which fields can be trimmed, as it's different between messages, e.g. it's safe to trim additional parameters in 506, but it would be wrong to trim the last field, the URL, of 115 message. We could have an API where the caller can decide which field to trim, but we shouldn't decide here that it should be the last field.

As it depends on the message which fields are optional and which are safe to trim, my view is that that trimming should be done by another layer and this SmartrestPayload type should only allow writing and reading valid payloads and minimize other assumptions.

[didier-wenzek]

As it depends on the message which fields are optional and which are safe to trim,

Good point. The caller has to tell which field can be trimmed.

my view is that that trimming should be done by another layer and this SmartrestPayload type should only allow writing and reading valid payloads and minimize other assumptions.

Okay for now, but I'm not really convinced with idea for yet another layer. We already have too many layers.

[albinsuresh]

Why not trim the failure reason the same way we do it for successful operation, appending ...<truncated> at the end?

[Bravo555]

Indeed it would be good idea to do it the same may, but perhaps in a follow-up PR since it would be a functional change

[didier-wenzek]

Approved.

[albinsuresh]

LGTM