fix: check for legacy local structured property values #365
Conversation
…g from base type refs googleapis#359
googlebot
added
the
cla: yes
| # See https://github.com/googleapis/python-ndb/issues/359 | ||
| if self._compressed and isinstance(value, ds_entity_module.Entity): | ||
| return | ||
|
|
||
| if self._compressed and not isinstance(value, _CompressedValue): |
There was a problem hiding this comment.
I know this isn't part of the PR, but it's drawn my eye to this code, and I have a couple of questions.
google/cloud/ndb/model.py
Outdated
| # See https://github.com/googleapis/python-ndb/issues/359 | ||
| if self._compressed and isinstance(value, ds_entity_module.Entity): | ||
| return | ||
|
|
||
| if self._compressed and not isinstance(value, _CompressedValue): | ||
| if not value.startswith(_ZLIB_COMPRESSION_MARKER): | ||
| value = zlib.compress(value) |
There was a problem hiding this comment.
Why compress here if we're just going to decompress below? Can we just return?
There was a problem hiding this comment.
And just below here, in that last if statement, don't we already know it's a _CompressedValue at that point? Do we need the if?
There was a problem hiding this comment.
I agree that compressing and then immediately decompressing is foolish. Will change that. I do think the last check is needed, because it could happen that we could be dealing with a non-compressed property.
There was a problem hiding this comment.
In fact, now that I look at it, I think that check is actually the only needed code in that method. Thanks for bringing it to my attention.
There was a problem hiding this comment.
Ok, I had to follow the different possibilities step by step to get a better grasp. The only possible improvement was your first suggestion to avoid compressing and return instead. Done.
…lue when converting
…when converting from base type
refs #359