incubator-couchdb-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Filipe Manana (JIRA)" <j...@apache.org>
Subject [jira] Commented: (COUCHDB-687) Add md5 hash to _attachments properties for documents
Date Tue, 07 Dec 2010 15:35:11 GMT

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


Mime
View raw message