fix: Trim `reason` field in "succeed custom operation" SmartREST message by Bravo555 · Pull Request #3179 · thin-edge/thin-edge.io

Bravo555 · 2024-10-09T16:13:46Z

TODO

add test
consider how to trim CSV output
remove unwraps
Rust unit test to improve coverage

Proposed changes

Trims reason field for sent "set operation to SUCCESSFUL" smartrest messages.

Additionally, ...<trimmed> indicator is inserted at the end to let the user know that the output was trimmed.

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

custom operation output can exceed Cumulocity's max allowed payload size #3171

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

This PR solves the issue for operation success message for c8y_Command, but may still possibly fail for other types of messages.
For a follow-up we should do a clean up and use types to verify that message is smaller than max limit.

codecov · 2024-10-09T16:29:51Z

Codecov Report

Attention: Patch coverage is 90.00000% with 7 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
...core/c8y_api/src/smartrest/smartrest_serializer.rs	90.00%	3 Missing and 4 partials ⚠️

Additional details and impacted files

📢 Thoughts on this report? Let us know!

github-actions · 2024-10-09T16:44:07Z

Robot Results

✅ Passed	❌ Failed	⏭️ Skipped	Total	Pass %	⏱️ Duration
509	0	2	509	100	1h26m25.799880999s

reubenmiller · 2024-10-09T18:32:37Z

@Bravo555 It is probably enough just to serialize the result field and apply the constraints to that field before constructing the full csv response.

For example with some python-like pseudo code (only accounting for the case where there is a result related to the command:

def serailize_successful_response():
    output = encode_csv(["503", fragment])
    # Note: 2 extra characters for the surrounding quotes which will be added to the result
    max_result_limit = MAX_PAYLOAD_LIMIT - len(output) - 2     

    result_encoded = strip_surrounding_quotes(encode_csv([result]))
    result_encoded = '"' + result_encoded[0:max_result_limit] + '"'
    return output + "," + result_encoded

Bravo555 · 2024-10-10T09:38:58Z

It is probably enough just to serialize the result field and apply the constraints to that field before constructing the full csv response.

Will use that approach, but is/should there be a way for the user to know that the response is truncated as opposed to not truncated but maximum allowed size?

reubenmiller · 2024-10-15T09:05:22Z

Will use that approach, but is/should there be a way for the user to know that the response is truncated as opposed to not truncated but maximum allowed size?

We can just the convention of appending the ...<truncated> as a suffix (or something similar).

didier-wenzek

The code is correct and thoroughly tested. I will be happy to approve once added the ...<truncated> suffix.

didier-wenzek · 2024-10-16T16:34:01Z

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

+        }
+        let trimmed_reason = &reason[..max_result_limit];
+
+        format!("{prefix},\"{trimmed_reason}\"")


It would be good to add a hint that the reason has been truncated as suggested by @reubenmiller: #3179 (comment)

Suggested change

format!("{prefix},\"{trimmed_reason}\"")

format!("{prefix},\"{trimmed_reason} ...<truncated>\"")

tests/RobotFramework/tests/cumulocity/shell/shell_operation.robot

Signed-off-by: Marcel Guzik <marcel.guzik@inetum.com>

didier-wenzek

Approved.

reubenmiller

Approved

Bravo555 had a problem deploying to Test Pull Request October 9, 2024 16:13 — with GitHub Actions Failure

Bravo555 force-pushed the fix/3171/custom-operation-output-over-max-payload-size branch from cd3e9a5 to ae7c870 Compare October 9, 2024 16:14

Bravo555 temporarily deployed to Test Pull Request October 9, 2024 16:14 — with GitHub Actions Inactive

Bravo555 temporarily deployed to Test Auto October 9, 2024 16:25 — with GitHub Actions Inactive

Bravo555 force-pushed the fix/3171/custom-operation-output-over-max-payload-size branch from ae7c870 to 2e789be Compare October 14, 2024 12:24

Bravo555 temporarily deployed to Test Pull Request October 14, 2024 12:24 — with GitHub Actions Inactive

Bravo555 temporarily deployed to Test Auto October 14, 2024 12:35 — with GitHub Actions Inactive

Bravo555 marked this pull request as ready for review October 15, 2024 09:00

Bravo555 requested review from albinsuresh, didier-wenzek, jarhodes314 and rina23q as code owners October 15, 2024 09:00

Bravo555 self-assigned this Oct 15, 2024

Bravo555 force-pushed the fix/3171/custom-operation-output-over-max-payload-size branch from 2e789be to 5df3697 Compare October 15, 2024 13:00

Bravo555 requested review from a team and gligorisaev as code owners October 15, 2024 13:00

Bravo555 temporarily deployed to Test Pull Request October 15, 2024 13:00 — with GitHub Actions Inactive

Bravo555 temporarily deployed to Test Auto October 15, 2024 13:06 — with GitHub Actions Inactive

Bravo555 changed the title ~~Fail operation if sanitized CSV output is too large~~ fix: Trim reason field in "succeed custom operation" smartrest message Oct 16, 2024

Bravo555 removed their assignment Oct 16, 2024

reubenmiller added theme:c8y Theme: Cumulocity related topics theme:mqtt Theme: mqtt and mosquitto related topics labels Oct 16, 2024

didier-wenzek reviewed Oct 16, 2024

View reviewed changes

Bravo555 self-assigned this Oct 17, 2024

gligorisaev reviewed Oct 17, 2024

View reviewed changes

tests/RobotFramework/tests/cumulocity/shell/shell_operation.robot Show resolved Hide resolved

Bravo555 temporarily deployed to Test Pull Request October 17, 2024 12:13 — with GitHub Actions Inactive

Bravo555 had a problem deploying to Test Auto October 17, 2024 12:20 — with GitHub Actions Failure

Trim reason field in succeed operation message

0efc1a6

Signed-off-by: Marcel Guzik <marcel.guzik@inetum.com>

Bravo555 force-pushed the fix/3171/custom-operation-output-over-max-payload-size branch from 9316fa1 to 0efc1a6 Compare October 17, 2024 12:31

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

Bravo555 temporarily deployed to Test Auto October 17, 2024 13:02 — with GitHub Actions Inactive

didier-wenzek approved these changes Oct 17, 2024

View reviewed changes

reubenmiller approved these changes Oct 17, 2024

View reviewed changes

Bravo555 added this pull request to the merge queue Oct 17, 2024

Bravo555 removed their assignment Oct 17, 2024

Merged via the queue into thin-edge:main with commit 0748986 Oct 17, 2024

Bravo555 deleted the fix/3171/custom-operation-output-over-max-payload-size branch October 18, 2024 09:02

reubenmiller changed the title ~~fix: Trim reason field in "succeed custom operation" smartrest message~~ fix: Trim reason field in "succeed custom operation" SmartREST message Dec 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Trim `reason` field in "succeed custom operation" SmartREST message#3179

fix: Trim `reason` field in "succeed custom operation" SmartREST message#3179
Bravo555 merged 1 commit intothin-edge:mainfrom
Bravo555:fix/3171/custom-operation-output-over-max-payload-size

Bravo555 commented Oct 9, 2024 •

edited

Loading

Uh oh!

codecov bot commented Oct 9, 2024 •

edited

Loading

Uh oh!

github-actions bot commented Oct 9, 2024 •

edited

Loading

Uh oh!

reubenmiller commented Oct 9, 2024 •

edited

Loading

Uh oh!

Bravo555 commented Oct 10, 2024

Uh oh!

reubenmiller commented Oct 15, 2024 •

edited

Loading

Uh oh!

didier-wenzek left a comment

Uh oh!

didier-wenzek Oct 16, 2024

Uh oh!

Uh oh!

didier-wenzek left a comment

Uh oh!

reubenmiller left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

	format!("{prefix},\"{trimmed_reason}\"")
	format!("{prefix},\"{trimmed_reason} ...<truncated>\"")

Conversation

Bravo555 commented Oct 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

TODO

Proposed changes

Types of changes

Paste Link to the issue

Checklist

Further comments

Uh oh!

codecov bot commented Oct 9, 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 Oct 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Robot Results

Uh oh!

reubenmiller commented Oct 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Bravo555 commented Oct 10, 2024

Uh oh!

reubenmiller commented Oct 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

didier-wenzek left a comment

Choose a reason for hiding this comment

Uh oh!

didier-wenzek Oct 16, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

didier-wenzek left a comment

Choose a reason for hiding this comment

Uh oh!

reubenmiller left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Bravo555 commented Oct 9, 2024 •

edited

Loading

codecov bot commented Oct 9, 2024 •

edited

Loading

github-actions bot commented Oct 9, 2024 •

edited

Loading

reubenmiller commented Oct 9, 2024 •

edited

Loading

reubenmiller commented Oct 15, 2024 •

edited

Loading