You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@commons.apache.org by jo...@apache.org on 2009/08/18 14:22:35 UTC
svn commit: r805384 -
/commons/proper/vfs/trunk/core/src/main/java/org/apache/commons/vfs/FileSystemOptions.java
Author: joehni
Date: Tue Aug 18 12:22:35 2009
New Revision: 805384
URL: http://svn.apache.org/viewvc?rev=805384&view=rev
Log:
Make FileSystemOptions cloneable (VFS-278).
Modified:
commons/proper/vfs/trunk/core/src/main/java/org/apache/commons/vfs/FileSystemOptions.java
Modified: commons/proper/vfs/trunk/core/src/main/java/org/apache/commons/vfs/FileSystemOptions.java
URL: http://svn.apache.org/viewvc/commons/proper/vfs/trunk/core/src/main/java/org/apache/commons/vfs/FileSystemOptions.java?rev=805384&r1=805383&r2=805384&view=diff
==============================================================================
--- commons/proper/vfs/trunk/core/src/main/java/org/apache/commons/vfs/FileSystemOptions.java (original)
+++ commons/proper/vfs/trunk/core/src/main/java/org/apache/commons/vfs/FileSystemOptions.java Tue Aug 18 12:22:35 2009
@@ -29,7 +29,7 @@
* @see org.apache.commons.vfs.provider.sftp.SftpFileSystemConfigBuilder
* @see org.apache.commons.vfs.provider.ftp.FtpFileSystemConfigBuilder
*/
-public class FileSystemOptions
+public class FileSystemOptions implements Cloneable
{
/** The options */
private Map options = new TreeMap();
@@ -161,5 +161,14 @@
// TODO: compare Entry by Entry
return 0;
}
+
+ /**
+ * {@inheritDoc}
+ */
+ public Object clone() {
+ FileSystemOptions clone = new FileSystemOptions();
+ clone.options = new TreeMap(options);
+ return clone;
+ }
}
Re: svn commit: r805384 - /commons/proper/vfs/trunk/core/src/main/java/org/apache/commons/vfs/FileSystemOptions.java
Posted by Ralph Goers <ra...@dslextreme.com>.
On Aug 18, 2009, at 11:07 PM, Mario Ivankovits wrote:
>
> Hi!
>
>>> IF you use the options in a way which allows you to know which kind
>>> of filesystem will be used - good - but then the builder is just
>>> another layer, but not awkward and brittle I think.
>>
>> No. They are awkward and brittle. WebDavFileSystemConfigBuilder
>> extends HttpFileSystemConfigBuilder. The getInstance method has to
>> return an HttpFileSystemConfigBuilder since you can't change the
>> return value of an overridden method.
>
> Probably it is time to upgrade the minor jdk version for VFS-2.0.
> Starting with JDK 1.5 you can change the return type of an
> overridden method. It is called, covariant return types.
> http://www.java-tips.org/java-se-tips/java.lang/covariant-return-types.html
>
> Probably long overdue and an easy fix then.
I'm aware of that but I wouldn't be so bold as to change the minimum
JDK version without looking for input from other users first. Frankly,
my organization has switched to JDK 6 (which has some other nice
features), but I know there are a lot of users who haven't gone to that.
I would love to introduce generics and for each loops where appropriate.
However, we should be fairly close to a 2.0 release. I've gotten
checkstyle down to about 1,000 errors and can easily get it down to
about 500. Unfortunately, the BZip classes have lots of constants in
them that cause quite a few checkstyle errors and I really don't want
to mess with it. I believe there are a few other issues outstanding
that should be fixed but I'm pretty happy with the way VFS is working
for me. If we just change the minimum version but don't make many code
changes to take advantage of it until 2.1 that would be OK with me.
>
>
>>> FileSystemOption then can be a concrete instance of a set of
>>> configurations for one specific filesystem, so you might have
>>> HttpFileSystemOption, SftpFileSystemOption etc. Each of them holding
>>> all possible filesystem options.
>>
>> I actually like this a lot more than the current implementation.
>> However, it wouldn't allow the get and set methods to be exposed at
>> compile time on the root FileSystemOptions class. I think that can be
>> worked around pretty easily.
>
> Yes, if we consistently name the new subclasses, we can instantiate
> these and reflect the getter/setter then.
>
I was thinking along the same lines.
Ralph
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
RE: svn commit: r805384 -
/commons/proper/vfs/trunk/core/src/main/java/org/apache/commons/vfs/FileSystemOptions.java
Posted by Mario Ivankovits <ma...@ops.co.at>.
Hi!
> > IF you use the options in a way which allows you to know which kind
> > of filesystem will be used - good - but then the builder is just
> > another layer, but not awkward and brittle I think.
>
> No. They are awkward and brittle. WebDavFileSystemConfigBuilder
> extends HttpFileSystemConfigBuilder. The getInstance method has to
> return an HttpFileSystemConfigBuilder since you can't change the
> return value of an overridden method.
Probably it is time to upgrade the minor jdk version for VFS-2.0.
Starting with JDK 1.5 you can change the return type of an overridden method. It is called, covariant return types.
http://www.java-tips.org/java-se-tips/java.lang/covariant-return-types.html
Probably long overdue and an easy fix then.
> > FileSystemOption then can be a concrete instance of a set of
> > configurations for one specific filesystem, so you might have
> > HttpFileSystemOption, SftpFileSystemOption etc. Each of them holding
> > all possible filesystem options.
>
> I actually like this a lot more than the current implementation.
> However, it wouldn't allow the get and set methods to be exposed at
> compile time on the root FileSystemOptions class. I think that can be
> worked around pretty easily.
Yes, if we consistently name the new subclasses, we can instantiate these and reflect the getter/setter then.
Ciao,
Mario
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: svn commit: r805384 - /commons/proper/vfs/trunk/core/src/main/java/org/apache/commons/vfs/FileSystemOptions.java
Posted by Ralph Goers <ra...@dslextreme.com>.
On Aug 18, 2009, at 10:18 AM, Mario Ivankovits wrote:
> Hi!
>
>> From personal experience, I've found working with it
>> to be awkward and brittle. I would much prefer to have each provider
>> subclass FileSystemOptions and provide the getters and setters
>> there. Then,
>> at least, you could do an instanceof on the FileSystemOptions and
>> determine
>> what options are actually supposed to be there.
>
> Look, The FileSystemOptions holds options for various filesystem
> implementations - at the same time!
> The reason for this is, that, given the url is provided by the user,
> you simply do not know which real implementation will be used.
> Thus, you have to provide e.g. ftp settings and sftp settings at the
> same time and let VFS decide which options to use - depending on the
> URL provided by the user.
>
> Why would you like to pass in an option-set using a concrete config
> class if the URL is anything else then "concrete".
> Also, a layered filesystem might require different settings e.g.
> compression-level of a zip-file on an ftp share.
>
> This might require you to configure the zip filesystem (compression)
> AND the ftp filesystem (active/passive mode)
>
> Also, the idea was to implement some sort of
> GlobalFileSystemOptions, the filesystem implementation then does not
> need to be changed, just the FileSystemOptions have to take care of
> that.
>
>
> IF you use the options in a way which allows you to know which kind
> of filesystem will be used - good - but then the builder is just
> another layer, but not awkward and brittle I think.
No. They are awkward and brittle. WebDavFileSystemConfigBuilder
extends HttpFileSystemConfigBuilder. The getInstance method has to
return an HttpFileSystemConfigBuilder since you can't change the
return value of an overridden method. So code calling getInstance has
to then cast it to the correct type to use it. Of course, The name of
the method could have been changed but then it would be instantiated
differently then every other ConfigBuilder.
>
>
> I wont say that there aren't other ways to solve that, but using
> simple inheritance and instanceof is not the correct way.
>
> Hmmm ... what I can think of is to refactor things that way:
>
> * FileSystemOptions holds just a map of configurations like
> Map<Class, FileSystemOption>
> * FileSystemOptions.set(Class vfsFilesystemClass, FileSystemOption
> options)
>
> FileSystemOption then can be a concrete instance of a set of
> configurations for one specific filesystem, so you might have
> HttpFileSystemOption, SftpFileSystemOption etc. Each of them holding
> all possible filesystem options.
I actually like this a lot more than the current implementation.
However, it wouldn't allow the get and set methods to be exposed at
compile time on the root FileSystemOptions class. I think that can be
worked around pretty easily.
>
> Sure, this completely breaks backward compatibility - and the
> GlobalFileSystemOptions thing needs to be solved somehow.
The ConfigBuilders can be deprecated and I'm pretty sure they can be
made to interact with this structure fairly easily.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: svn commit: r805384 - /commons/proper/vfs/trunk/core/src/main/java/org/apache/commons/vfs/FileSystemOptions.java
Posted by Jörg Schaible <jo...@gmx.de>.
Ralph Goers wrote at Mittwoch, 19. August 2009 09:16:
>
> On Aug 18, 2009, at 11:29 PM, Jörg Schaible wrote:
>
>> Hi Ralph,
>>
>> Ralph Goers wrote at Mittwoch, 19. August 2009 00:54:
>>
>>>
>>> On Aug 18, 2009, at 12:48 PM, Jörg Schaible wrote:
>>>
>>>> Hi Mario,
>>>>
>>>> Mario Ivankovits wrote:
>>>>
>>>>> I wont say that there aren't other ways to solve that, but using
>>>>> simple
>>>>> inheritance and instanceof is not the correct way.
>>>>
>>>> +1
>>>>
>>>> The current solution is somewhat unconventional, but this way you
>>>> always
>>>> know exactly which options are available for your FS.
>>>
>>> That argument doesn't fly. You'd get that from inheritance too.
>>>
>>>>
>>>>> Hmmm ... what I can think of is to refactor things that way:
>>>>>
>>>>> * FileSystemOptions holds just a map of configurations like
>>>>> Map<Class,
>>>>> FileSystemOption> * FileSystemOptions.set(Class vfsFilesystemClass,
>>>>> FileSystemOption options)
>>>>>
>>>>> FileSystemOption then can be a concrete instance of a set of
>>>>> configurations for one specific filesystem, so you might have
>>>>> HttpFileSystemOption, SftpFileSystemOption etc. Each of them
>>>>> holding all
>>>>> possible filesystem options.
>>>>>
>>>>> Sure, this completely breaks backward compatibility - and the
>>>>> GlobalFileSystemOptions thing needs to be solved somehow.
>>>>
>>>> This already exists with the DefaultFileSystemConfigBuilder that
>>>> provides
>>>> the UserAuthenticator. We actually derived from this class
>>>
>>> which class? DefaultFileSystemConfigBuilder or UserAuthenticator?
>>
>> Some code helps more that a 1000 words:
>>
>> =============== %< ====================
>> public class VFSConfigBuilder extends DefaultFileSystemConfigBuilder
>> {
>> private static final VFSConfigBuilder BUILDER = new
>> VFSConfigBuilder();
>>
>> public static VFSConfigBuilder getInstance()
>> {
>> return BUILDER;
>> }
>>
>> public void setId(final FileSystemOptions opts, final String id)
>> throws
>> FileSystemException
>> {
>> setParam(opts, "id", id);
>> }
>>
>> @Override
>> protected Class<? extends FileSystemConfigBuilder> getConfigClass()
>> {
>> return VFSConfigBuilder.class;
>> }
>> }
>> =============== %< ====================
>>
>> And despite your comment to Mario, I can overload the static method
>> with a
>> new return type. Welcome to Java 5 :) Maybe we should switch for VFS
>> 2.0
>> anyway ...
>
> Apples and Oranges. The current minimum version is JDK 1.4. Since
> WebDav is part of VFS it has to abide by that. But yes, if we change
> the minimum JDK then that can be done.
Actually I spoke too soon. To make the implementation above work, I have to
replace the INSTANCE of the DefaultFileSystemConfigBuilder, since VFS
internally uses DefaultFileSystemConfigBuilder.getInstance() to access the
UserAuthenticator. It does not help me if I derive from that config
builder :-/
So it seems that you've been right looking for an alternative
implementation ...
[snip]
> Yes, I'm familiar with that code. I was actually looking at it because
> I had concerns that there could be problems with multiple users trying
> to access the same file at the same time. Because of the code above
> the issues I was worried about can't happen.
>
> With the current design closing a FileSystem in a multithreaded system
> cannot be done safely. There are race conditions that currently cannot
> be avoided. SoftRefFilesCache used to call close and I commented it
> out for that reason. Actually though, closing the FileSystem shouldn't
> strictly be necessary to manage the connections. For example the Http
> Provider uses a pool of connections. The pool size can certainly be
> reduced and managed such that the connections are closed.
Yes, I also looked into that part, but I could not find a way to decouple
this for SFTP from the outside. Current implementation of the SFTP provider
ties the FS to the connection.
> I recently
> had to implement a Provider to access a Subversion repository
> (unfortunately, it can't reside at Apache due to the SVNKit license).
> I ended up having to have each FileObject establish its own connection
> to the repository. Perhaps the FTP provider(s) need to do something
> similar.
Definitely.
- Jörg
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: svn commit: r805384 - /commons/proper/vfs/trunk/core/src/main/java/org/apache/commons/vfs/FileSystemOptions.java
Posted by Ralph Goers <ra...@dslextreme.com>.
On Aug 18, 2009, at 11:29 PM, Jörg Schaible wrote:
> Hi Ralph,
>
> Ralph Goers wrote at Mittwoch, 19. August 2009 00:54:
>
>>
>> On Aug 18, 2009, at 12:48 PM, Jörg Schaible wrote:
>>
>>> Hi Mario,
>>>
>>> Mario Ivankovits wrote:
>>>
>>>> I wont say that there aren't other ways to solve that, but using
>>>> simple
>>>> inheritance and instanceof is not the correct way.
>>>
>>> +1
>>>
>>> The current solution is somewhat unconventional, but this way you
>>> always
>>> know exactly which options are available for your FS.
>>
>> That argument doesn't fly. You'd get that from inheritance too.
>>
>>>
>>>> Hmmm ... what I can think of is to refactor things that way:
>>>>
>>>> * FileSystemOptions holds just a map of configurations like
>>>> Map<Class,
>>>> FileSystemOption> * FileSystemOptions.set(Class vfsFilesystemClass,
>>>> FileSystemOption options)
>>>>
>>>> FileSystemOption then can be a concrete instance of a set of
>>>> configurations for one specific filesystem, so you might have
>>>> HttpFileSystemOption, SftpFileSystemOption etc. Each of them
>>>> holding all
>>>> possible filesystem options.
>>>>
>>>> Sure, this completely breaks backward compatibility - and the
>>>> GlobalFileSystemOptions thing needs to be solved somehow.
>>>
>>> This already exists with the DefaultFileSystemConfigBuilder that
>>> provides
>>> the UserAuthenticator. We actually derived from this class
>>
>> which class? DefaultFileSystemConfigBuilder or UserAuthenticator?
>
> Some code helps more that a 1000 words:
>
> =============== %< ====================
> public class VFSConfigBuilder extends DefaultFileSystemConfigBuilder
> {
> private static final VFSConfigBuilder BUILDER = new
> VFSConfigBuilder();
>
> public static VFSConfigBuilder getInstance()
> {
> return BUILDER;
> }
>
> public void setId(final FileSystemOptions opts, final String id)
> throws
> FileSystemException
> {
> setParam(opts, "id", id);
> }
>
> @Override
> protected Class<? extends FileSystemConfigBuilder> getConfigClass()
> {
> return VFSConfigBuilder.class;
> }
> }
> =============== %< ====================
>
> And despite your comment to Mario, I can overload the static method
> with a
> new return type. Welcome to Java 5 :) Maybe we should switch for VFS
> 2.0
> anyway ...
Apples and Oranges. The current minimum version is JDK 1.4. Since
WebDav is part of VFS it has to abide by that. But yes, if we change
the minimum JDK then that can be done.
>
>>> to provide an
>>> additional id as system option - simply to control which FileSystem
>>> instances are shared (hashCode of the FileSystemOptions is part of
>>> this
>>> decision).
>>
>> I'm not sure I understand this. Sample code would help. But I would
>> imagine something could be put in place to control which file systems
>> can be combined using the method Mario proposed above.
>
> A snippet from the AbstractFileProvider:
>
> =============== %< ====================
> /**
> * Adds a file system to those cached by this provider. The file
> system
> * may implement {@link VfsComponent}, in which case it is
> initialised.
> */
> protected void addFileSystem(final Comparable key, final
> FileSystem fs)
> throws FileSystemException
> {
> // Add to the cache
> addComponent(fs);
>
> FileSystemKey treeKey = new FileSystemKey(key,
> fs.getFileSystemOptions());
> ((AbstractFileSystem) fs).setCacheKey(treeKey);
>
> synchronized (this)
> {
> fileSystems.put(treeKey, fs);
> }
> }
>
> /**
> * Locates a cached file system
> *
> * @return The provider, or null if it is not cached.
> */
> protected FileSystem findFileSystem(final Comparable key, final
> FileSystemOptions fileSystemProps)
> {
> FileSystemKey treeKey = new FileSystemKey(key,
> fileSystemProps);
>
> synchronized (this)
> {
> return (FileSystem) fileSystems.get(treeKey);
> }
> }
> =============== %< ====================
>
> The cache is also triggered by the FileSystemOptions since it is
> part of the
> FileSystemKey. However, we use VFS in a JEE environment where we
> must have
> some more control over the allocated resources. Currently we cannot
> close a
> FileSystem, since it can be used in another thread/session.
> Unfortunately
> we're faced now with the effect that over time VFS eats up any
> connection
> to our SFTP server and never releases the connections. Since we use
> VFS to
> perform some kind of batch process, we could easily release anything
> related to this batch job right after it is done. Therefore we'd
> prefer a
> controlled behaviour for this kind of caching.
>
Yes, I'm familiar with that code. I was actually looking at it because
I had concerns that there could be problems with multiple users trying
to access the same file at the same time. Because of the code above
the issues I was worried about can't happen.
With the current design closing a FileSystem in a multithreaded system
cannot be done safely. There are race conditions that currently cannot
be avoided. SoftRefFilesCache used to call close and I commented it
out for that reason. Actually though, closing the FileSystem shouldn't
strictly be necessary to manage the connections. For example the Http
Provider uses a pool of connections. The pool size can certainly be
reduced and managed such that the connections are closed. I recently
had to implement a Provider to access a Subversion repository
(unfortunately, it can't reside at Apache due to the SVNKit license).
I ended up having to have each FileObject establish its own connection
to the repository. Perhaps the FTP provider(s) need to do something
similar.
Ralph
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: svn commit: r805384 - /commons/proper/vfs/trunk/core/src/main/java/org/apache/commons/vfs/FileSystemOptions.java
Posted by Jörg Schaible <jo...@gmx.de>.
Hi Ralph,
Ralph Goers wrote at Mittwoch, 19. August 2009 00:54:
>
> On Aug 18, 2009, at 12:48 PM, Jörg Schaible wrote:
>
>> Hi Mario,
>>
>> Mario Ivankovits wrote:
>>
>>> I wont say that there aren't other ways to solve that, but using
>>> simple
>>> inheritance and instanceof is not the correct way.
>>
>> +1
>>
>> The current solution is somewhat unconventional, but this way you
>> always
>> know exactly which options are available for your FS.
>
> That argument doesn't fly. You'd get that from inheritance too.
>
>>
>>> Hmmm ... what I can think of is to refactor things that way:
>>>
>>> * FileSystemOptions holds just a map of configurations like
>>> Map<Class,
>>> FileSystemOption> * FileSystemOptions.set(Class vfsFilesystemClass,
>>> FileSystemOption options)
>>>
>>> FileSystemOption then can be a concrete instance of a set of
>>> configurations for one specific filesystem, so you might have
>>> HttpFileSystemOption, SftpFileSystemOption etc. Each of them
>>> holding all
>>> possible filesystem options.
>>>
>>> Sure, this completely breaks backward compatibility - and the
>>> GlobalFileSystemOptions thing needs to be solved somehow.
>>
>> This already exists with the DefaultFileSystemConfigBuilder that
>> provides
>> the UserAuthenticator. We actually derived from this class
>
> which class? DefaultFileSystemConfigBuilder or UserAuthenticator?
Some code helps more that a 1000 words:
=============== %< ====================
public class VFSConfigBuilder extends DefaultFileSystemConfigBuilder
{
private static final VFSConfigBuilder BUILDER = new VFSConfigBuilder();
public static VFSConfigBuilder getInstance()
{
return BUILDER;
}
public void setId(final FileSystemOptions opts, final String id) throws
FileSystemException
{
setParam(opts, "id", id);
}
@Override
protected Class<? extends FileSystemConfigBuilder> getConfigClass()
{
return VFSConfigBuilder.class;
}
}
=============== %< ====================
And despite your comment to Mario, I can overload the static method with a
new return type. Welcome to Java 5 :) Maybe we should switch for VFS 2.0
anyway ...
>> to provide an
>> additional id as system option - simply to control which FileSystem
>> instances are shared (hashCode of the FileSystemOptions is part of
>> this
>> decision).
>
> I'm not sure I understand this. Sample code would help. But I would
> imagine something could be put in place to control which file systems
> can be combined using the method Mario proposed above.
A snippet from the AbstractFileProvider:
=============== %< ====================
/**
* Adds a file system to those cached by this provider. The file system
* may implement {@link VfsComponent}, in which case it is initialised.
*/
protected void addFileSystem(final Comparable key, final FileSystem fs)
throws FileSystemException
{
// Add to the cache
addComponent(fs);
FileSystemKey treeKey = new FileSystemKey(key,
fs.getFileSystemOptions());
((AbstractFileSystem) fs).setCacheKey(treeKey);
synchronized (this)
{
fileSystems.put(treeKey, fs);
}
}
/**
* Locates a cached file system
*
* @return The provider, or null if it is not cached.
*/
protected FileSystem findFileSystem(final Comparable key, final
FileSystemOptions fileSystemProps)
{
FileSystemKey treeKey = new FileSystemKey(key, fileSystemProps);
synchronized (this)
{
return (FileSystem) fileSystems.get(treeKey);
}
}
=============== %< ====================
The cache is also triggered by the FileSystemOptions since it is part of the
FileSystemKey. However, we use VFS in a JEE environment where we must have
some more control over the allocated resources. Currently we cannot close a
FileSystem, since it can be used in another thread/session. Unfortunately
we're faced now with the effect that over time VFS eats up any connection
to our SFTP server and never releases the connections. Since we use VFS to
perform some kind of batch process, we could easily release anything
related to this batch job right after it is done. Therefore we'd prefer a
controlled behaviour for this kind of caching.
- Jörg
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: svn commit: r805384 - /commons/proper/vfs/trunk/core/src/main/java/org/apache/commons/vfs/FileSystemOptions.java
Posted by Ralph Goers <ra...@dslextreme.com>.
On Aug 18, 2009, at 12:48 PM, Jörg Schaible wrote:
> Hi Mario,
>
> Mario Ivankovits wrote:
>
>> I wont say that there aren't other ways to solve that, but using
>> simple
>> inheritance and instanceof is not the correct way.
>
> +1
>
> The current solution is somewhat unconventional, but this way you
> always
> know exactly which options are available for your FS.
That argument doesn't fly. You'd get that from inheritance too.
>
>> Hmmm ... what I can think of is to refactor things that way:
>>
>> * FileSystemOptions holds just a map of configurations like
>> Map<Class,
>> FileSystemOption> * FileSystemOptions.set(Class vfsFilesystemClass,
>> FileSystemOption options)
>>
>> FileSystemOption then can be a concrete instance of a set of
>> configurations for one specific filesystem, so you might have
>> HttpFileSystemOption, SftpFileSystemOption etc. Each of them
>> holding all
>> possible filesystem options.
>>
>> Sure, this completely breaks backward compatibility - and the
>> GlobalFileSystemOptions thing needs to be solved somehow.
>
> This already exists with the DefaultFileSystemConfigBuilder that
> provides
> the UserAuthenticator. We actually derived from this class
which class? DefaultFileSystemConfigBuilder or UserAuthenticator?
> to provide an
> additional id as system option - simply to control which FileSystem
> instances are shared (hashCode of the FileSystemOptions is part of
> this
> decision).
>
I'm not sure I understand this. Sample code would help. But I would
imagine something could be put in place to control which file systems
can be combined using the method Mario proposed above.
But you guys are giving me ideas on things to try.
Ralph
RE: svn commit: r805384 - /commons/proper/vfs/trunk/core/src/main/java/org/apache/commons/vfs/FileSystemOptions.java
Posted by Jörg Schaible <jo...@gmx.de>.
Hi Mario,
Mario Ivankovits wrote:
> Hi!
>
>> From personal experience, I've found working with it
>> to be awkward and brittle. I would much prefer to have each provider
>> subclass FileSystemOptions and provide the getters and setters there.
>> Then, at least, you could do an instanceof on the FileSystemOptions and
>> determine what options are actually supposed to be there.
>
> Look, The FileSystemOptions holds options for various filesystem
> implementations - at the same time! The reason for this is, that, given
> the url is provided by the user, you simply do not know which real
> implementation will be used. Thus, you have to provide e.g. ftp settings
> and sftp settings at the same time and let VFS decide which options to use
> - depending on the URL provided by the user.
>
> Why would you like to pass in an option-set using a concrete config class
> if the URL is anything else then "concrete". Also, a layered filesystem
> might require different settings e.g. compression-level of a zip-file on
> an ftp share.
>
> This might require you to configure the zip filesystem (compression) AND
> the ftp filesystem (active/passive mode)
>
> Also, the idea was to implement some sort of GlobalFileSystemOptions, the
> filesystem implementation then does not need to be changed, just the
> FileSystemOptions have to take care of that.
>
>
> IF you use the options in a way which allows you to know which kind of
> filesystem will be used - good - but then the builder is just another
> layer, but not awkward and brittle I think.
>
>
> I wont say that there aren't other ways to solve that, but using simple
> inheritance and instanceof is not the correct way.
+1
The current solution is somewhat unconventional, but this way you always
know exactly which options are available for your FS.
> Hmmm ... what I can think of is to refactor things that way:
>
> * FileSystemOptions holds just a map of configurations like Map<Class,
> FileSystemOption> * FileSystemOptions.set(Class vfsFilesystemClass,
> FileSystemOption options)
>
> FileSystemOption then can be a concrete instance of a set of
> configurations for one specific filesystem, so you might have
> HttpFileSystemOption, SftpFileSystemOption etc. Each of them holding all
> possible filesystem options.
>
> Sure, this completely breaks backward compatibility - and the
> GlobalFileSystemOptions thing needs to be solved somehow.
This already exists with the DefaultFileSystemConfigBuilder that provides
the UserAuthenticator. We actually derived from this class to provide an
additional id as system option - simply to control which FileSystem
instances are shared (hashCode of the FileSystemOptions is part of this
decision).
- Jörg
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
RE: svn commit: r805384 -
/commons/proper/vfs/trunk/core/src/main/java/org/apache/commons/vfs/FileSystemOptions.java
Posted by Mario Ivankovits <ma...@ops.co.at>.
Hi!
> From personal experience, I've found working with it
> to be awkward and brittle. I would much prefer to have each provider
> subclass FileSystemOptions and provide the getters and setters there. Then,
> at least, you could do an instanceof on the FileSystemOptions and determine
> what options are actually supposed to be there.
Look, The FileSystemOptions holds options for various filesystem implementations - at the same time!
The reason for this is, that, given the url is provided by the user, you simply do not know which real implementation will be used.
Thus, you have to provide e.g. ftp settings and sftp settings at the same time and let VFS decide which options to use - depending on the URL provided by the user.
Why would you like to pass in an option-set using a concrete config class if the URL is anything else then "concrete".
Also, a layered filesystem might require different settings e.g. compression-level of a zip-file on an ftp share.
This might require you to configure the zip filesystem (compression) AND the ftp filesystem (active/passive mode)
Also, the idea was to implement some sort of GlobalFileSystemOptions, the filesystem implementation then does not need to be changed, just the FileSystemOptions have to take care of that.
IF you use the options in a way which allows you to know which kind of filesystem will be used - good - but then the builder is just another layer, but not awkward and brittle I think.
I wont say that there aren't other ways to solve that, but using simple inheritance and instanceof is not the correct way.
Hmmm ... what I can think of is to refactor things that way:
* FileSystemOptions holds just a map of configurations like Map<Class, FileSystemOption>
* FileSystemOptions.set(Class vfsFilesystemClass, FileSystemOption options)
FileSystemOption then can be a concrete instance of a set of configurations for one specific filesystem, so you might have HttpFileSystemOption, SftpFileSystemOption etc. Each of them holding all possible filesystem options.
Sure, this completely breaks backward compatibility - and the GlobalFileSystemOptions thing needs to be solved somehow.
Ciao,
Mario
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: svn commit: r805384 - /commons/proper/vfs/trunk/core/src/main/java/org/apache/commons/vfs/FileSystemOptions.java
Posted by "ralph.goers @dslextreme.com" <ra...@dslextreme.com>.
On Tue, Aug 18, 2009 at 7:48 AM, sebb <se...@gmail.com> wrote:
> On 18/08/2009, Jörg Schaible <jo...@gmx.de> wrote:
> > Hi Sebb,
> >
> > sebb wrote at Dienstag, 18. August 2009 14:48:
> >
> >
> > >> +
> > >> + /**
> > >> + * {@inheritDoc}
> > >> + */
> > >> + public Object clone() {
> > >> + FileSystemOptions clone = new FileSystemOptions();
> > >> + clone.options = new TreeMap(options);
> > >> + return clone;
> > >> + }
> > >
> > > This clone() does not call super.clone() so won't work properly for
> > > sub-classes.
> >
> >
> > I know, but since the key of the map is a private class anyway, it does
> not
> > make too much sense to derive from FileSystemOptions anyway ... don't
> you
> > think?
>
> In that case, you could make the class final.
>
> In any case, it would be helpful to document that the clone() method
> is not written to be sub-classable.
>
>
>
I actually don't agree that FileSystemOptions shouldn't be subclassed. I
have always disliked immensely having to manipulate FileSystemOptions using
a ConfigBuilder class. From personal experience, I've found working with it
to be awkward and brittle. I would much prefer to have each provider
subclass FileSystemOptions and provide the getters and setters there. Then,
at least, you could do an instanceof on the FileSystemOptions and determine
what options are actually supposed to be there.
Ralph
Re: svn commit: r805384 - /commons/proper/vfs/trunk/core/src/main/java/org/apache/commons/vfs/FileSystemOptions.java
Posted by Jörg Schaible <jo...@gmx.de>.
sebb wrote:
> On 18/08/2009, Jörg Schaible <jo...@gmx.de> wrote:
>> Hi Sebb,
>>
>> sebb wrote at Dienstag, 18. August 2009 14:48:
>>
>>
>> >> +
>> >> + /**
>> >> + * {@inheritDoc}
>> >> + */
>> >> + public Object clone() {
>> >> + FileSystemOptions clone = new FileSystemOptions();
>> >> + clone.options = new TreeMap(options);
>> >> + return clone;
>> >> + }
>> >
>> > This clone() does not call super.clone() so won't work properly for
>> > sub-classes.
>>
>>
>> I know, but since the key of the map is a private class anyway, it does
>> not
>> make too much sense to derive from FileSystemOptions anyway ... don't
>> you think?
>
> In that case, you could make the class final.
>
> In any case, it would be helpful to document that the clone() method
> is not written to be sub-classable.
You're right, it should be at least consistent. Committed.
- Jörg
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: svn commit: r805384 - /commons/proper/vfs/trunk/core/src/main/java/org/apache/commons/vfs/FileSystemOptions.java
Posted by sebb <se...@gmail.com>.
On 18/08/2009, Jörg Schaible <jo...@gmx.de> wrote:
> Hi Sebb,
>
> sebb wrote at Dienstag, 18. August 2009 14:48:
>
>
> >> +
> >> + /**
> >> + * {@inheritDoc}
> >> + */
> >> + public Object clone() {
> >> + FileSystemOptions clone = new FileSystemOptions();
> >> + clone.options = new TreeMap(options);
> >> + return clone;
> >> + }
> >
> > This clone() does not call super.clone() so won't work properly for
> > sub-classes.
>
>
> I know, but since the key of the map is a private class anyway, it does not
> make too much sense to derive from FileSystemOptions anyway ... don't you
> think?
In that case, you could make the class final.
In any case, it would be helpful to document that the clone() method
is not written to be sub-classable.
> - Jörg
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: svn commit: r805384 - /commons/proper/vfs/trunk/core/src/main/java/org/apache/commons/vfs/FileSystemOptions.java
Posted by Jörg Schaible <jo...@gmx.de>.
Hi Sebb,
sebb wrote at Dienstag, 18. August 2009 14:48:
>> +
>> + /**
>> + * {@inheritDoc}
>> + */
>> + public Object clone() {
>> + FileSystemOptions clone = new FileSystemOptions();
>> + clone.options = new TreeMap(options);
>> + return clone;
>> + }
>
> This clone() does not call super.clone() so won't work properly for
> sub-classes.
I know, but since the key of the map is a private class anyway, it does not
make too much sense to derive from FileSystemOptions anyway ... don't you
think?
- Jörg
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: svn commit: r805384 - /commons/proper/vfs/trunk/core/src/main/java/org/apache/commons/vfs/FileSystemOptions.java
Posted by sebb <se...@gmail.com>.
On 18/08/2009, joehni@apache.org <jo...@apache.org> wrote:
> Author: joehni
> Date: Tue Aug 18 12:22:35 2009
> New Revision: 805384
>
> URL: http://svn.apache.org/viewvc?rev=805384&view=rev
> Log:
> Make FileSystemOptions cloneable (VFS-278).
>
> Modified:
> commons/proper/vfs/trunk/core/src/main/java/org/apache/commons/vfs/FileSystemOptions.java
>
> Modified: commons/proper/vfs/trunk/core/src/main/java/org/apache/commons/vfs/FileSystemOptions.java
> URL: http://svn.apache.org/viewvc/commons/proper/vfs/trunk/core/src/main/java/org/apache/commons/vfs/FileSystemOptions.java?rev=805384&r1=805383&r2=805384&view=diff
> ==============================================================================
> --- commons/proper/vfs/trunk/core/src/main/java/org/apache/commons/vfs/FileSystemOptions.java (original)
> +++ commons/proper/vfs/trunk/core/src/main/java/org/apache/commons/vfs/FileSystemOptions.java Tue Aug 18 12:22:35 2009
> @@ -29,7 +29,7 @@
> * @see org.apache.commons.vfs.provider.sftp.SftpFileSystemConfigBuilder
> * @see org.apache.commons.vfs.provider.ftp.FtpFileSystemConfigBuilder
> */
> -public class FileSystemOptions
> +public class FileSystemOptions implements Cloneable
> {
> /** The options */
> private Map options = new TreeMap();
> @@ -161,5 +161,14 @@
> // TODO: compare Entry by Entry
> return 0;
> }
> +
> + /**
> + * {@inheritDoc}
> + */
> + public Object clone() {
> + FileSystemOptions clone = new FileSystemOptions();
> + clone.options = new TreeMap(options);
> + return clone;
> + }
This clone() does not call super.clone() so won't work properly for sub-classes.
> }
>
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org