Skip to content

Added v2 to v3 migration transforms - #20

Draft
HassanAkbar wants to merge 6 commits into
fix/parse-data-lossfrom
feat/migrate-version-transforms
Draft

Added v2 to v3 migration transforms#20
HassanAkbar wants to merge 6 commits into
fix/parse-data-lossfrom
feat/migrate-version-transforms

Conversation

@HassanAkbar

Copy link
Copy Markdown
Member

Dcc.migrate was a stub that re-read the file and changed a version number. This
makes it do the conversion: 29 transform rules covering renames, refId moves,
description merging, and the D-SI uncertainty upgrade.

Known and accepted:

  • Opens against fix/parse-data-loss, not main. The uncertainty values it upgrades
    are destroyed during parsing without that branch. On main this fails 4 examples.
  • Only 2.3.0 -> 3.3.0 is routed. Other pairs raise rather than silently pass the
    document through unchanged.
  • Constructs v3 cannot represent are reported on stderr, not raised. The document
    still converts; the caller is told what changed.
  • A trim that leaves a field empty raises instead of reporting. v3 rejects "" with
    [facet 'pattern'], so reporting it would look handled while emitting a file that
    cannot validate.
  • Merging descriptions rewrites every refId that pointed at a removed id, rather
    than reporting the loss. The XSD validator does not catch dangling references, so
    nothing downstream would have flagged it.
  • The end-to-end spec asserts migration adds zero new validation errors, rather than
    absolute validity. example.xml already fails v2.3.0 with 8 errors before migrating.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Implements a real v2.3.0 → v3.3.0 migration (instead of a schemaVersion-only stub) by introducing a routed XML transform that performs element renames, structural rewrites, D‑SI uncertainty upgrades, and loss reporting, with fixtures and specs to validate behavior.

Changes:

  • Add Dcc::Migrate::V2ToV3 XML transform and a Dcc::Migrate::Route registry to restrict migrations to verified version pairs.
  • Update Dcc::Migrate to validate inputs, reject downgrades/unregistered pairs, apply transforms, and re-parse under the target version.
  • Expand migration test coverage substantially and add v2 fixtures that exercise transform rules and edge cases.

Reviewed changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated no comments.

Show a summary per file
File Description
spec/fixtures/migrate/v2_si_list.xml Adds a hand-built v2.3.0 fixture covering v1-shaped si:list + previousReport cases that can’t round-trip through the object model.
spec/fixtures/migrate/v2_full_coverage.xml Adds a v2.3.0 fixture designed to exercise transform rules that can round-trip through the public API.
spec/dcc/migrate_spec.rb Adds comprehensive routing/version-handling tests plus transform and end-to-end migration assertions (including idempotency and XSD validity).
lib/dcc/v2/identification.rb Specializes v2 identification mapping so v2 uses description while shared base maps v3’s name.
lib/dcc/v2/condition.rb Specializes v2 condition mapping so v2 state maps into the shared status attribute.
lib/dcc/migrate/v2_to_v3.rb Introduces the v2→v3 migration transform: renames, refId moves, description merges + reference repointing, trimming rules, D‑SI upgrades, and loss reporting.
lib/dcc/migrate/route.rb Adds an explicit allowlist of supported migration pairs and a transform lookup.
lib/dcc/migrate.rb Replaces the stub with routed, validated, one-way migration that never mutates the input object.
lib/dcc/error.rb Adds UnsupportedMigrationError for downgrades and unverified pairs.
lib/dcc/base/identification.rb Updates shared identification model to v3’s name child (with v2 handled via the specialized v2 model).
lib/dcc/base/core_data.rb Adds performance_location mapping to support v3’s required performanceLocation.
lib/dcc/base/condition.rb Updates shared condition mapping to v3’s status child (with v2 handled via the specialized v2 model).
.rubocop_todo.yml Exempts the expanded migration spec from selected RSpec cops due to size/expectation count.

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

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 21 out of 21 changed files in this pull request and generated no new comments.

Suppressed comments (3)

lib/dcc/migrate/v2_to_v3.rb:153

  • Similar to the XML syntax error case: reject_rootless can be triggered by non-v2→v3 routes (since it underpins the shared root check), but the raised message hard-codes Dcc::Migrate::V2ToV3. Consider using a generic Dcc.migrate/Dcc::Migrate prefix so callers aren’t misled about which route failed.
        def reject_rootless(doc)
          root = doc.root
          return if root && root.name == ROOT_ELEMENT &&
            root.namespace&.href == ROOT_NAMESPACE

          raise ::Dcc::ParseError,
                "Dcc::Migrate::V2ToV3 expects a <dcc:#{ROOT_ELEMENT}> root " \
                "in the #{ROOT_NAMESPACE} namespace, got " \
                "#{describe_root(root)}."

lib/dcc/migrate.rb:56

  • same_version_result returns any parsed DCC document unchanged, regardless of whether it matches the requested from/to major. E.g., from: "3.3.0", to: "3.3.0" with a Dcc::V2::DigitalCalibrationCertificate will silently return the v2 object. Consider only no-op’ing when the input’s major matches target (otherwise parse/raise).
      def same_version_result(input, target)
        return input if dcc_document?(input)

        parse_as(source_xml(input), target)
      end

lib/dcc/migrate/v2_to_v3.rb:139

  • Dcc::Migrate::V2ToV3.parse is used for root/well-formedness checks across all migration routes (via Dcc::Migrate.source_xml), but the error message hard-codes Dcc::Migrate::V2ToV3, which is confusing when migrating within v3 or doing same-version validation. Consider making this error message route-agnostic (e.g., Dcc.migrate ...).

This issue also appears on line 145 of the same file.

        def parse(xml)
          ::Nokogiri::XML(xml, &:strict)
        rescue ::Nokogiri::XML::SyntaxError => e
          raise ::Dcc::ParseError,
                "Dcc::Migrate::V2ToV3 expects well-formed DCC XML: #{e.message}"
        end

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 21 out of 21 changed files in this pull request and generated 1 comment.

Comment thread lib/dcc/migrate.rb

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 21 out of 21 changed files in this pull request and generated no new comments.

Suppressed comments (2)

lib/dcc/migrate/v2_to_v3.rb:294

  • description assumes every dcc:item / dcc:measuringEquipment has a dcc:name element; if it’s missing (invalid input, or future schema variant), parent.at_xpath("dcc:name", NS) returns nil and this raises a NoMethodError rather than a clear Dcc::ParseError like performance_anchor does. It’d be better to fail with a targeted parse/migration error.
        def description(doc, parent)
          parent.at_xpath("dcc:description", NS) ||
            element(doc, "dcc:description").tap do |node|
              parent.at_xpath("dcc:name", NS).add_next_sibling(node)
            end

lib/dcc/base/core_data.rb:23

  • Dcc::Base::CoreData is included by both v2 and v3 wrappers, but performanceLocation does not exist in the v2.3.0 XSD (coreDataType ends at endPerformanceDate + optional previousReport). Adding attribute :performance_location + map_element "performanceLocation" means v2 models can now parse/emit a v3-only element and produce v2 XML that cannot validate.

Consider keeping performance_location in the v3 model only (or overriding Dcc::V2::CoreData like Dcc::V2::Identification/Condition do) so v2 serialization stays schema-accurate.

          attribute :performance_location, :string

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants