Conversation
fast-xml-parser converts anything that looks numeric, which corrupts ONIX
data. Measured:
default {"NotificationType":1,"IDValue":62124983,"Price":1200.5}
parseTagValue:false {"NotificationType":"01","IDValue":"062124983","Price":"1200.50"}
The code "01" becomes 1, an identifier loses its leading zero, and a
price loses a digit. None of this is new in v5 — v3 did the same — so it
has been wrong from the start.
The generated code already disagreed with it: code.ts declares
`export type NotificationType = string` while the reader returned a
number, so the types were lying. The Go templates generate string types
from the same schema, so the two languages also disagreed with each other
on identical input, which is the one thing a multi-language generator
should not do.
Set parseTagValue: false. Every ONIX element value is a string; the
numeric-looking ones are codes, identifiers and amounts that happen to be
made of digits. Fixing this per-element was rejected in ADR-0007: which
elements look numeric depends on the data, so IDValue would convert or
not depending on the value, and a type that varies with input is worse
than one that is always a string.
This does change output for consumers, from number to string. Recorded as
such — the side that breaks is the side that was wrong.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01P8rZakwAiz1jpViUX34x1Z
レビューコード変更そのものは正しく、そのままマージできる品質です。 必須 1: ADR-0007 の「Go は一貫して文字列型」は事実と異なるファイル:
実際の生成物 しかも ADR がこの節の直前で例に挙げている type NotificationType struct {
Body string `xml:",innerxml" json:",omitempty"`
Textformat TextFormatCode `xml:"textformat,attr,omitempty" json:",omitempty"`
Textcase TextCaseCode `xml:"textcase,attr,omitempty" json:",omitempty"`
...
}なぜ問題か: 「Go に合わせる」が決定の根拠として書かれているのに、その Go の実態が違います。この ADR を将来の判断材料に使う人が誤った前提を引き継ぎます。 修正案: この段落を実態に書き換える(「Go は code 型を struct / string / []string に振り分けており、コード値本体は struct の 必須 2: 値も一致しないので「言語間の一貫性」は本 PR では達成されないファイル:
Go の switch v {
// Use for a complete record issued earlier than approximately six months before publication
case "01":
c.Body = `Early notification`
case "02":
c.Body = `Advance notification (confirmed)`つまり同じ なぜ問題か: 決定の主要な根拠のひとつが、検証すると成り立ちません。放置すると「TS と Go は揃ったはず」という誤解が残ります。 修正案: 一貫性の主張を撤回し、「TS の型宣言と実際の返り値を一致させる」という範囲に論拠を限定する。Go との差(コード→説明文の変換の有無)に触れるなら、「これは別の未整合として残る」と明記する。 推奨 1:
|
| 入力 | 既定 | parseTagValue:false |
|---|---|---|
<Empty></Empty> |
"" |
""(同じ) |
<SelfClose/> |
"" |
""(同じ) |
<Ws> </Ws> |
"" |
""(同じ) |
<Mixed>text <b>bold</b> tail</Mixed> |
{"b":"bold","#text":"texttail"} |
同一 |
<TrueV>true</TrueV> |
boolean true |
"true" |
<Pad> 0123 </Pad> |
number 123 |
"0123"(trim される) |
<Hex>0x1A</Hex> |
number 26 |
"0x1A" |
<Exp>1e5</Exp> |
number 100000 |
"1e5" |
<Plus>+42</Plus> |
number 42 |
"+42" |
<Date>20240101</Date> |
number 20240101 |
"20240101" |
→ 形が変わるのは値の型だけ。空要素・空白のみ・混在内容の構造は不変。PR が触れていない差分は「真偽値も止まる」「trim は残る」の 2 点のみ(任意 1・2)。
3. 属性(PR の「範囲外」主張の検証)
DEFAULT (ignoreAttributes 既定)
{"ONIXMessage":{"Header":{"Sender":{"SenderName":"X"}},"Product":{"NotificationType":1}}}
→ refname / shortname / datestamp / release はすべて結果から消えています。PR の主張どおりです。 ignoreAttributes: false を付けると初めて @_refname 等が現れ、そのとき @_datestamp は既定で文字列のまま(parseAttributeValue 既定 false)、parseAttributeValue: true にすると 20240101(number)になります。
4. v3 の挙動(ADR の「最初から壊れていた」)— fast-xml-parser 3.17.6 を併置
v3 DEFAULT (parse(xml), オプション無し)
{"NotificationType":1,"ProductIDType":2,"IDValue":62124983,"ISBN13":9784062124983,"Price":1200.5,"TrueV":true}
v3 parseTrueNumberOnly:false
同上(=既定と同一)
v3 parseNodeValue:false
{"NotificationType":"01",...,"Price":"1200.50","TrueV":"true"}
v5 numberParseOptions.leadingZeros:true
{"A":1,"B":62124983,"C":9784062124983,"D":1200.5} ← v5 既定と同一
git show origin/main:template/typescript/v2/reader.mustache は xml.parse(file.toString()) でオプションを一切渡していません(=v3 既定)。→ 「v3 でも同じ」「最初から壊れていた」「parseTrueNumberOnly:false と v5 の leadingZeros:true が同じ結果」はいずれも正しいです。ちなみに v5 で leadingZeros:false にすると "01"/"062124983" は救われますが Price は 1200.5 のまま — 部分的対処では足りないという ADR の判断も裏付けられます。
5. テンプレートと生成物のバイト一致
$ diff <(git show $B:template/typescript/v2/reader.mustache) \
<(git show $B:generated/typescript/v2/reader.ts) # 差分なし
333192663d60f5607eca6528ec4bd7864570f6fb178c46edd2e516927e566be4 (両方)
6. 型検査 — 隔離ディレクトリに generated/typescript/v2/*.ts と tsconfig.json を複製し、typescript@5 + @types/node@22.20.2 + fast-xml-parser@5.11.1 で実行:
$ tsc --noEmit -p tsconfig.json
exit=0
7. 生成された型の確認
$ git show $B:generated/typescript/v2/code.ts | grep -c '^export type .* = string' # 116
$ ... | grep -cE '= *number|= *boolean' # 0
$ git show $B:generated/typescript/v2/model.ts | grep -cE '^export interface \w+ \{\}$' # 75 / 全 75
$ git show $B:generated/go/v2/code.go | grep -E '^type ' | awk '{print $3}' | sort | uniq -c
107 struct
8 string
2 []string
8. CI 適用範囲 — .github/workflows/test.yml は make test(Haskell)と npx bazelisk test //e2e/go:snapshot_test のみ。e2e/ 配下は e2e/go だけで、TypeScript の reader を実行するテストはリポジトリ内に存在しません。→ 「TypeScript reader は CI で実行されていない」は正しいです。ADR がこれを正直に明記している点は良いと思います。
9. ADR 周辺 — docs/adr/0005-...md は確かに本件を「既知の未解決問題」として記録しており、parseTagValue: false 相当の対処が要ると明記しています。参照は正確です。ADR 番号 0007 は他ブランチ(0003 / 0004 / 0005 / 0006 が使用中)と衝突しません。
検証できなかったこと
- 実 ONIX ファイル (
fixtures/20201200.onix) での変更前後の全量比較は行っていません。作業ツリーを触らない制約のため、合成した最小 XML での検証に留めています。変換の性質上、結論は変わらないと考えていますが、「実データで数値化されていた要素が具体的にどれだけあるか」の件数は不明です。 - 既存利用者への実影響は測っていません。
README.md上 TypeScript は Version 2 / 3 とも "Not yet" で、npm 公開物としての利用実績を確認する手段がありませんでした。破壊的変更を受け入れるという判断自体には異論ありません。 - Go 側の実行時挙動(
UnmarshalXMLが実際に説明文を返すこと)は生成コードの読解による確認で、Go を実行しての確認はしていません。ただし該当箇所(case "01"でEarly notificationを代入)は分岐が単純で、読解で十分と判断しました。
Generated by Claude Code
The ADR argued the fix restores cross-language consistency because Go generates string types from the same schema. Review showed both halves of that are wrong, and checking confirmed it: generated/go/v2/code.go: 107 struct, 8 string, 2 []string `type X string` only comes out of the template when a code is neither space-separatable nor has attributes, and NotificationType — the ADR's own example — is a struct in Go. Worse for the argument, Go's UnmarshalXML swaps the code for its description: case "01": c.Body = `Early notification` So after this change TypeScript returns "01" and Go returns "Early notification". The languages still disagree; the fix does not touch that. That is a bigger design split — only Go expands codes — and belongs in its own decision. The justification is narrowed to the thing that does hold: "01" becoming 1 and "1200.50" becoming 1200.5 corrupts data, whatever Go does. The code change is unaffected. Also recorded: parseAttributeValue must stay false when attributes are eventually read, or the same bug returns on that side; the generated types agreeing with the values is a convention rather than something typecheck enforces, since every interface in model.ts is empty and code.ts is imported by nothing; and parseTagValue: false does not stop trimValues, which still strips surrounding whitespace. The AGENTS.md citation is replaced with README, since AGENTS.md is not on this branch. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01P8rZakwAiz1jpViUX34x1Z
|
レビューありがとうございます。必須 2 件はどちらも正しく、ADR-0007 の論拠が誤っていました。 訂正しました (7332d96)。 必須 1・2: Go 側の実態自分でも確認しました。
さらに決定的なのがこちらです。 switch v {
// Use for a complete record issued earlier than ...
case "01":
c.Body = `Early notification`Go はコード値を人間可読な説明文に置換していました。 つまりこの PR を入れても TypeScript は 対応ADR の根拠を**「データを壊さないこと」の一点に絞り**ました。 そのうえで、Go だけがコードを説明文に展開しているという食い違いを、より大きな設計上の問題として ADR に明記しました。これは型の問題ではなく「どの言語でコードを展開するか」の設計判断なので、別途 ADR を立てて扱うべきものだと考えています。このリポジトリの価値の中心(同じスキーマから複数言語)に直接関わる論点を掘り出していただいた形です。 推奨
任意
Generated by Claude Code |
生成される TypeScript クライアントが ONIX のデータを壊しています。#59 のレビューで発覚し、ADR-0005 に既知の問題として記録していた件の対応です。
base は #62(npm 系のチェーン
#56 → #57 → #59 → #62 → 本 PR)。何が壊れているか
fast-xml-parser は既定で、数値に見える値をその型に変換します。実測値です。
"01"→1。ONIX のコード値は先頭ゼロを含めて意味を持つ 2 桁の文字列で、1は別物です"062124983"→62124983。識別子から先頭ゼロが落ちます"1200.50"→1200.5。金額の桁が落ちますこれは v5 で入った挙動ではありません。v3 でも同じで、最初から壊れていました(#59 のレビューで、両バージョンが同一に壊すことまで確認されています)。
生成された型が嘘をついていた
generated/typescript/v2/code.tsはコード型を文字列として宣言しています。宣言は
string、実際に返るのはnumberでした。さらに、Go 側は同じスキーマから一貫して文字列型を生成しています(
type {{xmlReferenceName}} string)。同じ入力に対して言語ごとに違う型が返るのは、「同じスキーマから複数言語のクライアントを生成する」というこのリポジトリの目的に照らして、最も避けたい不整合です。修正
parseTagValue: falseを指定し、値を一切変換しません。要素ごとに個別対処する案は採りませんでした。どの要素が数値に見えるかは入力データ次第で、
IDValueのように値によって変換されたりされなかったりします。入力に依存して型が変わるほうが、常に文字列であるより扱いにくいためです。判断は ADR-0007 に記録しました。
後方互換性
これは出力を変える変更です。 これまで数値だった値が文字列になります。ただし壊れる側が正しい出力なので受け入れます。生成される型宣言と実際の値がこれで一致します。
範囲外(既知)
fast-xml-parser v5 は既定で
ignoreAttributes: trueのため、refname/shortname/datestampといった ONIX の属性はそもそも読まれていません。これも実装上の欠落ですが、修正すると出力の形が変わるので別途扱います。ADR-0007 に記録しています。確認
diffで確認🤖 Generated with Claude Code
https://claude.ai/code/session_01P8rZakwAiz1jpViUX34x1Z
Generated by Claude Code