t.p.amp.AmpBox.serialize will _not_ return a string if any value is unicode

[dotz]

If you place an unicode value in AmpBox, AmpBox.serialize will return an unicode string, not str.

For example:

>>> from twisted.protocols.amp import AmpBox
>>> a = AmpBox(asdf = 'foo')
>>> type(a.serialize())
<type 'str'>
>>> a = AmpBox(asdf = u'foo')
>>> type(a.serialize())
<type 'unicode'>

This will cause funny errors on some platforms (see #3930).

This is also against AmpBox.serialize docstring, which says that it should return a str type, not unicode.

    def serialize(self):
        """
        Convert me into a wire-encoded string.

        @return: a str encoded according to the rules described in the module
        docstring.
        """

Please let me know what is the preferred way to get this fixed in trunk and I'll prepare a patch.

[dotz]

Do you need anything else from me to get this one small change merged?

[glyph]

Best solution: don't put unicode strings into AmpBox :).

Thanks for poking this issue, I certainly would have forgotten about it. I am not going to fix it myself. In most Python libraries, these sorts of checks are left out; if you put other random 'str' subclasses or non-string objects into an AmpBox you're going to get bogus results too, this isn't unique to 'unicode' objects.

If this is a problem that you're running into frequently, then please feel free to add a "strict" or "debug" mode which will reject anything but str:str mappings in an AmpBox; make sure that you cover .update, .__setitem__, .setdefault, and any other methods that dicts might have that I've forgotten about. Submit a patch, with tests, submit it for review.

[dotz]

Let's check keys and values during serialize and if the strict mode is enabled, let's fail. Also includes an unit test

[dotz]

I added a patch, that makes debugging a lot easier. Thanks for your time!

[therve]

I don't feel super great about this "strict" mode, so I'll let glyph review this.

[exarkun]

I think the "strict" mode is silly too, particularly if it defaults to on. Just do the checks all the time. Doing it in serialize is a bit later than would be ideal, but it has the advantage of avoiding the need to do it in every single dict mutation method. So I guess I like it. Please use the one-argument form of raise, though, and include the name of the offending key.