Checkbox

Code review comment for lp:~sylvain-pineau/checkbox/json_load_classmethods_bis

json_load_classmethods_bis
Merge into trunk

Revision history for this message

Zygmunt Krynicki (zyga) wrote on 2013-02-05:

Just for the record, replied on IRC:

15:42 < spineau> zyga: I've pushed a new version of my branch, the one that provides the json decoders.
15:43 < spineau> zyga: added tests and remove the encode code of ResourceExpression now that we'll recompute the inhibitor list on resume
15:44 < zyga> spineau: hey
15:44 < spineau> zyga: the next patch will rely on this one and will provide the Session resume method and corresponding tests
15:44 < zyga> spineau: I'm reading it already
15:44 < zyga> spineau: I have some questions
15:44 < spineau> zyga: ok
15:44 < zyga> spineau: update_job_state_map() is odd
15:44 < zyga> it's not updating anything, it just resets the state
15:45 < zyga> as a developer I don't know why I would ever want to call it, it really feels internal
15:45 < zyga> and the docstring is wrong, it's not updating, it is resetting the state really
15:45 < zyga> spineau: so, those are all details
15:46 < zyga> spineau: let me read the whole thing
15:46 < zyga> spineau: job_list=[] in __init__()
15:47 < spineau> zyga: ok, let me explain why I needed this, when deserializing a session state I can't find the current job_list.
15:47 < zyga> spineau: general comment about serialization and reversing classes, __class__ + __module__ == implementation detail + exploit source, use a map that uses fixed identifiers that bind to imlpementation specific classes
15:47 < zyga> spineau: ok, go, I'm listening
15:47 < spineau> zyga: so I recreate the session state with an empty job_list and call this method to (ok) reset the list
15:49 < zyga> spineau: I don't like how this changes all existing code, perhaps that could be avoided by computing it on demand (start with None, add internal checks where it is being accessed)
15:49 < zyga> spineau: but let me finish reading
15:49 < spineau> zyga: sure
15:49 < zyga> spineau: some imports are sorted incorrectly, that's trivial but should be fixed anyway
15:50 < spineau> zyga: ok I see them
15:50 < zyga> spineau: you seem to still have some code for readiness inhibitors, did you decide to serialize them again?
15:51 < spineau> zyga: It's an error, thanks for having catching it
15:52 < zyga> spineau: apart from that it looks ok
15:52 < spineau> zyga: I don't understand your comment about: __class__ + __module__ == implementation detail + exploit source
15:52 < zyga> spineau: fix those and ping me after the meeting, I'll get it locally and look at the details
15:53 < zyga> spineau: don't expose implementation details in that serialization format, it would allow anyone to call arbitrary code
15:53 < zyga> spineau: it's semi-fine but really just annoying in practice, what if we fix a typo in a class name? Use constant identifiers
15:54 < zyga> spineau: dict_to_object and back should just have a mapping of identifiers to classes
15:54 < zyga> spineau: you can probably even create a small class that has those two methods and keeps the mapping

Just for the record, replied on IRC:

15:42 < spineau> zyga: I've pushed a new version of my branch, the one that provides the json decoders. 
15:43 < spineau> zyga: added tests and remove the encode code of ResourceExpression now that we'll recompute the inhibitor list on resume
15:44 < zyga> spineau: hey
15:44 < spineau> zyga: the next patch will rely on this one and will provide the Session resume method and corresponding tests
15:44 < zyga> spineau: I'm reading it already
15:44 < zyga> spineau: I have some questions 
15:44 < spineau> zyga: ok
15:44 < zyga> spineau: update_job_state_map() is odd
15:44 < zyga> it's not updating anything, it just resets the state
15:45 < zyga> as a developer I don't know why I would ever want to call it, it really feels internal
15:45 < zyga> and the docstring is wrong, it's not updating, it is resetting the state really
15:45 < zyga> spineau: so, those are all details
15:46 < zyga> spineau: let me read the whole thing
15:46 < zyga> spineau: job_list=[] in __init__()
15:47 < spineau> zyga: ok, let me explain why I needed this, when deserializing a session state I can't find the current job_list.
15:47 < zyga> spineau: general comment about serialization and reversing classes, __class__ + __module__ == implementation detail + exploit source, use a map that uses fixed identifiers that bind to imlpementation specific classes
15:47 < zyga> spineau: ok, go, I'm listening
15:47 < spineau> zyga: so I recreate the session state with an empty job_list and call this method to (ok) reset the list
15:49 < zyga> spineau: I don't like how this changes all existing code, perhaps that could be avoided by computing it on demand (start with None, add internal checks where it is being accessed)
15:49 < zyga> spineau: but let me finish reading
15:49 < spineau> zyga: sure
15:49 < zyga> spineau: some imports are sorted incorrectly, that's trivial but should be fixed anyway
15:50 < spineau> zyga: ok I see them
15:50 < zyga> spineau: you seem to still have some code for readiness inhibitors, did you decide to serialize them again?
15:51 < spineau> zyga: It's an error, thanks for having catching it
15:52 < zyga> spineau: apart from that it looks ok
15:52 < spineau> zyga: I don't understand your comment about: __class__ + __module__ == implementation detail + exploit source
15:52 < zyga> spineau: fix those and ping me after the meeting, I'll get it locally and look at the details
15:53 < zyga> spineau: don't expose implementation details in that serialization format, it would allow anyone to call arbitrary code
15:53 < zyga> spineau: it's semi-fine but really just annoying in practice, what if we fix a typo in a class name? Use constant identifiers
15:54 < zyga> spineau: dict_to_object and back should just have a mapping of identifiers to classes
15:54 < zyga> spineau: you can probably even create a small class that has those two methods and keeps the mapping

« Back to merge proposal