You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@thrift.apache.org by "Jens-G (via GitHub)" <gi...@apache.org> on 2024/10/11 00:44:13 UTC
[PR] THRIFT-5823 Fix illegal uses of exceptions as normal struct type [thrift]
Jens-G opened a new pull request, #3051:
URL: https://github.com/apache/thrift/pull/3051
Patch: Jens Geyer
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] THRIFT-5823 Fix illegal uses of exceptions as normal struct type [thrift]
Posted by "fishy (via GitHub)" <gi...@apache.org>.
fishy commented on code in PR #3051:
URL: https://github.com/apache/thrift/pull/3051#discussion_r1797244505
##########
lib/go/test/ClientMiddlewareExceptionTest.thrift:
##########
@@ -25,11 +25,11 @@ exception Exception2 {
// This is a special case, we want to make sure that the middleware don't
// accidentally pull result as error.
-exception FooResponse {
+struct/*exception*/ FooResponse { // returning an exception by any means other than "throws" is illegal
Review Comment:
I think I can construct the test differently (construct the go struct directly instead of via thrift IDL) to keep the test to do what it does, though.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] THRIFT-5823 Fix illegal uses of exceptions as normal struct type [thrift]
Posted by "Jens-G (via GitHub)" <gi...@apache.org>.
Jens-G commented on code in PR #3051:
URL: https://github.com/apache/thrift/pull/3051#discussion_r1797403047
##########
lib/go/test/ClientMiddlewareExceptionTest.thrift:
##########
@@ -25,11 +25,11 @@ exception Exception2 {
// This is a special case, we want to make sure that the middleware don't
// accidentally pull result as error.
-exception FooResponse {
+struct/*exception*/ FooResponse { // returning an exception by any means other than "throws" is illegal
Review Comment:
> I think I can construct the test differently [...] to keep the test to do what it does, though.
Great, thanks!
> if your plan is to forbid that on the compiler level
I am actually thinking about this, indeed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] THRIFT-5823 Fix illegal uses of exceptions as normal struct type [thrift]
Posted by "cameron-martin (via GitHub)" <gi...@apache.org>.
cameron-martin commented on PR #3051:
URL: https://github.com/apache/thrift/pull/3051#issuecomment-2568059003
Not sure what's gone on here in terms of history, but presumably this was merged as commit 8b6d29d3ee7fb1bece7be1eb728e2e88e867cc2b.
I will repeat what I've said on the commit for visibility:
This change makes the thrift compiler segfault during execution of the NodeJS tests. Note these tests are not run on CI, which is why this will have slipped through the cracks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] THRIFT-5823 Fix illegal uses of exceptions as normal struct type [thrift]
Posted by "fishy (via GitHub)" <gi...@apache.org>.
fishy commented on code in PR #3051:
URL: https://github.com/apache/thrift/pull/3051#discussion_r1797242179
##########
lib/go/test/ClientMiddlewareExceptionTest.thrift:
##########
@@ -25,11 +25,11 @@ exception Exception2 {
// This is a special case, we want to make sure that the middleware don't
// accidentally pull result as error.
-exception FooResponse {
+struct/*exception*/ FooResponse { // returning an exception by any means other than "throws" is illegal
Review Comment:
the purpose of this test is to verify that the library code does not have a bug to accidentally pull the exception response out as the thrown exception, so this change will actually make the test always pass even if someone introduces such a bug in the future.
but if your plan is to forbid that on the compiler level so this scenario would never happen in the first place, then I think this is fine.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] THRIFT-5823 Fix illegal uses of exceptions as normal struct type [thrift]
Posted by "Jens-G (via GitHub)" <gi...@apache.org>.
Jens-G commented on PR #3051:
URL: https://github.com/apache/thrift/pull/3051#issuecomment-2408749177
> if your plan is to forbid that on the compiler level
implemented
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] THRIFT-5823 Fix illegal uses of exceptions as normal struct type [thrift]
Posted by "Jens-G (via GitHub)" <gi...@apache.org>.
Jens-G closed pull request #3051: THRIFT-5823 Fix illegal uses of exceptions as normal struct type
URL: https://github.com/apache/thrift/pull/3051
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] THRIFT-5823 Fix illegal uses of exceptions as normal struct type [thrift]
Posted by "Jens-G (via GitHub)" <gi...@apache.org>.
Jens-G commented on PR #3051:
URL: https://github.com/apache/thrift/pull/3051#issuecomment-2571420820
> Should these changes be reverted?
No but we accept pull reuiqests., Would that work for you?
Alternatively, a minimal viable example how to reproduce could help too.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] THRIFT-5823 Fix illegal uses of exceptions as normal struct type [thrift]
Posted by "cameron-martin (via GitHub)" <gi...@apache.org>.
cameron-martin commented on PR #3051:
URL: https://github.com/apache/thrift/pull/3051#issuecomment-2572833393
> > Should these changes be reverted?
>
> No but we accept pull requests., Would that work for you? Alternatively, a minimal viable example how to reproduce could help too.
This was actually an issue with incremental compilation. Ignore the above.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org