You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@couchdb.apache.org by GitBox <gi...@apache.org> on 2021/06/18 21:19:46 UTC
[GitHub] [couchdb] iilyak commented on a change in pull request #3632: Fix FDB max_document_size, binary_chunk_size, and max_attachment_size checks
iilyak commented on a change in pull request #3632:
URL: https://github.com/apache/couchdb/pull/3632#discussion_r654319251
##########
File path: src/couch/include/couch_db.hrl
##########
@@ -40,6 +40,8 @@
<<"_users">>
]).
+-define(DOCUMENT_SIZE_LIMIT, 8000000).
Review comment:
Please use units in name of constants (i.e. `DOCUMENT_SIZE_LIMIT_BYTES`).
##########
File path: src/couch/src/couch_att.erl
##########
@@ -680,11 +680,24 @@ is_compressible(Type) ->
max_attachment_size() ->
- case config:get("couchdb", "max_attachment_size", "infinity") of
+ max_attachment_size(config:get("couchdb", "max_attachment_size", "infinity")).
Review comment:
I think we can switch to `config:get_integer("couchdb", "max_attachment_size", ?ATT_SIZE_LIMIT)` to simplify `max_attachment_size` and for consistency with other places. The only possible type `config:get_integer/3` can return is integer. Which means we can write the following:
```
max_attachment_size() ->
min(config:get_integer("couchdb", "max_attachment_size", ?ATT_SIZE_LIMIT), ?ATT_SIZE_LIMIT).
```
##########
File path: src/couch/src/couch_att.erl
##########
@@ -680,11 +680,24 @@ is_compressible(Type) ->
max_attachment_size() ->
- case config:get("couchdb", "max_attachment_size", "infinity") of
+ max_attachment_size(config:get("couchdb", "max_attachment_size", "infinity")).
+
+max_attachment_size(MaxAttSizeConfig) ->
+ case MaxAttSizeConfig of
"infinity" ->
- infinity;
+ ?ATT_SIZE_LIMIT;
+ MaxAttSize when is_list(MaxAttSize) ->
+ try list_to_integer(MaxAttSize) of
+ Result -> min(Result, ?ATT_SIZE_LIMIT)
+ catch _:_ ->
+ couch_log:error("invalid config value for max attachment size: ~p ", [MaxAttSize]),
+ throw(internal_server_error)
+ end;
+ MaxAttSize when is_integer(MaxAttSize) ->
Review comment:
`config:get/3` always return a string. Which means the `when is_integer(MaxAttSize) -> ` and `MaxAttSize ->` clauses are not needed.
##########
File path: src/couch/src/couch_doc.erl
##########
@@ -130,7 +131,8 @@ from_json_obj_validate(EJson) ->
from_json_obj_validate(EJson, undefined).
from_json_obj_validate(EJson, DbName) ->
- MaxSize = config:get_integer("couchdb", "max_document_size", 4294967296),
+ ConfigMaxSize = config:get_integer("couchdb", "max_document_size", 4294967296),
Review comment:
Should we use `config:get_integer("couchdb", "max_document_size", ?DOCUMENT_SIZE_LIMIT)` to get rid of magic number in the codebase?
##########
File path: src/couch/src/couch_doc.erl
##########
@@ -130,7 +131,8 @@ from_json_obj_validate(EJson) ->
from_json_obj_validate(EJson, undefined).
from_json_obj_validate(EJson, DbName) ->
- MaxSize = config:get_integer("couchdb", "max_document_size", 4294967296),
+ ConfigMaxSize = config:get_integer("couchdb", "max_document_size", 4294967296),
Review comment:
> I was trying to iron out a bug last night where changing the magic number to ?DOCUMENT_SIZE_LIMIT broke the tests.
Did you try modifying this mock https://github.com/apache/couchdb/blob/main/src/couch/test/eunit/couch_doc_json_tests.erl#L42 ?
##########
File path: src/couch/src/couch_doc.erl
##########
@@ -130,7 +131,8 @@ from_json_obj_validate(EJson) ->
from_json_obj_validate(EJson, undefined).
from_json_obj_validate(EJson, DbName) ->
- MaxSize = config:get_integer("couchdb", "max_document_size", 4294967296),
+ ConfigMaxSize = config:get_integer("couchdb", "max_document_size", 4294967296),
Review comment:
I think it should be `fun("couchdb", "max_document_size", _) -> 1024 end),`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org