Merge lp:~sylvain-pineau/checkbox/json_load_classmethods_bis into lp:checkbox

[Sylvain Pineau]

Prerequisite to resume a full plainbox session

This MR adds classmethods to re create instances from JSON objects.

[Zygmunt Krynicki (zyga)]

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

[lp:~sylvain-pineau/checkbox/json_load_classmethods_bis]

1916. By Zygmunt Krynicki on 2013-02-05

"[r=roadmr][bug=][author=zkrynicki] automatic merge by tarmac"

1917. By Zygmunt Krynicki on 2013-02-06

"[r=sylvain-pineau,roadmr][bug=][author=zkrynicki] automatic merge by tarmac"

[Zygmunt Krynicki (zyga)]

73 + #if value.job is not self.job:
74 + # raise ValueError("result job does not match")

I probably know why you did this but I don't know if just disabling that (especially this way) is okay. Could you explain this please?

[lp:~sylvain-pineau/checkbox/json_load_classmethods_bis]

1918. By Launchpad Translations on behalf of checkbox-dev on 2013-02-07

Launchpad automatic translations update.

[Sylvain Pineau (sylvain-pineau)]

Well It won't break in this commit, only with the next one.
Deserialized objects are only a subset of the original __dict__, and the identity match will always fail.
Thus the removal of this check.
I would probably prefer a less strict comparison such as != that we can easily override if needed to match Jobdefinition based on their checksum.

This an example of object.__dict__ debug output, used in these two lines:

#################
 {'_data': {'description': 'Gets CPU resource info from /proc/cpuinfo', 'command': 'cpuinfo_resource', 'name': 'cpuinfo', 'plugin': 'resource'}, '_resource_program': None, '_origin': <Origin filename:'/home/sylvain/checkbox-repo/plainbox_localstorage_JSON/jobs/resource.txt.in' line_start:1 line_end:4>}
#################
 {'_data': {'name': 'cpuinfo', 'plugin': 'resource'}, '_resource_program': None, '_origin': None}

As you can see, the "is" statement can't help us.

[lp:~sylvain-pineau/checkbox/json_load_classmethods_bis]

1919. By Zygmunt Krynicki on 2013-02-07

"[r=zkrynicki][bug=][author=zkrynicki] automatic merge by tarmac"

1920. By Zygmunt Krynicki on 2013-02-07

"[r=zkrynicki][bug=][author=zkrynicki] automatic merge by tarmac"

1921. By Zygmunt Krynicki on 2013-02-07

"[r=roadmr][bug=][author=zkrynicki] automatic merge by tarmac"

1922. By Daniel Manrique on 2013-02-08

Bumped rev to 0.15.3 (0.15.2 is based on rev1907)

1923. By Zygmunt Krynicki on 2013-02-08

"[r=roadmr][bug=][author=zkrynicki] automatic merge by tarmac"

1924. By Zygmunt Krynicki on 2013-02-08

"[r=roadmr][bug=][author=zkrynicki] automatic merge by tarmac"

1925. By Sylvain Pineau on 2013-02-09

box.py: removed the unneeded open()

in with session.open(), contextmanager already calls open()

1926. By Sylvain Pineau on 2013-02-09

Added classmethods to create instances from JSON

JSON objects are decoded as dictionaries, added
classmethods to populate instances from dict values

1927. By Sylvain Pineau on 2013-02-09

Restore the session.open() call

The to avoid calling session.open() twice will be in __enter__()

1928. By Sylvain Pineau on 2013-02-09

Added encode/decode tests

For JobDefinition, ResourceExpression and JobResultTests
Fix broken tests where session.update_job_state_map() is now missing

1929. By Sylvain Pineau on 2013-02-09

Session can now be resumed from the JSON file

Added the update_job_state_map method to avoid modifying the
job_state_map with __init__(), This is required to decode JSON objects
as dict and create session instances. Resuming/updating SessionState
with previous data requires an object_hook parameter (dict_to_object),
used to create python class instances from dictionaries.
previous_sesion_file() check if a previous session file exists and
returns its full pathname. resume_prompt() is for now a pure CLI prompt
to get user input.

1930. By Sylvain Pineau on 2013-02-09

Cleanup after cherry picking

1931. By Sylvain Pineau on 2013-02-09

Removed uneeded ResourceExpression encoder/decoder helpers

The _readiness_inhibitor_list will be recomputed on resume

1932. By Sylvain Pineau on 2013-02-09

Don't save the _readiness_inhibitor_list, it will be evaluated on resume

1933. By Sylvain Pineau on 2013-02-09

Added tests for JobState encode/decode to json

1934. By Sylvain Pineau on 2013-02-09

Reworked test_session.py after cherry-picking

Mainly to use _get_persistance_subset() instead of __getstate__()

1935. By Sylvain Pineau on 2013-02-09

Removed deprecated code

1936. By Sylvain Pineau on 2013-02-09

Created a class/identifier mapping

To be used in serialization operations

1937. By Sylvain Pineau on 2013-02-09

PEP8 fixes

1938. By Sylvain Pineau on 2013-02-10

Restore the job identity check in the result setter of JobState

[Sylvain Pineau (sylvain-pineau)]

Restore the job identity check in the result setter of JobState.

Should be Ok to land now.

review: Needs Resubmitting

[Zygmunt Krynicki (zyga)]

+1

review: Approve

=== modified file 'plainbox/plainbox/impl/job.py'
--- plainbox/plainbox/impl/job.py	2013-02-08 20:00:14 +0000
+++ plainbox/plainbox/impl/job.py	2013-02-10 14:42:22 +0000
@@ -249,3 +249,10 @@
         # Compute the sha256 hash of the UTF-8 encoding of the canonical form
         # and return the hex digest as the checksum that can be displayed.
         return hashlib.sha256(canonical_form.encode('UTF-8')).hexdigest()
+
+    @classmethod
+    def from_json_record(cls, record):
+        """
+        Create a JobDefinition instance from JSON record
+        """
+        return cls(record['data'])

=== modified file 'plainbox/plainbox/impl/resource.py'
--- plainbox/plainbox/impl/resource.py	2013-02-01 09:59:13 +0000
+++ plainbox/plainbox/impl/resource.py	2013-02-10 14:42:22 +0000
@@ -418,11 +418,6 @@
     def __repr__(self):
         return "<ResourceExpression text:{!r}>".format(self._text)
 
-    def _get_persistance_subset(self):
-        state = {}
-        state['_text'] = self._text
-        return state
-
     @property
     def text(self):
         """

=== modified file 'plainbox/plainbox/impl/result.py'
--- plainbox/plainbox/impl/result.py	2013-02-01 09:59:13 +0000
+++ plainbox/plainbox/impl/result.py	2013-02-10 14:42:22 +0000
@@ -127,3 +127,10 @@
         state['data']['return_code'] = self._data.get('return_code')
         # io_log are stored on disk, see session.jobs_io_log_dir
         return state
+
+    @classmethod
+    def from_json_record(cls, record):
+        """
+        Create a JobResult instance from JSON record
+        """
+        return cls(record['data'])

=== modified file 'plainbox/plainbox/impl/session.py'
--- plainbox/plainbox/impl/session.py	2013-02-08 20:00:15 +0000
+++ plainbox/plainbox/impl/session.py	2013-02-10 14:42:22 +0000
@@ -37,7 +37,6 @@
 from plainbox.impl.resource import ExpressionCannotEvaluateError
 from plainbox.impl.resource import ExpressionFailedError
 from plainbox.impl.resource import Resource
-from plainbox.impl.resource import ResourceExpression
 from plainbox.impl.result import JobResult
 from plainbox.impl.rfc822 import RFC822SyntaxError
 from plainbox.impl.rfc822 import gen_rfc822_records
@@ -155,8 +154,6 @@
             return "resource expression {!r} evaluates to false".format(
                 self.related_expression.text)
 
-    def _get_persistance_subset(self):
-        return self.__dict__
 
 # A global instance of JobReadinessInhibitor with the UNDESIRED cause.
 # This is used a lot and it makes no sense to instantiate all the time.
@@ -252,22 +249,30 @@
             return "job can be started"
 
     def _get_persistance_subset(self):
-        state = {}
-        state['_job'] = self._job
         # Don't save resource job results, fresh data are required
         # so we can't reuse the old ones
+        # The inhibitor list needs to be recomputed as well, don't save it.
+        state = {}
+        state['_job'] = self._job
         if self._job.plugin == 'resource':
-            state['_readiness_inhibitor_list'] = \
-                [UndesiredJobReadinessInhibitor]
             state['_result'] = JobResult({
                 'job': self._job,
                 'outcome': JobResult.OUTCOME_NONE
             })
         else:
-            state['_readiness_inhibitor_list'] = self._readiness_inhibitor_list
             state['_result'] = self._result
         return state
 
+    @classmethod
+    def from_json_record(cls, record):
+        """
+        Create a JobState instance from JSON record
+        """
+        obj = cls(record['_job'])
+        obj._readiness_inhibitor_list = [UndesiredJobReadinessInhibitor]
+        obj._result = record['_result']
+        return obj
+
 
 class SessionState:
     """
@@ -301,7 +306,8 @@
         # XXX: this can loose data job_list has jobs with the same name. It
         # would be better to use job id as the keys here. A separate map could
         # be used for the name->job lookup.
-        self._job_state_map = {job.name: JobState(job) for job in job_list}
+        self._job_state_map = {job.name: JobState(job)
+                               for job in self._job_list}
         # A subset of job_list that was selected by the user for execution.
         # Used to compute run_list. Can be changed at will during lifetime
         # of this object
@@ -328,6 +334,16 @@
         state['_desired_job_list'] = self._desired_job_list
         return state
 
+    @classmethod
+    def from_json_record(cls, record):
+        """
+        Create a SessionState instance from JSON record
+        """
+        obj = cls([])
+        obj._job_state_map = record['_job_state_map']
+        obj._desired_job_list = record['_desired_job_list']
+        return obj
+
     def open(self):
         """
         Open session state for running jobs.
@@ -678,16 +694,38 @@
 
 
 class SessionStateEncoder(json.JSONEncoder):
-    """
-    JSON Serialize helper to encode SessionState attributes
-    Convert objects to a dictionary of their representation
-    """
+
+    _class_indentifiers = {
+        JobDefinition: 'JOB_DEFINITION',
+        JobResult: 'JOB_RESULT',
+        JobState: 'JOB_STATE',
+        SessionState: 'SESSION_STATE',
+    }
+
     def default(self, obj):
-        if (isinstance(obj, (JobDefinition, JobReadinessInhibitor, JobResult,
-                             ResourceExpression, JobState, SessionState))):
-            d = {'__class__': obj.__class__.__name__,
-                 '__module__': obj.__module__}
+        """
+        JSON Serialize helper to encode SessionState attributes
+        Convert objects to a dictionary of their representation
+        """
+        if (isinstance(obj, (JobDefinition, JobResult, JobState,
+                             SessionState))):
+            d = {'_class_id': self._class_indentifiers[obj.__class__]}
             d.update(obj._get_persistance_subset())
             return d
         else:
             return json.JSONEncoder.default(self, obj)
+
+    def dict_to_object(self, d):
+        """
+        JSON Decoder helper
+        Convert dictionary to python objects
+        """
+        if '_class_id' in d:
+            for c, id in self._class_indentifiers.items():
+                if id == d['_class_id']:
+                    cls = c
+                    inst = cls.from_json_record(d)
+                    break
+        else:
+            inst = d
+        return inst

=== modified file 'plainbox/plainbox/impl/test_job.py'
--- plainbox/plainbox/impl/test_job.py	2013-02-07 17:21:14 +0000
+++ plainbox/plainbox/impl/test_job.py	2013-02-10 14:42:22 +0000
@@ -24,11 +24,14 @@
 Test definitions for plainbox.impl.job module
 """
 
+import json
+
 from unittest import TestCase
 
 from plainbox.impl.job import JobDefinition
 from plainbox.impl.rfc822 import RFC822Record
 from plainbox.impl.rfc822 import Origin
+from plainbox.impl.session import SessionStateEncoder
 
 
 class TestJobDefinition(TestCase):
@@ -268,3 +271,16 @@
         self.assertEqual(
             job1.get_checksum(),
             "ad137ba3654827cb07a254a55c5e2a8daa4de6af604e84ccdbe9b7f221014362")
+
+    def test_decode(self):
+        raw_json = """{
+                "_class_id": "JOB_DEFINITION",
+                "data": {
+                    "name": "camera/still",
+                    "plugin": "user-verify"
+                }
+            }"""
+        job_dec = json.loads(raw_json, object_hook=SessionStateEncoder().dict_to_object)
+        self.assertIsInstance(job_dec, JobDefinition)
+        self.assertEqual(job_dec.name, "camera/still")
+        self.assertEqual(job_dec.plugin, "user-verify")

=== modified file 'plainbox/plainbox/impl/test_resource.py'
--- plainbox/plainbox/impl/test_resource.py	2013-02-01 09:59:13 +0000
+++ plainbox/plainbox/impl/test_resource.py	2013-02-10 14:42:22 +0000
@@ -273,12 +273,6 @@
         expr = ResourceExpression("obj.a == 2")
         self.assertRaises(TypeError, expr.evaluate, [{'a': 2}])
 
-    def test_encode(self):
-        text = "device.category == 'CAPTURE'"
-        expr = ResourceExpression(text)
-        expr_enc = expr._get_persistance_subset()
-        self.assertEqual(expr_enc['_text'], text)
-
 
 class ResourceProgramTests(TestCase):
 

=== modified file 'plainbox/plainbox/impl/test_result.py'
--- plainbox/plainbox/impl/test_result.py	2013-02-05 11:02:35 +0000
+++ plainbox/plainbox/impl/test_result.py	2013-02-10 14:42:22 +0000
@@ -23,11 +23,13 @@
 
 Test definitions for plainbox.impl.result module
 """
+import json
 
 from unittest import TestCase
 
 from plainbox.impl.result import JobResult
 from plainbox.impl.testing_utils import make_job
+from plainbox.impl.session import SessionStateEncoder
 
 
 class JobResultTests(TestCase):
@@ -80,3 +82,27 @@
         self.assertEqual(result_enc['data']['return_code'], result.return_code)
         with self.assertRaises(KeyError):
             result_enc['io_log']
+
+    def test_decode(self):
+        raw_json = """{
+                "_class_id": "JOB_RESULT",
+                "data": {
+                    "comments": null,
+                    "job": {
+                        "_class_id": "JOB_DEFINITION",
+                        "data": {
+                            "name": "__audio__",
+                            "plugin": "local"
+                        }
+                    },
+                    "outcome": "pass",
+                    "return_code": 0
+                }
+            }"""
+        result_dec = json.loads(raw_json, object_hook=SessionStateEncoder().dict_to_object)
+        self.assertIsInstance(result_dec, JobResult)
+        self.assertEqual(result_dec.job.name, "__audio__")
+        self.assertEqual(result_dec.outcome, JobResult.OUTCOME_PASS)
+        self.assertIsNone(result_dec.comments)
+        self.assertEqual(result_dec.io_log, ())
+        self.assertEqual(result_dec.return_code, 0)

=== modified file 'plainbox/plainbox/impl/test_session.py'
--- plainbox/plainbox/impl/test_session.py	2013-02-05 11:02:35 +0000
+++ plainbox/plainbox/impl/test_session.py	2013-02-10 14:42:22 +0000
@@ -24,6 +24,8 @@
 Test definitions for plainbox.impl.session module
 """
 
+import json
+
 from unittest import TestCase
 
 from plainbox.impl.resource import Resource
@@ -32,6 +34,7 @@
 from plainbox.impl.session import JobState
 from plainbox.impl.session import SessionState
 from plainbox.impl.session import UndesiredJobReadinessInhibitor
+from plainbox.impl.session import SessionStateEncoder
 from plainbox.impl.testing_utils import make_job
 from plainbox.impl.testing_utils import make_job_result
 
@@ -175,6 +178,70 @@
             self.job_state.get_readiness_description().startswith(
                 "job cannot be started: "))
 
+    def test_encode_resource_job(self):
+        self.job_R = make_job("R", plugin="resource")
+        result_R = JobResult({
+            'job': self.job_R,
+            'outcome': JobResult.OUTCOME_PASS,
+            'io_log': ((0, 'stdout', "attr: value\n"),)
+        })
+        jobstate = JobState(self.job_R)
+        jobstate.result = result_R
+        jobstate_enc = jobstate._get_persistance_subset()
+        # The inhibitor list is not saved
+        with self.assertRaises(KeyError):
+            jobstate_enc['_readiness_inhibitor_list']
+        # Resource have to be re evealutated on startup, outcome of the job
+        # must be reset to JobResult.OUTCOME_NONE
+        self.assertEqual(jobstate_enc['_result'].outcome,
+                         JobResult.OUTCOME_NONE)
+
+    def test_encode_normal_job(self):
+        result = JobResult({
+            'job': self.job,
+            'outcome': JobResult.OUTCOME_PASS,
+        })
+        self.job_state.result = result
+        jobstate_enc = self.job_state._get_persistance_subset()
+        # The inhibitor list is not saved
+        with self.assertRaises(KeyError):
+            jobstate_enc['_readiness_inhibitor_list']
+        # Normal jobs should keep their outcome value
+        self.assertEqual(jobstate_enc['_result'].outcome,
+                         JobResult.OUTCOME_PASS)
+
+    def test_decode(self):
+        raw_json = """{
+            "_class_id": "JOB_STATE",
+            "_job": {
+                "_class_id": "JOB_DEFINITION",
+                "data": {
+                    "name": "X",
+                    "plugin": "dummy"
+                }
+            },
+            "_result": {
+                "_class_id": "JOB_RESULT",
+                "data": {
+                    "comments": null,
+                    "job": {
+                        "_class_id": "JOB_DEFINITION",
+                        "data": {
+                            "name": "X",
+                            "plugin": "dummy"
+                        }
+                    },
+                    "outcome": "pass",
+                    "return_code": null
+                }
+            }
+        }"""
+        job_dec = json.loads(raw_json, object_hook=SessionStateEncoder().dict_to_object)
+        self.assertIsInstance(job_dec, JobState)
+        self.assertEqual(repr(job_dec._result),
+            ("<JobResult job:<JobDefinition name:'X'"
+             " plugin:'dummy'> outcome:'pass'>"))
+
 
 class SessionStateSmokeTests(TestCase):
 
@@ -234,7 +301,7 @@
     # most of their time adding (changing) jobs in an ever-growing pile that
     # they don't necessarily fully know, comprehend or remember.
 
-    def test_resource_job_affects_resouces(self):
+    def test_resource_job_affects_resources(self):
         pass