Make Payload case-insensitive — single streaming pass (Fixes #108)#113
Make Payload case-insensitive — single streaming pass (Fixes #108)#113russ wants to merge 1 commit into
Conversation
| return message(payload) if payload.command == "message" | ||
| # Compare case-insensitively so a command like "Subscribe" still | ||
| # dispatches correctly (see issue #108). | ||
| case payload.command.downcase |
There was a problem hiding this comment.
Instead of doing this, we can add an after_initialize method that just does @command = @command.downcase. That gets set in the Payload
| # Only the top-level keys (command/identifier/data) are touched; the nested | ||
| # values — channel names and the identifier/data blobs — are left exactly as | ||
| # sent, since those remain case-sensitive. See issue #108. | ||
| def self.from_json(string : String) : self |
There was a problem hiding this comment.
This is going to be a fairly large performance hit... I have a feeling that at the moment, this isn't really possible in Crystal without doing this method here.
JSON::Field supports a key option where you can do @[JSON::Field(key: "Command")], but now the lowercase version doesn't work. A "hacky" method would be to define both a @command : String? and @Command : String? then an instance method command which chooses one or the other 😂 but.... gross lol.
What would be cool is if the annotation allowed for a regex or something, but that would require a new feature added to Crystal. Maybe for now just having the command set to lowercase is a decent enough update and we just make sure to document that the key "command" must also be lowercase?
aa42b8a to
3abe314
Compare
Crystal's JSON parsing is case-sensitive, so a payload like
`{"Command": "Subscribe"}` failed two ways: the `Command` key raised
"Missing JSON attribute: command", and the `Subscribe` value never
matched the lowercase dispatch comparison.
This is the hot path for every inbound frame, so it has to stay fast.
Rather than re-parsing the message (parse to a tree, downcase the keys,
re-serialize, re-parse — ~2.8x slower with ~2.5x the allocations), the
protocol keys stay as regular JSON::Serializable fields. Correctly-cased
messages are matched directly in a single streaming pass with no extra
work; a mis-cased key falls through to #on_unknown_json_attribute, where
String#compare(case_insensitive:) matches it without allocating a
downcased copy. command/identifier become nilable so a mis-cased-only key
isn't rejected before we can capture it; the accessors re-enforce
presence (a genuinely missing command/identifier still raises
SerializableError).
Connection#receive matches the command case-insensitively the same way,
avoiding a per-message downcase allocation.
Benchmarked against the original case-sensitive parse: correctly-cased
payloads are within ~3% with identical allocations; mis-cased ones are
~15% slower with no extra allocation.
Fixes cable-cr#108
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BMW1EsdCMExBhm3tg7TQhs
3abe314 to
4abe97d
Compare
Problem
Fixes #108.
Crystal's JSON parsing is case-sensitive, so a payload like
{"Command": "Subscribe"}failed two ways:Payload.from_jsonraisedMissing JSON attribute: commandbecause the key wasCommand, notcommand.Connection#receivedispatched onpayload.command == "subscribe", so aSubscribevalue never matched.Approach (and why not the obvious one)
Payload.from_jsonis the hot path — it runs for every inbound frame — so it has to stay fast. The tempting fix (parse the message into aJSON::Anytree, lowercase the keys, re-serialize, re-parse) is ~2.8× slower with ~2.5× the allocations, which isn't acceptable here.Instead, this keeps the protocol keys as regular
JSON::Serializablefields, so a correctly-cased message is matched directly in a single streaming pass with no extra work. A mis-cased key isn't matched, so it falls through toon_unknown_json_attribute, whereString#compare(case_insensitive: true)matches it without allocating a downcased copy of the key — all still within the one pass, no re-parsing.command/identifierbecome nilable internally so a mis-cased-only key isn't rejected by the "Missing JSON attribute" check before we can capture it. The public accessors re-enforce presence, so a genuinely missingcommand/identifierstill raisesJSON::SerializableError(now at access, still caught byCable::Handler).data(read fromjson_unmapped) is captured under its lowercase key the same way.Connection#receivematches the command value case-insensitively with the same allocation-freecompare.Benchmark
Parsing a typical payload, vs the original case-sensitive parse:
The happy path is within ~3% of today's parse with identical allocations; a mis-cased payload is ~15% slower with no extra allocation.
Tests
Payloadparses mixed-case top-level keys ("Command","Identifier","DATA").commandstill raisesJSON::SerializableError.#receiveaccepts{"Command": "Subscribe", "Identifier": ...}and confirms the subscription.Green locally on Crystal 1.10.0 and current stable.
Note
The
nightlyCI job iscontinue-on-error: trueand fails repo-wide because ameba won't compile against Crystal nightly — unrelated to this change.