Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
## [Unreleased]
* no unreleased changes

### Fixed
* Fix bug in VCF metadata
* Fix bug where table columns are mutated by a regexp column

## 11.3.0/ 2025-02-11
### Fixed
Expand Down
10 changes: 9 additions & 1 deletion lib/ndr_import/table.rb
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@ def initialize(options = {})
end

@row_index = 0
# Keep a copy of the original column mappings for use later if columns are mutated
@original_columns = @columns.deep_dup
end

def match(filename, tablename)
Expand All @@ -51,12 +53,18 @@ def transform(lines, &block)
@header_valid = false
@header_best_guess = nil
@notifier.try(:started)

last_col = last_column_to_transform
skip_footer_lines(lines, footer_lines).each do |line|
line.is_a?(Array) ? process_line(line[0..last_col], &block) : process_line(line, &block)
end

# @columns may have been mutated where column is a regular expression.
# We want to restore `@columns` back to its original state, so the column regexp
# will work on the next file
@columns = @original_columns
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be @columns = @original_columns.deep_dup otherwise it will work twice but not 3 times.

Maybe also move this to immediately after @notifier.try(:started) ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I've moved it up to @notifier.try(:started) locally, but lot of tests are erroring.

Despite instance_variable_set("@#{key}", options[key], it seems like @columns is not set in the initializer, so @original_columns is nil.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A downstream issue appears to be loading NdrImport::Table classes via YAML; this doesn't hit the initializer.

# Also ensure that @masked_mappings are recalculated
@masked_mappings = nil

@notifier.try(:finished)
end

Expand Down
6 changes: 5 additions & 1 deletion lib/ndr_import/vcf/table.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,14 @@ module Vcf
# Syntatic sugar to ensure `header_lines` and `footer_lines` are 1 and 0 respectively.
# All other Table logic is inherited from `NdrImport::Table`
class Table < ::NdrImport::Table
VCF_OPTIONS = %w[vcf_file_metadata].freeze

def self.all_valid_options
super - %w[delimiter header_lines footer_lines] + %w[vcf_file_metadata]
super - %w[delimiter header_lines footer_lines] + VCF_OPTIONS
end

attr_reader(*VCF_OPTIONS)

def header_lines
1
end
Expand Down
Binary file added test/resources/regex_column_names.zip
Binary file not shown.
24 changes: 24 additions & 0 deletions test/universal_importer_helper_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -222,6 +222,30 @@ def setup
assert_equal 4, table_enums.first.last.count
end

test 'mutated columns get reset' do
table_mappings = [
NdrImport::Table.new(filename_pattern: /\.csv\z/i,
canonical_name: 'a_table',
header_lines: 1,
footer_lines: 0,
klass: 'SomeTestClass',
columns: [{ 'column' => 'one' },
{ 'column' => 'two' },
{ 'column' => /\A[AB]\d{3}\z/i }])
]
source_file = @permanent_test_files.join('regex_column_names.zip')
@test_importer.stubs(:get_table_mapping).returns(table_mappings.first)
mapped_rows = []
@test_importer.extract(source_file) { |table, rows| mapped_rows << table.transform(rows).to_a }

expected_mapped_data = [
[['SomeTestClass', { rawtext: { 'one' => '2', 'two' => '2', 'b456' => '2' } }, 1]],
[['SomeTestClass', { rawtext: { 'one' => '1', 'two' => '1', 'a123' => '1' } }, 1]]
]

assert_equal expected_mapped_data, mapped_rows
end

test 'get_notifier' do
class TestImporterWithoutNotifier
include NdrImport::UniversalImporterHelper
Expand Down