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..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(":"): @@ -95,17 +114,36 @@ 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)) - 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: + 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(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/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 diff --git a/tests/commands/test_datasources_and_workbooks_command.py b/tests/commands/test_datasources_and_workbooks_command.py index 8fa60dde..3142fabd 100644 --- a/tests/commands/test_datasources_and_workbooks_command.py +++ b/tests/commands/test_datasources_and_workbooks_command.py @@ -40,6 +40,76 @@ 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_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, 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) + 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" request_options = tsc.PDFRequestOptions()