From eb12faf96395e4a05da4fea7778dbba589adb188 Mon Sep 17 00:00:00 2001 From: nicolasbisurgi Date: Wed, 8 Jul 2026 18:08:58 -0300 Subject: [PATCH] fix(tm1): validate results-CSV header to prevent measure drift (#169) }rushti.load.results maps CSV columns to TI variables positionally. A process built before 2.3.0 (pre-chore column) and left in place after an in-place rushti upgrade silently shifted every value from chore/ parameters onward into the next measure. Read the header row (asciiHeaderRecords=0) and validate each column against the variable it feeds in the metadata tab; ProcessQuit with the offending columns on mismatch instead of writing scrambled data. The check is generated from PROCESS_VARIABLES so it can't drift from the CSV contract. Verified end-to-end on TM1 v11 and v12. Co-Authored-By: Claude Opus 4.8 (1M context) --- CHANGELOG.md | 13 +++++ docs/advanced/migration-guide.md | 3 + src/rushti/tm1_objects.py | 98 ++++++++++++++++++++++++++++---- tests/unit/test_tm1_build.py | 50 ++++++++++++++++ 4 files changed, 153 insertions(+), 11 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7295c7e..6b11b89 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,19 @@ All notable changes to RushTI are documented in this file. ## [Unreleased] +- **Fixed: results written to the wrong measures after an in-place upgrade** + (closes #169). `}rushti.load.results` maps CSV columns to TI variables + positionally. A cube/process built by a pre-2.3.0 rushti (before the `chore` + column) that was then upgraded without re-running `rushti build` would + silently shift every value from `chore`/`parameters` onward into the next + measure. The loader now reads the CSV header row (`asciiHeaderRecords=0`) and + validates each column against the variable it feeds in the metadata tab: on + mismatch it logs the offending columns and `ProcessQuit`s instead of writing + scrambled data. **After upgrading rushti you must re-run + `rushti build --tm1-instance X`** to refresh the process (and additively add + any new measure element) — the guard only turns future drift into a clear + error, it does not repair an already-stale process. Existing rows written + under the wrong measures should be cleared and the workflow re-run. - **Added: `--config PATH` CLI flag** (closes #164). Overrides the location of `config.ini` (TM1 connection parameters) for a single invocation, on every TM1-connecting command (`run`, `build`, `tasks …`, `resume`). Precedence: diff --git a/docs/advanced/migration-guide.md b/docs/advanced/migration-guide.md index 9c8b328..88eb98e 100644 --- a/docs/advanced/migration-guide.md +++ b/docs/advanced/migration-guide.md @@ -171,6 +171,9 @@ Task file validation failed: You don't need a separate command — bare `rushti build` does the right thing. CI/CD pipelines that run `rushti build` on every release continue to work across version upgrades without manual intervention. +!!! warning "Re-run `rushti build` after every upgrade" + `}rushti.load.results` maps result-CSV columns to its variables **by position**. If a release adds a measure column (as 2.3.0 did with `chore`) and you upgrade the Python package **without** re-running `rushti build`, the stale process reads each column into the wrong variable and writes values to the next measure over (issue [#169](https://github.com/cubewise-code/rushti/issues/169)). The loader now validates the CSV header and fails with a clear error instead of silently scrambling data — but the fix for a stale deployment is still to **re-run `rushti build --tm1-instance `**. If you already have scrambled rows, clear them and re-run the workflow. + --- ## Step-by-Step Migration diff --git a/src/rushti/tm1_objects.py b/src/rushti/tm1_objects.py index 98262d7..4ff8706 100644 --- a/src/rushti/tm1_objects.py +++ b/src/rushti/tm1_objects.py @@ -150,6 +150,8 @@ nErrors = 0; nMetadataRecordCount= 0; nDataRecordCount = 0; +nHeaderChecked = 0; +nDataHeaderChecked = 0; ### Process Specific Constants cFileSrc = pSourceFile; @@ -220,7 +222,9 @@ If( nErrors = 0 ); DatasourceType = 'CHARACTERDELIMITED'; DatasourceNameForServer = cFileSrc; - DatasourceASCIIHeaderRecords = 1; + # 0 (not 1): the header row is consumed as the first record so the + # metadata tab can validate it against the expected columns (issue #169). + DatasourceASCIIHeaderRecords = 0; DatasourceASCIIDelimiter=','; DatasourceASCIIDecimalSeparator='.'; DatasourceASCIIThousandSeparator=''; @@ -290,6 +294,8 @@ nErrors = 0; nMetadataRecordCount= 0; nDataRecordCount = 0; +nHeaderChecked = 0; +nDataHeaderChecked = 0; ### Process Specific Constants cFileSrc = pSourceFile; @@ -356,7 +362,9 @@ If( nErrors = 0 ); DatasourceType = 'CHARACTERDELIMITED'; DatasourceNameForServer = cFileSrc; - DatasourceASCIIHeaderRecords = 1; + # 0 (not 1): the header row is consumed as the first record so the + # metadata tab can validate it against the expected columns (issue #169). + DatasourceASCIIHeaderRecords = 0; DatasourceASCIIDelimiter=','; DatasourceASCIIDecimalSeparator='.'; DatasourceASCIIThousandSeparator=''; @@ -366,10 +374,17 @@ ################################################################################################# """ -PROCESS_METADATA = r"""#****Begin: Generated Statements*** +# Marker pair TM1 Architect uses to inject auto-generated variable/parameter +# code at the top of a tab. Kept as the first lines of every generated +# procedure so the process round-trips cleanly if later opened in Architect. +_GENERATED_STATEMENTS_MARKER = """#****Begin: Generated Statements*** #****End: Generated Statements**** +""" -If( pLogOutput >= 1 ); +# Body of the metadata tab. The header-validation preamble is generated from +# PROCESS_VARIABLES and prepended in ``build_metadata_procedure`` so it stays +# in lockstep with the CSV column contract (see issue #169). +_PROCESS_METADATA_BODY = r"""If( pLogOutput >= 1 ); nMetadataRecordCount = nMetadataRecordCount + 1; EndIf; @@ -388,10 +403,9 @@ DimensionElementInsertDirect(pRunId_Dim, '', vrun_id, 'N'); endif;""" -PROCESS_DATA = r"""#****Begin: Generated Statements*** -#****End: Generated Statements**** - -If( pLogOutput >= 1 ); +# Body of the data tab. The header-skip preamble is prepended in +# ``build_data_procedure`` so the CSV header row is not written to the cube. +_PROCESS_DATA_BODY = r"""If( pLogOutput >= 1 ); nDataRecordCount = nDataRecordCount + 1; EndIf; @@ -544,8 +558,9 @@ # declaration list. Must match the column order produced by # ``rushti.tm1_integration.upload_results_to_tm1`` (which inserts # ``workflow`` and ``run_id`` ahead of the columns built by -# ``build_results_dataframe``). Mismatched ordering silently scrambles -# the loaded payload. +# ``build_results_dataframe``). Mismatched ordering scrambles the loaded +# payload; the metadata-tab header check generated from this list turns a +# stale-process mismatch into a hard error instead (see issue #169). PROCESS_VARIABLES = [ "vworkflow", "vrun_id", @@ -571,12 +586,73 @@ "vsucceed_on_minor_errors", ] + +def build_metadata_procedure() -> str: + """Assemble the ``}rushti.load.results`` metadata tab. + + Prepends a header-validation preamble generated from ``PROCESS_VARIABLES`` + so it stays in lockstep with the CSV column contract. The loader maps CSV + columns to variables positionally; with ``asciiHeaderRecords=0`` the CSV + header row is the first source record, so validating each column name + against the variable it feeds makes a process left stale by an in-place + rushti upgrade fail loudly instead of silently writing values to the wrong + measures (issue #169). Re-run ``rushti build`` to refresh a stale process. + """ + checks = "\n".join( + f" If( {var} @<> '{var[1:]}' );\n" + f" sHeaderError = sHeaderError | ' {var[1:]}';\n" + f" EndIf;" + for var in PROCESS_VARIABLES + ) + preamble = f"""### Validate the CSV header against the columns this process expects. +### The loader maps columns to variables positionally, so a process built by +### an older rushti scrambles measures (issue #169). Fail loudly on mismatch +### and skip the header so it is not loaded as data. +If( nHeaderChecked = 0 ); + nHeaderChecked = 1; + sHeaderError = ''; +{checks} + If( Trim( sHeaderError ) @<> '' ); + nErrors = nErrors + 1; + sMessage = Expand( 'Results CSV header does not match the columns }}rushti.load.results expects (mismatched:%sHeaderError% ). This process is out of date with the rushti version that produced the file - re-run: rushti build --tm1-instance .' ); + LogOutput( cMsgErrorLevel, Expand( cMsgErrorContent ) ); + ProcessQuit; + EndIf; + ItemSkip; +EndIf; + +""" + return _GENERATED_STATEMENTS_MARKER + "\n" + preamble + _PROCESS_METADATA_BODY + + +def build_data_procedure() -> str: + """Assemble the ``}rushti.load.results`` data tab. + + Skips the CSV header row (validated in the metadata tab) so it is not + written to the cube, then delegates to the static body. See issue #169. + """ + preamble = """### Skip the CSV header row (asciiHeaderRecords=0; validated in the +### metadata tab). See issue #169. +If( nDataHeaderChecked = 0 ); + nDataHeaderChecked = 1; + ItemSkip; +EndIf; + +""" + return _GENERATED_STATEMENTS_MARKER + "\n" + preamble + _PROCESS_DATA_BODY + + +PROCESS_METADATA = build_metadata_procedure() +PROCESS_DATA = build_data_procedure() + PROCESS_DATASOURCE = { "Type": "ASCII", "asciiDecimalSeparator": ".", "asciiDelimiterChar": ",", "asciiDelimiterType": "Character", - "asciiHeaderRecords": 1, + # 0: the header row is validated (not skipped) by the metadata tab; see + # the DatasourceASCIIHeaderRecords note in the prolog and issue #169. + "asciiHeaderRecords": 0, "asciiQuoteCharacter": '"', "asciiThousandSeparator": ",", "dataSourceNameForClient": "", diff --git a/tests/unit/test_tm1_build.py b/tests/unit/test_tm1_build.py index a14d12e..162e2d4 100644 --- a/tests/unit/test_tm1_build.py +++ b/tests/unit/test_tm1_build.py @@ -10,6 +10,12 @@ from rushti.tm1_objects import ( MEASURE_ELEMENTS, MEASURE_ATTRIBUTES, + PROCESS_DATA, + PROCESS_DATASOURCE, + PROCESS_METADATA, + PROCESS_PROLOG_V11, + PROCESS_PROLOG_V12, + PROCESS_VARIABLES, TASK_ID_ELEMENT_COUNT, WORKFLOW_SEED_ELEMENTS, RUN_ID_SEED_ELEMENTS, @@ -101,6 +107,50 @@ def test_sample_data_records_have_required_keys(self): self.assertIn("value", record) +class TestLoadResultsHeaderValidation(unittest.TestCase): + """Guards the }rushti.load.results header-validation contract (issue #169). + + The loader maps CSV columns to process variables positionally. A process + built by an older rushti silently wrote values to the wrong measures after + a column was added. These tests lock in the metadata-tab header check that + turns such drift into a hard error. + """ + + def test_datasource_reads_header_as_data(self): + """Header row must reach the tabs (records=0) so it can be validated.""" + self.assertEqual(PROCESS_DATASOURCE["asciiHeaderRecords"], 0) + + def test_prologs_read_header_as_data_and_seed_skip_flags(self): + """Both prolog variants set records=0 and init the header-skip flags.""" + for prolog in (PROCESS_PROLOG_V11, PROCESS_PROLOG_V12): + self.assertIn("DatasourceASCIIHeaderRecords = 0;", prolog) + self.assertNotIn("DatasourceASCIIHeaderRecords = 1;", prolog) + self.assertIn("nHeaderChecked", prolog) + self.assertIn("nDataHeaderChecked", prolog) + + def test_metadata_validates_every_expected_column(self): + """Every CSV column is checked against the variable that feeds it. + + Generated from PROCESS_VARIABLES, so adding a column without updating + the loader keeps the check and the CSV contract in lockstep. + """ + for var in PROCESS_VARIABLES: + column = var[1:] # strip the leading 'v' + self.assertIn(f"If( {var} @<> '{column}' );", PROCESS_METADATA) + + def test_metadata_fails_loud_and_skips_header(self): + """On mismatch the process quits; the header itself is never loaded.""" + self.assertIn("If( nHeaderChecked = 0 );", PROCESS_METADATA) + self.assertIn("ProcessQuit;", PROCESS_METADATA) + self.assertIn("ItemSkip;", PROCESS_METADATA) + self.assertIn("rushti build --tm1-instance", PROCESS_METADATA) + + def test_data_tab_skips_header_row(self): + """The data tab drops the header record so it is not written to cube.""" + self.assertIn("If( nDataHeaderChecked = 0 );", PROCESS_DATA) + self.assertIn("ItemSkip;", PROCESS_DATA) + + class TestDimensionBuilders(unittest.TestCase): """Tests for dimension builder functions."""