Opened 11 years ago

Closed 11 years ago

#1553 closed defect (fixed)

cjson parser too buggy for exchange formats (such as Journal Entry Bundles)

Reported by: martin.langhoff Owned by: alsroot
Priority: Unspecified by Maintainer Milestone: Unspecified
Component: journal Version: 0.84.x
Severity: Unspecified Keywords: r+
Cc: dsd Distribution/OS: OLPC
Bug Status: Unconfirmed

Description

This commit http://git.sugarlabs.org/projects/sugar/repos/mainline/commits/ee4535c98ae74347e7072909d49dcf8a5e16ca7b switched from JSON to CJSON.

The fallout of it is that now the reader of "exchange" formats such as Journal Entry Bundles is failing to read perfectly valid files.

JEBs are sometimes produced outside of Sugar, for example in Moodle code (which is PHP). PHP has no way to produce CJSON -- the _only_ CJSON writer in existence is written in Python, and CJSON is actually an OLPC/Sugar invention.

Attached is a patch fixing simple JEB imports, but it is very limited: if the JEB has a buddies entry, the CJSON parser still kicks in and fails to parse it.

Perhaps a better fix is to just revert the patch above. I just don't know the rationale.

More info on the fallout on this OLPC bug: http://dev.laptop.org/ticket/9651

Attachments (2)

Change History (9)

comment:1 Changed 11 years ago by martin.langhoff

  • Summary changed from CJSON parser too strict for exchange formats (such as Journal Entry Bundles) to cjson parser too buggy for exchange formats (such as Journal Entry Bundles)

Changed summary. As Tomeu correctly points out 'import cjson' imports the C implementation of the json parser -- different from 'import bitfrost.util.cjson' which imports the Canonical json implementation.

This is a bug in cjson, the C implementation of the json parser.

the version of cjson is current
$ rpm -q python-cjson
python-cjson-1.0.5-3.fc11.i586

... in a python session:

json.loads('"foo\/bar"')

u'foo/bar'

cjson.decode('"foo\/bar"')

'foo
/bar'

comment:2 Changed 11 years ago by dsd

  • Keywords r? added

comment:3 Changed 11 years ago by dsd

  • Cc dsd added

comment:4 Changed 11 years ago by tomeu

  • Keywords r+ added; r? removed
-        return cjson.decode(open(metadata_path, 'r').read())
+        f = open(metadata_path, 'r')
+        try:
+            json_data = f.read()
+        finally:
+            f.close()
+        return json.read(json_data)

AFAIK, the fd is closed when the file object is released, so json_data = open(metadata_path, 'r').read() should be enough.

Otherwise, r+ for the patch, though before committing I think we should post a big notice to sugar-devel and wait 2 days for any comments.

comment:5 follow-up: Changed 11 years ago by alsroot

maybe instead of adding two old dependencies, add simplejson(for 2.5) and native json for(for 2.6), that have the same API.

comment:6 in reply to: ↑ 5 Changed 11 years ago by tomeu

Replying to alsroot:

maybe instead of adding two old dependencies, add simplejson(for 2.5) and native json for(for 2.6), that have the same API.

Fine with me, though I don't see much problem with depending on simplejson while we don't require >2.5.

comment:7 Changed 11 years ago by dsd

  • Resolution set to fixed
  • Status changed from new to closed

Closing as this is committed now. Yes, I'd also be in support of dropping the simplejson requirement on systems where the Python inbuilt json parser is available.

Note: See TracTickets for help on using tickets.