Fix for Issue #8 #9
Conversation
|
Jake Wharton » DiskLruCache #3 FAILURE |
|
Jake Wharton » DiskLruCache #4 SUCCESS |
|
URL encoding is not the right solution to the problem. Instead the caller should do his own encoding to a format that's legal according to the DiskLruCach documentation. Android's HttpResponseCache uses the URLs MD5 sum, which has the nice property of limiting the URLs length. |
|
Delegating the encoding to the user is fine, however, this restriction I don't see documented. What documentation on DiskLruCache are you referencing? Neither the readme nor the javadoc mentions any restriction on the key. The source code has a validateKey method but the validation is only to restrict spaces ' ' and newlines '\r' and '\n'. Perhaps it should validate on '/' characters as well because otherwise DiskLruCache fails unexpectedly when calling 'newInputStream' on an Editor (see added test case method testWriteAndReadEncodedEntry from the pull-request commit). |
|
My bad; the class doesn't document legal keys. It should. Legal keys should match this regex: [a-z0-9_]{1,64}. They shouldn't include non-ASCII characters or care about case preservation, since those attributes are not portable across file systems. Symbolic characters are generally non-portable and should not be used. They should also be relatively short since the names are repeatedly written in metadata and compared with one another. |
|
Ok. So I'll close this request and make another with commits which change the validateKey method to check against regex: [a-z0-9_]{1,64}. I'll also document as such and add a test case. Sound good? |
|
That would be awesome. (Jake gets the final say!) |
|
Jake Wharton » DiskLruCache #5 SUCCESS |
|
(Close/Reopen is just me being clumsy using a mobile browser.) |
|
Works for me.
|
|
Forcing lower case keys complicates normal usage. Even some of the test cases would need to change and call .toLowerCase() before interacting with DiskLruCache. Perhaps the legal keys should be loosened to [a-zA-Z0-9_]{1,64}, sacrificing some portability for usage. Thoughts? |
|
It's probably worth fixing the test case. Most consumer file systems are case insensitive, and DiskLruCache will behave incorrectly since "A" and "a" are non-equal strings but yield the same files on the file system. |
|
Makes sense. |
Includes test case demonstrating the issue.