Skip to content

Commit 8f5afcd

Browse files
ahilgerfacebook-github-bot
authored andcommitted
fix bug where named exceptions translated to ApplicationError.UNKNOWN
Summary: Tests were showing services throwing `ApplicationError.UNKNOWN` when a named thrift exception was expected. This is a result of `types/cython_python_type` resolving to `object` when in-place migrate enabled. In the two places changed in this diff, the python type is needed, not the cython type. For regular build, `types/cython_python_type` and `types/python_type` are equivalent, so there is no change to trunk behavior. The other flavor of issues is being careful about py3 vs python type before/after thrift-python converter. The inter-conversion is cheap, but have to get it right. Reviewed By: prakashgayasen Differential Revision: D72479041 fbshipit-source-id: 21363b1c7ed2844a8ca5ed6210098eed51c8abaf
1 parent 6534cfc commit 8f5afcd

File tree

7 files changed

+23
-21
lines changed

7 files changed

+23
-21
lines changed

third-party/thrift/src/thrift/compiler/generate/templates/py3/services.pyx.mustache

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -167,10 +167,10 @@ async def runGenerator_{{service:name}}_{{function:name}}(object generator, Prom
167167
promise.cPromise.setValue(optional[{{> types/cython_cpp_type}}]())
168168
{{#function:stream_exceptions}}
169169
{{#field:type}}
170-
except {{> types/cython_python_type}} as ex:
170+
except {{> types/python_type}} as ex:
171171
promise.cPromise.setException({{!
172172
}}{{#program:inplace_migrate?}}{{#type:struct}}{{!
173-
}}{{type:capi_converter_path}}.{{struct:name}}_convert_to_cpp(ex){{!
173+
}}{{type:capi_converter_path}}.{{struct:name}}_convert_to_cpp(ex._to_python()){{!
174174
}}{{/type:struct}}{{/program:inplace_migrate?}}{{^program:inplace_migrate?}}{{!
175175
}}deref((<{{> types/cython_python_type}}> ex).{{> types/cpp_obj}}){{!
176176
}}{{/program:inplace_migrate?}})

third-party/thrift/src/thrift/compiler/generate/templates/py3/services/callback.mustache

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -111,7 +111,7 @@ async def {{service:name}}_{{function:name}}_coro(
111111
{{/function:return_type}}
112112
{{#function:exceptions}}
113113
{{#field:type}}
114-
except {{> types/cython_python_type}} as ex:
114+
except {{> types/python_type}} as ex:
115115
promise.cPromise.setException{{!
116116
}}{{^program:inplace_migrate?}}{{!
117117
}}(deref((<{{> types/cython_python_type}}> ex).{{> types/cpp_obj}})){{!

third-party/thrift/src/thrift/compiler/generate/templates/py3/stream/cython_cpp_value_to_python_value.mustache

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -40,8 +40,9 @@ to a python type.
4040
}}{{#type:binary?}}_value{{/type:binary?}}{{!
4141
}}{{#type:struct}}{{!
4242
}}{{#program:inplace_migrate?}}{{!
43+
}}{{> types/python_type}}.from_python({{!
4344
}}{{type:capi_converter_path}}.{{struct:name}}_from_cpp({{!
44-
}}__deref_const[{{> types/cython_cpp_type }}](_value)){{!
45+
}}__deref_const[{{> types/cython_cpp_type }}](_value))){{!
4546
}}{{/program:inplace_migrate?}}{{^program:inplace_migrate?}}{{!
4647
}}{{> types/cython_python_type}}._create_FBTHRIFT_ONLY_DO_NOT_USE({{!
4748
}}make_shared[{{> types/cython_cpp_type}}](_value)){{!

third-party/thrift/src/thrift/compiler/generate/templates/py3/types.pyx.mustache

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -599,8 +599,9 @@ cdef class ClientBufferedStream__{{type:flat_name}}(ClientBufferedStream):
599599
}}result.hasException[{{> types/cython_cpp_type}}]():
600600
pyfuture.set_exception({{!
601601
}}{{#program:inplace_migrate?}}{{#type:struct}}{{!
602+
}}{{> types/python_type}}.from_python({{!
602603
}}{{type:capi_converter_path}}.{{struct:name}}_from_cpp({{!
603-
}}thrift.py3.exceptions.unwrap_exception[{{> types/cython_cpp_type}}](result.exception())){{!
604+
}}thrift.py3.exceptions.unwrap_exception[{{> types/cython_cpp_type}}](result.exception()))){{!
604605
}}{{/type:struct}}{{/program:inplace_migrate?}}{{^program:inplace_migrate?}}{{!
605606
}}{{> types/cython_python_type}}._create_FBTHRIFT_ONLY_DO_NOT_USE({{!
606607
}}thrift.py3.exceptions.try_make_shared_exception[{{> types/cython_cpp_type}}](result.exception())){{!

third-party/thrift/src/thrift/compiler/test/fixtures/py3/out/py3_inplace/gen-py3/module/services.pyx

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1981,7 +1981,7 @@ async def SimpleService_expected_exception_coro(
19811981
):
19821982
try:
19831983
result = await self.expected_exception()
1984-
except object as ex:
1984+
except _module_types.SimpleException as ex:
19851985
promise.cPromise.setException(_module_thrift_converter.SimpleException_convert_to_cpp(ex._to_python()))
19861986
except __ApplicationError as ex:
19871987
# If the handler raised an ApplicationError convert it to a C++ one

third-party/thrift/src/thrift/compiler/test/fixtures/stream/out/py3_inplace/gen-py3/module/services.pyx

Lines changed: 14 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -139,8 +139,8 @@ async def runGenerator_PubSubStreamingService_streamthrows(object generator, Pro
139139
item = await generator.asend(None)
140140
except StopAsyncIteration:
141141
promise.cPromise.setValue(optional[cint32_t]())
142-
except object as ex:
143-
promise.cPromise.setException(_module_thrift_converter.FooStreamEx_convert_to_cpp(ex))
142+
except _module_types.FooStreamEx as ex:
143+
promise.cPromise.setException(_module_thrift_converter.FooStreamEx_convert_to_cpp(ex._to_python()))
144144
except __ApplicationError as ex:
145145
# If the handler raised an ApplicationError convert it to a C++ one
146146
promise.cPromise.setException(cTApplicationException(
@@ -246,8 +246,8 @@ async def runGenerator_PubSubStreamingService_boththrows(object generator, Promi
246246
item = await generator.asend(None)
247247
except StopAsyncIteration:
248248
promise.cPromise.setValue(optional[cint32_t]())
249-
except object as ex:
250-
promise.cPromise.setException(_module_thrift_converter.FooStreamEx_convert_to_cpp(ex))
249+
except _module_types.FooStreamEx as ex:
250+
promise.cPromise.setException(_module_thrift_converter.FooStreamEx_convert_to_cpp(ex._to_python()))
251251
except __ApplicationError as ex:
252252
# If the handler raised an ApplicationError convert it to a C++ one
253253
promise.cPromise.setException(cTApplicationException(
@@ -283,8 +283,8 @@ async def runGenerator_PubSubStreamingService_responseandstreamstreamthrows(obje
283283
item = await generator.asend(None)
284284
except StopAsyncIteration:
285285
promise.cPromise.setValue(optional[cint32_t]())
286-
except object as ex:
287-
promise.cPromise.setException(_module_thrift_converter.FooStreamEx_convert_to_cpp(ex))
286+
except _module_types.FooStreamEx as ex:
287+
promise.cPromise.setException(_module_thrift_converter.FooStreamEx_convert_to_cpp(ex._to_python()))
288288
except __ApplicationError as ex:
289289
# If the handler raised an ApplicationError convert it to a C++ one
290290
promise.cPromise.setException(cTApplicationException(
@@ -355,8 +355,8 @@ async def runGenerator_PubSubStreamingService_responseandstreamboththrows(object
355355
item = await generator.asend(None)
356356
except StopAsyncIteration:
357357
promise.cPromise.setValue(optional[cint32_t]())
358-
except object as ex:
359-
promise.cPromise.setException(_module_thrift_converter.FooStreamEx_convert_to_cpp(ex))
358+
except _module_types.FooStreamEx as ex:
359+
promise.cPromise.setException(_module_thrift_converter.FooStreamEx_convert_to_cpp(ex._to_python()))
360360
except __ApplicationError as ex:
361361
# If the handler raised an ApplicationError convert it to a C++ one
362362
promise.cPromise.setException(cTApplicationException(
@@ -928,7 +928,7 @@ async def PubSubStreamingService_servicethrows_coro(
928928
result = await result
929929
if isinstance(result, AsyncIterator):
930930
result = ServerStream_cint32_t._fbthrift_create(cmove(createAsyncIteratorFromPyIterator[cint32_t](result, get_executor(), &getNextGenerator_PubSubStreamingService_servicethrows)))
931-
except object as ex:
931+
except _module_types.FooEx as ex:
932932
promise.cPromise.setException(_module_thrift_converter.FooEx_convert_to_cpp(ex._to_python()))
933933
except __ApplicationError as ex:
934934
# If the handler raised an ApplicationError convert it to a C++ one
@@ -964,9 +964,9 @@ async def PubSubStreamingService_servicethrows2_coro(
964964
result = await result
965965
if isinstance(result, AsyncIterator):
966966
result = ServerStream_cint32_t._fbthrift_create(cmove(createAsyncIteratorFromPyIterator[cint32_t](result, get_executor(), &getNextGenerator_PubSubStreamingService_servicethrows2)))
967-
except object as ex:
967+
except _module_types.FooEx as ex:
968968
promise.cPromise.setException(_module_thrift_converter.FooEx_convert_to_cpp(ex._to_python()))
969-
except object as ex:
969+
except _module_types.FooEx2 as ex:
970970
promise.cPromise.setException(_module_thrift_converter.FooEx2_convert_to_cpp(ex._to_python()))
971971
except __ApplicationError as ex:
972972
# If the handler raised an ApplicationError convert it to a C++ one
@@ -1002,7 +1002,7 @@ async def PubSubStreamingService_boththrows_coro(
10021002
result = await result
10031003
if isinstance(result, AsyncIterator):
10041004
result = ServerStream_cint32_t._fbthrift_create(cmove(createAsyncIteratorFromPyIterator[cint32_t](result, get_executor(), &getNextGenerator_PubSubStreamingService_boththrows)))
1005-
except object as ex:
1005+
except _module_types.FooEx as ex:
10061006
promise.cPromise.setException(_module_thrift_converter.FooEx_convert_to_cpp(ex._to_python()))
10071007
except __ApplicationError as ex:
10081008
# If the handler raised an ApplicationError convert it to a C++ one
@@ -1076,7 +1076,7 @@ async def PubSubStreamingService_responseandstreamservicethrows_coro(
10761076
result = await result
10771077
if isinstance(result, AsyncIterator):
10781078
result = ServerStream_cint32_t._fbthrift_create(cmove(createAsyncIteratorFromPyIterator[cint32_t](result, get_executor(), &getNextGenerator_PubSubStreamingService_responseandstreamservicethrows)))
1079-
except object as ex:
1079+
except _module_types.FooEx as ex:
10801080
promise.cPromise.setException(_module_thrift_converter.FooEx_convert_to_cpp(ex._to_python()))
10811081
except __ApplicationError as ex:
10821082
# If the handler raised an ApplicationError convert it to a C++ one
@@ -1114,7 +1114,7 @@ async def PubSubStreamingService_responseandstreamboththrows_coro(
11141114
result = await result
11151115
if isinstance(result, AsyncIterator):
11161116
result = ServerStream_cint32_t._fbthrift_create(cmove(createAsyncIteratorFromPyIterator[cint32_t](result, get_executor(), &getNextGenerator_PubSubStreamingService_responseandstreamboththrows)))
1117-
except object as ex:
1117+
except _module_types.FooEx as ex:
11181118
promise.cPromise.setException(_module_thrift_converter.FooEx_convert_to_cpp(ex._to_python()))
11191119
except __ApplicationError as ex:
11201120
# If the handler raised an ApplicationError convert it to a C++ one

third-party/thrift/src/thrift/compiler/test/fixtures/stream/out/py3_inplace/gen-py3/module/types.pyx

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -79,7 +79,7 @@ cdef class ClientBufferedStream__i32(ClientBufferedStream):
7979
cdef cint32_t _value
8080
stream, pyfuture, rpc_options = <object> userdata
8181
if result.hasException[_module_cbindings.cFooStreamEx]():
82-
pyfuture.set_exception(_module_thrift_converter.FooStreamEx_from_cpp(thrift.py3.exceptions.unwrap_exception[_module_cbindings.cFooStreamEx](result.exception())))
82+
pyfuture.set_exception(FooStreamEx.from_python(_module_thrift_converter.FooStreamEx_from_cpp(thrift.py3.exceptions.unwrap_exception[_module_cbindings.cFooStreamEx](result.exception()))))
8383
elif result.hasException():
8484
pyfuture.set_exception(
8585
thrift.python.exceptions.create_py_exception(result.exception(), <__RpcOptions>rpc_options)

0 commit comments

Comments
 (0)