Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add initial files to allow protobuf 4.20 #327

Merged
merged 11 commits into from
Jul 1, 2022
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ jobs:
strategy:
matrix:
python: ['3.6', '3.7', '3.8', '3.9', '3.10']
variant: ['', cpp]
variant: ['', 'cpp', 'upb']
steps:
- name: Cancel Previous Runs
uses: styfle/[email protected]
Expand Down
10 changes: 8 additions & 2 deletions noxfile.py
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,8 @@ def unit(session, proto="python"):
session.env["PROTOCOL_BUFFERS_PYTHON_IMPLEMENTATION"] = proto
session.install("coverage", "pytest", "pytest-cov", "pytz")
session.install("-e", ".[testing]", "-c", constraints_path)

if proto == "cpp": # 4.20 does not have cpp.
session.install("protobuf==3.19.0")
session.run(
"py.test",
"-W=error",
Expand All @@ -54,7 +55,7 @@ def unit(session, proto="python"):
"--cov-config=.coveragerc",
"--cov-report=term",
"--cov-report=html",
os.path.join("tests", ""),
os.path.join("tests"),
atulep marked this conversation as resolved.
Show resolved Hide resolved
]
),
)
Expand All @@ -68,6 +69,11 @@ def unitcpp(session):
return unit(session, proto="cpp")


@nox.session(python=PYTHON_VERSIONS)
def unitupb(session):
return unit(session, proto="upb")


# Just use the most recent version for docs
@nox.session(python=PYTHON_VERSIONS[-1])
def docs(session):
Expand Down
7 changes: 6 additions & 1 deletion proto/_package_info.py
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,12 @@ def compile(name, attrs):
proto_module = getattr(module, "__protobuf__", object())

# A package should be present; get the marshal from there.
package = getattr(proto_module, "package", module_name)
# When package is empty, upb based protobuf fails with an
atulep marked this conversation as resolved.
Show resolved Hide resolved
# "TypeError: Couldn't build proto file into descriptor pool: invalid name: empty part ()' means"
# during an attempt to add to descriptor pool.
package = getattr(
proto_module, "package", module_name if module_name else "_default_package"
)
marshal = Marshal(name=getattr(proto_module, "marshal", package))

# Done; return the data.
Expand Down
12 changes: 9 additions & 3 deletions proto/marshal/collections/maps.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
import collections

from proto.utils import cached_property
from google.protobuf.message import Message


class MapComposite(collections.abc.MutableMapping):
Expand Down Expand Up @@ -58,15 +59,20 @@ def __getitem__(self, key):

def __setitem__(self, key, value):
pb_value = self._marshal.to_proto(self._pb_type, value, strict=True)

# Directly setting a key is not allowed; however, protocol buffers
# is so permissive that querying for the existence of a key will in
# of itself create it.
#
# Therefore, we create a key that way (clearing any fields that may
# be set) and then merge in our values.
self.pb[key].Clear()
self.pb[key].MergeFrom(pb_value)
# In UPB, sometime self.pb[key] is not always a proto.
atulep marked this conversation as resolved.
Show resolved Hide resolved
# This happens during marshalling when the pb_value is upb.MapCompositeContainer
# so it's not marshalled correcrtly (i.e. should be scalar values not composite).
if isinstance(self.pb[key], Message):
self.pb[key].Clear()
self.pb[key].MergeFrom(pb_value)
else:
self.pb[key] = value

def __delitem__(self, key):
self.pb.pop(key)
Expand Down
8 changes: 8 additions & 0 deletions proto/marshal/compat.py
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,17 @@
except ImportError:
_message = None

# Try UPB.
if not _message:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please test it more as we discussed. In case if ptotobuf 3.20 and 4.21 re mutually exclusive in this logic, please prioritize workflow, when newer version of protobuf if assumed first, and only if it fails, fallback to older version, not vice versa. This is to make sure that throwing an exception is not an expected behavior for the prioritized protobuf version (which I guess should be the new one)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

try:
from google._upb import _message
except ImportError:
_message = None

repeated_composite_types = (containers.RepeatedCompositeFieldContainer,)
repeated_scalar_types = (containers.RepeatedScalarFieldContainer,)
map_composite_types = (containers.MessageMap,)

if _message:
repeated_composite_types += (_message.RepeatedCompositeContainer,)
repeated_scalar_types += (_message.RepeatedScalarContainer,)
Expand Down
2 changes: 1 addition & 1 deletion proto/marshal/marshal.py
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
from proto.marshal.collections import MapComposite
from proto.marshal.collections import Repeated
from proto.marshal.collections import RepeatedComposite

from proto.marshal.rules import bytes as pb_bytes
from proto.marshal.rules import stringy_numbers
from proto.marshal.rules import dates
Expand Down Expand Up @@ -219,7 +220,6 @@ def to_proto(self, proto_type, value, *, strict: bool = False):
got=pb_value.__class__.__name__,
),
)

# Return the final value.
return pb_value

Expand Down
25 changes: 20 additions & 5 deletions proto/message.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,11 @@
from google.protobuf import message
from google.protobuf.json_format import MessageToDict, MessageToJson, Parse

try:
from google._upb import _message
except ImportError:
_message = None

from proto import _file_info
from proto import _package_info
from proto.fields import Field
Expand Down Expand Up @@ -568,11 +573,21 @@ def __init__(
# See related issue
# https://github.com/googleapis/python-api-core/issues/227
if isinstance(value, dict):
keys_to_update = [
item
for item in value
if not hasattr(pb_type, item) and hasattr(pb_type, f"{item}_")
]
if _message is None:
atulep marked this conversation as resolved.
Show resolved Hide resolved
keys_to_update = [
item
for item in value
if not hasattr(pb_type, item)
and hasattr(pb_type, f"{item}_")
]
else:
# In UPB, pb_type is MessageMeta which doesn't expose attrs like it used to in Python/CPP.
keys_to_update = [
item
for item in value
if item not in pb_type.DESCRIPTOR.fields_by_name
and f"{item}_" in pb_type.DESCRIPTOR.fields_by_name
]
for item in keys_to_update:
value[f"{item}_"] = value.pop(item)

Expand Down
21 changes: 19 additions & 2 deletions tests/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -37,10 +37,27 @@ def pytest_runtest_setup(item):
mock.patch.object(symbol_database, "Default", return_value=sym_db),
]
if descriptor_pool._USE_C_DESCRIPTORS:
from google.protobuf.pyext import _message
has_upb = False
try:
from google._upb import _message

has_upb = True
assert _message
except ImportError:
_message = None
if not _message:
try:
from google.protobuf.pyext import _message
except ImportError:
_message = None
atulep marked this conversation as resolved.
Show resolved Hide resolved

item._mocks.append(
mock.patch("google.protobuf.pyext._message.default_pool", pool)
mock.patch(
"google._upb._message.default_pool"
if has_upb
else "google.protobuf.pyext._message.default_pool",
pool,
)
)

[i.start() for i in item._mocks]
Expand Down
1 change: 1 addition & 0 deletions tests/test_fields_repeated_composite.py
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ class Baz(proto.Message):
assert len(baz.foos) == 1
assert baz.foos == baz.foos
assert baz.foos[0].bar == 42
assert isinstance(baz.foos[0], Foo)


def test_repeated_composite_equality():
Expand Down
5 changes: 4 additions & 1 deletion tests/test_message_filename.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,4 +20,7 @@ def test_filename_includes_classname_salt():
class Foo(proto.Message):
bar = proto.Field(proto.INT32, number=1)

assert Foo.pb(Foo()).DESCRIPTOR.file.name == "test_message_filename_foo.proto"
assert (
Foo.pb(Foo()).DESCRIPTOR.file.name
== "test_message_filename__default_package.foo.proto"
)