FSA Refactor FsAdaptor for future MockFileDelegate testability.

FSA Refactor FsAdaptor for future MockFileDelegate testability.

This removes all direct file and directory manipulation from
FsAdaptor, moving it into the FileDelegate.  All access to
files and directories from FsAdaptor are now done via the
delegate.

A new NioFileDelegate was added, performing most direct file
and directory access using NIO Paths and Files classes.

WindowsFileDelegate extends NioFileDelegate, adding ACL and
Monitor support, I moved the only general purpose function,
getDocId(), into NioFileDelegate.

[ejona, 2013-12-06 05:29:37]

https://codereview.appspot.com/38140043/diff/1/src/com/google/enterprise/adap...
File src/com/google/enterprise/adaptor/fs/FileDelegate.java (right):

https://codereview.appspot.com/38140043/diff/1/src/com/google/enterprise/adap...
src/com/google/enterprise/adaptor/fs/FileDelegate.java:54: boolean isHidden(Path
doc) throws IOException;
What is this and isReadable doing here? They seem unused.

https://codereview.appspot.com/38140043/diff/1/src/com/google/enterprise/adap...
File src/com/google/enterprise/adaptor/fs/FsAdaptor.java (right):

https://codereview.appspot.com/38140043/diff/1/src/com/google/enterprise/adap...
src/com/google/enterprise/adaptor/fs/FsAdaptor.java:487: Set<Path> docs =
Sets.newHashSet();
Why change this?

https://codereview.appspot.com/38140043/diff/1/src/com/google/enterprise/adap...
File src/com/google/enterprise/adaptor/fs/NioFileDelegate.java (right):

https://codereview.appspot.com/38140043/diff/1/src/com/google/enterprise/adap...
src/com/google/enterprise/adaptor/fs/NioFileDelegate.java:31: public class
NioFileDelegate implements FileDelegate {
I know we talked about not having an AbstractFileDelegate, but it seems this
class should be abstract (but be NioFileDelegate, as named). Things like
getDfsUncActiveStorageUnc() are easy to miss and the
UnsupportedOperationException doesn't seem to help anybody.

https://codereview.appspot.com/38140043/diff/1/src/com/google/enterprise/adap...
src/com/google/enterprise/adaptor/fs/NioFileDelegate.java:31: public class
NioFileDelegate implements FileDelegate {
Can this be package-private?

https://codereview.appspot.com/38140043/diff/1/src/com/google/enterprise/adap...
src/com/google/enterprise/adaptor/fs/NioFileDelegate.java:31: public class
NioFileDelegate implements FileDelegate {
Some JavaDoc would be nice.

[ejona, 2013-12-06 06:59:38]

https://codereview.appspot.com/38140043/diff/1/src/com/google/enterprise/adap...
File src/com/google/enterprise/adaptor/fs/NioFileDelegate.java (right):

https://codereview.appspot.com/38140043/diff/1/src/com/google/enterprise/adap...
src/com/google/enterprise/adaptor/fs/NioFileDelegate.java:102: public DocId
newDocId(Path doc) throws IOException {
newDocId() (and assume that is what you meant instead of getDocId() in the
description), is not actually a general purpose method. It is Windows-only. If I
remember correctly, Miguel's intention was to have the Linux version not do \ to
/ replacements, and that is why this method got moved out of FsAdaptor into the
delegate in the first place.

[Brett, 2013-12-06 17:51:45]

https://codereview.appspot.com/38140043/diff/1/src/com/google/enterprise/adap...
File src/com/google/enterprise/adaptor/fs/FileDelegate.java (right):

https://codereview.appspot.com/38140043/diff/1/src/com/google/enterprise/adap...
src/com/google/enterprise/adaptor/fs/FileDelegate.java:54: boolean isHidden(Path
doc) throws IOException;
On 2013/12/06 05:29:37, ejona wrote:
> What is this and isReadable doing here? They seem unused.

Laying the groundwork for b/11753517 - don't index hidden files and folders.

https://codereview.appspot.com/38140043/diff/1/src/com/google/enterprise/adap...
File src/com/google/enterprise/adaptor/fs/FsAdaptor.java (right):

https://codereview.appspot.com/38140043/diff/1/src/com/google/enterprise/adap...
src/com/google/enterprise/adaptor/fs/FsAdaptor.java:487: Set<Path> docs =
Sets.newHashSet();
On 2013/12/06 05:29:37, ejona wrote:
> Why change this?

Sets.newHashSet() was already used above.
I was removing a bunch of unnecessary imports.
Changing this allowed me to drop import of HashSet.

https://codereview.appspot.com/38140043/diff/1/src/com/google/enterprise/adap...
File src/com/google/enterprise/adaptor/fs/NioFileDelegate.java (right):

https://codereview.appspot.com/38140043/diff/1/src/com/google/enterprise/adap...
src/com/google/enterprise/adaptor/fs/NioFileDelegate.java:31: public class
NioFileDelegate implements FileDelegate {
On 2013/12/06 05:29:37, ejona wrote:
> I know we talked about not having an AbstractFileDelegate, but it seems this
> class should be abstract (but be NioFileDelegate, as named). Things like
> getDfsUncActiveStorageUnc() are easy to miss and the
> UnsupportedOperationException doesn't seem to help anybody.

I went with stubbed functions because of your distaste for abstract classes.
Really I should label them as TODO, for doing the linux version.  If we have
to do funky non-nio stuff for linux, then perhaps make this abstract then?

[autocorrect keeps changing "nio" to "bio" or "nip" :( ]

https://codereview.appspot.com/38140043/diff/1/src/com/google/enterprise/adap...
src/com/google/enterprise/adaptor/fs/NioFileDelegate.java:31: public class
NioFileDelegate implements FileDelegate {
On 2013/12/06 05:29:37, ejona wrote:
> Can this be package-private?

Done.  WindowsFileDelegate as well.

https://codereview.appspot.com/38140043/diff/1/src/com/google/enterprise/adap...
src/com/google/enterprise/adaptor/fs/NioFileDelegate.java:31: public class
NioFileDelegate implements FileDelegate {
On 2013/12/06 05:29:37, ejona wrote:
> Some JavaDoc would be nice.

Done.

https://codereview.appspot.com/38140043/diff/1/src/com/google/enterprise/adap...
src/com/google/enterprise/adaptor/fs/NioFileDelegate.java:102: public DocId
newDocId(Path doc) throws IOException {
On 2013/12/06 06:59:38, ejona wrote:
> newDocId() (and assume that is what you meant instead of getDocId() in the
> description), is not actually a general purpose method. It is Windows-only. If
I
> remember correctly, Miguel's intention was to have the Linux version not do \
to
> / replacements, and that is why this method got moved out of FsAdaptor into
the
> delegate in the first place.

OK.  I restored this to WindowsFileDelegate and eliminated slash/backslash
flipping here.

[Brett, 2013-12-06 18:06:24]

Eric's feedback

[ejona, 2013-12-07 02:07:11]

Sending what responses I have. Normally I'd read over them again and such stuff,
but I have had no time today, thus sending what I have.

https://codereview.appspot.com/38140043/diff/1/src/com/google/enterprise/adap...
File src/com/google/enterprise/adaptor/fs/FileDelegate.java (right):

https://codereview.appspot.com/38140043/diff/1/src/com/google/enterprise/adap...
src/com/google/enterprise/adaptor/fs/FileDelegate.java:54: boolean isHidden(Path
doc) throws IOException;
On 2013/12/06 17:51:46, Brett wrote:
> On 2013/12/06 05:29:37, ejona wrote:
> > What is this and isReadable doing here? They seem unused.
> 
> Laying the groundwork for b/11753517 - don't index hidden files and folders.

Please don't do that here. It is close to impossible to notice changes like that
and review them properly. They should be added in a separate CL or even better,
the CL that fixes that bug.

If there was a technical reason why it was beneficial to do so now, that could
be okay. But I would need to be aware that changes like this can be mixed in,
and there doesn't seem to be any such benefit in this case.

https://codereview.appspot.com/38140043/diff/1/src/com/google/enterprise/adap...
File src/com/google/enterprise/adaptor/fs/FsAdaptor.java (right):

https://codereview.appspot.com/38140043/diff/1/src/com/google/enterprise/adap...
src/com/google/enterprise/adaptor/fs/FsAdaptor.java:487: Set<Path> docs =
Sets.newHashSet();
On 2013/12/06 17:51:46, Brett wrote:
> On 2013/12/06 05:29:37, ejona wrote:
> > Why change this?
> 
> Sets.newHashSet() was already used above.
> I was removing a bunch of unnecessary imports.
> Changing this allowed me to drop import of HashSet.

Which gains us very little and clutters the CL. This change is completely
separate from what you were trying to accomplish in this CL.

I don't see removing the import as being a very strong reason for the change.
You still depend on other things in that package, and HashSet is a very common
import. In this particular case, we can swap to using java.util.* import instead
of individual imports. But again, that wouldn't probably be appropriate in this
CL. But even so, using separate imports was a conscious decision by Miguel so
you should at least ping him for his opinion if you go that route.

https://codereview.appspot.com/38140043/diff/1/src/com/google/enterprise/adap...
File src/com/google/enterprise/adaptor/fs/NioFileDelegate.java (right):

https://codereview.appspot.com/38140043/diff/1/src/com/google/enterprise/adap...
src/com/google/enterprise/adaptor/fs/NioFileDelegate.java:31: public class
NioFileDelegate implements FileDelegate {
On 2013/12/06 17:51:46, Brett wrote:
> I went with stubbed functions because of your distaste for abstract classes.

Yeah, I sorta understood where the miscommunication happened. Basically,
abstract classes do have their place, but when we see Abstract* class names that
does raise a red flag. When you make an interface and then expect every
implementation to inherit common functionality from a base class, it is a code
smell.

In this case, we have our interface that is generic, and this class is
implementing some but not all of the methods in a very concrete matter: they use
NIO. This is not some base abstract class that every implementation of the
interface would use (tests for instance, certainly wouldn't).

Contrast this with AbstractAdaptor, which I've been killing slowly over time. It
was providing default implementations of various methods and included a main().
Some default implementations did nothing and for authz we had long discussions
on what it should do. In short, the problem was that these various things should
not have been part of the same interface, and the AbstractAdaptor was a symptom
of the real issue. We have since split most things out of AbstractAdaptor and
know how to kill it in the future.

And contrast that with Principal, which is abstract. We have UserPrincipal and
GroupPrincipal extend it and do almost no work themselves. This forms a really
nice hierarchy and it is important that Principal is a class and not an
interface, because we don't want arbitrary Principal implementations.

So again, we tend to get concerned when we see Abstract* classes, not
necessarily abstract classes.

> Really I should label them as TODO, for doing the linux version.  If we have
> to do funky non-nio stuff for linux, then perhaps make this abstract then?

We will get to do non-nio stuff for linux for the ACL stuff and
getDfsUncActiveStorageUnc(). That just leaves the path-monitoring stuff, which
we know will be different. It seems now is appropriate for abstract.

[mifern, 2013-12-07 02:40:33]

I think we should make NioFileDelegate abstract and avoid implementing stubs
that throw not implemented exceptions. I also feel we should avoid having to
call super in the WindowsFileDelegete which implies that NioFileDelegate needs
be designed with the idea of not maintaining any state or local member
variables. If we need to store some local state then it should be in the
extender of NioFileDelegate like WindowsFileDelegate.

https://codereview.appspot.com/38140043/diff/20001/src/com/google/enterprise/...
File src/com/google/enterprise/adaptor/fs/NioFileDelegate.java (right):

https://codereview.appspot.com/38140043/diff/20001/src/com/google/enterprise/...
src/com/google/enterprise/adaptor/fs/NioFileDelegate.java:36: class
NioFileDelegate implements FileDelegate {
This should be an abstract class and exclude the use of not implemented
exceptions. Making it abstract will force compile time errors instead of runtime
errors for future implementors like the LinuxFileDelegate.

https://codereview.appspot.com/38140043/diff/20001/src/com/google/enterprise/...
src/com/google/enterprise/adaptor/fs/NioFileDelegate.java:38: public
NioFileDelegate() {
There is no need for a constructor.

https://codereview.appspot.com/38140043/diff/20001/src/com/google/enterprise/...
src/com/google/enterprise/adaptor/fs/NioFileDelegate.java:57: public boolean
isHidden(Path doc) throws IOException {
This is also not used at the moment. Remove it.

https://codereview.appspot.com/38140043/diff/20001/src/com/google/enterprise/...
src/com/google/enterprise/adaptor/fs/NioFileDelegate.java:62: public boolean
isReadable(Path doc) throws IOException {
This is not used. Remove it from this CL and add it with the CL that adds the
implementation.

https://codereview.appspot.com/38140043/diff/20001/src/com/google/enterprise/...
src/com/google/enterprise/adaptor/fs/NioFileDelegate.java:67: public
BasicFileAttributes readAttributes(Path doc) throws IOException {
This should probably be readBasicAttributes since there will also be a
readExtendedAttributes possibly.

https://codereview.appspot.com/38140043/diff/20001/src/com/google/enterprise/...
src/com/google/enterprise/adaptor/fs/NioFileDelegate.java:94: throw new
UnsupportedOperationException("ACLs are not yet supported");
Remove the use of UnsupportedOperationException.

https://codereview.appspot.com/38140043/diff/20001/src/com/google/enterprise/...
src/com/google/enterprise/adaptor/fs/NioFileDelegate.java:110: public DocId
newDocId(Path doc) throws IOException {
Let's remove this for know. We have a Windows specific implementation of this at
the moment. We haven't decide what we will be doing for Linux, but I see this
being implemented by the LinuxFileDelegate.

https://codereview.appspot.com/38140043/diff/20001/src/com/google/enterprise/...
File src/com/google/enterprise/adaptor/fs/WindowsFileDelegate.java (right):

https://codereview.appspot.com/38140043/diff/20001/src/com/google/enterprise/...
src/com/google/enterprise/adaptor/fs/WindowsFileDelegate.java:63: super();
No need for super.

https://codereview.appspot.com/38140043/diff/20001/src/com/google/enterprise/...
src/com/google/enterprise/adaptor/fs/WindowsFileDelegate.java:339:
super.destroy();
No need for super.

[Brett, 2013-12-10 22:29:41]

Make NioFileDelegate abstract. Drop unused methods.

[ejona, 2013-12-11 01:50:40]

LGTM, assuming NioFileDelegate.getDfsUncActiveStorageUnc() is removed.

https://codereview.appspot.com/38140043/diff/40001/src/com/google/enterprise/...
File src/com/google/enterprise/adaptor/fs/NioFileDelegate.java (right):

https://codereview.appspot.com/38140043/diff/40001/src/com/google/enterprise/...
src/com/google/enterprise/adaptor/fs/NioFileDelegate.java:79: public Path
getDfsUncActiveStorageUnc(Path doc) throws IOException {
This looks like it shouldn't be implemented.

https://codereview.appspot.com/38140043/diff/40001/src/com/google/enterprise/...
File src/com/google/enterprise/adaptor/fs/WindowsFileDelegate.java (right):

https://codereview.appspot.com/38140043/diff/40001/src/com/google/enterprise/...
src/com/google/enterprise/adaptor/fs/WindowsFileDelegate.java:109: Path realPath
= doc.toRealPath(LinkOption.NOFOLLOW_LINKS);
Okay, so you changed the implementation here to use nio and
FileDelegate.isDirectory()

[Brett, 2013-12-18 19:08:21]

Committed 11 December 2013 to File System Adaptor