Type each known TLV's value by its tag, on read and on write #30

Merged
lilleman merged 14 commits from tlv-types into main 2026-09-27 16:48:06 +02:00
8 changed files with 34 additions and 29 deletions
Showing only changes of commit 9fb8f348c8 - Show all commits
+4 -5
View File
@@ -14,8 +14,7 @@ not for structure or style.
The goals, in priority order, live in The goals, in priority order, live in
[README.md](https://gitea.larvit.se/larvit/smpp-js/src/branch/main/README.md#goals) — they say where this library is heading, which an outside [README.md](https://gitea.larvit.se/larvit/smpp-js/src/branch/main/README.md#goals) — they say where this library is heading, which an outside
reader judges it by. The README states the audience alongside them. Everything below cites a goal by reader judges it by. The README states the audience alongside them.
number.
## Hard rules ## Hard rules
@@ -83,7 +82,7 @@ src/
constants.ts consts + constsById, and the SMPP version constants constants.ts consts + constsById, and the SMPP version constants
encodings.ts GSM 03.38, LATIN1, UCS2, detection, data_coding resolution encodings.ts GSM 03.38, LATIN1, UCS2, detection, data_coding resolution
errors.ts errors + errorsById (ESME_*) errors.ts errors + errorsById (ESME_*)
tlvs.ts TLV definitions, tlvsById, the input shape, and reading and writing a TLV stream tlvs.ts TLV definitions, tlvsById, the typed read and input shapes, and reading and writing a TLV stream
types.ts Wire types: int8/int16/int32/string/cstring/buffer/arrays types.ts Wire types: int8/int16/int32/string/cstring/buffer/arrays
``` ```
@@ -162,8 +161,8 @@ decision under [The wire](docs/decisions.md#the-wire).
- Hard tabs. Alphabetical ordering for keys, imports and lists unless order is logic-significant. - Hard tabs. Alphabetical ordering for keys, imports and lists unless order is logic-significant.
Two deliberate exceptions: command parameters are in wire order (above), and the `errors` and TLV Two deliberate exceptions: command parameters are in wire order (above), and the `errors` and TLV
tables are ordered by their numeric id so they can be diffed against the spec and gaps stay visible. tables are ordered by their numeric id so they can be diffed against the spec and gaps stay visible.
- Comments are the exception, not the default — see the root `CLAUDE.md` rules. Do not write file - Comments are the exception, not the default. Do not write file preambles or restate what the
preambles or restate what the code says. code says.
- Test data uses real randomised UUID v7 values, never `aaaa-0000` placeholders. - Test data uses real randomised UUID v7 values, never `aaaa-0000` placeholders.
- Fixtures that encode the wire are shared so no two files can drift on it: `test/raw-pdus.ts` builds - Fixtures that encode the wire are shared so no two files can drift on it: `test/raw-pdus.ts` builds
the octets a test writes straight to a socket, the PDUs `objToPdu()` refuses to build included. So the octets a test writes straight to a socket, the PDUs `objToPdu()` refuses to build included. So
+5 -3
View File
@@ -59,8 +59,8 @@
for the first occurrence. `objToPdu()`, `session.send()` and `session.sendReturn()` take for the first occurrence. `objToPdu()`, `session.send()` and `session.sendReturn()` take
`{ tagValue: [value] }` for them and refuse a lone value before anything goes out. `{ tagValue: [value] }` for them and refuse a lone value before anything goes out.
- `pduObj.tlvs` and every `tlvs` input are typed per tag, as `Tlvs` and `TlvInputs`: - `pduObj.tlvs` and every `tlvs` input are typed per tag, as `Tlvs` and `TlvInputs`:
`receipted_message_id` reads as a `string`, `callback_num` as a `Buffer[]`, and a value of the wrong `receipted_message_id` reads as a `string`, `message_state` as a `number`, and a value the tag
type for its tag fails to compile. Annotate with `Tlvs` or `TlvInputs` where you wrote cannot carry fails to compile. Annotate with `Tlvs` or `TlvInputs` where you wrote
`Record<string, Tlv>` or `Record<string, TlvInput>`; `TlvInput` is gone. `Record<string, Tlv>` or `Record<string, TlvInput>`; `TlvInput` is gone.
**A TLV input is keyed by its name, or by its decimal id where the table names none, and a `tagId` **A TLV input is keyed by its name, or by its decimal id where the table names none, and a `tagId`
@@ -68,9 +68,11 @@
`{ vendor: { tagId: 5142, … } }`; `{ message_state: { tagId: 5, … } }` used to go out as tag 5. A `{ vendor: { tagId: 5142, … } }`; `{ message_state: { tagId: 5, … } }` used to go out as tag 5. A
parsed PDU's `tlvs` still relay as they are. A number for an octet TLV, vendor tags included, is parsed PDU's `tlvs` still relay as they are. A number for an octet TLV, vendor tags included, is
refused, where it went out as its ASCII digits. refused, where it went out as its ASCII digits.
A decimal key naming a tag the table knows, `{ 1063: … }`, is refused in favour of the name, and A decimal key naming a tag the table knows, `{ 1063: … }`, is refused in favour of the name, and
so are `alert_on_msg_delivery` and `failed_broadcast_area_identifier` in favour of so are `alert_on_msg_delivery` and `failed_broadcast_area_identifier` in favour of
`alert_on_message_delivery` and `broadcast_area_identifier`, the names they read back under. `alert_on_message_delivery` and `broadcast_area_identifier`, the names they read back under. The
two alternate names are gone from `tlvs` too.
- `cmds.broadcast_sm_resp.tlvMap` is removed; nothing read it. - `cmds.broadcast_sm_resp.tlvMap` is removed; nothing read it.
## 0.5.0 ## 0.5.0
+3 -2
View File
@@ -32,9 +32,10 @@ shape is the same, connect, send, listen for delivery reports, with callbacks re
`consts.ESM_CLASS.STORE_FORWARD` reads `undefined`, which OR-s into an `esm_class` carrying no mode. `consts.ESM_CLASS.STORE_FORWARD` reads `undefined`, which OR-s into an `esm_class` carrying no mode.
- **A TLV is keyed by its name, or by its decimal id where the table names none**, and a `tagId` - **A TLV is keyed by its name, or by its decimal id where the table names none**, and a `tagId`
disagreeing with its key is refused: `{ 5142: { tagValue } }`, not disagreeing with its key is refused: `{ 5142: { tagValue } }`, not
`{ vendor: { tagId: 5142, tagValue } }`. A number for an octet TLV is refused; give a Buffer. `{ vendor: { tagId: 5142, tagValue } }`. A number for an octet TLV is refused; give a Buffer or a string.
Write `alert_on_message_delivery` and `broadcast_area_identifier`, the names they read back Write `alert_on_message_delivery` and `broadcast_area_identifier`, the names they read back
under, for `alert_on_msg_delivery` and `failed_broadcast_area_identifier`. under, for `alert_on_msg_delivery` and `failed_broadcast_area_identifier`, which are gone from
`tlvs` too.
- **The `error` event is `sessionError`**, and `serverError` on the server handle. - **The `error` event is `sessionError`**, and `serverError` on the server handle.
- **`log`** takes any object with `debug`, `error`, `info`, `verbose` and `warn` methods instead of - **`log`** takes any object with `debug`, `error`, `info`, `verbose` and `warn` methods instead of
a `larvitutils` one, and is silent by default: [README](README.md#logging). a `larvitutils` one, and is silent by default: [README](README.md#logging).
+2 -2
View File
@@ -59,8 +59,8 @@ Against real peers, same driver, window 50, 20,000 messages:
| SMSC | msgs/s | failed | | SMSC | msgs/s | failed |
| --- | --- | --- | | --- | --- | --- |
| this library's sink | 37,125 | 0 | | this library's sink | 37,125 | 0 |
| Jasmin 0.10 | 2,207 | 0 | | Jasmin 0.11.0 | 2,207 | 0 |
| SMPPSim 3.0.0 | — | 19,000 of 20,000 | | SMPPSim 2.6.11 | — | 19,000 of 20,000 |
## Against the other client libraries ## Against the other client libraries
+1 -1
View File
@@ -93,7 +93,7 @@ ways the next experiment must see. One fix per defect class, as its own change:
1. A worktree on a branch off `origin/main` (never `origin/v0.4.0`, the 0.4.0 code), named 1. A worktree on a branch off `origin/main` (never `origin/v0.4.0`, the 0.4.0 code), named
for the defect. for the defect.
2. Regression tests in `test/` first, naming the behaviour with the reproducer from the findings; 2. Regression tests in `test/` first, naming the behaviour with the reproducer from the findings;
then the implementation; then the decision record in the root `AGENTS.md` where the fix settles then the implementation; then the decision record in `docs/decisions.md` where the fix settles
a question of the wire or the session's life. a question of the wire or the session's life.
3. `/larv-review` on the branch, with the pull request based on `main`. When it marks the PR 3. `/larv-review` on the branch, with the pull request based on `main`. When it marks the PR
ready, fast-forward it. ready, fast-forward it.
+2 -2
View File
@@ -1,6 +1,6 @@
# interop-tests # interop-tests
Eight real SMPP implementations, run against this library in both directions, with every session Ten real SMPP implementations, run against this library in both directions, with every session
decoded independently by tshark so no result rests on our own view of the wire. It exists because decoded independently by tshark so no result rests on our own view of the wire. It exists because
the unit suite and this library's own dummy peers agree with themselves; these peers do not. the unit suite and this library's own dummy peers agree with themselves; these peers do not.
@@ -40,7 +40,7 @@ These bind to our server:
| Peer | What it is for | Run | | Peer | What it is for | Run |
| --- | --- | --- | | --- | --- | --- |
| **Kannel 1.4.5** | The most deployed real ESME there is; parses our receipts with the parser most operators' customers run, and declares 3.4 or 3.3 on demand | `debian:bookworm-slim` + the distribution package; four `.conf` variants under `peers/kannel/` | | **Kannel 1.4.5** | The most deployed real ESME there is; parses our receipts with the parser most operators' customers run, and declares 3.4 or 3.3 on demand | `debian:bookworm-20260824-slim` + the distribution package; four `.conf` variants under `peers/kannel/` |
| **jsmpp** | Strict and low-level: the driver builds UDH, `sar_*` and `message_payload` bytes by hand, and rejects an answer it dislikes | Maven build at a pinned commit, `peers/jsmpp/` | | **jsmpp** | Strict and low-level: the driver builds UDH, `sar_*` and `message_payload` bytes by hand, and rejects an answer it dislikes | Maven build at a pinned commit, `peers/jsmpp/` |
| **Cloudhopper** | The one peer with real windowing knobs, plus a TLS client | Maven build at a pinned commit, `peers/cloudhopper/`. Its 2015-era TLS client cannot do 1.3, so that scenario caps the server at 1.2 | | **Cloudhopper** | The one peer with real windowing knobs, plus a TLS client | Maven build at a pinned commit, `peers/cloudhopper/`. Its 2015-era TLS client cannot do 1.3, so that scenario caps the server at 1.2 |
| **python-smpplib 2.2.4** | An independent GSM 03.38 table to cross-check ours character by character | `python:3.12.14-slim-bookworm`, `peers/python/` | | **python-smpplib 2.2.4** | An independent GSM 03.38 table to cross-check ours character by character | `python:3.12.14-slim-bookworm`, `peers/python/` |
+8 -10
View File
@@ -83,13 +83,13 @@ type Specs = typeof specs;
export type TlvName = keyof Specs; export type TlvName = keyof Specs;
// Alternate spellings; the definition behind each keeps its canonical name. // SMPP 5.0's other spellings, which only name the tag to key instead.
const alternates = { const alternates: Record<string, TlvName> = {
alert_on_msg_delivery: specs.alert_on_message_delivery, alert_on_msg_delivery: 'alert_on_message_delivery',
failed_broadcast_area_identifier: specs.broadcast_area_identifier, failed_broadcast_area_identifier: 'broadcast_area_identifier',
}; };
export const tlvs: Record<TlvName, TlvDefinition> & Record<string, TlvDefinition> = { ...specs, ...alternates }; export const tlvs: Record<TlvName, TlvDefinition> & Record<string, TlvDefinition> = specs;
export const tlvsById: Record<number, TlvDefinition> = {}; export const tlvsById: Record<number, TlvDefinition> = {};
@@ -114,7 +114,6 @@ type WriteValue<K extends TlvName> = Specs[K] extends { multiple: true } ? ReadV
type KnownTlv<K extends TlvName> = { tagId: number; tagName: K; tagValue: ReadValue<K> }; type KnownTlv<K extends TlvName> = { tagId: number; tagName: K; tagValue: ReadValue<K> };
/** A tag the TLV table does not define. */
type UnknownTlv = { tagId: number; tagName: undefined; tagValue: Buffer }; type UnknownTlv = { tagId: number; tagName: undefined; tagValue: Buffer };
/** Keyed by tag name, or by its decimal id where the table defines no name. */ /** Keyed by tag name, or by its decimal id where the table defines no name. */
@@ -122,7 +121,7 @@ export type Tlvs = { [K in TlvName]?: KnownTlv<K> } & Partial<Record<`${number}`
export type Tlv = { [K in TlvName]: KnownTlv<K> }[TlvName] | UnknownTlv; export type Tlv = { [K in TlvName]: KnownTlv<K> }[TlvName] | UnknownTlv;
/** Keyed like `Tlvs`, by tag name or by the decimal id of a tag the table does not define. */ /** Keyed like `Tlvs`. */
export type TlvInputs = { [K in TlvName]?: { tagValue: WriteValue<K> } } export type TlvInputs = { [K in TlvName]?: { tagValue: WriteValue<K> } }
& Partial<Record<`${number}`, { tagValue: Buffer | string }>>; & Partial<Record<`${number}`, { tagValue: Buffer | string }>>;
@@ -139,9 +138,9 @@ function isTlvInput(input: unknown): input is { tagValue: TlvValue } {
function keyedTagId(name: string): Result<{ tagId: number }> { function keyedTagId(name: string): Result<{ tagId: number }> {
if (isTlvName(name)) return { tagId: specs[name].id }; if (isTlvName(name)) return { tagId: specs[name].id };
const alternate = Object.hasOwn(tlvs, name) ? tlvs[name] : undefined; const alternate = Object.hasOwn(alternates, name) ? alternates[name] : undefined;
if (alternate) return { err: new Error(`TLV "${name}": key it ${alternate.tag}, the name it reads back under`) }; if (alternate) return { err: new Error(`TLV "${name}": key it ${alternate}, the name it reads back under`) };
if (!/^(0|[1-9]\d*)$/.test(name)) { if (!/^(0|[1-9]\d*)$/.test(name)) {
return { err: new Error(`TLV "${name}": unknown tag name; key a tag the table does not define by its decimal id`) }; return { err: new Error(`TLV "${name}": unknown tag name; key a tag the table does not define by its decimal id`) };
@@ -156,7 +155,6 @@ function keyedTagId(name: string): Result<{ tagId: number }> {
return known ? { err: new Error(`TLV "${name}": the table names this tag ${known.tag}, key it by that`) } : { tagId }; return known ? { err: new Error(`TLV "${name}": the table names this tag ${known.tag}, key it by that`) } : { tagId };
} }
/** The key names the tag; a `tagId` beside it, as a parsed TLV carries, has to agree. */
function entryOf(name: string, input: unknown): Result<{ tagId: number; tagValue: TlvValue }> { function entryOf(name: string, input: unknown): Result<{ tagId: number; tagValue: TlvValue }> {
if (!isTlvInput(input)) { if (!isTlvInput(input)) {
return { err: new Error(`TLV "${name}": give it as { tagValue }, holding a Buffer, a number, a string, or an array of Buffers or of numbers`) }; return { err: new Error(`TLV "${name}": give it as { tagValue }, holding a Buffer, a number, a string, or an array of Buffers or of numbers`) };
+9 -4
View File
@@ -194,10 +194,6 @@ and is also what the panel ranked hardest — two methods, one answer.
rest silently, which goal 3 argues against; listing it would change every such tag's shape. rest silently, which goal 3 argues against; listing it would change every such tag's shape.
From the architecture review of #25. From the architecture review of #25.
- [ ] **Refuse a TLV input naming one tag under both its spellings.** `broadcast_area_identifier`
and `failed_broadcast_area_identifier` in one `tlvs` record both write, so the peer receives
the union of two lists the caller may have meant as one. From the architecture review of #25.
- [ ] **Test that a multipart send which errors never fires `messageDlr`.** Goal 2 now says so and - [ ] **Test that a multipart send which errors never fires `messageDlr`.** Goal 2 now says so and
README promises it; `session-extras.test.ts` covers a drop *after* the send, not one during it. README promises it; `session-extras.test.ts` covers a drop *after* the send, not one during it.
@@ -397,6 +393,15 @@ and is also what the panel ranked hardest — two methods, one answer.
## Worth doing, not blocking ## Worth doing, not blocking
- [ ] **Move the decisions out of AGENTS.md's fixtures paragraph and the two tooling READMEs.** The
one dummy SMSC, `smscPeer()` staying separate and which copied helpers are tolerated
(AGENTS.md Conventions), and why Kannel is absent from `benchmarks/README.md`, go to
`docs/decisions.md` with index lines. Move the planned work written into
`interop-tests/AGENTS.md` (an expected count per peer) and `benchmarks/README.md` (the default
window gap) to this file. Give the `run.py`-in-background footgun in `interop-tests/AGENTS.md`
a rule of its own. Delete AGENTS.md's "message_id values … are UUID v7" line. From the prose
pass of #30.
- [ ] **Make the dumbclient soak's memory sample evidence of no library leak again.** Its rss ends at - [ ] **Make the dumbclient soak's memory sample evidence of no library leak again.** Its rss ends at
its maximum (298 MiB, heapUsed 81 MiB after 173,820 messages), which the harness's own per-id its maximum (298 MiB, heapUsed 81 MiB after 173,820 messages), which the harness's own per-id
`Set` and `answerOrder` explain but cannot separate from a leak in `src/`: sample the heap `Set` and `answerOrder` explain but cannot separate from a leak in `src/`: sample the heap