You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@jmeter.apache.org by "FSchumacher (via GitHub)" <gi...@apache.org> on 2023/04/15 15:41:09 UTC

[GitHub] [jmeter] FSchumacher opened a new pull request, #5805: Escape control characters in CSV log file

FSchumacher opened a new pull request, #5805:
URL: https://github.com/apache/jmeter/pull/5805

   ## Description
   
   This patch escapes new line and carriage return characters when writing and unescapes them when reading those CSV log files.
   
   ## Motivation and Context
   
   As reported in #5770 and on the users mailing list, we currently do not escape new line characters when writing the CSV log file.
   
   
   ## How Has This Been Tested?
   
   Tested on a local test plan and add JUnit tests.
   
   
   ## Types of changes
   <!--- What types of changes does your code introduce? Delete as appropriate -->
   - Bug fix (non-breaking change which fixes an issue)
     But could also be seen as a new feature or even breaking change.
   
   ## Checklist:
   <!--- Go over all the following points, and put an `x` in all the boxes that apply. -->
   <!--- If you're unsure about any of these, don't hesitate to ask. We're here to help! -->
   - [x] My code follows the [code style][style-guide] of this project.
   - [ ] I have updated the documentation accordingly.
   
   [style-guide]: https://wiki.apache.org/jmeter/CodeStyleGuidelines
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@jmeter.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [jmeter] FSchumacher commented on pull request #5805: Escape control characters in CSV log file

Posted by "FSchumacher (via GitHub)" <gi...@apache.org>.
FSchumacher commented on PR #5805:
URL: https://github.com/apache/jmeter/pull/5805#issuecomment-1516489111

   I wonder, if we should use `StringEscapeUtils#escapeCsv(String)` from commons-text instead of our own implementation.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@jmeter.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [jmeter] vlsi commented on pull request #5805: Escape control characters in CSV log file

Posted by "vlsi (via GitHub)" <gi...@apache.org>.
vlsi commented on PR #5805:
URL: https://github.com/apache/jmeter/pull/5805#issuecomment-1554405145

   I think the escaping of `\r` and `\n` should not be done according to the RFC: https://github.com/apache/jmeter/issues/5770#issuecomment-1554402532


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@jmeter.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [jmeter] vlsi commented on a diff in pull request #5805: Escape control characters in CSV log file

Posted by "vlsi (via GitHub)" <gi...@apache.org>.
vlsi commented on code in PR #5805:
URL: https://github.com/apache/jmeter/pull/5805#discussion_r1198825273


##########
src/core/src/main/java/org/apache/jmeter/save/CSVSaveService.java:
##########
@@ -786,7 +785,13 @@ private void addDelim() {
         // quotes:
         public void append(String s) {
             addDelim();
-            sb.append(quoteDelimiters(s, specials));
+            sb.append(escapeControlChars(quoteDelimiters(s, specials)));
+        }
+
+        private static String escapeControlChars(String value) {
+            return value
+                    .replace("\n", "\\n")
+                    .replace("\r", "\\r");

Review Comment:
   This looks strange as `\n` and `\r` are already a part of `specials`, so they should already be replaced



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@jmeter.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org