Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix a method to split SmartREST messages and improve failure reason #1692

Merged

Conversation

rina23q
Copy link
Member

@rina23q rina23q commented Jan 24, 2023

Proposed changes

When receiving multiline SmartREST messages, splitting simply by \n is not a right approach because a SmartREST message can also contain \n, e.g. an argument of c8y_Command.

Also, in case of an operation failure, giving only the last line of stderr is not easy enough for users to understand why the operation failed. It should give also the whole context as much as possible.

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

#1687

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

@reubenmiller
Copy link
Contributor

reubenmiller commented Jan 24, 2023

Case 1: Quotes are ignored on multiline commands if the line ends with a quote

There seems to be an issue when handling the newline split logic and double quotes. I have a suspicion it is swallowing the last character (or the last character is just being forgotten)

For example if you take a multiline command, if a line ends in a double quote (") then a newline, the last character does not get passed to the plugin. If a space is added to the end of the line (before the newline char), then it magically works. So it is very likely just a small logical error when processing the characters.

Works (with a dummy trailing space at the end of the first line, after the closing double quote)

echo "testme" 
echo "test again"

Does not work (no trailing space on the first line)

echo "testme"
echo "test again"

Case 2: Commas cause the command to be truncated

If a comma occurs on another line, then only the lines up until the line with the comma is sent to the plugin.

For example the following command (which should produce 3 lines on the stdout, only returns the first 2)
Command

echo "line 1";
echo "line 2";
echo "line, 3";

Output

line 1
line 2

Where as the expected output is:

line 1
line 2
line, 3

Copy link
Contributor

@didier-wenzek didier-wenzek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved

When receiving multiline SmartREST messages, splitting simply by `\n`
is not a right approach because a SmartREST message can also contain
`\n`, e.g. an argument of c8y_Command.

Also, in case of an operation failure, giving only the last line of
stderr is not easy enough for users to understand why the operation
failed. It should give also the whole context as much as possible.

Resolves thin-edge#1687

Signed-off-by: Rina Fujino <18257209+rina23q@users.noreply.github.com>
@rina23q rina23q force-pushed the bugfix/1687/support-multi-line-command branch from 39f2dd8 to 0e6d88d Compare January 25, 2023 14:59
Copy link
Contributor

@reubenmiller reubenmiller left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved

@reubenmiller
Copy link
Contributor

Case 1: Quotes are ignored on multiline commands if the line ends with a quote

There seems to be an issue when handling the newline split logic and double quotes. I have a suspicion it is swallowing the last character (or the last character is just being forgotten)

For example if you take a multiline command, if a line ends in a double quote (") then a newline, the last character does not get passed to the plugin. If a space is added to the end of the line (before the newline char), then it magically works. So it is very likely just a small logical error when processing the characters.

Works (with a dummy trailing space at the end of the first line, after the closing double quote)

echo "testme" 
echo "test again"

Does not work (no trailing space on the first line)

echo "testme"
echo "test again"

Case 2: Commas cause the command to be truncated

If a comma occurs on another line, then only the lines up until the line with the comma is sent to the plugin.

For example the following command (which should produce 3 lines on the stdout, only returns the first 2) Command

echo "line 1";
echo "line 2";
echo "line, 3";

Output

line 1
line 2

Where as the expected output is:

line 1
line 2
line, 3

We found the root cause and it was the shell handler script which was causing the errors. The example in the ticket has also been updated, #1687

@rina23q rina23q merged commit 982df72 into thin-edge:main Jan 25, 2023
@rina23q rina23q deleted the bugfix/1687/support-multi-line-command branch January 25, 2023 16:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants