Hi Vincent,
On Mon, Mar 23, 2009 at 2:04 AM, Vincent Massol <[email protected]> wrote:
>
> On Mar 22, 2009, at 10:54 AM, asiri (SVN) wrote:
>
> > Author: asiri
> > Date: 2009-03-22 10:54:52 +0100 (Sun, 22 Mar 2009)
> > New Revision: 17895
> >
> > Added:
> > platform/core/trunk/xwiki-officeimporter/src/test/java/org/xwiki/
> > officeimporter/internal/OfficeImporterFileStorageTest.java
> > Modified:
> > platform/core/trunk/xwiki-officeimporter/src/main/java/org/xwiki/
> > officeimporter/internal/OfficeImporterFileStorage.java
> > Log:
> > XWIKI-3406: Office Importer not working under windows environments
> >
> > * Fixed the problem with invalid file name characters.
> >
> > Modified: platform/core/trunk/xwiki-officeimporter/src/main/java/org/
> > xwiki/officeimporter/internal/OfficeImporterFileStorage.java
> > ===================================================================
> > --- platform/core/trunk/xwiki-officeimporter/src/main/java/org/xwiki/
> > officeimporter/internal/OfficeImporterFileStorage.java 2009-03-22
> > 07:16:13 UTC (rev 17894)
> > +++ platform/core/trunk/xwiki-officeimporter/src/main/java/org/xwiki/
> > officeimporter/internal/OfficeImporterFileStorage.java 2009-03-22
> > 09:54:52 UTC (rev 17895)
> > @@ -50,13 +50,18 @@
> > private File outputFile = null;
> >
> > /**
> > + * A regular expression matching any invalid file name character.
>
> I think this comment should be improved as these characters are not
> invalid on all OSes AFAIK.
Fixed.
>
>
> > + */
> > + public static final String INVALID_FILE_NAME_CHARS = "[/\\\\:\\*
> > \\?\"<>|]";
> > +
> > + /**
> > * Default constructor.
> > *
> > * @param tempDirName name of the temporary files directory.
> > */
> > public OfficeImporterFileStorage(String tempDirName)
> > {
> > - tempDir = new File(System.getProperty("java.io.tmpdir"),
> > tempDirName);
> > + tempDir = new File(System.getProperty("java.io.tmpdir"),
> > tempDirName.replaceAll(INVALID_FILE_NAME_CHARS, "-"));
> > tempDir.mkdir();
>
> hmm, shouldn't you ensure these files are deleted on jvm exit?
> (using deleteOnExit())
>
I have the dedicated method OfficeImporterFileStorage:cleanUp() for cleaning
up the temporary storage. I use this to ensure that for each user for each
request, a temporary storage is created and then destroyed right after the
request is served. I can't wait for the jvm termination because these
temporary files can be large and i must make sure that multiple requests do
not use the same temporary directory for it's data.
>
> Also note that all temporary files should be created through a common
> generic xwiki api. This has been discussed last week on the xwiki dev
> list I think. We were wondering where to put it and ApplicationContext
> was discussed. Using java.io.tmpdir is not completely correct in a
> servlet environment for ex...
I Agree.
>
>
> >
> > inputFile = new File(tempDir, "input.tmp");
> > outputDir = new File(tempDir, "output");
> >
> > Added: platform/core/trunk/xwiki-officeimporter/src/test/java/org/
> > xwiki/officeimporter/internal/OfficeImporterFileStorageTest.java
> > ===================================================================
> > --- platform/core/trunk/xwiki-officeimporter/src/test/java/org/xwiki/
> > officeimporter/internal/
> > OfficeImporterFileStorageTest.java (rev 0)
> > +++ platform/core/trunk/xwiki-officeimporter/src/test/java/org/xwiki/
> > officeimporter/internal/OfficeImporterFileStorageTest.java
> > 2009-03-22 09:54:52 UTC (rev 17895)
> > @@ -0,0 +1,58 @@
> > +/*
> > + * See the NOTICE file distributed with this work for additional
> > + * information regarding copyright ownership.
> > + *
> > + * This is free software; you can redistribute it and/or modify it
> > + * under the terms of the GNU Lesser General Public License as
> > + * published by the Free Software Foundation; either version 2.1 of
> > + * the License, or (at your option) any later version.
> > + *
> > + * This software is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> > + * Lesser General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU Lesser General Public
> > + * License along with this software; if not, write to the Free
> > + * Software Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
> > + * 02110-1301 USA, or see the FSF site: http://www.fsf.org.
> > + */
> > +package org.xwiki.officeimporter.internal;
> > +
> > +import com.xpn.xwiki.test.AbstractXWikiComponentTestCase;
> > +
> > +/**
> > + * Test calss for {...@link OfficeImporterFileStorage}.
>
> typo.
Fixed.
>
>
> > + *
> > + * @version $Id$
> > + * @since 1.9M1
> > + */
> > +public class OfficeImporterFileStorageTest extends
> > AbstractXWikiComponentTestCase
> > +{
> > + /**
> > + * {...@inheritdoc}
> > + */
> > + protected void setUp() throws Exception
> > + {
> > +
> > getComponentManager
> > ().registerComponentDescriptor
> > (MockDocumentAccessBridge.getComponentDescriptor());
> > + super.setUp();
> > + }
> > +
> > + /**
> > + * Test filtering of invalid file name characters.
> > + */
> > + public void testInvalidFileNameCharacterFiltering()
> > + {
> > + String[] invalidCharacters = {"/", "\\", ":", "*", "?",
> > "\"", "<", ">", "|"};
> > + String filePath = "Temp%sDir";
> > + OfficeImporterFileStorage storage = null;
> > + try {
> > + for (String s : invalidCharacters) {
> > + storage = new
> > OfficeImporterFileStorage(String.format(filePath, s));
>
> why not create a single string name containing all chars?
Fixed.
>
>
> >
> > + assertEquals("Temp-Dir",
> > storage.getTempDir().getName());
> > + }
> > + } catch (RuntimeException ex) {
> > + fail(ex.getMessage());
>
> See previous mail on this anti pattern for junit tests.
Fixed.
Thanks.
- Asiri
_______________________________________________
devs mailing list
[email protected]
http://lists.xwiki.org/mailman/listinfo/devs
