FSA Fix b/15667934: Removed files log The path ... is not a supported file type.

Files that were moved or deleted would log a message claiming the file
is not a supported file type. This is because FsAdaptor.isSupportedPath()
returns false because the non-existent file is neither a directory, nor
a regular file.

Reading the BasicFileAttributes on a non-existent file throws an
exception. So I catch the various not found exceptions, and
moved the call to isSupportedPath() to after the attributes are fetched.

Moving the call to isSupportedPath() down also fixed a broken FsAdaptor
test, so I removed the comment that said the test was invalid.

[pjo, 2014-07-02 00:44:11]

Thank you.

Miguel, please take a look.

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

https://codereview.appspot.com/110330046/diff/1/src/com/google/enterprise/ada...
src/com/google/enterprise/adaptor/fs/FsAdaptor.java:489: }
maybe all the should be INFO instead of WARNING?

[mifern, 2014-07-02 16:05:18]

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

https://codereview.appspot.com/110330046/diff/1/src/com/google/enterprise/ada...
src/com/google/enterprise/adaptor/fs/FsAdaptor.java:472: BasicFileAttributes
attrs;
I don't understand the reasoning for flipping this code other than to fix a
test. isSupportedPath is called before delegate.readBasicAttributes to avoid
adding the try-catch blocks that were added. It seems that is we just want a
better error message then we just need to add a check if the file exists.

[pjo, 2014-07-02 16:37:32]

Thank you

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

https://codereview.appspot.com/110330046/diff/1/src/com/google/enterprise/ada...
src/com/google/enterprise/adaptor/fs/FsAdaptor.java:472: BasicFileAttributes
attrs;
On 2014/07/02 16:05:18, mifern wrote:
> I don't understand the reasoning for flipping this code other than to fix a
> test. isSupportedPath is called before delegate.readBasicAttributes to avoid
> adding the try-catch blocks that were added. It seems that is we just want a
> better error message then we just need to add a check if the file exists.

(1) fixing test is a good thing
(2) code seems to check for existance by 
handling exceptions of readbasicAttributes().
Do you have a different suggestion for chking
for existance?

[mifern, 2014-07-02 17:49:52]

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

https://codereview.appspot.com/110330046/diff/1/src/com/google/enterprise/ada...
src/com/google/enterprise/adaptor/fs/FsAdaptor.java:472: BasicFileAttributes
attrs;
I don't disagree with fixing a test being good. I disagree with changing
production code for a non-bug fix to fix a test. The test should be the one
being adjusted as needed.

Files.exists() or Files.notExists() would be alternatives.

This also changes the flow of the code. We are now getting the
BasicFileAttributes on a non-supported path since the isSupportedPath check
comes after. We run the overhead of getting BasicFileAttributes when not needed
in addition to adding a testing overhead to verify what happens when getting the
BasicFileAttributes for items not supported.


On 2014/07/02 16:37:31, pjo wrote:
> On 2014/07/02 16:05:18, mifern wrote:
> > I don't understand the reasoning for flipping this code other than to fix a
> > test. isSupportedPath is called before delegate.readBasicAttributes to avoid
> > adding the try-catch blocks that were added. It seems that is we just want a
> > better error message then we just need to add a check if the file exists.
> 
> (1) fixing test is a good thing
> (2) code seems to check for existance by 
> handling exceptions of readbasicAttributes().
> Do you have a different suggestion for chking
> for existance?  
>

[pjo, 2014-07-02 18:06:28]

Thank you

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

https://codereview.appspot.com/110330046/diff/1/src/com/google/enterprise/ada...
src/com/google/enterprise/adaptor/fs/FsAdaptor.java:472: BasicFileAttributes
attrs;
On 2014/07/02 17:49:51, mifern wrote:
> I don't disagree with fixing a test being good. I disagree with changing
> production code for a non-bug fix to fix a test. The test should be the one
> being adjusted as needed.
> 
> Files.exists() or Files.notExists() would be alternatives.
> 
> This also changes the flow of the code. We are now getting the
> BasicFileAttributes on a non-supported path since the isSupportedPath check
> comes after. We run the overhead of getting BasicFileAttributes when not
needed
> in addition to adding a testing overhead to verify what happens when getting
the
> BasicFileAttributes for items not supported.
> 
> 
> On 2014/07/02 16:37:31, pjo wrote:
> > On 2014/07/02 16:05:18, mifern wrote:
> > > I don't understand the reasoning for flipping this code other than to fix
a
> > > test. isSupportedPath is called before delegate.readBasicAttributes to
avoid
> > > adding the try-catch blocks that were added. It seems that is we just want
a
> > > better error message then we just need to add a check if the file exists.
> > 
> > (1) fixing test is a good thing
> > (2) code seems to check for existance by 
> > handling exceptions of readbasicAttributes().
> > Do you have a different suggestion for chking
> > for existance?  
> > 
> 

What does isSupportedPath do if file does not exist?

Should isSupportedPath be annotated to say it assumes
file exists?

It seems like current log message about file type 
implies extension like "pdf" or "txt" or whatever,
but really was referring to not dir and not regular
file.  Also the name isSupportedPath didn't help
disspell that wrong impression.

https://codereview.appspot.com/110330046/diff/1/src/com/google/enterprise/ada...
src/com/google/enterprise/adaptor/fs/FsAdaptor.java:486: log.log(Level.WARNING,
"The path {0} is not a supported file type.", doc);
isSupportedPath seems like a poort name.

"file type" seems very misleading as an internet
search seems to show the common interpretation
is about format.

[brett.michael.johnson_gmail.com, 2014-07-02 20:02:28]

I didn’t change to production code to fix a test.
I changed the production code to fix a misleading
log message when fetching the contents of a moved
or deleted file. As a consequence of that change,
the invalid test was also fixed.

I can’t call Files.exists() because I need to go
through the delegate to determine that the file
exists, or the Mocks won’t work. I either have to
add an exists() method to the delegate (remember
when it did have that method?), or use the behavior
of the existing methods.

I agree with PJ about isSupportedPath() being ill-
named, and its log message even more confusing.

I disagree that moving the call to isSupportedPath
down causes us to fetch attributes extraneously,
since isSupportedPath’s calls to isDirectory and
isRegular file would fetch the attributes under
the covers anyways, and as Miguel found out, they
are cached in the NIO layers.

—
Brett M. Johnson

On Jul 2, 2014, at 10:49 AM, mifern@google.com wrote:

> 
>
https://codereview.appspot.com/110330046/diff/1/src/com/google/enterprise/ada...
> File src/com/google/enterprise/adaptor/fs/FsAdaptor.java (right):
> 
>
https://codereview.appspot.com/110330046/diff/1/src/com/google/enterprise/ada...
> src/com/google/enterprise/adaptor/fs/FsAdaptor.java:472:
> BasicFileAttributes attrs;
> I don't disagree with fixing a test being good. I disagree with changing
> production code for a non-bug fix to fix a test. The test should be the
> one being adjusted as needed.
> 
> Files.exists() or Files.notExists() would be alternatives.
> 
> This also changes the flow of the code. We are now getting the
> BasicFileAttributes on a non-supported path since the isSupportedPath
> check comes after. We run the overhead of getting BasicFileAttributes
> when not needed in addition to adding a testing overhead to verify what
> happens when getting the BasicFileAttributes for items not supported.
> 
> 
> On 2014/07/02 16:37:31, pjo wrote:
>> On 2014/07/02 16:05:18, mifern wrote:
>> > I don't understand the reasoning for flipping this code other than
> to fix a
>> > test. isSupportedPath is called before delegate.readBasicAttributes
> to avoid
>> > adding the try-catch blocks that were added. It seems that is we
> just want a
>> > better error message then we just need to add a check if the file
> exists.
> 
>> (1) fixing test is a good thing
>> (2) code seems to check for existance by
>> handling exceptions of readbasicAttributes().
>> Do you have a different suggestion for chking
>> for existance?
> 
> 
> https://codereview.appspot.com/110330046/

[brett.michael.johnson_gmail.com, 2014-07-02 20:07:20]

I have also argued in the past that the order of the
validity checks at the top of getDocumentContent()
was incorrect (hence the comment about the broken
test), and now b/15667934 shows that I was correct.
The tests were indeed performed in the wrong order.

—
Brett M. Johnson

On Jul 2, 2014, at 1:02 PM, Brett Johnson <brett.michael.johnson@gmail.com>
wrote:

> I didn’t change to production code to fix a test.
> I changed the production code to fix a misleading
> log message when fetching the contents of a moved
> or deleted file. As a consequence of that change,
> the invalid test was also fixed.
> 
> I can’t call Files.exists() because I need to go
> through the delegate to determine that the file
> exists, or the Mocks won’t work. I either have to
> add an exists() method to the delegate (remember
> when it did have that method?), or use the behavior
> of the existing methods.
> 
> I agree with PJ about isSupportedPath() being ill-
> named, and its log message even more confusing.
> 
> I disagree that moving the call to isSupportedPath
> down causes us to fetch attributes extraneously,
> since isSupportedPath’s calls to isDirectory and
> isRegular file would fetch the attributes under
> the covers anyways, and as Miguel found out, they
> are cached in the NIO layers.
> 
> —
> Brett M. Johnson
> 
> On Jul 2, 2014, at 10:49 AM, mifern@google.com wrote:
> 
>> 
>>
https://codereview.appspot.com/110330046/diff/1/src/com/google/enterprise/ada...
>> File src/com/google/enterprise/adaptor/fs/FsAdaptor.java (right):
>> 
>>
https://codereview.appspot.com/110330046/diff/1/src/com/google/enterprise/ada...
>> src/com/google/enterprise/adaptor/fs/FsAdaptor.java:472:
>> BasicFileAttributes attrs;
>> I don't disagree with fixing a test being good. I disagree with changing
>> production code for a non-bug fix to fix a test. The test should be the
>> one being adjusted as needed.
>> 
>> Files.exists() or Files.notExists() would be alternatives.
>> 
>> This also changes the flow of the code. We are now getting the
>> BasicFileAttributes on a non-supported path since the isSupportedPath
>> check comes after. We run the overhead of getting BasicFileAttributes
>> when not needed in addition to adding a testing overhead to verify what
>> happens when getting the BasicFileAttributes for items not supported.
>> 
>> 
>> On 2014/07/02 16:37:31, pjo wrote:
>>> On 2014/07/02 16:05:18, mifern wrote:
>>>> I don't understand the reasoning for flipping this code other than
>> to fix a
>>>> test. isSupportedPath is called before delegate.readBasicAttributes
>> to avoid
>>>> adding the try-catch blocks that were added. It seems that is we
>> just want a
>>>> better error message then we just need to add a check if the file
>> exists.
>> 
>>> (1) fixing test is a good thing
>>> (2) code seems to check for existance by
>>> handling exceptions of readbasicAttributes().
>>> Do you have a different suggestion for chking
>>> for existance?
>> 
>> 
>> https://codereview.appspot.com/110330046/
> 

[mifern, 2014-07-02 20:25:57]

The caching only happens in Windows not in Linux, but I'll let you and PJ decide
how you want to move forward.

[pjo, 2014-07-02 21:05:56]

Thank you for thoughtful conversation.

Brett's sequencing and arguments make sense to me.

Brett, please also adjust the name of isSuported
and change the message away from "file type".

Thank you,
PJ




- technology's compounding interest
-


On Wed, Jul 2, 2014 at 1:25 PM, <mifern@google.com> wrote:

> The caching only happens in Windows not in Linux, but I'll let you and
> PJ decide how you want to move forward.
>
> https://codereview.appspot.com/110330046/
>

[Brett, 2014-07-02 22:21:40]

PJ and Miguel's feedback. Improved log messages and levels.

[pjo, 2014-07-02 23:28:25]

LGTM.  Thank you.

[Brett, 2014-07-02 23:33:50]

Committed 02 July 2014 to the Filesystem Adaptor:

To https://code.google.com/p/plexi.fs/
   137071c..d3b8276  master -> master