Re: [PR] Fix misleading NPE when request parameter is not Serializable [dubbo]

2026-06-24 Thread via GitHub 


zrlw merged PR #16299:
URL: https://github.com/apache/dubbo/pull/16299


-- 
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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Re: [PR] Fix misleading NPE when request parameter is not Serializable [dubbo]

2026-06-12 Thread via GitHub 


chensishang commented on PR #16299:
URL: https://github.com/apache/dubbo/pull/16299#issuecomment-4688607850

   @zrlw
   
   Sorry for the extra round — the review above pointed out that my error 
message was too specific, so I've updated the wording, which dismissed your 
earlier approval. Could you take another
 ▎ look when you have time? Thanks!
   


-- 
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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Re: [PR] Fix misleading NPE when request parameter is not Serializable [dubbo]

2026-06-12 Thread via GitHub 


chensishang commented on PR #16299:
URL: https://github.com/apache/dubbo/pull/16299#issuecomment-4688580250

   
   @zrlw 抱歉又要麻烦你一轮——上面的 review 
指出我的报错信息写得太具体了,所以我更新了措辞,这导致你之前的批准被自动作废了。方便的时候能再帮忙看一眼吗?谢谢!
   


-- 
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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Re: [PR] Fix misleading NPE when request parameter is not Serializable [dubbo]

2026-06-12 Thread via GitHub 


chensishang commented on PR #16299:
URL: https://github.com/apache/dubbo/pull/16299#issuecomment-4688527941

@zrlw Sorry for the extra round — the review above pointed out that my 
error message was too specific, so I've updated the wording, which dismissed 
your earlier approval. Could you take another
  look when you have time? Thanks!
   


-- 
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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Re: [PR] Fix misleading NPE when request parameter is not Serializable [dubbo]

2026-06-12 Thread via GitHub 


chensishang commented on code in PR #16299:
URL: https://github.com/apache/dubbo/pull/16299#discussion_r3401466735


##
dubbo-rpc/dubbo-rpc-dubbo/src/main/java/org/apache/dubbo/rpc/protocol/dubbo/DubboProtocol.java:
##
@@ -290,6 +290,14 @@ Invoker getInvoker(Channel channel, Invocation inv) 
throws RemotingException
 boolean isStubServiceInvoke;
 int port = channel.getLocalAddress().getPort();
 String path = (String) inv.getObjectAttachmentWithoutConvert(PATH_KEY);
+if (path == null) {
+throw new RemotingException(

Review Comment:
   Thanks for the careful review — you're right, and I appreciate you pointing 
this out. A null path indicates missing or corrupted invocation metadata in 
general, and my original message attributing   ▎ it solely to non-serializable 
parameters could indeed cause another misleading diagnosis

  I've updated the message to describe the problem generically and only 
mention deserialization failure as one of several possible causes (along with 
protocol incompatibility and custom 
  codec/invocation implementations). Please let me know if the wording 
could be further improved — happy to adjust.
   



-- 
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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Re: [PR] Fix misleading NPE when request parameter is not Serializable [dubbo]

2026-06-11 Thread via GitHub 


RainYuY commented on code in PR #16299:
URL: https://github.com/apache/dubbo/pull/16299#discussion_r3401016310


##
dubbo-rpc/dubbo-rpc-dubbo/src/main/java/org/apache/dubbo/rpc/protocol/dubbo/DubboProtocol.java:
##
@@ -290,6 +290,14 @@ Invoker getInvoker(Channel channel, Invocation inv) 
throws RemotingException
 boolean isStubServiceInvoke;
 int port = channel.getLocalAddress().getPort();
 String path = (String) inv.getObjectAttachmentWithoutConvert(PATH_KEY);
+if (path == null) {
+throw new RemotingException(

Review Comment:
   Is non-serializable parameter really the only scenario that can make path 
null here? In the normal Dubbo request format, the service path is 
encoded/decoded before arguments, and DubboInvoker also sets PATH_KEY before 
sending the request. So a null path seems more generally like missing/malformed 
invocation metadata, custom codec/invocation behavior, or decode/protocol 
incompatibility. Could we make this error message less specific than “Please 
ensure all parameter types implement Serializable” to avoid another misleading 
diagnosis?



-- 
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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Re: [PR] Fix misleading NPE when request parameter is not Serializable [dubbo]

2026-06-11 Thread via GitHub 


chensishang commented on PR #16299:
URL: https://github.com/apache/dubbo/pull/16299#issuecomment-4680712630

   @RainYuY
   
   Hi, thanks for your time.
   
   This PR has already received one approval and all checks are passing. It 
fixes the misleading NPE issue described in #16293 and includes a corresponding 
unit test.
   
   Could you please help take a look when you have time?
   Thank you!


-- 
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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Re: [PR] Fix misleading NPE when request parameter is not Serializable [dubbo]

2026-06-01 Thread via GitHub 


chensishang commented on PR #16299:
URL: https://github.com/apache/dubbo/pull/16299#issuecomment-4594331489

   
 Hi @zrlw, thanks for the review!
   
 The PR contains two commits: the actual fix (`6a870129`) and a merge 
commit for syncing with the latest 3.3 branch. The only real change is in 
`DubboProtocol.java` and `DubboProtocolTest.java`.
   
 The maintainer can squash on merge if needed. Sorry for the noise!
   


-- 
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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Re: [PR] Fix misleading NPE when request parameter is not Serializable [dubbo]

2026-05-31 Thread via GitHub 


chensishang commented on PR #16299:
URL: https://github.com/apache/dubbo/pull/16299#issuecomment-4589654056

   Thanks for the review!


-- 
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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Re: [PR] Fix misleading NPE when request parameter is not Serializable [dubbo]

2026-05-30 Thread via GitHub 


chensishang commented on PR #16299:
URL: https://github.com/apache/dubbo/pull/16299#issuecomment-4585687449

   Hi reviewers, I noticed the Codecov check failed with "0% coverage". After 
checking, this seems to be due to an incomplete report upload 
("HEAD比BASE少7次上传") rather than a problem with the code change itself.
   
   I have already added a unit test (testGetInvokerThrowsOnNullPath) which 
covers the new null-check logic, and it passes locally. The Codecov result 
appears to be a false negative caused by the CI infrastructure.
   
   Could you please take another look? Thanks!


-- 
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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Re: [PR] Fix misleading NPE when request parameter is not Serializable [dubbo]

2026-05-30 Thread via GitHub 


codecov-commenter commented on PR #16299:
URL: https://github.com/apache/dubbo/pull/16299#issuecomment-4585632305

   ## 
[Codecov](https://app.codecov.io/gh/apache/dubbo/pull/16299?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
 Report
   :x: Patch coverage is `0%` with `3 lines` in your changes missing coverage. 
Please review.
   :white_check_mark: Project coverage is 29.74%. Comparing base 
([`dfdf39b`](https://app.codecov.io/gh/apache/dubbo/commit/dfdf39bbdd9fed1ffef73cb487ab059ce0b1b4ec?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache))
 to head 
([`6a87012`](https://app.codecov.io/gh/apache/dubbo/commit/6a87012949247bf70ceae552e505ae7f9b38467d?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
   
   | [Files with missing 
lines](https://app.codecov.io/gh/apache/dubbo/pull/16299?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
 | Patch % | Lines |
   |---|---|---|
   | 
[...apache/dubbo/rpc/protocol/dubbo/DubboProtocol.java](https://app.codecov.io/gh/apache/dubbo/pull/16299?src=pr&el=tree&filepath=dubbo-rpc%2Fdubbo-rpc-dubbo%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fdubbo%2Frpc%2Fprotocol%2Fdubbo%2FDubboProtocol.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZHViYm8tcnBjL2R1YmJvLXJwYy1kdWJiby9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvZHViYm8vcnBjL3Byb3RvY29sL2R1YmJvL0R1YmJvUHJvdG9jb2wuamF2YQ==)
 | 0.00% | [2 Missing and 1 partial :warning: 
](https://app.codecov.io/gh/apache/dubbo/pull/16299?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
 |
   > :exclamation:  There is a different number of reports uploaded between 
BASE (dfdf39b) and HEAD (6a87012). Click for more details.
   > 
   > HEAD has 8 uploads less than BASE
   >
   >| Flag | BASE (dfdf39b) | HEAD (6a87012) |
   >|--|--|--|
   >|samples-tests-java21|1|0|
   >|integration-tests-java21|1|0|
   >|integration-tests-java8|1|0|
   >|unit-tests-java25|1|0|
   >|unit-tests-java11|1|0|
   >|unit-tests-java21|1|0|
   >|unit-tests-java17|1|0|
   >|unit-tests-java8|1|0|
   >
   
   Additional details and impacted files
   
   
   
   ```diff
   @@  Coverage Diff  @@
   ##3.3   #16299   +/-   ##
   =
   - Coverage 60.78%   29.74%   -31.05% 
   + Complexity11772 9408 -2364 
   =
 Files  1953 1952-1 
 Lines 8918889169   -19 
 Branches  1345413371   -83 
   =
   - Hits  5421626521-27695 
   - Misses2940658530+29124 
   + Partials   5566 4118 -1448 
   ```
   
   | 
[Flag](https://app.codecov.io/gh/apache/dubbo/pull/16299/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
 | Coverage Δ | |
   |---|---|---|
   | 
[integration-tests-java21](https://app.codecov.io/gh/apache/dubbo/pull/16299/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
 | `?` | |
   | 
[integration-tests-java8](https://app.codecov.io/gh/apache/dubbo/pull/16299/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
 | `?` | |
   | 
[samples-tests-java21](https://app.codecov.io/gh/apache/dubbo/pull/16299/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
 | `?` | |
   | 
[samples-tests-java8](https://app.codecov.io/gh/apache/dubbo/pull/16299/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
 | `29.74% <0.00%> (-0.03%)` | :arrow_down: |
   | 
[unit-tests-java11](https://app.codecov.io/gh/apache/dubbo/pull/16299/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
 | `?` | |
   | 
[unit-tests-java17](https://app.codecov.io/gh/apache/dubbo/pull/16299/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
 | `?` | |
   | 
[unit-tests-java21](https://app.codecov.io/gh/apache/dubbo/pull/16299/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
 | `?` | |
   | 
[unit-tests-java25](https://app.codecov.io/gh/apache/dubbo/pull/16299/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_