Skip to content

Commit 8049ffd

Browse files
THRIFT-4623 Fix python complex nested struct includes
Fix the bug where the following: ``` struct C { 1: i32 value } struct A { 1: C nested = {} } struct B { 1: A itm = {} } ``` would generate the following invalid python code: ``` class C: thrift_spec = None def __init__(self, value=None): self.value = value class A: thrift_spec = None def __init__(self, nested=C()): if nested is self.thrift_spec[1][4]: nested = C() self.nested = nested class B: thrift_spec = None def __init__(self, itm=A()): if itm is self.thrift_spec[1][4]: itm = A() self.itm = itm ``` The problem with the above code is that `self.thrift_spec` was initialized and set after the module was loaded. In python this would work when B is instantiated but since B immediately invokes A() it happens before the thrift_spec is set and this would result in `TypeError: 'NoneType' object is not subscriptable`. By first initializing a temporary placeholder with a unique address to compare to called `_THRIFT_DEFAULT` the generated code would look like: ``` _THRIFT_DEFAULT = object() class C: def __init__(self, value=None): self.value = value class A: def __init__(self, nested=_THRIFT_DEFAULT): if nested is _THRIFT_DEFAULT: nested = C() class B: def __init__(self, itm=_THRIFT_DEFAULT): if itm is _THRIFT_DEFAULT: itm = A() ``` Here the instantiation of the default object is defered until after the module has been fully loaded resulting in the correct value being assigned.
1 parent 405a95f commit 8049ffd

4 files changed

Lines changed: 100 additions & 4 deletions

File tree

compiler/cpp/src/thrift/generate/t_py_generator.cc

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -452,6 +452,7 @@ void t_py_generator::init_generator() {
452452
} else {
453453
f_types_ << "all_structs = []" << '\n';
454454
}
455+
f_types_ << "_THRIFT_DEFAULT = object()" << '\n';
455456

456457
f_consts_ <<
457458
py_autogen_comment() << '\n' <<
@@ -907,10 +908,9 @@ void t_py_generator::generate_py_struct_definition(ostream& out,
907908

908909
for (m_iter = members.begin(); m_iter != members.end(); ++m_iter) {
909910
// Initialize fields
910-
t_type* type = (*m_iter)->get_type();
911+
t_type* type = get_true_type((*m_iter)->get_type());
911912
if (!type->is_base_type() && !type->is_enum() && (*m_iter)->get_value() != nullptr) {
912-
indent(out) << "if " << maybe_escape_identifier((*m_iter)->get_name()) << " is "
913-
<< "self.thrift_spec[" << (*m_iter)->get_key() << "][4]:" << '\n';
913+
indent(out) << "if " << maybe_escape_identifier((*m_iter)->get_name()) << " is _THRIFT_DEFAULT:" << '\n';
914914
indent_up();
915915
indent(out) << maybe_escape_identifier((*m_iter)->get_name()) << " = " << render_field_default_value(*m_iter)
916916
<< '\n';
@@ -1319,6 +1319,7 @@ void t_py_generator::generate_service(t_service* tservice) {
13191319
}
13201320

13211321
f_service_ << "all_structs = []" << '\n';
1322+
f_service_ << "_THRIFT_DEFAULT = object()" << '\n';
13221323

13231324
// Generate the three main parts of the service
13241325
generate_service_interface(tservice);
@@ -2784,7 +2785,12 @@ string t_py_generator::declare_argument(t_field* tfield) {
27842785

27852786
result << " = ";
27862787
if (tfield->get_value() != nullptr) {
2787-
result << render_field_default_value(tfield);
2788+
t_type* type = get_true_type(tfield->get_type());
2789+
if (!type->is_base_type() && !type->is_enum()) {
2790+
result << "_THRIFT_DEFAULT";
2791+
} else {
2792+
result << render_field_default_value(tfield);
2793+
}
27882794
} else {
27892795
result << "None";
27902796
}

lib/py/Makefile.am

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -63,6 +63,7 @@ check-local: all py3-test
6363
$(PYTHON) test/thrift_TNonblockingServer.py
6464
$(PYTHON) test/thrift_TSerializer.py
6565
THRIFT=${THRIFT} $(PYTHON) test/test_compiler/test_keyword_escape.py
66+
THRIFT=${THRIFT} $(PYTHON) test/test_compiler/test_default_struct.py
6667
$(PYTHON) test/test_recursion_depth.py
6768

6869

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
1+
//
2+
// Licensed to the Apache Software Foundation (ASF) under one
3+
// or more contributor license agreements. See the NOTICE file
4+
// distributed with this work for additional information
5+
// regarding copyright ownership. The ASF licenses this file
6+
// to you under the Apache License, Version 2.0 (the
7+
// "License"); you may not use this file except in compliance
8+
// with the License. You may obtain a copy of the License at
9+
//
10+
// http://www.apache.org/licenses/LICENSE-2.0
11+
//
12+
// Unless required by applicable law or agreed to in writing,
13+
// software distributed under the License is distributed on an
14+
// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
15+
// KIND, either express or implied. See the License for the
16+
// specific language governing permissions and limitations
17+
// under the License.
18+
19+
namespace py thrift4623
20+
21+
struct C {
22+
1: i32 value
23+
}
24+
25+
struct A {
26+
1: C nested = {}
27+
}
28+
29+
struct B {
30+
1: A itm = {}
31+
}
Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,58 @@
1+
#!/usr/bin/env python
2+
# Licensed to the Apache Software Foundation (ASF) under one
3+
# or more contributor license agreements. See the NOTICE file
4+
# distributed with this work for additional information
5+
# regarding copyright ownership. The ASF licenses this file
6+
# to you under the Apache License, Version 2.0 (the
7+
# "License"); you may not use this file except in compliance
8+
# with the License. You may obtain a copy of the License at
9+
#
10+
# http://www.apache.org/licenses/LICENSE-2.0
11+
#
12+
# Unless required by applicable law or agreed to in writing,
13+
# software distributed under the License is distributed on an
14+
# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
15+
# KIND, either express or implied. See the License for the
16+
# specific language governing permissions and limitations
17+
# under the License.
18+
19+
import importlib
20+
import os
21+
import subprocess
22+
import sys
23+
import tempfile
24+
25+
from test_keyword_escape import find_thrift
26+
27+
28+
def test_default_struct_value():
29+
thrift_file = os.path.join(os.path.dirname(__file__), 'Thrift4623.thrift')
30+
thrift_bin = find_thrift()
31+
if not thrift_bin:
32+
print("WARNING: thrift compiler not found, skipping test")
33+
return 0
34+
35+
with tempfile.TemporaryDirectory() as tmpdir:
36+
result = subprocess.run(
37+
[thrift_bin, '-gen', 'py', '-out', tmpdir, thrift_file],
38+
capture_output=True, text=True)
39+
if result.returncode != 0:
40+
raise AssertionError("thrift compiler failed: " + result.stderr)
41+
42+
sys.path.insert(0, tmpdir)
43+
try:
44+
types = importlib.import_module('thrift4623.ttypes')
45+
value = types.B()
46+
assert isinstance(value.itm, types.A)
47+
assert isinstance(value.itm.nested, types.C)
48+
assert value.itm.nested.value is None
49+
finally:
50+
sys.path.pop(0)
51+
for name in list(sys.modules):
52+
if name == 'thrift4623' or name.startswith('thrift4623.'):
53+
del sys.modules[name]
54+
55+
56+
if __name__ == '__main__':
57+
test_default_struct_value()
58+
print('OK: default-valued struct fields are usable')

0 commit comments

Comments
 (0)