ecrf importer converter - #31
Conversation
Support --task=convert with --converter to normalize input (e.g. Interfast3 lab CSV) into EcrfDataHorizontal intermediate files before import. Co-authored-by: Cursor <cursoragent@cursor.com>
Match columns by eCRF field externalId, add CompleteEcrfField client, and let CSV FileProcessors report a single sheet. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
Warning Review limit reached
Next review available in: 33 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds pluggable eCRF file conversion, an Interfast3 lab CSV-to-XLSX converter, REST-based eCRF field lookup, CLI conversion-task support, and a single-stream sheet-name contract. ChangeseCRF conversion
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant EcrfImport
participant Interfast3LabData
participant CTMS REST API
participant XLSX
CLI->>EcrfImport: convert_ecrf_data(file, converter)
EcrfImport->>Interfast3LabData: convert(input file)
Interfast3LabData->>CTMS REST API: fetch trial, field, and proband data
CTMS REST API-->>Interfast3LabData: return lookup results
Interfast3LabData->>XLSX: write mapped worksheet rows
XLSX-->>CLI: return intermediate filename
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Stop falling back to the sub claim in get_username_from_jwt. Co-authored-by: Cursor <cursoragent@cursor.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CTSMS/BulkProcessor/Projects/ETL/EcrfImport.pm`:
- Around line 158-194: Update _load_converter to validate $spec as a safe Perl
package name before converting it into a filesystem path, rejecting traversal
segments or other invalid shapes before constructing $module_file. Remove the
global `@INC` mutation and require the validated absolute module path directly,
while preserving the existing file-existence check and converter-method
validation.
In
`@CTSMS/BulkProcessor/Projects/ETL/EcrfImporter/Converter/Interfast3LabData.pm`:
- Around line 258-296: Update _create_lab_sheets so each external_id is
normalized with the same _normalize_colnames transformation used for CSV headers
before looking it up in %csv_index_by_lc. Preserve the existing case-insensitive
matching and matched-column metadata, ensuring punctuation differences such as
slashes resolve to the normalized CSV column.
In `@CTSMS/BulkProcessor/Projects/ETL/EcrfImporter/process.pl`:
- Around line 285-288: Update the conversion result validation in the eval block
around convert_ecrf_data so success requires $outfile to identify a regular,
readable, non-empty file, not merely a non-empty string. Preserve the existing
failure path for invalid output and only assign/pass the path to
import_ecrf_data_horizontal when this validation succeeds.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4dd1e3dc-a42c-42d7-907a-7503a6a9a665
📒 Files selected for processing (5)
CTSMS/BulkProcessor/FileProcessor.pmCTSMS/BulkProcessor/Projects/ETL/EcrfImport.pmCTSMS/BulkProcessor/Projects/ETL/EcrfImporter/Converter/Interfast3LabData.pmCTSMS/BulkProcessor/Projects/ETL/EcrfImporter/process.plCTSMS/BulkProcessor/RestRequests/ctsms/shared/ToolsService/CompleteEcrfField.pm
| sub convert_ecrf_data { | ||
| my ($file,$converter) = @_; | ||
|
|
||
| my $convert_code = _load_converter($converter); | ||
| my $infile = get_input_filename($file,$ecrf_import_filename); | ||
| my $outfile = &$convert_code($infile); | ||
| if (length($outfile)) { | ||
| scriptinfo("converter '$converter' wrote intermediate file $outfile",getlogger(__PACKAGE__)); | ||
| } | ||
| return $outfile; | ||
| } | ||
|
|
||
| sub _load_converter { | ||
| my ($spec) = @_; | ||
| scripterror('converter module required (e.g. --converter=Converter::MyConverter)',getlogger(getscriptpath())) | ||
| unless length($spec); | ||
|
|
||
| # Converters live next to process.pl: EcrfImporter/Converter/*.pm | ||
| my $importer_dir = Cwd::abs_path(File::Basename::dirname(__FILE__) . '/EcrfImporter'); | ||
| (my $rel_path = $spec) =~ s|::|/|g; | ||
| my $module_file = $importer_dir . '/' . $rel_path . '.pm'; | ||
| scripterror("converter module not found: $module_file",getlogger(getscriptpath())) | ||
| unless -f $module_file; | ||
|
|
||
| unshift(@INC,$importer_dir) unless grep { $_ eq $importer_dir } @INC; | ||
| eval { | ||
| require $rel_path . '.pm'; | ||
| 1; | ||
| } or do { | ||
| scripterror("failed to load converter '$spec': " . ($@ // 'unknown error'),getlogger(getscriptpath())); | ||
| }; | ||
|
|
||
| my $convert_code = $spec->can('convert') || $spec->can('process'); | ||
| scripterror("converter '$spec' must expose convert() or process()",getlogger(getscriptpath())) | ||
| unless $convert_code; | ||
| return $convert_code; | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Validate converter specs before require and avoid mutating global @INC.
$spec is converted into a filesystem path with simple :: → / substitution and then required. A spec like ..::..::..::somewhere::payload can resolve outside EcrfImporter/Converter/ and be required if .pm exists at that path. Reject non-package-name shapes before building $module_file, and use the already-validated absolute path directly instead of pushing/importing EcrfImporter into @INC.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@CTSMS/BulkProcessor/Projects/ETL/EcrfImport.pm` around lines 158 - 194,
Update _load_converter to validate $spec as a safe Perl package name before
converting it into a filesystem path, rejecting traversal segments or other
invalid shapes before constructing $module_file. Remove the global `@INC` mutation
and require the validated absolute module path directly, while preserving the
existing file-existence check and converter-method validation.
| my %csv_index_by_lc = (); | ||
| for (my $i = 0; $i < scalar @$csv_colnames; $i++) { | ||
| $csv_index_by_lc{lc($csv_colnames->[$i])} = $i; | ||
| } | ||
|
|
||
| $context->{sheets_by_name} = {}; | ||
| $context->{anfonr_index} = $csv_index_by_lc{lc('AnfoNr')}; | ||
|
|
||
| foreach my $ecrf (@{$context->{lab_ecrfs}}) { | ||
| my $name = $ecrf->{name}; | ||
| my @header = ('proband_id'); | ||
| my @matched = (); | ||
| foreach my $col (@{$context->{lab_columns_by_name}->{$name} // []}) { | ||
| my $csv_index = $csv_index_by_lc{lc($col->{external_id})}; | ||
| next unless defined $csv_index; | ||
| push(@header,$col->{colname}); | ||
| push(@matched,{ | ||
| colname => $col->{colname}, | ||
| csv_index => $csv_index, | ||
| external_id => $col->{external_id}, | ||
| }); | ||
| } | ||
|
|
||
| my $sheetname = sanitize_spreadsheet_name($name); | ||
| my $worksheet = $context->{workbook}->add_worksheet($sheetname); | ||
| for (my $c = 0; $c < scalar @header; $c++) { | ||
| $worksheet->write_string(0,$c,$header[$c],$context->{header_format}); | ||
| } | ||
|
|
||
| $context->{sheets_by_name}->{$name} = { | ||
| worksheet => $worksheet, | ||
| header => \@header, | ||
| matched => \@matched, | ||
| next_row => 1, | ||
| ecrf => $ecrf, | ||
| }; | ||
| processing_info($context->{tid},"sheet '$sheetname': " . (scalar @matched) . ' matched column(s)',getlogger(__PACKAGE__)); | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd -a 'CSVDB.pm'
rg -n 'sub sanitize_column_name' -A 20 $(fd -a 'CSVDB.pm')Repository: phoenixctms/bulk-processor
Length of output: 706
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== imports and relevant subroutines =="
sed -n '1,80p' CTSMS/BulkProcessor/Projects/ETL/EcrfImporter/Converter/Interfast3LabData.pm
sed -n '220,310p' CTSMS/BulkProcessor/Projects/ETL/EcrfImporter/Converter/Interfast3LabData.pm
sed -n '390,460p' CTSMS/BulkProcessor/Projects/ETL/EcrfImporter/Converter/Interfast3LabData.pm
echo "== sanitize_column_name occurrences =="
rg -n 'sanitize_column_name|normalize_colnames|AnfoNr|external_id|colname' CTSMS/BulkProcessor/Projects/ETL/EcrfImporter/Converter/Interfast3LabData.pm
echo "== standalone Perl behavior probe for comparison transform =="
perl -Mstrict -Mwarnings -e '
sub sanitize_column_name {
my $column_name = shift;
$column_name =~ s/\\W/_/g;
return $column_name;
}
my `@raw` = ("Proband ID","Creatinine","Glucose_mg/dL","HCT%","WBC");
my `@sanitized` = map { sanitize_column_name($_) } `@raw`;
my %csv_index_by_lc_sanitized = ();
for (my $i = 0; $i < scalar `@sanitized`; $i++) {
$csv_index_by_lc_sanitized{lc($sanitized[$i])} = $i;
}
foreach my $config ("Proband ID","Creatinine","Glucose_mg/dL","HCT%","WBC") {
my $csv_index = $csv_index_by_lc_sanitized{lc($config)};
print "raw=$config csv_index=$csv_index\n";
}
'Repository: phoenixctms/bulk-processor
Length of output: 9617
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== lab_columns_by_name population =="
sed -n '120,215p' CTSMS/BulkProcessor/Projects/ETL/EcrfImporter/Converter/Interfast3LabData.pm
echo "== CSVDB sanitize_column_name implementation =="
sed -n '80,105p' CTSMS/BulkProcessor/SqlConnectors/CSVDB.pmRepository: phoenixctms/bulk-processor
Length of output: 4543
Normalize both CSV headers and external_id before matching columns
_normalize_colnames() replaces every non-word header character with _ before filling %csv_index_by_lc, but _create_lab_sheets() looks up raw external_id values. Headers like creatinine_mg/dL become creatinine_mg_dL; if the eCRF field external_id is the original analyte title, it will never find its CSV column and the mapped data is skipped silently. Apply the same transform to external_id before lookup, or compare against the raw trimmed header instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@CTSMS/BulkProcessor/Projects/ETL/EcrfImporter/Converter/Interfast3LabData.pm`
around lines 258 - 296, Update _create_lab_sheets so each external_id is
normalized with the same _normalize_colnames transformation used for CSV headers
before looking it up in %csv_index_by_lc. Preserve the existing case-insensitive
matching and matched-column metadata, ensuring punctuation differences such as
slashes resolve to the normalized CSV column.
| eval { | ||
| $outfile = convert_ecrf_data($file,$converter); | ||
| $result = length($outfile) ? 1 : 0; | ||
| if ($result) { |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Validate that conversion produced a usable file.
length($outfile) only verifies that the converter returned a non-empty string. A missing, unreadable, or zero-byte path will be reported as successful, assigned to $file, and passed to import_ecrf_data_horizontal. Check that the output is a regular, readable, non-empty file before returning success.
Proposed fix
- $result = length($outfile) ? 1 : 0;
+ $result = (
+ defined($outfile)
+ && length($outfile)
+ && -f($outfile)
+ && -r($outfile)
+ && -s($outfile)
+ ) ? 1 : 0;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| eval { | |
| $outfile = convert_ecrf_data($file,$converter); | |
| $result = length($outfile) ? 1 : 0; | |
| if ($result) { | |
| eval { | |
| $outfile = convert_ecrf_data($file,$converter); | |
| $result = ( | |
| defined($outfile) | |
| && length($outfile) | |
| && -f($outfile) | |
| && -r($outfile) | |
| && -s($outfile) | |
| ) ? 1 : 0; | |
| if ($result) { |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@CTSMS/BulkProcessor/Projects/ETL/EcrfImporter/process.pl` around lines 285 -
288, Update the conversion result validation in the eval block around
convert_ecrf_data so success requires $outfile to identify a regular, readable,
non-empty file, not merely a non-empty string. Preserve the existing failure
path for invalid output and only assign/pass the path to
import_ecrf_data_horizontal when this validation succeeds.
1 participant
Summary by CodeRabbit