Re: [dev] RFC: Transaction meta data

tl;dr  We've decided:

   - Transaction user and description are text (unicode) and it should be
   an error to try to set/update them with bytes, even if the bytes only
   contain ascii data,


   - Storages only deal with bytes


   - ZODB will encode transaction user and description when giving them to
   storages and decide then when getting them out. (text above, bytes below)


On Tue, Nov 15, 2016 at 12:25 PM, Jason Madden <[email protected]
> wrote:

>
> > On Nov 15, 2016, at 10:39, Jim Fulton <[email protected]> wrote:
> >
> >
> >> I'm in general agreement with option 1. I just have one nit about the
> current implementation.
> >>
> >> Even parts of modern frameworks like Pyramid (anything that uses WebOb)
> are still going to be native strings (e.g, Request.path and Request.url)
> and those are likely to end up in the transaction metadata. However, under
> Python 2, the auto-encoding that happens in the transaction setters (val +
> u'') will often mask that fact---until you get a TypeError.
> >
> > I don't understand. If values are non-ascii bytes, the error will happen
> when an attempt is made to set the data on a transaction.  That seems like
> the right time.
>
> It's an error that's new and unexpected, and worst of all, *data
> dependent* <shudder>. People have never had to worry about using the value
> of Request.path as the description before (so it's not something they've
> been testing), now they suddenly do.

Even if they see that the version number of transaction gets a major bump,
> and they carefully read the changenotes, and they carefully test setting
> description to a unicode value, what are the odds that they'll test setting
> description to a *bytes* value with non-ascii characters?
>
> >
> >> On traversal-using applications, where URLs are constructed
> dynamically, this may not happen until production; heck, because URLs
> coming in are under the control of a remote person, even a static URL
> scheme is vulnerable to this.
> >
> > So, basically, you're worried about applications that aren't carefully
> written.
>
> Even if the application was carefully written, the fact that this is a
> new, data-dependent error case is likely to catch people off guard.
> Especially since things like Request.path are still str (bytes), it's not
> something that's ever been necessary to worry about.
>

> >
> >>
> >> So, if we're going to go with transaction metadata being text, can we
> either (1) make it more strict and `assert isinstance(val, text)`
> >
> > That would cause lots of breakage, but might be for the best. IDK. I
> wonder what other folks think.
>
> If it causes "lots" of breakage as we're both guessing, then doesn't that
> mean that "lots" of places are currently setting bytes values? :) And since
> they're bytes values, whatever encoding the storage does for text would
> never come into play, so there's no guarantee they're ascii-compatible (or
> utf-8 compatible).
>
> >
> >> or (2) make it more lenient and `val.decode('utf-8')`, possibly with a
> fallback to latin-1?
> >>
> > That feels too guessy to me, although I could live with utf-8.
>
> The nice thing about latin-1 as a fallback is that it's non-lossy. You may
> wind up with unreadable characters, but at least you didn't lose any data.
> It's the same situation we have now with released versions of transaction.
> If we're going to be implicitly doing data-dependent conversions
> (especially for descriptive, non-critical meta data---history-free
> RelStorage doesn't even store the transaction meta data) then the least we
> could do is not let them fail and handle all the cases we currently handle.
> It's the polite thing to do :)
>
>   py> b'Foo: \xf0\x9f\x92\xa9' + u''
>   Traceback (most recent call last):
>     File "<stdin>", line 1, in <module>
>   UnicodeDecodeError: 'ascii' codec can't decode byte 0xf0 in position 5:
> ordinal not in range(128)
>   py> b'Foo: \xf0\x9f\x92\xa9'.decode('latin-1').encode('utf-8').decode('
> utf-8')
>   u'Foo: \xf0\x9f\x92\xa9'
>
> >
> >> Option (1) probably breaks lots of Python 2 code (I know it would ours).
> >
> >
> > But I'm guessing the code it breaks is pretty centralized. A framework
> that adds the path to the transaction note during traversal is likely do
> this in one place that's easily fixed.
>
> Very true. Once it's found, likely from encountering it in the wild.
>

But if we always error (option 1), then it would be caught during testing.


>
> I'm fine with either option. I'd just prefer it to always error, or never
> error, not be data dependent.


Yup



> IIRC, this sort of data dependent error is one of the major arguments
> against the way Python 2 implicitly handles unicode conversions and why
> Python 3 is better.
>
> But maybe it's not a big deal. Our internal framework can be adapted, as
> can pyramid_tm. Those are the only two I know about. Maybe others know of
> more.
>
> >
> >> (We could even make this configurable by giving TransactionManager a
> `transaction_factory` attribute that it uses to instantiate the Transaction
> object, and have two Transaction subclasses that handle it both ways.)
> >
> > We could, but that's another knob for a pretty minor feature.
>
> True, just tossing it out there.
>

I'm sold on always erroring.

In fact, the current behavior is to (almost) always error on Python 3.

Jim

-- 
Jim Fulton
http://jimfulton.info

-- 
You received this message because you are subscribed to the Google Groups "zodb" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [email protected].
For more options, visit https://groups.google.com/d/optout.