Repository navigation
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a strictProtocolChecks parameter to the Client and Client.withoutJson constructors in json_rpc_2, allowing clients to accept responses that omit the jsonrpc parameter when set to false. This change is also integrated into Peer and supported by new unit tests. A review comment suggests simplifying the validation logic in _isResponseValid to improve readability and avoid redundant key lookups.
Comment on lines
+251
to
+257
| if (strictProtocolChecks && !response.containsKey('jsonrpc')) { | ||
| return false; | ||
| } | ||
| if ((strictProtocolChecks || response.containsKey('jsonrpc')) && | ||
| response['jsonrpc'] != '2.0') { | ||
| return false; | ||
| } |
Contributor
There was a problem hiding this comment.
The validation logic can be simplified and made more readable by checking if the response contains the 'jsonrpc' key first. This avoids redundant checks and multiple lookups of the same key.
if (response.containsKey('jsonrpc')) {
if (response['jsonrpc'] != '2.0') return false;
} else if (strictProtocolChecks) {
return false;
}
Member
There was a problem hiding this comment.
+1, the current conditionals appear redundant.
natebosch
requested changes
Aug 26, 2026
Comment on lines
+251
to
+257
| if (strictProtocolChecks && !response.containsKey('jsonrpc')) { | ||
| return false; | ||
| } | ||
| if ((strictProtocolChecks || response.containsKey('jsonrpc')) && | ||
| response['jsonrpc'] != '2.0') { | ||
| return false; | ||
| } |
Member
There was a problem hiding this comment.
+1, the current conditionals appear redundant.
hkarmoush
force-pushed
the
json-rpc2-strict-protocol-checks-client-2422
branch
from
August 27, 2026 21:40
3e67bb0 to
0a0baff
Compare
natebosch
approved these changes
Aug 27, 2026
Member
|
@hkarmoush - As I understand it we're not currently able to accept contributions attributed to claude, which can't sign the CLA. We'll only be able to land this if it's authored by your account, which I think has signed the CLA. |
hkarmoush
force-pushed
the
json-rpc2-strict-protocol-checks-client-2422
branch
from
August 30, 2026 20:51
0a0baff to
f4a0150
Compare
Server and Peer already accept a strictProtocolChecks flag that allows requests missing the jsonrpc field. Client had no equivalent, so there was no way to interop with peers whose responses omit that field. Mirrors the Server implementation: when disabled, a missing jsonrpc key is tolerated, but a present-and-wrong value is still rejected.
Restructure the jsonrpc key checks to look up response['jsonrpc'] once instead of via separate containsKey/value checks, per review feedback from natebosch and gemini-code-assist. Behavior is unchanged.
hkarmoush
force-pushed
the
json-rpc2-strict-protocol-checks-client-2422
branch
from
September 25, 2026 09:02
f4a0150 to
2e51298
Compare
copybara-service Bot
pushed a commit
to dart-lang/sdk
that referenced
this pull request
Oct 13, 2026
Revisions updated by `dart tools/rev_sdk_deps.dart`. dartdoc (https://github.com/dart-lang/dartdoc/compare/5eb0f2c..e1859cf): e1859cfc Fri Oct 9 14:53:01 2026 +0200 Sarah Zakarias Link a bare prefix only when it maps to exactly one library (dart-lang/dartdoc#4295) c48f6fb9 Fri Oct 9 14:25:42 2026 +0200 Sarah Zakarias Resolve prefixes through the analyzer's PrefixElement.scopeLibraries (dart-lang/dartdoc#4294) 3a6297e2 Wed Oct 7 12:56:14 2026 +0200 Sarah Zakarias Require analyzer 14.5.0 and bump to 9.0.10 (dart-lang/dartdoc#4292) ecosystem (https://github.com/dart-lang/ecosystem/compare/16dca7a..622ef61): 622ef61 Mon Oct 12 00:23:27 2026 -0700 Kevin Moore fix(firehose): enforce -wip on published versions and tighten changelog checks (dart-lang/ecosystem#467) 5576a85 Wed Oct 7 09:29:44 2026 +0200 Moritz Support use-flutter option in groundskeeper workflow (dart-lang/ecosystem#444) http (https://github.com/dart-lang/http/compare/585d433..23f8574): 23f8574 Thu Oct 8 23:27:25 2026 +0700 Cong Bang feat(http2): configurable receive windows and frame size (dart-lang/http#1999) 18051a6 Thu Oct 8 10:39:38 2026 +0200 Moritz Enable Flutter in Groundskeeper workflow for Flutter packages (dart-lang/http#2004) native (https://github.com/dart-lang/native/compare/a2a6179..10515af): 10515afca Mon Oct 12 12:21:52 2026 +0200 Daco Harkes [native_toolchain_c] Do not pass linker flags when compiling static libraries (dart-lang/native#3731) native is held back at 10515af, before dart-lang/native#3728, because it breaks the dartdev `dart install` native assets tests. See dart-lang/native#3743. test (https://github.com/dart-lang/test/compare/d749b10..30acb69): 30acb69e Mon Oct 12 12:42:54 2026 -0700 Nate Bosch Time out browser loads just before the suite load timeout (dart-lang/test#2786) 597a0326 Mon Oct 12 12:22:53 2026 -0700 Kevin Moore Fix misplaced changelog entries and remove no-op health check (dart-lang/test#2789) 0b6efee8 Mon Oct 12 12:16:29 2026 -0700 Nate Bosch [checks] Collapse nested subjects in condition descriptions (dart-lang/test#2784) 17c98493 Mon Oct 12 12:08:23 2026 -0700 Nate Bosch Follow symlinks in precompiled browser test directories (dart-lang/test#2787) 1cc813ec Fri Oct 9 11:59:56 2026 -0700 Nate Bosch [checks] Join nested collapsed subjects with "that" (dart-lang/test#2783) ada7b950 Thu Oct 8 12:03:42 2026 -0700 Nate Bosch Prepare to publish (dart-lang/test#2780) c4b165b3 Wed Oct 7 11:29:08 2026 -0700 Kevin Moore [checks] Require package:meta ^1.17.0 (dart-lang/test#2779) 4a363d70 Tue Oct 6 10:46:31 2026 -0700 Nate Bosch Let Chrome choose its remote debugging port (dart-lang/test#2778) 465d66ab Tue Oct 6 09:35:22 2026 -0700 Nate Bosch Keep Chrome stderr in the browser output when debugging (dart-lang/test#2777) tools (https://github.com/dart-lang/tools/compare/a66d517..2b54c71): 2b54c71af Mon Oct 12 10:16:45 2026 +0200 dart-ecosystem-bot[bot] [boring] Update BoringSSL to 3fed4f20 (dart-lang/tools#2655) 2337f995a Sat Oct 10 03:11:40 2026 +0300 Hasan Armoush [json_rpc_2] Add strictProtocolChecks parameter to Client (dart-lang/tools#2555) c65c69ed1 Fri Oct 9 08:54:25 2026 +0200 Moritz [boring] Move package:boring to dart-lang/tools (2/4): FFI helpers from package:webcrypto (dart-lang/tools#2615) a04e45491 Thu Oct 8 21:07:18 2026 -0700 Kevin Moore chore: wire package:api_summary and package:string_scanner PR auto-labeler (dart-lang/tools#2652) 8bdd4b92c Thu Oct 8 09:20:48 2026 +0200 Morgan :) [code_builder] Allow built_collection 6.x. (dart-lang/tools#2645) c16040d35 Wed Oct 7 17:30:08 2026 -0700 Kevin Moore [api_summary] Sort primary barrel first and experimental entry points last (v1.2.0) (dart-lang/tools#2619) b2d0780db Wed Oct 7 11:27:16 2026 -0700 Kevin Moore [stream_channel] Require package:async ^2.6.0 (dart-lang/tools#2644) 3ab7bf9d0 Wed Oct 7 11:35:27 2026 +0200 Moritz [boring] Move package:boring to dart-lang/tools (1/4): vendored BoringSSL, bindings and source build (dart-lang/tools#2614) 29df6348b Tue Oct 6 15:21:44 2026 -0700 Kevin Moore feat(coverage): support web coverage reports in test_with_coverage (dart-lang/tools#2448) Regenerate pkg/{analyzer,analyzer_plugin,analyzer_testing}/api.txt. The rolled package:api_summary v1.2.0 (dart-lang/tools#2619) sorts the primary barrel first and other entry points after it, so entries in these files are reordered. The APIs themselves are unchanged. Change-Id: Ie8ff19c5a981ea8ecee8939c75457986a947e0a7 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/563220 Reviewed-by: Samuel Rawlins <[email protected]> Auto-Submit: Nate Bosch <[email protected]> Reviewed-by: Paul Berry <[email protected]> Commit-Queue: Paul Berry <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #2422
ServerandPeeralready accept astrictProtocolChecksflag that allows requests which don't include thejsonrpcfield.Clienthad no equivalent, so there was no way to interoperate with a peer whose responses omit that field.This mirrors the
Serverimplementation exactly: whenstrictProtocolChecksisfalse, a response missing thejsonrpckey is tolerated, but a response with ajsonrpckey set to the wrong value is still rejected.Also threads the flag through
Peer.withoutJsonto the internalClient, soPeer(strictProtocolChecks: false)relaxes response validation on the client side too, consistent with its existing effect on the server side.Changes
strictProtocolChecksparam toClientandClient.withoutJson(defaulttrue, matchingServer).Client._isResponseValidto only require thejsonrpckey when strict.strictProtocolChecksthrough to the internalClientinPeer.withoutJson.4.2.0-wipwith a CHANGELOG entry.Test plan
jsonrpckey whenstrictProtocolChecks: false, and confirms a wrongjsonrpcversion is still rejected even when non-strict.dart test -p vm: 127/127 passing.dart analyze: no issues.dart format --set-exit-if-changed: clean.