camel-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Babak Vahdat <babak.vah...@swissonline.ch>
Subject Re: svn commit: r1407776 - in /camel/trunk/camel-core/src: main/java/org/apache/camel/component/properties/DefaultPropertiesResolver.java test/java/org/apache/camel/component/properties/PropertiesComponentLoadPropertiesFromFileTrimValuesTest.java
Date Sun, 11 Nov 2012 20:45:27 GMT


Am 11.11.12 13:58 schrieb "Claus Ibsen" unter <claus.ibsen@gmail.com>:

>On Sun, Nov 11, 2012 at 12:29 PM, Babak Vahdat
><babak.vahdat@swissonline.ch> wrote:
>>
>>
>> Am 11.11.12 11:25 schrieb "Claus Ibsen" unter <claus.ibsen@gmail.com>:
>>
>>>On Sat, Nov 10, 2012 at 1:12 PM, Babak Vahdat
>>><babak.vahdat@swissonline.ch> wrote:
>>>> Hi
>>>>
>>>> There is no need to remove the leading whitespaces (that's the space
>>>> character ' ') as java.uti.Properties does already that for free:
>>>>
>>>>
>>>>http://docs.oracle.com/javase/6/docs/api/java/util/Properties.html#load
>>>>(j
>>>>av
>>>> a.io.Reader)
>>>>
>>>> So that we should only take care of the tailing spaces (see also the
>>>> comment inside the commit).
>>>>
>>>> As this's a special case for "trimming logic" I don't see any good
>>>>reason
>>>> why we should pop it up into a XXXHelper utility class because (as
>>>>already
>>>> said) we just touch the tail of the String but *not* it's head.
>>>>
>>>> Thoughts?
>>>>
>>>
>>>I haven't looked but Camel's property placeholder supports loading
>>>properties from different sources.
>>>For example people can load from a database etc.
>>
>> Yes this for sure, but as far as I see there's no such a out-of-box
>> PropertiesResolver so that ppl will have to implement their own logic
>>for
>> this and then plug it in into their PropertiesComponent. Then they will
>> decide by their own if they *want* to cut the tailing spaces or not.
>>Just
>> to be clear neither JDK nor Spring does this. And in the meanwhile, by
>>my
>> concrete use case I've switched to Bridging which of course made the
>> problem to completely disappear as Spring doesn't cut the tailing spaces
>> inside the property files (so no need anymore for me to inject my own
>> PropertiesResolver into the PropertiesComponent).
>>
>>>
>>>Just wonder if the same code logic is used in such use-cases. If not
>>>then IMHO we should support the leading/trailing trims.
>>
>> As already said there's NO need to trim the leading spaces as there will
>> be no such spaces at all, the *contract* of java.util.Properties is to
>> trim them all already before Camel comes into the play (see the
>>Javadoc).
>>
>
>
>
>>>
>>>And you may at least consider moving the logic to a private method in
>>>the class so the code is nicely separated.
>>
>> We've got already that separation given through the strategy method
>> prepareLoadedProperties() being introduced by CAMEL-3565. So don't see
>>why
>> we should still define another level of the separation inside this
>> separation/strategy by prepareLoadedProperties() method, as trimming is
>> *exactly* what *this* level of separation is supposed to do. This would
>>be
>> an over-engineering for no good gain. So IMHO "lets keep things as
>>simple
>> as possible but not simpler".
>>
>
>Well adding 12 lines of low level String manipulation code inside an
>existing for loop (the 1st loop), makes it harder
>to read the code.
>
>Instead refactorting that code to a private method, makes it easier to
>read the overall intend, of the for looping
>of the properties (eg the 1st for loop).

Indeed this last reasoning of yours convinced me to "do refactor to a
separate method". And the corresponding tests/asserts were already added
by my original first commit of this ticket so that nothing changed there.

Babak

>
>And there is often bugs in String manipulation code, as its hard to
>get the indexing right, and whatnot.
>So having an util method with unit test helps ensure this properly, to
>have it test throughly.
>
>Code like this is easer to read (= only 1 for loop, and logic at higher
>level)
>
>    /**
>     * Strategy to prepare loaded properties before being used by Camel.
>     * <p/>
>     * This implementation will ensure values are trimmed, as loading
>properties from
>     * a file with values having trailing spaces is not automatic
>trimmed by the Properties API
>     * from the JDK.
>     *
>     * @param properties  the properties
>     * @return the prepared properties
>     */
>    protected Properties prepareLoadedProperties(Properties properties) {
>        Properties answer = new Properties();
>        for (Map.Entry<Object, Object> entry : properties.entrySet()) {
>            Object key = entry.getKey();
>            Object value = entry.getValue();
>            if (value instanceof String) {
>                String s = (String) value;
>                value = trimTrailingWhitespaces(s);
>            }
>            answer.put(key, value);
>        }
>        return answer;
>    }
>
>
>
>
>> Babak
>>
>>>If it should not be moved to a StringHelper in the util package.
>>>
>>>> Babak
>>>>
>>>> Am 10.11.12 12:59 schrieb "Claus Ibsen" unter <claus.ibsen@gmail.com>:
>>>>
>>>>>Hi
>>>>>
>>>>>I suggest to add an util method to StringHelper (or what we call it).
>>>>>And then add unit test for the trim method.
>>>>>
>>>>>Then it's easier to verify, and also re-use this method in other
>>>>>places.
>>>>>And mind that it should also remove leading whitespaces.
>>>>>
>>>>>
>>>>>
>>>>>On Sat, Nov 10, 2012 at 12:48 PM,  <bvahdat@apache.org> wrote:
>>>>>> Author: bvahdat
>>>>>> Date: Sat Nov 10 11:48:43 2012
>>>>>> New Revision: 1407776
>>>>>>
>>>>>> URL: http://svn.apache.org/viewvc?rev=1407776&view=rev
>>>>>> Log:
>>>>>> CAMEL-5784: Preparing the loaded properties should only trim any
>>>>>>potential whitespace characters.
>>>>>>
>>>>>> Modified:
>>>>>>
>>>>>>camel/trunk/camel-core/src/main/java/org/apache/camel/component/prope
>>>>>>rt
>>>>>>ie
>>>>>>s/DefaultPropertiesResolver.java
>>>>>>
>>>>>>camel/trunk/camel-core/src/test/java/org/apache/camel/component/prope
>>>>>>rt
>>>>>>ie
>>>>>>s/PropertiesComponentLoadPropertiesFromFileTrimValuesTest.java
>>>>>>
>>>>>> Modified:
>>>>>>camel/trunk/camel-core/src/main/java/org/apache/camel/component/prope
>>>>>>rt
>>>>>>ie
>>>>>>s/DefaultPropertiesResolver.java
>>>>>> URL:
>>>>>>http://svn.apache.org/viewvc/camel/trunk/camel-core/src/main/java/org
>>>>>>/a
>>>>>>pa
>>>>>>che/camel/component/properties/DefaultPropertiesResolver.java?rev=140
>>>>>>77
>>>>>>76
>>>>>>&r1=1407775&r2=1407776&view=diff
>>>>>>
>>>>>>=====================================================================
>>>>>>==
>>>>>>==
>>>>>>=====
>>>>>> ---
>>>>>>camel/trunk/camel-core/src/main/java/org/apache/camel/component/prope
>>>>>>rt
>>>>>>ie
>>>>>>s/DefaultPropertiesResolver.java (original)
>>>>>> +++
>>>>>>camel/trunk/camel-core/src/main/java/org/apache/camel/component/prope
>>>>>>rt
>>>>>>ie
>>>>>>s/DefaultPropertiesResolver.java Sat Nov 10 11:48:43 2012
>>>>>> @@ -131,11 +131,20 @@ public class DefaultPropertiesResolver i
>>>>>>          for (Map.Entry<Object, Object> entry :
>>>>>>properties.entrySet()) {
>>>>>>              Object key = entry.getKey();
>>>>>>              Object value = entry.getValue();
>>>>>> -            // trim string values which can be a problem when
>>>>>>loading
>>>>>>from a properties file and there
>>>>>> -            // is leading or trailing spaces in the value
>>>>>> +            // trim any trailing spaces which can be a problem when
>>>>>>loading from
>>>>>> +            // a properties file, note that java.util.Properties
>>>>>>does
>>>>>>already this
>>>>>> +            // for any potential leading spaces so there's nothing
>>>>>>to
>>>>>>do there
>>>>>>              if (value instanceof String) {
>>>>>>                  String s = (String) value;
>>>>>> -                s = s.trim();
>>>>>> +                int endIndex = s.length();
>>>>>> +                for (int index = s.length() - 1; index >= 0;
>>>>>>index--) {
>>>>>> +                    if (s.charAt(index) == ' ') {
>>>>>> +                        endIndex = index;
>>>>>> +                    } else {
>>>>>> +                        break;
>>>>>> +                    }
>>>>>> +                }
>>>>>> +                s = s.substring(0, endIndex);
>>>>>>                  value = s;
>>>>>>              }
>>>>>>              answer.put(key, value);
>>>>>>
>>>>>> Modified:
>>>>>>camel/trunk/camel-core/src/test/java/org/apache/camel/component/prope
>>>>>>rt
>>>>>>ie
>>>>>>s/PropertiesComponentLoadPropertiesFromFileTrimValuesTest.java
>>>>>> URL:
>>>>>>http://svn.apache.org/viewvc/camel/trunk/camel-core/src/test/java/org
>>>>>>/a
>>>>>>pa
>>>>>>che/camel/component/properties/PropertiesComponentLoadPropertiesFromF
>>>>>>il
>>>>>>eT
>>>>>>rimValuesTest.java?rev=1407776&r1=1407775&r2=1407776&view=diff
>>>>>>
>>>>>>=====================================================================
>>>>>>==
>>>>>>==
>>>>>>=====
>>>>>> ---
>>>>>>camel/trunk/camel-core/src/test/java/org/apache/camel/component/prope
>>>>>>rt
>>>>>>ie
>>>>>>s/PropertiesComponentLoadPropertiesFromFileTrimValuesTest.java
>>>>>>(original)
>>>>>> +++
>>>>>>camel/trunk/camel-core/src/test/java/org/apache/camel/component/prope
>>>>>>rt
>>>>>>ie
>>>>>>s/PropertiesComponentLoadPropertiesFromFileTrimValuesTest.java Sat
>>>>>>Nov
>>>>>>10 11:48:43 2012
>>>>>> @@ -16,7 +16,6 @@
>>>>>>   */
>>>>>>  package org.apache.camel.component.properties;
>>>>>>
>>>>>> -import java.io.File;
>>>>>>  import java.io.FileOutputStream;
>>>>>>
>>>>>>  import org.apache.camel.CamelContext;
>>>>>> @@ -39,15 +38,27 @@ public class PropertiesComponentLoadProp
>>>>>>          CamelContext context = super.createCamelContext();
>>>>>>
>>>>>>          // create space.properties file
>>>>>> -        File file = new File("target/space/space.properties");
>>>>>> -        file.createNewFile();
>>>>>> -        FileOutputStream fos = new FileOutputStream(file);
>>>>>> -        fos.write("cool.leading= Leading
>>>>>>space\ncool.trailing=Trailing
>>>>>>space \ncool.both= Both leading and trailing space ".getBytes());
>>>>>> +        FileOutputStream fos = new
>>>>>>FileOutputStream("target/space/space.properties");
>>>>>> +        String cool = "cool.leading= Leading space" + LS +
>>>>>>"cool.trailing=Trailing space " + LS + "cool.both= Both leading and
>>>>>>trailing space ";
>>>>>> +        fos.write(cool.getBytes());
>>>>>> +        fos.write(LS.getBytes());
>>>>>> +
>>>>>> +        String space = "space.leading=   \\r\\n" + LS +
>>>>>>"space.trailing=\\t   " + LS + "space.both=  \\r   \\t  \\n   ";
>>>>>> +        fos.write(space.getBytes());
>>>>>> +        fos.write(LS.getBytes());
>>>>>> +
>>>>>> +        String mixed = "mixed.leading=   Leading space\\r\\n" +
LS
>>>>>>+
>>>>>>"mixed.trailing=Trailing space\\t   " + LS + "mixed.both=  Both
>>>>>>leading
>>>>>>and trailing space\\r   \\t  \\n   ";
>>>>>> +        fos.write(mixed.getBytes());
>>>>>> +        fos.write(LS.getBytes());
>>>>>> +
>>>>>> +        String empty = "empty.line=
>>>>>>";
>>>>>> +        fos.write(empty.getBytes());
>>>>>> +
>>>>>>          fos.close();
>>>>>>
>>>>>>          PropertiesComponent pc = new PropertiesComponent();
>>>>>>          pc.setCamelContext(context);
>>>>>> -        pc.setLocations(new
>>>>>>String[]{"file:target/space/space.properties"});
>>>>>> +        pc.setLocation("file:target/space/space.properties");
>>>>>>          context.addComponent("properties", pc);
>>>>>>
>>>>>>          return context;
>>>>>> @@ -57,6 +68,16 @@ public class PropertiesComponentLoadProp
>>>>>>          assertEquals("Leading space",
>>>>>>context.resolvePropertyPlaceholders("{{cool.leading}}"));
>>>>>>          assertEquals("Trailing space",
>>>>>>context.resolvePropertyPlaceholders("{{cool.trailing}}"));
>>>>>>          assertEquals("Both leading and trailing space",
>>>>>>context.resolvePropertyPlaceholders("{{cool.both}}"));
>>>>>> +
>>>>>> +        assertEquals("\r\n",
>>>>>>context.resolvePropertyPlaceholders("{{space.leading}}"));
>>>>>> +        assertEquals("\t",
>>>>>>context.resolvePropertyPlaceholders("{{space.trailing}}"));
>>>>>> +        assertEquals("\r   \t  \n",
>>>>>>context.resolvePropertyPlaceholders("{{space.both}}"));
>>>>>> +
>>>>>> +        assertEquals("Leading space\r\n",
>>>>>>context.resolvePropertyPlaceholders("{{mixed.leading}}"));
>>>>>> +        assertEquals("Trailing space\t",
>>>>>>context.resolvePropertyPlaceholders("{{mixed.trailing}}"));
>>>>>> +        assertEquals("Both leading and trailing space\r   \t  \n",
>>>>>>context.resolvePropertyPlaceholders("{{mixed.both}}"));
>>>>>> +
>>>>>> +        assertEquals("",
>>>>>>context.resolvePropertyPlaceholders("{{empty.line}}"));
>>>>>>      }
>>>>>>
>>>>>>  }
>>>>>> \ No newline at end of file
>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>>
>>>>>--
>>>>>Claus Ibsen
>>>>>-----------------
>>>>>Red Hat, Inc.
>>>>>FuseSource is now part of Red Hat
>>>>>Email: cibsen@redhat.com
>>>>>Web: http://fusesource.com
>>>>>Twitter: davsclaus
>>>>>Blog: http://davsclaus.com
>>>>>Author of Camel in Action: http://www.manning.com/ibsen
>>>>
>>>>
>>>
>>>
>>>
>>>--
>>>Claus Ibsen
>>>-----------------
>>>Red Hat, Inc.
>>>FuseSource is now part of Red Hat
>>>Email: cibsen@redhat.com
>>>Web: http://fusesource.com
>>>Twitter: davsclaus
>>>Blog: http://davsclaus.com
>>>Author of Camel in Action: http://www.manning.com/ibsen
>>
>>
>
>
>
>-- 
>Claus Ibsen
>-----------------
>Red Hat, Inc.
>FuseSource is now part of Red Hat
>Email: cibsen@redhat.com
>Web: http://fusesource.com
>Twitter: davsclaus
>Blog: http://davsclaus.com
>Author of Camel in Action: http://www.manning.com/ibsen



Mime
View raw message