Test failures from incorrect json string escaping

Bug #871450 reported by Martin Packman
10
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Meliae
Fix Committed
Undecided
Unassigned

Bug Description

As noted in _loader._MemObjectProxy.to_json the way values are stringified is not compatible with json encoding, resulting in the following test failures:

======================================================================
FAIL: test_to_json (meliae.tests.test_loader.TestMemObj)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "meliae\tests\test_loader.py", line 239, in test_to_json
    self.assertEqual(expected, [obj.to_json() for obj in objs])
AssertionError: Lists differ: ['{"address": 1, "type": "tupl... != ['{"address": 1, "type": "tupl...

First differing element 5:
{"address": 6, "type": "str", "size": 29, "value": "a str", "refs": []}
{"address": 6, "type": "str", "size": 29, "value": "a str"", "refs": []}

Diff is 720 characters long. Set self.maxDiff to None to see it.

======================================================================
FAIL: test_expand_refs_as_dict (meliae.tests.test_loader.TestObjManager)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "meliae\tests\test_loader.py", line 404, in test_expand_refs_as_dict
    self.assertEqual({1: 'c', 'b': 'c'}, as_dict)
AssertionError: {1: 'c', 'b': 'c'} != {1: 'c"', 'b"': 'c"'}
- {1: 'c', 'b': 'c'}
+ {1: 'c"', 'b"': 'c"'}
? + + +

It would be possible to use json.encoder.encode_basestring_ascii to correct this, but currently json is not a hard dependency which complicates things a little.

Revision history for this message
Martin Pool (mbp) wrote :

This patch improves it so that the tests at least pass. Things may still break on some input data, though, so it might be better to use a real parser as bug 871451 suggests.

Revision history for this message
John A Meinel (jameinel) wrote :

For the location of where it is being done, I think it would be fine to just use the json/simplejson module here. The only place that actively uses the code is in "remove_expensive_references", as that is the only place that writes out a new file.

The test suite passes here, so I'm not sure what the specific problem is, but I think just depending on json is reasonable at this point. (The compatibility hacks have long since served their purpose.)

Revision history for this message
Martin Pool (mbp) wrote : Re: [Bug 871450] Re: Test failures from incorrect json string escaping

I guess it's passing because you have simplejson on your path? I think it
will reliably fail if you don't. The reason is that in the regexp used
before this patch, the .* will consume the trailing quote, like in this
simplified example:

>>> re.match('"?(.*)"?', '"hello"').groups(1)
('hello"',)

I can do another patch that just cuts out the fallback and requires a
proper json parser. Do you want to require simplejson, or have a fallback
to Python2.6's standard json? <http://docs.python.org/2/library/json.html>

--
Martin

Revision history for this message
John A Meinel (jameinel) wrote :

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 2013-02-20 11:15, Martin Pool wrote:
> I guess it's passing because you have simplejson on your path? I
> think it will reliably fail if you don't. The reason is that in
> the regexp used before this patch, the .* will consume the trailing
> quote, like in this simplified example:
>
>>>> re.match('"?(.*)"?', '"hello"').groups(1)
> ('hello"',)
>
> I can do another patch that just cuts out the fallback and requires
> a proper json parser. Do you want to require simplejson, or have a
> fallback to Python2.6's standard json?
> <http://docs.python.org/2/library/json.html>
>

A fallback is fine. 'json' gets better with newer versions, I think
with 2.6 it was pure-python only, but we can live with that.

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.13 (Cygwin)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iEYEARECAAYFAlEreYgACgkQJdeBCYSNAAMW4gCgwE6hyCbJpi5B+cZoLTr1HHdg
M8EAnAos/hF0+Vg3RiAJCGbD7/6EXXmK
=htff
-----END PGP SIGNATURE-----

Revision history for this message
Martin Pool (mbp) wrote :

John, do you have any opinion on whether we should prefer simplejson or
json?

I would tend to prefer using the standard library one. I don't know if
there are any substantial performance or robustness differences.

Revision history for this message
John A Meinel (jameinel) wrote :

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 2013-02-26 6:25, Martin Pool wrote:
> John, do you have any opinion on whether we should prefer
> simplejson or json?
>
> I would tend to prefer using the standard library one. I don't
> know if there are any substantial performance or robustness
> differences.

In python 2.6 there are reasons to prefer simplejson (no C
accelerator), however I don't think it is worth worrying about rather
than just using whatever is available.

John
=:->

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.13 (Cygwin)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iEYEARECAAYFAlEsSE8ACgkQJdeBCYSNAAMyeQCfUwh6wWTvWQvt7vhiWNyq3vK5
QdwAoK+h/0sELPftQS+YfUqcPJ2Dg3eX
=N1je
-----END PGP SIGNATURE-----

Revision history for this message
Martin Pool (mbp) wrote :

Hi John,

Here is an updated version of this patch that fixes the case where there is no json library. It's actually a bit obsolete since I've changed our build to make sure it can find one, but since this was mostly done I thought I might as well send it.

I'm not 100% sure the fix is covering what you intend for the test to do but I think it's correct.

By the way, people here are finding meliae really useful for Python memory problems - thanks!

Revision history for this message
John A Meinel (jameinel) wrote :

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 2013-05-14 12:16, Martin Pool wrote:
> Hi John,
>
> Here is an updated version of this patch that fixes the case where
> there is no json library. It's actually a bit obsolete since I've
> changed our build to make sure it can find one, but since this was
> mostly done I thought I might as well send it.
>
> I'm not 100% sure the fix is covering what you intend for the test
> to do but I think it's correct.
>
> By the way, people here are finding meliae really useful for Python
> memory problems - thanks!
>
> ** Patch added: "20130220-stringquote.diff"
> https://bugs.launchpad.net/meliae/+bug/871450/+attachment/3676157/+files/20130220-stringquote.diff
>
>
>
I'm glad you're finding it useful. I'm a bit surprised you're
submitting a patch rather than a branch, though. :)

Landed on trunk.

 status: fixcommitted

John
=:->

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.13 (Cygwin)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iEYEARECAAYFAlGYbaYACgkQJdeBCYSNAAMyewCgjLLiky14WLaEZZ7T/96em+7v
n+wAoJU4a1y2F7wp9hDr6AEHva3lXzBM
=Sb1N
-----END PGP SIGNATURE-----

Changed in meliae:
status: New → Fix Committed
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.