CSVDataSet does not trim spaces in <filename>.csv

[asfimport]

Dzmitry Kashlach (Bug 56877):
CSVDataSet does not trim spaces in <filename>.csv
E.g. if user entered in text-box "<filename>.csv ", then filename will be set as
"<filename>.csv " but not "<filename>.csv".
I suggest simple patch for it.

Created attachment csv_dataset.patch: csv_dataset.patch

csv_dataset.patch

Index: src/components/org/apache/jmeter/config/CSVDataSet.java
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- src/components/org/apache/jmeter/config/CSVDataSet.java	(revision 25897c69d454701793f8506bb7680bb115190eb2)
+++ src/components/org/apache/jmeter/config/CSVDataSet.java	(revision )
@@ -226,7 +226,7 @@
      *            The filename to set.
      */
     public void setFilename(String filename) {
-        this.filename = filename;
+        this.filename = filename.trim();
     }
 
     /**

OS: All

[asfimport]

Andrey Pokhilko (migrated from Bugzilla):
Maybe this is better be fixed in org.apache.jmeter.services.FileServer class to have the effect everywhere.

[asfimport]

Dzmitry Kashlach (migrated from Bugzilla):
I've made some more research and tried to fixed this issue for all String properties wherever they occur.

Created attachment bug_56877.patch: TestBeanHelper&GenericTestBeanCustomizer

bug_56877.patch

Index: src/core/org/apache/jmeter/testbeans/TestBeanHelper.java
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- src/core/org/apache/jmeter/testbeans/TestBeanHelper.java	(date 1388842110000)
+++ src/core/org/apache/jmeter/testbeans/TestBeanHelper.java	(revision )
@@ -94,7 +94,7 @@
                 {
                     Method writeMethod = desc.getWriteMethod();
                     if (writeMethod!=null) {
-                        invokeOrBailOut(el, writeMethod, new Object[] {value});
+                        invokeOrBailOut(el, writeMethod, new Object[] {value instanceof String?((String) value).trim():value});
                     }
                 }
             }
Index: src/core/org/apache/jmeter/testbeans/gui/GenericTestBeanCustomizer.java
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- src/core/org/apache/jmeter/testbeans/gui/GenericTestBeanCustomizer.java	(date 1388842110000)
+++ src/core/org/apache/jmeter/testbeans/gui/GenericTestBeanCustomizer.java	(revision )
@@ -747,7 +747,7 @@
                         log.debug("Unset " + name);
                     }
                 } else {
-                    propertyMap.put(name, value);
+                    propertyMap.put(name, value instanceof String?((String) value).trim():value);
                     if (log.isDebugEnabled()) {
                         log.debug("Set " + name + "= " + value);
                     }

[asfimport]

@pmouawad (migrated from Bugzilla):
I think the fix of last patch is too coarse and impacting as it hits much more than simple path in Beans.

[asfimport]

Dzmitry Kashlach (migrated from Bugzilla):
Third variant is to fix CSVDataSet and TCLogParser, because they both reserve files using FilerServer. In this case patch may look like latest one in attachement.

[asfimport]

Dzmitry Kashlach (migrated from Bugzilla):
Created attachment bug_56877.patch: CSVDataSet&TCLogParser

bug_56877.patch

Index: src/components/org/apache/jmeter/config/CSVDataSet.java
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- src/components/org/apache/jmeter/config/CSVDataSet.java	(date 1388842110000)
+++ src/components/org/apache/jmeter/config/CSVDataSet.java	(revision )
@@ -156,7 +156,7 @@
             delim=",";
         }
         if (vars == null) {
-            String _fileName = getFilename();
+            String _fileName = getFilename().trim();
             String mode = getShareMode();
             int modeInt = CSVDataSetBeanInfo.getShareModeAsInt(mode);
             switch(modeInt){
Index: src/protocol/http/org/apache/jmeter/protocol/http/util/accesslog/TCLogParser.java
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- src/protocol/http/org/apache/jmeter/protocol/http/util/accesslog/TCLogParser.java	(date 1388842110000)
+++ src/protocol/http/org/apache/jmeter/protocol/http/util/accesslog/TCLogParser.java	(revision )
@@ -183,7 +183,7 @@
      */
     @Override
     public void setSourceFile(String source) {
-        this.FILENAME = source;
+        this.FILENAME = source.trim();
     }
 
     /**
\ No newline at end of file

[asfimport]

@pmouawad (migrated from Bugzilla):
Hello,
Thanks for contribution.

To be clean, I think we should look at all elements that use FileNames and add trimming (not only to 2 elements).

Also, for cleanness, I suggest setter are not modified, only the code that uses fileName should trim it.

[asfimport]

Dzmitry Kashlach (migrated from Bugzilla):
(In reply to Philippe Mouawad from comment 6)

To be clean, I think we should look at all elements that use FileNames and
add trimming (not only to 2 elements).
Agree. But here is a little dilemma for me: either to fix two elements(which use FileServer, CSVDataSet&TCLogParser), or make deep fix, which you've already rejected(trimm all StringProperties in TestBeanHelper&GenericTestBeanCustomizer).
Do you know any intermediate variant?

Also, for cleanness, I suggest setter are not modified, only the code that uses >fileName should trim it.
I think, it could be better to trim filename higher than fileserver level, because filename is used also as alias. In this case we'll do far less changes in code.

[asfimport]

@pmouawad (migrated from Bugzilla):
Hi,
I rejected the trim on all StringProperties in TestBeanHelper&GenericTestBeanCustomizer as it would have affected also non filenames which may have side effects for existing plans.

The best thing IMHO is to review all components that have filenames and trim spaces for them.

It is more work but I think it is more coherent.
Regards

[asfimport]

Antonio Gomes Rodrigues (migrated from Bugzilla):
Hi,

I would like to know if you think it's need to be fixed?

Because have spaces at the begin or/and at the end of a file is authorized (tested in Windows + Linux), I don't know what it needed to do

Do you think the issue must be closed?

If not, I will split it in separate issue and fix them one by one

Antonio

[asfimport]

@pmouawad (migrated from Bugzilla):
Hi Antonio,
if you can take into account my previous comment, then yes a PR is welcome.

Regards