Polymodel instances' `class_` attribute should contain a list of `str` objects on creation

[spiqueras]

Expected Behavior

Polyclass models have a read-only attribute called class_ that contains the class hierarchy in form of a list. This attribute should always contain a list of either str or bytes, preferably the former.

Actual Behavior

On instance creation, the class_ attribute is filled with bytes objects. After the object is put and on any subsequent read, this attribute will contain str objects instead. This makes the attribute hard to rely on, as if you ever want to use it you need to know if the entity has already been written to the database or not or convert between types. It also breaks instance equality checks when one of the instance has just been created and the other has been retrieved from the database.

This is similar in Python 2 SDK (the attribute is of type str pre-put and unicode after), but due to Python 2's str and unicode being mostly compatible, it wasn't that much of an issue.

Steps to Reproduce the Problem

The following test will work fine in Python 2 but fail in Python 3.

from google.appengine.ext import testbed
from google.appengine.ext.ndb import polymodel

class APolymodel(polymodel.PolyModel):
    pass

def test_class_in_polymodel():
    bed = testbed.Testbed()
    bed.activate()
    bed.init_datastore_v3_stub()
    bed.init_memcache_stub()

    concrete = APolymodel(id=1)
    concrete.put()

    concrete_2 = APolymodel(id=1)
    assert concrete == concrete_2

    bed.deactivate()

The error

FAILED tests_py3/model_test.py::test_class_in_polymodel - AssertionError: assert APolymodel(key=Key('APolymodel', 1), class_=['APolymodel']) == APolymodel(key=Key('APolymodel', 1), class_=[b'APolymodel'])

The offending code is likely the following: https://github.com/GoogleCloudPlatform/appengine-python-standard/blob/ad6e5cc1927557aa16e656bc9a8444da69b8bbb5/src/google/appengine/ext/ndb/polymodel.py#L214-L219 Unless there's a clear reason to enforce class_ being a bytes list on instance creation, I'd be great to remove the ensure_binary, making it consistently an str list.

Goes without saying, but this issue does not happen to classes that inherit from ndb.Model.

Specifications

• Version: 1.1.1
• Platform: python3.9

[spiqueras]

AFAICS, Cloud NDB always uses str for class_ field:

https://github.com/googleapis/python-ndb/blob/22aff177256b0cb1d63446b7c1fa3707282f2f24/google/cloud/ndb/polymodel.py#L204-L210

https://github.com/googleapis/python-ndb/blob/22aff177256b0cb1d63446b7c1fa3707282f2f24/tests/unit/test_polymodel.py#L76-L89

[shreejad]

We are considering this change. It will take some time because we need to ensure this will not break existing users. My current plan is to introduce this change in the next Major Version update enabled by a flag. In a subsequent update, we can update the flag to be enabled by default.

[estrellis]

This was added as part providing py2->py3 support. b/139757579 cl/276076727

On Tue, Jul 25, 2023 at 11:21 AM shreejad @.***> wrote:

We are considering this change. It will take some time because we need to ensure this will not break existing users. My current plan is to introduce this change in the next Major Version update enabled by a flag. In a subsequent update, we can update the flag to be enabled by default.

— Reply to this email directly, view it on GitHub https://github.com/GoogleCloudPlatform/appengine-python-standard/issues/89#issuecomment-1650323661, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADUIHGYM6WNPGXRPNJIFQELXSAFELANCNFSM6AAAAAAYQMTV7A . You are receiving this because you are subscribed to this thread.Message ID: <GoogleCloudPlatform/appengine-python-standard/issues/89/1650323661@ github.com>