Issue 345941005: tryton-env: Add data limitation capabilities on inbound parameters of RPC calls
DescriptionPatch Set 1 #
Total comments: 10
Patch Set 2 : Add doc and take into account remarks #
Total comments: 29
Patch Set 3 : Fix remarks #
Total comments: 13
Patch Set 4 : Raise 413 and make function private #
Total comments: 6
MessagesTotal messages: 22
flake8 OK URL: https://codereview.tryton.org/345941005
Sign in to reply to this message.
I've only had a quick look, and I'm not sure whether "rate" is the right way to describe this functionality, because rate is about "how fast". From what I can it looks like (but I'm not sure) this is about limiting how many items are handled? So based on that perhaps? rate_limitations -> data_limits
Sign in to reply to this message.
Mising documentation
Sign in to reply to this message.
https://codereview.tryton.org/345941005/diff/359511007/trytond/trytond/model/... File trytond/trytond/model/modelstorage.py (right): https://codereview.tryton.org/345941005/diff/359511007/trytond/trytond/model/... trytond/trytond/model/modelstorage.py:147: 'limit': lambda v: min(v or 100_000, 100_000), Why is the default limit for exporting data different from the one in read and search_read?
Sign in to reply to this message.
On 2021/04/16 09:59:58, dave wrote: > I've only had a quick look, and I'm not sure whether "rate" is the right way to > describe this functionality, because rate is about "how fast". > > From what I can it looks like (but I'm not sure) this is about limiting how many > items are handled? > > So based on that perhaps? > rate_limitations -> data_limits You're right I'll take that into account for my next upload.
Sign in to reply to this message.
https://codereview.tryton.org/345941005/diff/359511007/trytond/trytond/model/... File trytond/trytond/model/modelstorage.py (right): https://codereview.tryton.org/345941005/diff/359511007/trytond/trytond/model/... trytond/trytond/model/modelstorage.py:147: 'limit': lambda v: min(v or 100_000, 100_000), On 2021/04/16 11:30:12, albert wrote: > Why is the default limit for exporting data different from the one in read and > search_read? It seems to me that exporting/importing data is probably a bigger burden than searching / reading hence the factor 10. But this is debatable and the figure are their mainly as an indication.
Sign in to reply to this message.
https://codereview.tryton.org/345941005/diff/359511007/trytond/trytond/model/... File trytond/trytond/model/modelstorage.py (right): https://codereview.tryton.org/345941005/diff/359511007/trytond/trytond/model/... trytond/trytond/model/modelstorage.py:130: 'ids': lambda v: v[:1_000_000], Should raise an ValueError. https://codereview.tryton.org/345941005/diff/359511007/trytond/trytond/model/... trytond/trytond/model/modelstorage.py:147: 'limit': lambda v: min(v or 100_000, 100_000), On 2021/04/16 17:11:38, nicoe wrote: > On 2021/04/16 11:30:12, albert wrote: > > Why is the default limit for exporting data different from the one in read and > > search_read? > > It seems to me that exporting/importing data is probably a bigger burden than > searching / reading hence the factor 10. > > But this is debatable and the figure are their mainly as an indication. For me the limit should be the same for all calls but it should be a configuration value. https://codereview.tryton.org/345941005/diff/359511007/trytond/trytond/rpc.py File trytond/trytond/rpc.py (right): https://codereview.tryton.org/345941005/diff/359511007/trytond/trytond/rpc.py... trytond/trytond/rpc.py:22: rate_limitations: A dictionary of function used to impose rate limitation I think it is better named: defaults (like on the route). https://codereview.tryton.org/345941005/diff/359511007/trytond/trytond/rpc.py... trytond/trytond/rpc.py:93: def apply_limitations(self, method, c_args, c_kwargs): I think inspect the method signature is not error prone because modules can override the method definition and change the parameter names. Also it prevent to put limits on ModelStorage.write which uses *args (so without name). Instead I think the configuration could be a dict with as keys: * string: to check kwargs only (and set default value) * int: to check the argument at the position * tuple: that is converted to a slice to check a subset of arguments https://codereview.tryton.org/345941005/diff/359511007/trytond/trytond/rpc.py... trytond/trytond/rpc.py:93: def apply_limitations(self, method, c_args, c_kwargs): The application could be done in convert directly this will prevent to forget to call it.
Sign in to reply to this message.
flake8 OK URL: https://codereview.tryton.org/345941005
Sign in to reply to this message.
Just a few minor suggestions https://codereview.tryton.org/345941005/diff/349861002/trytond/CHANGELOG File trytond/CHANGELOG (right): https://codereview.tryton.org/345941005/diff/349861002/trytond/CHANGELOG#newc... trytond/CHANGELOG:1: * Add data limitation capabilities on inboud parameters of RPC calls Perhaps: limitation capabilities on inboud -> limits to inbound https://codereview.tryton.org/345941005/diff/349861002/trytond/doc/ref/rpc.rst File trytond/doc/ref/rpc.rst (right): https://codereview.tryton.org/345941005/diff/349861002/trytond/doc/ref/rpc.rs... trytond/doc/ref/rpc.rst:8: .. class:: RPC([readonly[, instantiate[, result[, check_access[, unique[, fresh_session[, cache[, data_limitations]]]]]]]]) Perhaps: data_limitations -> data_limits https://codereview.tryton.org/345941005/diff/349861002/trytond/doc/ref/rpc.rs... trytond/doc/ref/rpc.rst:45: A dictionary of functions used to impose data limitation on incoming limitation on -> limits to https://codereview.tryton.org/345941005/diff/349861002/trytond/doc/ref/rpc.rs... trytond/doc/ref/rpc.rst:50: * A number: the function will be applied to the positional argument at positional argument -> argument https://codereview.tryton.org/345941005/diff/349861002/trytond/doc/ref/rpc.rs... trytond/doc/ref/rpc.rst:51: said position said -> the specified https://codereview.tryton.org/345941005/diff/349861002/trytond/doc/ref/rpc.rs... trytond/doc/ref/rpc.rst:55: the positional arguments found by slicing the list of positional list of positional arguments with -> argument list using https://codereview.tryton.org/345941005/diff/349861002/trytond/trytond/rpc.py File trytond/trytond/rpc.py (right): https://codereview.tryton.org/345941005/diff/349861002/trytond/trytond/rpc.py... trytond/trytond/rpc.py:28: data_limitations: A dictionary of function used to impose data limitation limitation(s) -> limits https://codereview.tryton.org/345941005/diff/349861002/trytond/trytond/rpc.py... trytond/trytond/rpc.py:28: data_limitations: A dictionary of function used to impose data limitation function -> functions
Sign in to reply to this message.
https://codereview.tryton.org/345941005/diff/349861002/trytond/trytond/model/... File trytond/trytond/model/modelstorage.py (right): https://codereview.tryton.org/345941005/diff/349861002/trytond/trytond/model/... trytond/trytond/model/modelstorage.py:33: _rpc_max_size = config.getint('request', 'argument_max_size') I think for consistency, it should be: max_size_argument Or maybe just max_length https://codereview.tryton.org/345941005/diff/349861002/trytond/trytond/model/... trytond/trytond/model/modelstorage.py:144: v or _rpc_max_size, _rpc_max_size), For me we should raise an error if the limit is too big instead of silently return less. https://codereview.tryton.org/345941005/diff/349861002/trytond/trytond/protoc... File trytond/trytond/protocols/dispatcher.py (right): https://codereview.tryton.org/345941005/diff/349861002/trytond/trytond/protoc... trytond/trytond/protocols/dispatcher.py:180: meth, c_args, c_kwargs) I think this should be done automatically by rpc.convert Also there is no meth argument. https://codereview.tryton.org/345941005/diff/349861002/trytond/trytond/rpc.py File trytond/trytond/rpc.py (right): https://codereview.tryton.org/345941005/diff/349861002/trytond/trytond/rpc.py... trytond/trytond/rpc.py:11: def truncate(size): I find the name confusing because it suppose that an operation will be applied when it is just a validation. https://codereview.tryton.org/345941005/diff/349861002/trytond/trytond/rpc.py... trytond/trytond/rpc.py:37: data_limitations=None): I think it will be better named 'limits' All the RPC setup is about the data of the call so no need for prefix. https://codereview.tryton.org/345941005/diff/349861002/trytond/trytond/rpc.py... trytond/trytond/rpc.py:100: n_args, n_kwargs = c_args.copy(), c_kwargs.copy() Why using different names c_* is never used. https://codereview.tryton.org/345941005/diff/349861002/trytond/trytond/rpc.py... trytond/trytond/rpc.py:104: elif isinstance(arg_spec, str): I do not think we can support keyword arguments because RPC calls do not and they can be ignored by using positional argument. https://codereview.tryton.org/345941005/diff/349861002/trytond/trytond/rpc.py... trytond/trytond/rpc.py:106: elif isinstance(arg_spec, tuple): Why using a tuple when it can be just a slice. https://codereview.tryton.org/345941005/diff/349861002/trytond/trytond/rpc.py... trytond/trytond/rpc.py:110: I'm not sure we need a function for the limit. Indeed we have for now only two cases: * the value is an int and so it must be lower than the limit * the value is a object with __len__ so the length must be lower. And in both case we should raise an error message.
Sign in to reply to this message.
https://codereview.tryton.org/345941005/diff/359511007/trytond/trytond/model/... File trytond/trytond/model/modelstorage.py (right): https://codereview.tryton.org/345941005/diff/359511007/trytond/trytond/model/... trytond/trytond/model/modelstorage.py:130: 'ids': lambda v: v[:1_000_000], On 2021/04/19 11:41:31, ced wrote: > Should raise an ValueError. Done. https://codereview.tryton.org/345941005/diff/359511007/trytond/trytond/model/... trytond/trytond/model/modelstorage.py:147: 'limit': lambda v: min(v or 100_000, 100_000), On 2021/04/19 11:41:31, ced wrote: > On 2021/04/16 17:11:38, nicoe wrote: > > On 2021/04/16 11:30:12, albert wrote: > > > Why is the default limit for exporting data different from the one in read > and > > > search_read? > > > > It seems to me that exporting/importing data is probably a bigger burden than > > searching / reading hence the factor 10. > > > > But this is debatable and the figure are their mainly as an indication. > > For me the limit should be the same for all calls but it should be a > configuration value. Done. https://codereview.tryton.org/345941005/diff/359511007/trytond/trytond/rpc.py File trytond/trytond/rpc.py (right): https://codereview.tryton.org/345941005/diff/359511007/trytond/trytond/rpc.py... trytond/trytond/rpc.py:22: rate_limitations: A dictionary of function used to impose rate limitation On 2021/04/19 11:41:31, ced wrote: > I think it is better named: defaults (like on the route). defaults is too generic to my liking, it's almost like it is default values. https://codereview.tryton.org/345941005/diff/349861002/trytond/trytond/model/... File trytond/trytond/model/modelstorage.py (right): https://codereview.tryton.org/345941005/diff/349861002/trytond/trytond/model/... trytond/trytond/model/modelstorage.py:33: _rpc_max_size = config.getint('request', 'argument_max_size') On 2021/07/07 11:35:37, ced wrote: > I think for consistency, it should be: max_size_argument > Or maybe just max_length I don't know the other config data are _section_option so _request_max_size seems better no? https://codereview.tryton.org/345941005/diff/349861002/trytond/trytond/model/... trytond/trytond/model/modelstorage.py:144: v or _rpc_max_size, _rpc_max_size), On 2021/07/07 11:35:37, ced wrote: > For me we should raise an error if the limit is too big instead of silently > return less. Done. https://codereview.tryton.org/345941005/diff/349861002/trytond/trytond/protoc... File trytond/trytond/protocols/dispatcher.py (right): https://codereview.tryton.org/345941005/diff/349861002/trytond/trytond/protoc... trytond/trytond/protocols/dispatcher.py:180: meth, c_args, c_kwargs) On 2021/07/07 11:35:37, ced wrote: > I think this should be done automatically by rpc.convert > Also there is no meth argument. Done. https://codereview.tryton.org/345941005/diff/349861002/trytond/trytond/rpc.py File trytond/trytond/rpc.py (right): https://codereview.tryton.org/345941005/diff/349861002/trytond/trytond/rpc.py... trytond/trytond/rpc.py:11: def truncate(size): On 2021/07/07 11:35:37, ced wrote: > I find the name confusing because it suppose that an operation will be applied > when it is just a validation. Done. https://codereview.tryton.org/345941005/diff/349861002/trytond/trytond/rpc.py... trytond/trytond/rpc.py:28: data_limitations: A dictionary of function used to impose data limitation On 2021/05/10 19:22:07, dave wrote: > limitation(s) -> limits Done. https://codereview.tryton.org/345941005/diff/349861002/trytond/trytond/rpc.py... trytond/trytond/rpc.py:28: data_limitations: A dictionary of function used to impose data limitation On 2021/05/10 19:22:08, dave wrote: > function -> functions Done. https://codereview.tryton.org/345941005/diff/349861002/trytond/trytond/rpc.py... trytond/trytond/rpc.py:37: data_limitations=None): On 2021/07/07 11:35:37, ced wrote: > I think it will be better named 'limits' > All the RPC setup is about the data of the call so no need for prefix. Done. https://codereview.tryton.org/345941005/diff/349861002/trytond/trytond/rpc.py... trytond/trytond/rpc.py:100: n_args, n_kwargs = c_args.copy(), c_kwargs.copy() On 2021/07/07 11:35:37, ced wrote: > Why using different names c_* is never used. Done. https://codereview.tryton.org/345941005/diff/349861002/trytond/trytond/rpc.py... trytond/trytond/rpc.py:104: elif isinstance(arg_spec, str): On 2021/07/07 11:35:37, ced wrote: > I do not think we can support keyword arguments because RPC calls do not and > they can be ignored by using positional argument. It's easier to understand when defining the limits to use the argument name if possible (cfr modelstorage.py patch), maybe convert should take care of filling the kwargs? But there is indeed an issue because the same method could use different names when it's overloaded (maybe not in our modules but in others). https://codereview.tryton.org/345941005/diff/349861002/trytond/trytond/rpc.py... trytond/trytond/rpc.py:106: elif isinstance(arg_spec, tuple): On 2021/07/07 11:35:37, ced wrote: > Why using a tuple when it can be just a slice. It's because slices are not hashable. https://codereview.tryton.org/345941005/diff/349861002/trytond/trytond/rpc.py... trytond/trytond/rpc.py:110: On 2021/07/07 11:35:37, ced wrote: > I'm not sure we need a function for the limit. > Indeed we have for now only two cases: > > * the value is an int and so it must be lower than the limit > * the value is a object with __len__ so the length must be lower. > > And in both case we should raise an error message. Done.
Sign in to reply to this message.
https://codereview.tryton.org/345941005/diff/441261003/trytond/trytond/model/... trytond/trytond/model/modelstorage.py:23: 'trytond.rpc.truncate' imported but unused URL: https://codereview.tryton.org/345941005
Sign in to reply to this message.
Missing update of doc/topics/configuration.rst https://codereview.tryton.org/345941005/diff/349861002/trytond/trytond/model/... File trytond/trytond/model/modelstorage.py (right): https://codereview.tryton.org/345941005/diff/349861002/trytond/trytond/model/... trytond/trytond/model/modelstorage.py:33: _rpc_max_size = config.getint('request', 'argument_max_size') On 2022/07/05 17:13:25, nicoe wrote: > On 2021/07/07 11:35:37, ced wrote: > > I think for consistency, it should be: max_size_argument > > Or maybe just max_length > > I don't know the other config data are _section_option so _request_max_size > seems better no? I was talking about the config name, as we already have max_size and max_size_authenticated. I think it should be more coherent to have max_size_argument https://codereview.tryton.org/345941005/diff/441261003/trytond/doc/ref/rpc.rst File trytond/doc/ref/rpc.rst (right): https://codereview.tryton.org/345941005/diff/441261003/trytond/doc/ref/rpc.rs... trytond/doc/ref/rpc.rst:45: .. attribute:: RPC.data_limitations I think it should be named: RPC.max_sizes for the correspondance with the configuration request.max_size https://codereview.tryton.org/345941005/diff/441261003/trytond/doc/ref/rpc.rs... trytond/doc/ref/rpc.rst:47: A dictionary of functions used to impose data limitation on incoming I think it should talk about argument like in instantiate documentation. https://codereview.tryton.org/345941005/diff/441261003/trytond/doc/ref/rpc.rs... trytond/doc/ref/rpc.rst:54: * A string: the function will be applied to the keyword argument with This must be removed. https://codereview.tryton.org/345941005/diff/441261003/trytond/trytond/model/... File trytond/trytond/model/modelstorage.py (right): https://codereview.tryton.org/345941005/diff/441261003/trytond/trytond/model/... trytond/trytond/model/modelstorage.py:39: _rpc_max_size = config.getint('request', 'argument_max_size') _rpc_max_size should follow the same naming convention as other config so _request_argument_max_size https://codereview.tryton.org/345941005/diff/441261003/trytond/trytond/rpc.py File trytond/trytond/rpc.py (right): https://codereview.tryton.org/345941005/diff/441261003/trytond/trytond/rpc.py... trytond/trytond/rpc.py:93: def check_limitations(self, args, kwargs): I think this should be private function. https://codereview.tryton.org/345941005/diff/441261003/trytond/trytond/rpc.py... trytond/trytond/rpc.py:93: def check_limitations(self, args, kwargs): maybe it makes more sense to use *args, **kwargs https://codereview.tryton.org/345941005/diff/441261003/trytond/trytond/rpc.py... trytond/trytond/rpc.py:98: elif isinstance(arg_spec, tuple): I think we do not need to test for a tuple. It is better to fail if a wrong type was used.
Sign in to reply to this message.
https://codereview.tryton.org/345941005/diff/441261003/trytond/trytond/rpc.py File trytond/trytond/rpc.py (right): https://codereview.tryton.org/345941005/diff/441261003/trytond/trytond/rpc.py... trytond/trytond/rpc.py:108: raise ValueError I think it should raise 413 http error.
Sign in to reply to this message.
https://codereview.tryton.org/345941005/diff/441261003/trytond/trytond/rpc.py File trytond/trytond/rpc.py (right): https://codereview.tryton.org/345941005/diff/441261003/trytond/trytond/rpc.py... trytond/trytond/rpc.py:93: def check_limitations(self, args, kwargs): On 2022/09/11 22:12:35, ced wrote: > I think this should be private function. Done. https://codereview.tryton.org/345941005/diff/441261003/trytond/trytond/rpc.py... trytond/trytond/rpc.py:93: def check_limitations(self, args, kwargs): On 2022/09/11 22:12:35, ced wrote: > maybe it makes more sense to use *args, **kwargs We're manipulating the args and kwargs it seems sensible to consider them as the structure manipulated, no? https://codereview.tryton.org/345941005/diff/441261003/trytond/trytond/rpc.py... trytond/trytond/rpc.py:98: elif isinstance(arg_spec, tuple): On 2022/09/11 22:12:35, ced wrote: > I think we do not need to test for a tuple. > It is better to fail if a wrong type was used. Done. https://codereview.tryton.org/345941005/diff/441261003/trytond/trytond/rpc.py... trytond/trytond/rpc.py:108: raise ValueError On 2022/09/11 22:34:02, ced wrote: > I think it should raise 413 http error. Done.
Sign in to reply to this message.
https://codereview.tryton.org/345941005/diff/441261003/trytond/trytond/rpc.py File trytond/trytond/rpc.py (right): https://codereview.tryton.org/345941005/diff/441261003/trytond/trytond/rpc.py... trytond/trytond/rpc.py:93: def check_limitations(self, args, kwargs): On 2022/09/26 15:11:06, nicoe wrote: > On 2022/09/11 22:12:35, ced wrote: > > maybe it makes more sense to use *args, **kwargs > > We're manipulating the args and kwargs it seems sensible to consider them as the > structure manipulated, no? I do not see why.
Sign in to reply to this message.
https://codereview.tryton.org/345941005/diff/433521003/trytond/trytond/model/... trytond/trytond/model/modelstorage.py:23: 'trytond.rpc.truncate' imported but unused URL: https://codereview.tryton.org/345941005
Sign in to reply to this message.
https://codereview.tryton.org/345941005/diff/433521003/trytond/doc/ref/rpc.rst File trytond/doc/ref/rpc.rst (right): https://codereview.tryton.org/345941005/diff/433521003/trytond/doc/ref/rpc.rs... trytond/doc/ref/rpc.rst:45: .. attribute:: RPC.data_limitations must be updated https://codereview.tryton.org/345941005/diff/433521003/trytond/doc/ref/rpc.rs... trytond/doc/ref/rpc.rst:52: * A number: the function will be applied to the positional argument at Use present form https://codereview.tryton.org/345941005/diff/433521003/trytond/doc/ref/rpc.rs... trytond/doc/ref/rpc.rst:52: * A number: the function will be applied to the positional argument at It is no more a function. https://codereview.tryton.org/345941005/diff/433521003/trytond/trytond/model/... File trytond/trytond/model/modelstorage.py (right): https://codereview.tryton.org/345941005/diff/433521003/trytond/trytond/model/... trytond/trytond/model/modelstorage.py:39: _rpc_max_size = config.getint('request', 'argument_max_size') I think it should be named: max_size_argument to follow existing pattern. https://codereview.tryton.org/345941005/diff/433521003/trytond/trytond/model/... trytond/trytond/model/modelstorage.py:39: _rpc_max_size = config.getint('request', 'argument_max_size') Missing documentation. https://codereview.tryton.org/345941005/diff/433521003/trytond/trytond/rpc.py File trytond/trytond/rpc.py (right): https://codereview.tryton.org/345941005/diff/433521003/trytond/trytond/rpc.py... trytond/trytond/rpc.py:27: data_limitations: A dictionary of functions used to impose limits not more valid
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
