From 806b23cd6d1343188919bdd30ca39b3fc795ae50 Mon Sep 17 00:00:00 2001 From: Jac Fitzgerald Date: Wed, 29 Jul 2026 15:45:34 -0700 Subject: [PATCH 1/5] fix: preserve '=' inside --filter values Previously `apply_filter_value` called `value.split("=")`, so a filter like `Notes=x=y` silently truncated to name=Notes, value=x. Split on the first '=' only (maxsplit=1) so multi-'=' values round-trip intact, and raise a clear error when the input isn't in name=value form. Verified end-to-end against a real workbook: `Product Name=x=y Config Kit` now filters correctly instead of returning empty results. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../datasources_and_workbooks_command.py | 12 ++++++++++-- .../test_datasources_and_workbooks_command.py | 19 +++++++++++++++++++ 2 files changed, 29 insertions(+), 2 deletions(-) diff --git a/tabcmd/commands/datasources_and_workbooks/datasources_and_workbooks_command.py b/tabcmd/commands/datasources_and_workbooks/datasources_and_workbooks_command.py index ae577bfa..62145ebf 100644 --- a/tabcmd/commands/datasources_and_workbooks/datasources_and_workbooks_command.py +++ b/tabcmd/commands/datasources_and_workbooks/datasources_and_workbooks_command.py @@ -103,9 +103,17 @@ def apply_encoded_filter_value(logger, request_options: RequestOptionsType, valu @staticmethod def apply_filter_value(logger, request_options: RequestOptionsType, value: str) -> None: logger.debug("handling filter param {}".format(value)) - data_filter = value.split("=") + # Split on the first '=' only so that filter values containing '=' are + # preserved intact (e.g. Notes=x=y should filter Notes to the value "x=y"). + parts = value.split("=", maxsplit=1) + if len(parts) != 2: + Errors.exit_with_error( + logger, + message="Filter clause '{}' must be in name=value form".format(value), + ) + name, filter_value = parts # we should export the _DataExportOptions class from tsc - request_options.vf(data_filter[0], data_filter[1]) # type: ignore + request_options.vf(name, filter_value) # type: ignore # this is called from within from_url_params, for each param value # expects either ImageRequestOptions or PDFRequestOptions diff --git a/tests/commands/test_datasources_and_workbooks_command.py b/tests/commands/test_datasources_and_workbooks_command.py index 8fa60dde..d41ef428 100644 --- a/tests/commands/test_datasources_and_workbooks_command.py +++ b/tests/commands/test_datasources_and_workbooks_command.py @@ -40,6 +40,25 @@ def test_apply_encoded_filters_from_url_params(self): DatasourcesAndWorkbooks.apply_values_from_url_params(mock_logger, request_options, query_params) assert request_options.view_filters == expected + def test_apply_filter_value_with_equals_in_value(self): + # A filter value containing '=' should survive parsing intact. + # Old behavior: value.split("=") truncated at the second '=', + # so "Notes=x=y" incorrectly became name="Notes", value="x". + request_options = tsc.PDFRequestOptions() + DatasourcesAndWorkbooks.apply_filter_value(mock_logger, request_options, "Notes=x=y") + assert request_options.view_filters == [("Notes", "x=y")] + + def test_apply_filter_value_with_multiple_equals_in_value(self): + request_options = tsc.PDFRequestOptions() + DatasourcesAndWorkbooks.apply_filter_value(mock_logger, request_options, "Config=k1=v1=extra") + assert request_options.view_filters == [("Config", "k1=v1=extra")] + + def test_apply_filter_value_with_trailing_equals(self): + # Empty string after the '=' should produce an empty-string value. + request_options = tsc.PDFRequestOptions() + DatasourcesAndWorkbooks.apply_filter_value(mock_logger, request_options, "Name=") + assert request_options.view_filters == [("Name", "")] + def test_apply_options_from_url_params(self): query_params = "?:iid=5&:refresh=yes&:size=600,700" request_options = tsc.PDFRequestOptions() From 439b06751d726049285b751b9050d9823ece74c8 Mon Sep 17 00:00:00 2001 From: Jac Fitzgerald Date: Wed, 29 Jul 2026 16:16:50 -0700 Subject: [PATCH 2/5] test: add filter_test_data.csv fixture for e2e filter tests Dataset with product names containing '=', '&', ',', backslashes, and metacharacter samples for exercising --filter parsing edge cases against a real server. Also includes columns whose names contain '&', '#', and '\' so future tests can exercise special characters on both sides of the name=value pair. Co-Authored-By: Claude Opus 4.7 (1M context) --- tests/assets/filter_test_data.csv | 33 +++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) create mode 100644 tests/assets/filter_test_data.csv diff --git a/tests/assets/filter_test_data.csv b/tests/assets/filter_test_data.csv new file mode 100644 index 00000000..6034d83a --- /dev/null +++ b/tests/assets/filter_test_data.csv @@ -0,0 +1,33 @@ +Product Name,Category,Region,Sales,Sales & Cost,Order#,Path\File +Widget Plain,Standard,West,100,10,W001,C:\a +AT&T 841000 Phone,Electronics,East,200,20,W002,C:\b +Salt & Pepper Shaker,Kitchenware,West,150,15,W003,C:\c +K&N Air Filter,Automotive,South,175,17,W004,C:\d +x=y Config Kit,Software,North,250,25,W005,C:\e +Config=default Bundle,Software,East,300,30,W006,C:\f +Formula=E=mc2 Poster,Books,West,50,5,W007,C:\g +"Rock, Paper, Scissors Game",Toys,South,80,8,W008,C:\h +"Comma, Separated, Product",Standard,North,90,9,W009,C:\i +AT&T x=y Combo,Electronics,East,400,40,W010,C:\j +Region=West Reference Book,Books,West,60,6,W011,C:\k +Zurich Special,Standard,East,120,12,W012,C:\l +Zürich Special,Standard,East,130,13,W013,C:\m +Empty Value Test,Standard,West,0,0,W014,C:\n +"Path C:\temp\file",Software,North,500,50,W015,"C:\temp\file" +"Escape\, comma",Software,East,510,51,W016,C:\o +"Trail\",Software,West,520,52,W017,C:\p +META /slash/,Test,North,100,10,W018,C:\q +META /percent%/,Test,North,101,10,W019,C:\r +META /plus+/,Test,North,102,10,W020,C:\s +META /colon:/,Test,North,103,10,W021,C:\t +META /semi;/,Test,North,104,10,W022,C:\u +META /brace{}/,Test,North,105,10,W023,C:\v +META /bracket[]/,Test,North,106,10,W024,C:\w +META /paren()/,Test,North,107,10,W025,C:\x +META /question?/,Test,North,108,10,W026,C:\y +META /hash#/,Test,North,109,10,W027,C:\z +META /star*/,Test,North,110,10,W028,C:\aa +META /at@/,Test,North,111,10,W029,C:\bb +"META /quote""/",Test,North,112,10,W030,C:\cc +META /apos'/,Test,North,113,10,W031,C:\dd +"META /less<>greater/",Test,North,114,10,W032,C:\ee From 8d2dd4964c43f956e3db9c7363be9f032b0b7dca Mon Sep 17 00:00:00 2001 From: Jac Fitzgerald Date: Mon, 3 Aug 2026 17:23:26 -0700 Subject: [PATCH 3/5] fix: don't hard-error on unparseable URL filter fragments (Classic parity) The earlier tightening in this PR made apply_filter_value raise a clean error when a clause isn't in name=value form. That's the right behavior for the --filter flag, but URL-syntax exports inherited from tabcmd Classic scripts often contain literal '&' inside filter values (e.g. `?Product Name=AT&T 841000 Phone`), which get split into fragments the parser can't understand. Classic silently skipped such fragments. Verified end-to-end against a Tableau Server today: with the strict path, tabcmd 2 errored out ("Filter clause 'T 841000 Phone' must be in name=value form") where Classic silently continued and let the server return whatever the well- formed portion matched. Add strict: bool = True to apply_filter_value. --filter callers stay strict; apply_encoded_filter_value (the URL path) passes strict=False, which logs a WARNING and skips the fragment instead of exiting. Adds three tests: strict path still exits, non-strict path skips silently, and end-to-end apply_values_from_url_params tolerates a value with '&'. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../datasources_and_workbooks_command.py | 25 +++++++++++++---- .../test_datasources_and_workbooks_command.py | 28 +++++++++++++++++++ 2 files changed, 47 insertions(+), 6 deletions(-) diff --git a/tabcmd/commands/datasources_and_workbooks/datasources_and_workbooks_command.py b/tabcmd/commands/datasources_and_workbooks/datasources_and_workbooks_command.py index 62145ebf..957eac03 100644 --- a/tabcmd/commands/datasources_and_workbooks/datasources_and_workbooks_command.py +++ b/tabcmd/commands/datasources_and_workbooks/datasources_and_workbooks_command.py @@ -95,22 +95,35 @@ def apply_encoded_filter_value(logger, request_options: RequestOptionsType, valu # so we run url.decode, which will be a no-op if they are not encoded. decoded_value = urllib.parse.unquote(value) logger.debug("url had `{0}`, saved as `{1}`".format(value, decoded_value)) - DatasourcesAndWorkbooks.apply_filter_value(logger, request_options, decoded_value) + # URL-embedded filters can legitimately contain '&' when a user drops in a + # tabcmd Classic script that put `Field=x&y` in the URL. Classic silently + # skipped fragments that didn't parse; keep that behavior here so scripts + # migrate cleanly. The --filter flag path stays strict (see apply_filter_value). + DatasourcesAndWorkbooks.apply_filter_value(logger, request_options, decoded_value, strict=False) # this is called for each filter value, # from apply_options, which expects an un-encoded input, # or from apply_url_params via apply_encoded_filter_value which decodes the input @staticmethod - def apply_filter_value(logger, request_options: RequestOptionsType, value: str) -> None: + def apply_filter_value( + logger, request_options: RequestOptionsType, value: str, strict: bool = True + ) -> None: logger.debug("handling filter param {}".format(value)) # Split on the first '=' only so that filter values containing '=' are # preserved intact (e.g. Notes=x=y should filter Notes to the value "x=y"). parts = value.split("=", maxsplit=1) if len(parts) != 2: - Errors.exit_with_error( - logger, - message="Filter clause '{}' must be in name=value form".format(value), - ) + if strict: + Errors.exit_with_error( + logger, + message="Filter clause '{}' must be in name=value form".format(value), + ) + # Non-strict: called from apply_encoded_filter_value on a URL-embedded + # fragment. Match tabcmd Classic's silent-skip behavior so drop-in + # migration of scripts that contain literal '&' in filter values + # (which the parser splits on) doesn't hard-fail. + logger.warning("Skipping unparseable filter clause from URL: %r", value) + return name, filter_value = parts # we should export the _DataExportOptions class from tsc request_options.vf(name, filter_value) # type: ignore diff --git a/tests/commands/test_datasources_and_workbooks_command.py b/tests/commands/test_datasources_and_workbooks_command.py index d41ef428..59fddf32 100644 --- a/tests/commands/test_datasources_and_workbooks_command.py +++ b/tests/commands/test_datasources_and_workbooks_command.py @@ -59,6 +59,34 @@ def test_apply_filter_value_with_trailing_equals(self): DatasourcesAndWorkbooks.apply_filter_value(mock_logger, request_options, "Name=") assert request_options.view_filters == [("Name", "")] + def test_apply_filter_value_unparseable_strict_exits(self): + # Default strict=True: a clause missing '=' exits with an error, matching + # the --filter flag contract. + request_options = tsc.PDFRequestOptions() + with self.assertRaises(SystemExit): + DatasourcesAndWorkbooks.apply_filter_value(mock_logger, request_options, "no_equals_here") + + def test_apply_filter_value_unparseable_non_strict_skips(self): + # strict=False: unparseable clause is logged and skipped (tabcmd Classic + # parity for URL-embedded filter fragments like `Field=x&y` where '&' is + # part of the value and got split into a bogus second fragment). + request_options = tsc.PDFRequestOptions() + DatasourcesAndWorkbooks.apply_filter_value( + mock_logger, request_options, "no_equals_here", strict=False + ) + assert request_options.view_filters == [] + + def test_apply_values_from_url_params_tolerates_ampersand_in_value(self): + # Regression: `?Product Name=AT&T 841000 Phone` (Classic drop-in) previously + # errored on the "T 841000 Phone" fragment produced by the '&' split. + # Classic silently skipped such fragments; we now match that behavior on + # the URL-syntax path (the --filter flag path stays strict). + query = "?Product%20Name=AT&T%20841000%20Phone" + request_options = tsc.PDFRequestOptions() + DatasourcesAndWorkbooks.apply_values_from_url_params(mock_logger, request_options, query) + # The first fragment is applied; the bogus second fragment is dropped. + assert request_options.view_filters == [("Product Name", "AT")] + def test_apply_options_from_url_params(self): query_params = "?:iid=5&:refresh=yes&:size=600,700" request_options = tsc.PDFRequestOptions() From d3f61636ce1cc09c8992faca4da1973500bf50bc Mon Sep 17 00:00:00 2001 From: Jac Fitzgerald Date: Fri, 7 Aug 2026 00:00:16 -0700 Subject: [PATCH 4/5] style: apply black to satisfy CI Black 22 (pinned in pyproject.toml) reformats these two files; the CI 'Check formatting with black' step was failing. --- .../datasources_and_workbooks_command.py | 4 +--- tests/commands/test_datasources_and_workbooks_command.py | 4 +--- 2 files changed, 2 insertions(+), 6 deletions(-) diff --git a/tabcmd/commands/datasources_and_workbooks/datasources_and_workbooks_command.py b/tabcmd/commands/datasources_and_workbooks/datasources_and_workbooks_command.py index 957eac03..7f79f539 100644 --- a/tabcmd/commands/datasources_and_workbooks/datasources_and_workbooks_command.py +++ b/tabcmd/commands/datasources_and_workbooks/datasources_and_workbooks_command.py @@ -105,9 +105,7 @@ def apply_encoded_filter_value(logger, request_options: RequestOptionsType, valu # from apply_options, which expects an un-encoded input, # or from apply_url_params via apply_encoded_filter_value which decodes the input @staticmethod - def apply_filter_value( - logger, request_options: RequestOptionsType, value: str, strict: bool = True - ) -> None: + def apply_filter_value(logger, request_options: RequestOptionsType, value: str, strict: bool = True) -> None: logger.debug("handling filter param {}".format(value)) # Split on the first '=' only so that filter values containing '=' are # preserved intact (e.g. Notes=x=y should filter Notes to the value "x=y"). diff --git a/tests/commands/test_datasources_and_workbooks_command.py b/tests/commands/test_datasources_and_workbooks_command.py index 59fddf32..02ec9f59 100644 --- a/tests/commands/test_datasources_and_workbooks_command.py +++ b/tests/commands/test_datasources_and_workbooks_command.py @@ -71,9 +71,7 @@ def test_apply_filter_value_unparseable_non_strict_skips(self): # parity for URL-embedded filter fragments like `Field=x&y` where '&' is # part of the value and got split into a bogus second fragment). request_options = tsc.PDFRequestOptions() - DatasourcesAndWorkbooks.apply_filter_value( - mock_logger, request_options, "no_equals_here", strict=False - ) + DatasourcesAndWorkbooks.apply_filter_value(mock_logger, request_options, "no_equals_here", strict=False) assert request_options.view_filters == [] def test_apply_values_from_url_params_tolerates_ampersand_in_value(self): From cd6b0348e403a5bc73828b9c39daa7163dcbd6eb Mon Sep 17 00:00:00 2001 From: Jac Fitzgerald Date: Sat, 8 Aug 2026 01:21:53 -0700 Subject: [PATCH 5/5] Rejoin unencoded '&' inside URL-embedded filter values The prior fix for `?Product Name=AT&T 841000 Phone` matched Classic by skipping the "T 841000 Phone" fragment, but it also applied the truncated first fragment (`Product Name=AT`) as a real filter. On any dataset with a matching "AT" row that produces silent wrong data with only a WARNING log the user might miss. Detect the pattern: if a fragment after '&' has no '=' and isn't a tabcmd options key (":..."), assume it's a continuation of the previous value and rejoin. Warns loudly that the URL should encode '&' as '%26' so the ambiguity is visible. Tests cover the AT&T case, multiple '&' in a value, options mixed with filters, and multiple legitimate filters. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../datasources_and_workbooks_command.py | 21 ++++++++++- .../test_datasources_and_workbooks_command.py | 35 ++++++++++++++++--- 2 files changed, 50 insertions(+), 6 deletions(-) diff --git a/tabcmd/commands/datasources_and_workbooks/datasources_and_workbooks_command.py b/tabcmd/commands/datasources_and_workbooks/datasources_and_workbooks_command.py index 7f79f539..c768636b 100644 --- a/tabcmd/commands/datasources_and_workbooks/datasources_and_workbooks_command.py +++ b/tabcmd/commands/datasources_and_workbooks/datasources_and_workbooks_command.py @@ -74,7 +74,26 @@ def apply_values_from_url_params(logger, request_options: RequestOptionsType, ur logger.debug("No query parameters present in url") return - params = query.split("&") + # A filter value that contains a literal '&' (e.g. `Product=AT&T`) + # gets split by query.split("&") into ["Product=AT", "T"]. Applying + # the first fragment as a filter silently returns rows that match + # "AT", which is wrong data with no error signal. Detect the pattern: + # if a fragment has no '=' AND isn't an options key (":..."), assume + # it's a continuation of the previous fragment's value and rejoin. + raw_params = query.split("&") + params: list[str] = [] + for fragment in raw_params: + if params and not fragment.startswith(":") and "=" not in fragment: + logger.warning( + "URL contains an unencoded '&' inside a filter value; " + "rejoining '%s&%s' as a single filter. Please URL-encode " + "'&' as '%%26' to avoid ambiguity.", + params[-1], + fragment, + ) + params[-1] = params[-1] + "&" + fragment + else: + params.append(fragment) logger.debug(params) for value in params: if value.startswith(":"): diff --git a/tests/commands/test_datasources_and_workbooks_command.py b/tests/commands/test_datasources_and_workbooks_command.py index 02ec9f59..3142fabd 100644 --- a/tests/commands/test_datasources_and_workbooks_command.py +++ b/tests/commands/test_datasources_and_workbooks_command.py @@ -76,14 +76,39 @@ def test_apply_filter_value_unparseable_non_strict_skips(self): def test_apply_values_from_url_params_tolerates_ampersand_in_value(self): # Regression: `?Product Name=AT&T 841000 Phone` (Classic drop-in) previously - # errored on the "T 841000 Phone" fragment produced by the '&' split. - # Classic silently skipped such fragments; we now match that behavior on - # the URL-syntax path (the --filter flag path stays strict). + # errored on the "T 841000 Phone" fragment produced by the '&' split, and + # a prior iteration applied "Product Name=AT" as a partial filter (silent + # wrong-data bug when a dataset had matching "AT" rows). The correct fix + # is to rejoin fragments so the user's intended value is preserved. query = "?Product%20Name=AT&T%20841000%20Phone" request_options = tsc.PDFRequestOptions() DatasourcesAndWorkbooks.apply_values_from_url_params(mock_logger, request_options, query) - # The first fragment is applied; the bogus second fragment is dropped. - assert request_options.view_filters == [("Product Name", "AT")] + assert request_options.view_filters == [("Product Name", "AT&T 841000 Phone")] + + def test_apply_values_from_url_params_rejoins_multiple_ampersands(self): + # Multiple '&'s inside a single value should all be rejoined. + query = "?Company=A%20&%20B%20&%20C" + request_options = tsc.PDFRequestOptions() + DatasourcesAndWorkbooks.apply_values_from_url_params(mock_logger, request_options, query) + assert request_options.view_filters == [("Company", "A & B & C")] + + def test_apply_values_from_url_params_options_after_ampersand_not_rejoined(self): + # An options key (starts with ':') after '&' should NOT be rejoined -- + # that's a legitimate multi-parameter URL. + query = "?Region=West&:refresh=yes" + request_options = tsc.PDFRequestOptions() + DatasourcesAndWorkbooks.apply_values_from_url_params(mock_logger, request_options, query) + assert request_options.view_filters == [("Region", "West")] + assert request_options.max_age == 0 + + def test_apply_values_from_url_params_multiple_filters_not_rejoined(self): + # Two legitimate filters separated by '&' should stay separate (each + # fragment has its own '=', so the rejoin heuristic doesn't fire). + query = "?Region=West&Product=Widget" + request_options = tsc.PDFRequestOptions() + DatasourcesAndWorkbooks.apply_values_from_url_params(mock_logger, request_options, query) + assert ("Region", "West") in request_options.view_filters + assert ("Product", "Widget") in request_options.view_filters def test_apply_options_from_url_params(self): query_params = "?:iid=5&:refresh=yes&:size=600,700"