Return ASCOM INVALID_VALUE for out-of-range integer parameters

[ivonnyssen]

Summary

Replace serde_plain::from_str with a custom deserializer so Alpaca integer parameters that parse successfully but don't fit the target type return ASCOM INVALID_VALUE (error 1025) instead of HTTP 400 BadRequest.

Closes #5

Motivation

The ASCOM Alpaca spec and ConformU conformance tests require INVALID_VALUE for parseable-but-out-of-range values — e.g. sending Id=999 to a Switch with only 4 ports should return HTTP 200 with ErrorNumber: 1025, not HTTP 400.

Previously, OpaqueParams::maybe_extract used serde_plain::from_str::<T>, which surfaces a single ParseIntError for every failure mode: malformed input, negative values for unsigned targets, and values too large for the target type. Everything collapsed into Error::BadParameter → HTTP 400, so ConformU's out-of-range checks were reported as protocol errors instead of device-level errors.

How it works

Integer parsing now goes through a two-step split:

1. str.parse::<i64>() — fails only for genuinely malformed input (AlpacaParseError::BadFormat → HTTP 400)
2. T::try_from(i64_value) — fails only for values that parse as integers but don't fit the target (AlpacaParseError::OutOfRange → ASCOM INVALID_VALUE)

i64 is the narrowest intermediate that covers the full range of every Alpaca numeric target (i8/i16/i32/u8/u16/u32/usize — including u32 transaction IDs up to 4.3B and negative i32 device values), so a single generic parse_integer<T: TryFrom<i64>> handles all of them without per-type dispatch.

The inner AlpacaParseError is carried on Error::BadParameter, and the ResponseWithTransaction<Result<T>> IntoResponse impl destructures the OutOfRange variant to produce the ASCOM-formatted error response (HTTP 200 + 1025). BadFormat continues to return HTTP 400.

Changes

• src/server/params.rs:
  • New AlpacaParseError enum (BadFormat/OutOfRange) and AlpacaDeserializer implementing serde::Deserializer
  • Integer methods parse via i64 with TryFrom narrowing; float/bool/char/enum/etc. paths preserved
  • Unit tests for range vs format classification, u32-above-i32::MAX, negative-as-OutOfRange, whitespace trimming
• src/server/error.rs: Error::BadParameter now carries AlpacaParseError (was serde_plain::Error)
• src/server/response.rs: BadParameter { err: OutOfRange { .. } } maps to ASCOMError::invalid_value
• Cargo.toml: drop serde_plain dependency (no longer needed)

Test plan

• cargo check --features server,all-devices passes
• cargo clippy --features server,all-devices clean
• New unit tests in params.rs pass (10/10)
• Manual verification with ConformU: out-of-range Switch.Id returns ASCOM 1025, not HTTP 400
• Manual verification: malformed input (e.g. Id=abc) still returns HTTP 400

[Copilot]

Pull request overview

This PR updates Alpaca server parameter parsing to distinguish unparseable integer inputs (HTTP 400) from parseable but out-of-range integer inputs (ASCOM INVALID_VALUE / 1025), and ensures u32 request parameters can exceed i32::MAX by widening to an intermediate signed type first.

Changes:

• Introduces a custom serde Deserializer for request parameters that parses integers via an intermediate type, enabling explicit range-error classification.
• Adds a new Error::ParameterOutOfRange and maps it to ASCOM INVALID_VALUE in transaction-wrapped responses.
• Adds unit tests for integer parsing/range behavior in params.rs.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

File	Description
src/server/params.rs	Replaces serde_plain::from_str parsing with a custom deserializer and adds tests for integer parsing and range classification.
src/server/error.rs	Adds a new ParameterOutOfRange error variant and updates HTTP status mapping in IntoResponse.
src/server/response.rs	Maps ParameterOutOfRange to an ASCOM INVALID_VALUE response (HTTP 200) when returning ResponseWithTransaction<Result<T>>.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[ivonnyssen]

Analysis of Copilot review comments 3 & 4 (out-of-range parameters escaping the transaction wrapper)

Updated 2026-04-21: code snippet refreshed to match the new Error::BadParameter { err: AlpacaParseError::OutOfRange { .. } } shape after folding ParameterOutOfRange into BadParameter. The underlying gap is unchanged.

Copilot flagged that out-of-range integer errors during ClientID/ClientTransactionID extraction bypass the ResponseWithTransaction wrapper and produce HTTP 400 instead of ASCOM INVALID_VALUE. This is a valid observation — here's the full analysis and a proposed fix.

How the issue arises

ClientID and ClientTransactionID are extracted as u32 in RequestTransaction::extract() (transaction.rs:39-46), before the ResponseWithTransaction wrapper is constructed (mod.rs:106-108). The ? on line 106 propagates any error out of exec(), where it hits Error::into_response → HTTP 400.

So sending e.g. ClientID=5000000000 (valid i64, out of range for u32) returns HTTP 400 instead of HTTP 200 + ASCOM error 1025. Note that this same behavior exists on main today (serde_plain::from_str::<u32> fails → BadParameter → HTTP 400), so this is a pre-existing gap rather than something introduced by this PR.

The device parameters that ConformU actually tests (like Switch.Id or negative index values) are extracted inside the make_response closure (mod.rs:123), which is correctly wrapped in ResponseWithTransaction.

Proposed fix

Catch the out-of-range case in exec() before it escapes. The change is ~10 lines in mod.rs:

// mod.rs — add to imports:
use super::params::AlpacaParseError;
use crate::ASCOMError;

// mod.rs — replace line 106:
//     let request_transaction = RequestTransaction::extract(&mut self.params)?;
// with:
let request_transaction = match RequestTransaction::extract(&mut self.params) {
    Ok(t) => t,
    Err(Error::BadParameter {
        name,
        err: AlpacaParseError::OutOfRange { value, target_type },
    }) => {
        return Ok(ResponseWithTransaction {
            transaction: ResponseTransaction::new(None),
            response: Err::<(), _>(ASCOMError::invalid_value(format!(
                "Parameter {name:?} value {value} is out of range for {target_type}"
            ))),
        }
        .into_response());
    }
    Err(e) => return Err(e.into()),
};

This works because ResponseWithTransaction<ASCOMResult<()>> already has an IntoResponse impl, producing HTTP 200 with {"ServerTransactionID": N, "ErrorNumber": 1025, "ErrorMessage": "..."}. ClientTransactionID is correctly omitted (None) since we couldn't parse it. BadFormat parse errors, MissingParameter, and other Error variants still propagate as HTTP 400 via the final Err(e) arm.

I also considered a two-phase extraction approach (making extract() infallible and deferring errors) which would preserve a valid ClientTransactionID even when ClientID is the one that's out of range, but that adds complexity for an unlikely edge case.

Let me know if you'd like this fix included in the PR or if you'd prefer to leave it for a follow-up.

— Igor (via Claude Code)

[RReverser]

uint32 overflow: ClientID and ClientTransactionID are uint32 per spec, so values above i32::MAX (e.g. 2147483648) must parse successfully. The previous code used serde_plain::from_str which parses directly to the target type with no intermediate widening, so valid uint32 values were rejected. Using i64 as the intermediate type covers the full uint32 range.

I've been re-reading this several times and I don't get it. How is parsing directly to the target type (u32) a bad thing, but using an unrelated i64 type a good thing?

[RReverser]

If we have custom deserializer, why do we still keep serde_plain around?

[ivonnyssen]

gone with the latest push

[ivonnyssen]

@RReverser you're right — the PR description framing was wrong and I've rewritten it. serde_plain::from_str::<u32>("3000000000") parses fine; u32 targets were never broken. What the old code couldn't do was classify the failure mode: any integer parse error (malformed input, negative for unsigned, too large for target) collapsed into a single BadParameter → HTTP 400. ConformU and the ASCOM spec require INVALID_VALUE (HTTP 200, error 1025) for parseable-but-out-of-range values — e.g. Switch.Id=999 when the device only has 4 ports.

The i64 intermediate isn't about widening u32; it's the narrowest type that covers the full range of every Alpaca numeric target (i8/i16/i32/u8/u16/u32/usize). That lets a single generic parse_integer<T: TryFrom<i64>> split the two failure modes cleanly: str.parse::<i64>() fails only for genuinely malformed input (→ BadFormat → HTTP 400), while T::try_from(i64) fails only for values that parse as integers but don't fit the target (→ OutOfRange → ASCOM 1025).

I've also addressed your three inline comments in the latest push:

• ParameterOutOfRange folded into BadParameter — the variant is gone; Error::BadParameter now carries an AlpacaParseError (BadFormat or OutOfRange), and ResponseWithTransaction<Result<T>>::into_response destructures the inner variant to route HTTP 400 vs ASCOM 1025.
• serde_plain dropped — removed from Cargo.toml (dep + server feature) and all source references.
• ALPACA → Alpaca — fixed throughout params.rs.