Skip to content

Import wellknown types (fixes #9) #76

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.

Sign up for GitHub

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

Closed
wants to merge 3 commits into from
Closed

Import wellknown types (fixes #9) #76

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
41164f4
#9 Support importing wellknown types Struct and Empty
boukeversteegh May 28, 2020
67a6289
#9 Support importing wellknown types Value and ListValue
boukeversteegh May 28, 2020
ac52320
Blacken
boukeversteegh May 28, 2020
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
19 changes: 19 additions & 0 deletions betterproto/plugin.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@
from betterproto.casing import safe_snake_case

import google.protobuf.wrappers_pb2 as google_wrappers
import google.protobuf.empty_pb2
import google.protobuf.struct_pb2

WRAPPER_TYPES: Dict[str, Optional[Type]] = defaultdict(
lambda: None,
Expand All @@ -44,6 +46,16 @@
},
)

WELLKNOWN_TYPES: Dict[str, Optional[Type]] = defaultdict(
lambda: None,
{
"google.protobuf.Empty": google.protobuf.empty_pb2.Empty,
"google.protobuf.Struct": google.protobuf.struct_pb2.Struct,
"google.protobuf.Value": google.protobuf.struct_pb2.Value,
"google.protobuf.ListValue": google.protobuf.struct_pb2.ListValue,
},
)


def get_ref_type(
package: str, imports: set, type_name: str, unwrap: bool = True
Expand Down Expand Up @@ -82,6 +94,13 @@ def get_ref_type(
cased = [stringcase.pascalcase(part) for part in parts]
type_name = f'"{"".join(cased)}"'

if type_name in WELLKNOWN_TYPES:
type_module = WELLKNOWN_TYPES[type_name].__module__
type_name = WELLKNOWN_TYPES[type_name].__name__
type_module_alias = safe_snake_case(type_module)
imports.add(f"import {type_module} as {type_module_alias}")
return f"{type_module_alias}.{type_name}"

if "." in type_name:
# This is imported from another package. No need
# to use a forward ref and we need to add the import.
Expand Down
5 changes: 2 additions & 3 deletions betterproto/tests/inputs/config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,16 +8,15 @@
"import_circular_dependency", # failing because of other bugs now
"import_packages_same_name", # 25
"oneof_enum", # 63
"googletypes_service_returns_empty", # 9
"casing_message_field_uppercase", # 11
"namespace_keywords", # 70
"namespace_builtin_types" # 53
"namespace_builtin_types", # 53
}

services = {
"googletypes_response",
"googletypes_response_embedded",
"service",
"import_service_input_message",
"googletypes_service_returns_empty",
"googletypes_service_returns_googletype",
Copy link
Collaborator

Choose a reason for hiding this comment

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

is this sufficient for it to get included in the test run?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yup~ :-D

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It will get included in the test run, meaning that the service will be instantiated.
actually, whether that's a sufficient test is another thing..

i have tested in a separate unit test that the import statements are generated correctly, but i'm having problems running it in the same go with the other tests.

the datadescriptor pool from google protobuf is filled twice with the same protobufs, for some reason. if i run the test separately, it works, but not together with the other tests.

i know from fixing #40 that returning the google-compiled wellknown value works, as long as its imported, so i think this will be good, but i haven't done a manual end-to-end-test

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

the test i have problems with, and haven't committed, is this one:

betterproto/tests/test_get_ref_type.py

import pytest

from ..plugin import get_ref_type


@pytest.mark.parametrize(
    ['google_type', 'expected_name', 'expected_import'], [
        ('.google.protobuf.Empty', 'google_protobuf_empty_pb2.Empty',
         'import google.protobuf.empty_pb2 as google_protobuf_empty_pb2'),
        ('.google.protobuf.Struct', 'google_protobuf_struct_pb2.Struct',
         'import google.protobuf.struct_pb2 as google_protobuf_struct_pb2'),
        ('.google.protobuf.ListValue', 'google_protobuf_struct_pb2.ListValue',
         'import google.protobuf.struct_pb2 as google_protobuf_struct_pb2'),
        ('.google.protobuf.Value', 'google_protobuf_struct_pb2.Value',
         'import google.protobuf.struct_pb2 as google_protobuf_struct_pb2'),
    ])
def test_import_google_wellknown_types(google_type: str, expected_name: str, expected_import: str):
    imports = set()
    name = get_ref_type(package='', imports=imports, type_name=google_type)

    assert name == expected_name
    assert imports.__contains__(expected_import)

When running all tests, this error is raised by google protobuf lib

>       return _message.default_pool.AddSerializedFile(serialized_pb)
E       TypeError: Couldn't build proto file into descriptor pool!
E       Invalid proto descriptor for file "repeatedpacked.proto":
E         Test: "Test" is already defined in file "double.proto".

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, weird. It would be preferable to have full functional coverage, even if as a stopgap we have to mark it as a test that has to be run in isolation, so isn't included in the main test run. What do you think?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah i would also prefer full coverage!

we could also concider to isolate the test_binary_compatibility instead, because that's the one causing problems (probably because of libmodule.import), as i'm expecting we'll need more unit tests that import plugin.py, and each of them will conflict with test_binary_compatibility

we can update the CI to run both test sets separately

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

any idea how to mark the tests?
we could probably add groups/tags or something, but it does mean that contributors will need to know to run these tests separately.

would be great if we could let the standard pytest command run without problems 🤔

Copy link
Collaborator

@nat-n nat-n May 28, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Maybe worthwhile to make the input test loading a bit more sophisticated to handle this stuff more smoothly.

I'm thinking instead of expecting there to be a module member called Test, make it scan the members and import anything that starts with Test. Should be easy?

Copy link
Collaborator

Choose a reason for hiding this comment

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

As for marking the tests as separate test runs (requiring separate python runtimes), there might be a pytest feature or plugin to handle this otherwise having a test task (pipenv script, make target or wtv) that does the right tging should cover it. Though maybe solving the naming problem with smarter import logic makes the problem go away?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That could work, and i would like it to support Test* messages instead of mandatory names..

although using it as the only fix for this conflict, would make it mandatory that all proto messages have unique names... would be nice if we can avoid that

}
11 changes: 0 additions & 11 deletions ...to/tests/inputs/googletypes_service_returns_empty/googletypes_service_returns_empty.proto
View file
Open in desktop

This file was deleted.

14 changes: 14 additions & 0 deletions ...nputs/googletypes_service_returns_googletype/googletypes_service_returns_googletype.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
syntax = "proto3";

import "google/protobuf/empty.proto";
import "google/protobuf/struct.proto";

service Test {
rpc GetEmpty (RequestMessage) returns (google.protobuf.Empty);
rpc GetStruct (RequestMessage) returns (google.protobuf.Struct);
rpc GetListValue (RequestMessage) returns (google.protobuf.ListValue);
rpc GetValue (RequestMessage) returns (google.protobuf.Value);
}

message RequestMessage {
}