Re: [dev] RFC: Transaction meta data

> 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.

I'm fine with either option. I'd just prefer it to always error, or never error, not be data dependent. 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.

Jason

-- 
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.