fix: change JSON-RPC request ID type from u32 to i64 (#416)

The JSON-RPC 2.0 specification allows the ID field to be any JSON number,
including negative integers and large values. The previous u32 implementation
was limited to 0-4,294,967,295 and couldn't handle negative IDs.

Changes:
- Changed NumberOrString::Number from u32 to i64 to support full JSON number range
- Updated deserializer to handle both signed and unsigned integers
- Modified AtomicU32Provider to use AtomicU64 internally with i64 conversion
- Fixed progress token handling in meta.rs for i64 values
- Added comprehensive test for negative and large request IDs

This ensures full compliance with the JSON-RPC 2.0 specification.

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-authored-by: Claude <noreply@anthropic.com>
This commit is contained in:
David Soria Parra 2025-09-04 20:22:44 +01:00 • committed by GitHub
commit 9105ba7963
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
3 changed files with 114 additions and 14 deletions

View file

@ -186,7 +186,7 @@ impl<'de> Deserialize<'de> for ProtocolVersion {
#[derive(Debug, Clone, Eq, PartialEq, Hash)]
pub enum NumberOrString {
/// A numeric identifier
Number(u32),
Number(i64),
/// A string identifier
String(Arc<str>),
}
@ -228,10 +228,20 @@ impl<'de> Deserialize<'de> for NumberOrString {
{
let value: Value = Deserialize::deserialize(deserializer)?;
match value {
Value::Number(n) => Ok(NumberOrString::Number(
n.as_u64()
.ok_or(serde::de::Error::custom("Expect an integer"))? as u32,
)),
Value::Number(n) => {
if let Some(i) = n.as_i64() {
Ok(NumberOrString::Number(i))
} else if let Some(u) = n.as_u64() {
// Handle large unsigned numbers that fit in i64
if u <= i64::MAX as u64 {
Ok(NumberOrString::Number(u as i64))
} else {
Err(serde::de::Error::custom("Number too large for i64"))
}
} else {
Err(serde::de::Error::custom("Expected an integer"))
}
}
Value::String(s) => Ok(NumberOrString::String(s.into())),
_ => Err(serde::de::Error::custom("Expect number or string")),
}
@ -1737,6 +1747,85 @@ mod tests {
assert_eq!(server_response_json, raw_response_json);
}
#[test]
fn test_negative_and_large_request_ids() {
// Test negative ID
let negative_id_json = json!({
"jsonrpc": "2.0",
"id": -1,
"method": "test",
"params": {}
});
let message: JsonRpcMessage =
serde_json::from_value(negative_id_json.clone()).expect("Should parse negative ID");
match &message {
JsonRpcMessage::Request(r) => {
assert_eq!(r.id, RequestId::Number(-1));
}
_ => panic!("Expected Request"),
}
// Test roundtrip serialization
let serialized = serde_json::to_value(&message).expect("Should serialize");
assert_eq!(serialized, negative_id_json);
// Test large negative ID
let large_negative_json = json!({
"jsonrpc": "2.0",
"id": -9007199254740991i64, // JavaScript's MIN_SAFE_INTEGER
"method": "test",
"params": {}
});
let message: JsonRpcMessage = serde_json::from_value(large_negative_json.clone())
.expect("Should parse large negative ID");
match &message {
JsonRpcMessage::Request(r) => {
assert_eq!(r.id, RequestId::Number(-9007199254740991i64));
}
_ => panic!("Expected Request"),
}
// Test large positive ID (JavaScript's MAX_SAFE_INTEGER)
let large_positive_json = json!({
"jsonrpc": "2.0",
"id": 9007199254740991i64,
"method": "test",
"params": {}
});
let message: JsonRpcMessage = serde_json::from_value(large_positive_json.clone())
.expect("Should parse large positive ID");
match &message {
JsonRpcMessage::Request(r) => {
assert_eq!(r.id, RequestId::Number(9007199254740991i64));
}
_ => panic!("Expected Request"),
}
// Test zero ID
let zero_id_json = json!({
"jsonrpc": "2.0",
"id": 0,
"method": "test",
"params": {}
});
let message: JsonRpcMessage =
serde_json::from_value(zero_id_json.clone()).expect("Should parse zero ID");
match &message {
JsonRpcMessage::Request(r) => {
assert_eq!(r.id, RequestId::Number(0));
}
_ => panic!("Expected Request"),
}
}
#[test]
fn test_protocol_version_order() {
let v1 = ProtocolVersion::V_2024_11_05;

View file

@ -116,9 +116,19 @@ impl Meta {
pub fn get_progress_token(&self) -> Option<ProgressToken> {
self.0.get(PROGRESS_TOKEN_FIELD).and_then(|v| match v {
Value::String(s) => Some(ProgressToken(NumberOrString::String(s.to_string().into()))),
Value::Number(n) => n
.as_u64()
.map(|n| ProgressToken(NumberOrString::Number(n as u32))),
Value::Number(n) => {
if let Some(i) = n.as_i64() {
Some(ProgressToken(NumberOrString::Number(i)))
} else if let Some(u) = n.as_u64() {
if u <= i64::MAX as u64 {
Some(ProgressToken(NumberOrString::Number(u as i64)))
} else {
None
}
} else {
None
}
}
_ => None,
})
}

View file

@ -193,7 +193,7 @@ impl<R: ServiceRole, S: Service<R>> DynService<R> for S {
use std::{
collections::{HashMap, VecDeque},
ops::Deref,
sync::{Arc, atomic::AtomicU32},
sync::{Arc, atomic::AtomicU64},
time::Duration,
};
@ -212,20 +212,21 @@ pub type AtomicU32ProgressTokenProvider = AtomicU32Provider;
#[derive(Debug, Default)]
pub struct AtomicU32Provider {
id: AtomicU32,
id: AtomicU64,
}
impl RequestIdProvider for AtomicU32Provider {
fn next_request_id(&self) -> RequestId {
RequestId::Number(self.id.fetch_add(1, std::sync::atomic::Ordering::SeqCst))
let id = self.id.fetch_add(1, std::sync::atomic::Ordering::SeqCst);
// Safe conversion: we start at 0 and increment by 1, so we won't overflow i64::MAX in practice
RequestId::Number(id as i64)
}
}
impl ProgressTokenProvider for AtomicU32Provider {
fn next_progress_token(&self) -> ProgressToken {
ProgressToken(NumberOrString::Number(
self.id.fetch_add(1, std::sync::atomic::Ordering::SeqCst),
))
let id = self.id.fetch_add(1, std::sync::atomic::Ordering::SeqCst);
ProgressToken(NumberOrString::Number(id as i64))
}
}