Arvados

At 0c082b7

None of the "valid" test cases are actually valid: they are all missing a trailing newline.

I think errors will be more readable if we use word.inspect instead of just word:

-Manifest invalid for stream #{line_count}. Invalid file token foo
+Manifest invalid for stream #{line_count}. Invalid file token "foo" 

The last condition has an unnecessary regexp check: if word =~ FILE_REGEXP then we'd still be in the loop. I think it would be more helpful if split into two distinct error reports:

-        if(count == 0) or (word and word !~ FILE_REGEXP)
-          raise ArgumentError.new "Manifest invalid for stream #{line_count}. Missing or invalid file name #{word}" 
-        end
+        if word
+          raise ArgumentError.new "Manifest invalid for stream #{line_count}: invalid #{'file ' if count>0}token #{word.inspect}" 
+        elsif count == 0
+          raise ArgumentError.new "Manifest invalid for stream #{line_count}: no file tokens" 
+        end

(The current version reports "invalid file token" for ". {locator} bogus-string {locator} {file} {file}\n" which seems a bit confusing, hence the "'file ' if count>0" bit above.)

Here are some more valid test cases.

". d41d8cd98f00b204e9800998ecf8427e+0 0:0:foo/bar.txt\n" 
". d41d8cd98f00b204e9800998ecf8427e+0 000000000000000000000000000000:0777:foo.txt\n" 
". d41d8cd98f00b204e9800998ecf8427e+0 0:0:0:0\n" 
". d41d8cd98f00b204e9800998ecf8427e+0 0:0:\\040\n" 
". 00000000000000000000000000000000+0 0:0:0\n" 
". 00000000000000000000000000000000+0 0:0:d41d8cd98f00b204e9800998ecf8427e+0+Ad41d8cd98f00b204e9800998ecf8427e00000000@ffffffff\n" 

Invalid:

". d41d8cd98f00b204e9800998ecf8427e+0 0:0:foo.txt d41d8cd98f00b204e9800998ecf8427e+0\n" 
". d41d8cd98f00b204e9800998ecf8427e+0 0:0:\n" 
". d41d8cd98f00b204e9800998ecf8427e+0\n" 
". 0:0:foo.txt d41d8cd98f00b204e9800998ecf8427e+0\n" 
". 0:0:foo.txt\n" 
".\n" 
"." 
". \n" 
".  \n" 
".\td41d8cd98f00b204e9800998ecf8427e+0 0:0:foo.txt\n" 
" . d41d8cd98f00b204e9800998ecf8427e+0 0:0:foo.txt\n" 
". d41d8cd98f00b204e9800998ecf8427e+0 0:0:foo.txt \n" 
". d41d8cd98f00b204e9800998ecf8427e+0  0:0:foo.txt\n" 
". d41d8cd98f00b204e9800998ecf8427e+0 0:0:foo.txt\n \n" 
". d41d8cd98f00b204e9800998ecf8427e+0 0:0:foo.txt\n\n" 
". d41d8cd98f00b204e9800998ecf8427e+0 0:0:foo.txt\n " 
"\n. d41d8cd98f00b204e9800998ecf8427e+0 0:0:foo.txt\n" 
" \n. d41d8cd98f00b204e9800998ecf8427e+0 0:0:foo.txt\n" 
". d41d8cd98f00b204e9800998ecf8427e+0 0:0:/foo.txt\n" 
"./ d41d8cd98f00b204e9800998ecf8427e+0 0:0:foo.txt\n" 
".//foo d41d8cd98f00b204e9800998ecf8427e+0 0:0:foo.txt\n" 
"./foo/ d41d8cd98f00b204e9800998ecf8427e+0 0:0:foo.txt\n" 
"./foo//bar d41d8cd98f00b204e9800998ecf8427e+0 0:0:foo.txt\n" 
". d41d8cd98f00b204e9800998ecf8427e+0 0:0:foo//bar.txt\n" 
". d41d8cd98f00b204e9800998ecf8427e+0 0:0:foo/\n" 

Tom:

• Updated to add more conditions to the manifest format, such as stream names and file names should not contain '//' etc.

• Add the tests you provided. All but two tests are not working as per your expectation. Of the two unmet tests:
  • ".\td41d8cd98f00b204e9800998ecf8427e+0 0:0:foo.txt\n" : here the "\t" is being interpreted as one white space character and hence is being used as split separator
  • ". d41d8cd98f00b204e9800998ecf8427e+0 0:0:foo.txt \n" : here the space character after foo.txt is used as split seperator
  • Due to this these two tests actually "validate".

• I was able to get the regexps to work with " should not start with '/' " and " should not end with '/' ". However, I could not get the regexp working for " should not include '//' ". Hence, I am checking this condition separately. Please let me know if have better regular expressions for stream names and file names.

Thanks.

Doh... From the Ruby docs for split, "If pattern is a single space, str is split on whitespace, with leading whitespace and runs of contiguous whitespace characters ignored." But split(/ /) just plain splits on spaces, which is what we want. (The "ignore empty trailing fields" behavior is undocumented, afaics.)

I've pushed 6277-manifest-validation-TC with the "\t" bug fixed and a bunch more tests. The wiki spec didn't actually say anything about TAB, so I updated it.

I also updated the wiki spec to forbid "." and ".." components in stream and filenames, and added code/tests for those.

Tom, thanks for the updates in the -TC branch and also updating the wiki with more clarity.

I ran all tests and saw no test failures.

Merging into master. Thanks.

Branch 6277-manifest-validation-api:

Added the check_manifest_validity before_filter in collection model. However, as discussed, this method logs any validation errors and always returns true for the time being.

Pushed a small adjustment to the collection_test.rb on 6277-manifest-validation-api-TC. The rest LGTM, thanks.

Presumably we are leaving this ticket open (or moving "check logs for offending clients", "really enable the validation", and "migrate old data" to another ticket) until those things are done.

One more thing. Can we rename the validation to "log_invalid_manifest_format" so we don't forget it doesn't actually validate?

I did a:

cd /var/www/qr1hi.arvadosapi.com/current/log
bzgrep -i invalid  production.log-201507*

and found no results related to manifests.

same in su92l

Moved the summary output files into #6859

004243ab59e3a2fc36a708b66373297dd83e0b91 in branch 6277-check_manifest_validity

• Updated before_validation filter to return false if manifest validation fails
• Added API controller tests to create collections with invalid manifest
• Updated one failing fuse test to omit unsupported streams (. and ..)

We need a unit test, too: the model should prevent invalid manifests from getting into the database ever, not just through collections#create.

• Currently the "nil" case is throwing #<NoMethodError: undefined method `encoding' for nil:NilClass>. (I think we should put self.manifest_text ||= '' somewhere, much like we do with serialized fields, so Collection.new.save gives you an empty collection instead of an error.)
• The way the exception is swallowed, it looks like the validation error isn't propagated to the client at all right now. We probably want something like

• self.manifest_text ||= ''
  begin
    Keep::Manifest.validate! manifest_text
  rescue ArgumentError => e
    errors.add :manifest_text, e.message
    return false
  end
  

• Good catch. Could you add a comment to the sanitize_filename function itself in fusedir.py to explain that the '.' and '..' cases are believed to be unreachable if the API server is newer than #6277?

27daf08f38eec505c224e7776678b32d50241e13

Addressed all your comments. I also added "default_empty_manifest" before_validation filter and now Collection.new.save also works. Thanks.

Couple of minor things

The "nil manifest_text" tests don't belong in the "create/update collection with invalid manifest #{manifest_text}" loops -- they test entirely different things so they should just be separate test cases.

Do we expect validate! to raise an exception other than ArgumentError? If not, we should say rescue ArgumentError => e instead of rescue => e.

Tom wrote:

"nil manifest_text" tests don't belong in the "create/update collection with invalid manifest ...

Yes, I broke these into separate tests and added tests for a couple more valid manifests.

Do we expect validate! to raise an exception other than ArgumentError?

You are right. We only expect ArgumentError. Updated accordingly.

Thanks.