[jira] Commented: (COUCHDB-687) Add md5 hash to _attachments properties for documents

["Filipe Manana (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/COUCHDB-687?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12968789#action_12968789 ] 

Filipe Manana commented on COUCHDB-687:
---------------------------------------

Hi Juuso,

Again, thanks for your efforts.

I haven't tried the patch, however it looks good in general.
My comments bellow.

1) To accept it, I need tests. Test the md5 property for compressible and non-compressible attachments. Test that after a compaction the md5 attribute is still there and with the same value as before the compaction. Test that after every kind of replication (local-local, local-remote, remote-remote, remote-local), the md5 attribute has the same value on the target. Etc. I would add the tests in attachments.js and eventually replication.js.

2) I think it's a bad idea to give it the string value "undefined" when we don't have the attachments' identity md5 (why "undefined" btw?). Simply omitting it from the JSON is the way to go (or at the very least assign it the null value).

3) Here:

+to_md5_hex(Binary) ->
+    ?l2b(lists:flatten(couch_util:to_hex(Binary))).

Why the lists:flatten call? Afaik, couch_util:to_hex/1 always returns a string. I would also avoid declaring this function, since it's only used in one place. Simply replace this function call with a direct call to ?l2b(couch_util:to_hex(Bin)).

4) The hex_to_bin/1 function should probably go to couch_util.

5) Here:

+                try [{<<"md5">>, to_md5_hex(Att#att.identity_md5)}]
+                catch error: _ -> [{<<"md5">>, <<"undefined">>}] end++

Why the try catch expression? Afaik couch_util:to_hex/1 never throws an exception, even if the argument is an empty binary.

6) I told in your previous submission (other ticket) to keep lines up to 80 characters wide. See http://wiki.apache.org/couchdb/Coding_Standards

> Add md5 hash to _attachments properties for documents
> -----------------------------------------------------
>
>                 Key: COUCHDB-687
>                 URL: https://issues.apache.org/jira/browse/COUCHDB-687
>             Project: CouchDB
>          Issue Type: Improvement
>         Environment: CouchDB
>            Reporter: mikeal
>            Assignee: Filipe Manana
>         Attachments: couchdb-md5-in-attachment-COUCHDB-687.patch, md5.patch
>
>
> The current attachment information looks like this:
> GET /dbname/docid
> "_attachments": {
>       "jquery-1.4.1.min.js": {
>           "content_type": "text/javascript"
>           "revpos": 138
>           "length": 70844
>           "stub": true
>       }
> }
> If a client wanted to sync local files as attachments with a document it would not currently be able to do so without keeping a local store of the revpos. If this information included an md5 hash of the attachment clients could compare it against a hash of the local file to see if they match.
> -Mikeal

-- 
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.