From 4a3dbc56557907abc17517ab8c517a8848263ad6 Mon Sep 17 00:00:00 2001 From: Ronald Tse Date: Fri, 7 Aug 2026 15:31:22 +0800 Subject: [PATCH] Audit A2 + A3: delete dead format_set, preserve strings in Parcel writer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A2 — delete dead code in CDDAL serializer: format_set() and set_like() in lib/opencdd/cddal/serializer.rb were unreachable. format_value() calls string_literal?() first, which returns true for any string not matching UNQUOTED_VALUE_RE — and that regex excludes parenthesized lists, so set_like?() could never be reached. Removed both methods; format_value now does quote-or-pass-through only. A3 — preserve leading-zero strings through xlsx round-trip: Parcel::Writer::add_data_sheets now passes types: :string to ws.add_row. Previously caxlsx auto-typed numeric-looking strings ("001", "01") as Integer cells; roo read them back as 1, 1, breaking semantically_equal?. Now all property values survive as the strings they were stored as. New spec/parcel_string_preservation_spec.rb verifies version="001" and revision="01" survive a write → reload cycle. import_pipeline_spec.rb scenario 3's TODO comment is updated: the value-normalization gap is fixed for the specific case it described, but a full semantically_equal? round-trip is still blocked by a separate multilingual key normalization gap (MDC_P004_1 vs MDC_P004.en) — flagged as a separate follow-up. 931 specs pass, 0 failures. --- lib/opencdd/cddal/serializer.rb | 10 ------- lib/opencdd/parcel/writer.rb | 6 ++++- spec/import_pipeline_spec.rb | 16 ++++------- spec/parcel_string_preservation_spec.rb | 36 +++++++++++++++++++++++++ 4 files changed, 46 insertions(+), 22 deletions(-) create mode 100644 spec/parcel_string_preservation_spec.rb diff --git a/lib/opencdd/cddal/serializer.rb b/lib/opencdd/cddal/serializer.rb index bc1982f..1f25ee1 100644 --- a/lib/opencdd/cddal/serializer.rb +++ b/lib/opencdd/cddal/serializer.rb @@ -174,7 +174,6 @@ def language_suffix(property_id) def format_value(value) s = value.to_s return quote_string(s) if string_literal?(s) - return format_set(s) if set_like?(s) s end @@ -194,15 +193,6 @@ def quote_string(s) "\"#{escaped}\"" end - def set_like?(s) - s.start_with?("(") && s.end_with?(")") - end - - def format_set(s) - elements = Opencdd::StructuredValues.unwrap_and_split(s) - "{ #{elements.join(', ')} }" - end - def symbol_name_for(entity) code = entity.code&.to_s return code if code && !code.empty? diff --git a/lib/opencdd/parcel/writer.rb b/lib/opencdd/parcel/writer.rb index 67106a4..af6af56 100644 --- a/lib/opencdd/parcel/writer.rb +++ b/lib/opencdd/parcel/writer.rb @@ -147,7 +147,11 @@ def add_data_sheets(workbook, built_sheets, source_language, hidden_directives = rows = emitter.emit(built.sheet, built.entities) workbook.add_worksheet(name: built.sheet.name) do |ws| rows.each do |row| - emitted = ws.add_row(row) + # All property values are strings per IEC 61360 wire format. + # Force string cell type so values like "001" survive round-trip + # (otherwise caxlsx auto-types numeric-looking strings and roo + # reads them back as integers, breaking semantically_equal?). + emitted = ws.add_row(row, types: :string) emitted.hidden = true if row_hidden?(row, hidden_directives) end end diff --git a/spec/import_pipeline_spec.rb b/spec/import_pipeline_spec.rb index 53fa7c2..4ef7b86 100644 --- a/spec/import_pipeline_spec.rb +++ b/spec/import_pipeline_spec.rb @@ -102,17 +102,11 @@ def write_parcel(database, parcel_id:, **opts) end describe "scenario 3: parcel → cddal → parcel (round-trip)" do - # Cross-format round-trips hit a known Parcel writer normalization: - # version/revision codes stored as strings ("001") are written as - # numeric cells (1) and read back as "1". +semantically_equal?+ - # does exact property-hash comparison and reports this as a - # mismatch. The entity graph itself (IRDIs, types, counts) survives - # the detour cleanly — that is the invariant tested here. - # - # TODO: the value-normalization gap should be resolved either in - # the Parcel writer (preserve string values) or in - # +semantically_equal?+ (normalize numeric strings). Flagged, not - # fixed — see [[ask-before-semantic-changes]]. + # Cross-format round-trips preserve the entity graph (IRDIs, types, + # counts) cleanly. A full +semantically_equal?+ assertion is blocked + # by a separate multilingual key normalization gap (MDC_P004_1 vs + # MDC_P004.en) — see spec/parcel_string_preservation_spec.rb for the + # narrower string-preservation test that verifies audit A3's fix. let(:parcel_source) do path = write_parcel(source_database, parcel_id: "OCDDSRC3") Opencdd::Database.load_workbook(path) diff --git a/spec/parcel_string_preservation_spec.rb b/spec/parcel_string_preservation_spec.rb new file mode 100644 index 0000000..2694aa6 --- /dev/null +++ b/spec/parcel_string_preservation_spec.rb @@ -0,0 +1,36 @@ +require "spec_helper" +require "tmpdir" + +RSpec.describe "Parcel writer string preservation (audit A3)" do + # Reproduces the original bug: numeric-looking strings like "001" were + # written as numeric cells and read back as 1, breaking semantically_equal?. + # Fix: Parcel::Writer#add_data_sheets forces `types: :string` per row. + let(:database) do + klass = Opencdd::Klass.new( + irdi: Opencdd::IRDI.parse("0112/2///62656_4#AAA001"), + properties: { + Opencdd::PropertyIds::MDC_P001_5 => "0112/2///62656_4#AAA001", + Opencdd::PropertyIds::MDC_P004_1 => "Sample class", # preferred_name.en + Opencdd::PropertyIds::MDC_P002_1 => "001", # version (the bug) + Opencdd::PropertyIds::MDC_P002_2 => "01", # revision + }, + meta_class_irdi: Opencdd::IRDI.parse("0112/2///62656_1#MDC_C002"), + ) + Opencdd::Database.new.add_entity(klass) + end + + it "preserves leading-zero strings through xlsx round-trip" do + tmp = File.join(Dir.mktmpdir, "round-trip.xlsx") + Opencdd::Parcel::Writer.new(database).write(tmp, parcel_id: "TEST") + + reloaded = Opencdd::Database.load_workbook(tmp) + sample = reloaded.entities.first + + expect(sample.version).to eq("001") + expect(sample.revision).to eq("01") + end + + # Note: a full semantically_equal? round-trip test is blocked by a separate + # multilingual key normalization gap (MDC_P004_1 vs MDC_P004.en). That's a + # different audit item; not in scope here. +end