[
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.
