Improve handling of semicolons in --command

As copilot review pointed out and was verified by reading the bcftools
manual, there are bcftools view commands that CAN take ;, often within
its own quotes. This adds support for such commands, with the caveat
that the inner quotes need to be escaped on the CLI side.

example CLI command that can be used with
tests/fixtures/vcf_specification_v45_example11.vcf.gz

divbase-cli query vcf --samples "NA00001,NA00002,NA00003" \
--command "view -i 'FILTER~\"q10\"'"

Author: brinkdp

docs/user-guides/vcf-query-syntax.md

@@ -267,6 +267,22 @@ The DivBase server will check if these are included in the `--command` string be
     #  • Pipe segment 3, token '-W': Option '-W/--write-index' is handled by the DivBase server.
     ```
 
+### 4.3. Special considerations
+
+#### 4.3.1. Semicolons inside `FILTER` expressions
+
+As described above in e.g. [Section 4](#4-writing-the-bcftools-command-argument), users can create pipes of several `bcftools` commands by delimiting them with a semicolon (`;`). However, a few `bcftools` commands can possibly contain semicolons as part of a value. One example is  `bcftools view -i` [filter expressions](https://samtools.github.io/bcftools/bcftools.html#expressions), such as `bcftools view -i FILTER="q10;s50"`. If you want to use this for DivBase queries, you will need to ensure that the inner double quotes are escaped in the `--command` string so the expression is preserved correctly through CLI/API parsing.
+
+Example with escaped inner double quotes:
+
+```bash
+divbase-cli query vcf \
+  --all-samples \
+  --command "view -i 'FILTER=\"q10;s50\"'; view -r 20:1-2000000"
+```
+
+Failing to not escape the inner double quotes might lead to the string given by `--command` being altered befored it reaches the  `bcftools` layer of DivBase, and can thus lead to incorrect results or errors in the query.
+
 ## 5. What happens after submitting a VCF query? (Job lifecycle and outputs)
 
 ### 5.1. What the user can see after submitting the job

packages/divbase-api/src/divbase_api/services/queries.py

@@ -33,6 +33,7 @@
     TaskUserError,
 )
 from divbase_lib.metadata_validator import SharedMetadataValidator, ValidationCategory
+from divbase_lib.utils import split_semicolon_bcftools_command_segments
 
 logger = logging.getLogger(__name__)
 
@@ -121,7 +122,7 @@ def _parse_command_segments(command: str) -> list[ParsedCommandSegment]:
     """
     segments: list[ParsedCommandSegment] = []
 
-    for position, raw_cmd in enumerate(command.split(";"), start=1):
+    for position, raw_cmd in enumerate(split_semicolon_bcftools_command_segments(command), start=1):
         cmd = raw_cmd.strip()
         if not cmd:
             # Defensive skip only. Empty segments are rejected by schema validation upstream.
@@ -461,7 +462,7 @@ def validate_user_submitted_bcftools_command(command: str, all_samples: bool = F
         cmd_name = segment.cmd_name
 
         if not cmd:
-            # Empty command/empty ';' segments are already rejected by Pydantic (NonEmptyCommand) upstream of this function. This skip is only defensive.
+            # Empty command/empty ';' segments are already rejected by upstream Pydantic command field validation.
             continue
 
         if cmd_name not in valid_commands:
@@ -662,7 +663,7 @@ def build_commands_config(
 
         if not command or command.strip() == ";" or command.strip() == "":
             raise BcftoolsPipeEmptyCommandError()
-        command_list = command.split(";")
+        command_list = split_semicolon_bcftools_command_segments(command)
         pipe_has_sample_placeholder = any(
             self._command_has_sample_placeholder(cmd.strip()) for cmd in command_list if cmd.strip()
         )

packages/divbase-lib/src/divbase_lib/api_schemas/queries.py

@@ -2,9 +2,11 @@
 Schemas for query routes.
 """
 
-from typing import Annotated, Optional, Self
+from typing import Optional, Self
 
-from pydantic import AfterValidator, BaseModel, ConfigDict, model_validator
+from pydantic import BaseModel, ConfigDict, field_validator, model_validator
+
+from divbase_lib.utils import split_semicolon_bcftools_command_segments
 
 # Shared helper functions and models
 
@@ -14,8 +16,7 @@ def validate_command_not_empty(command: str) -> str:
     Validator function to ensure that the user-submitted bcftools command string is not empty or just whitespace.
     If there are multiple segments separated by semicolons, it also validates that none of the segments are empty or whitespace.
 
-    This is shared by the BcftoolsQueryRequest.command field and the BcftoolsQueryKwargs.command fields, and is used instead of @field_validator()
-    to avoid replicating the code.
+    Shared helper for command field validators in BcftoolsQueryRequest.command and BcftoolsQueryKwargs.command.
     """
     command_string = command.strip()
     if command_string == "":
@@ -24,7 +25,7 @@ def validate_command_not_empty(command: str) -> str:
             'If you only want to subset based on samples, use --command "view -s" in combination with one of the sample-selection options: --tsv-filter, --samples, --samples-file.'
         )
 
-    segments = command_string.split(";")
+    segments = split_semicolon_bcftools_command_segments(command_string)
     for position, segment in enumerate(segments, start=1):
         if segment.strip() == "":
             raise ValueError(
@@ -35,9 +36,6 @@ def validate_command_not_empty(command: str) -> str:
     return command
 
 
-NonEmptyCommand = Annotated[str, AfterValidator(validate_command_not_empty)]
-
-
 class SharedBaseModel(BaseModel):
     """Shared pydantic BaseModel for VCF query response and kwarg schemas."""
 
@@ -65,10 +63,15 @@ class BcftoolsQueryRequest(SharedBaseModel):
 
     tsv_filter: str | None = None  # Used for metadata mode only
     metadata_tsv_name: str | None = None  # Used for metadata mode only
-    command: NonEmptyCommand  # TODO add field to describe that this is bcftools commands
+    command: str  # bcftools command input with --command
     samples: list[str] | None = None
     all_samples: bool = False
 
+    @field_validator("command")
+    @classmethod
+    def validate_command(_cls, value: str) -> str:
+        return validate_command_not_empty(value)
+
     @model_validator(mode="after")
     def validate_sample_selection_mode(self) -> Self:
         tsv_filter = self.tsv_filter
@@ -117,7 +120,7 @@ class BcftoolsQueryKwargs(SharedBaseModel):
 
     tsv_filter: str | None = None  # Used for metadata mode only
     metadata_tsv_name: str | None = None  # Used for metadata mode only
-    command: NonEmptyCommand
+    command: str  # bcftools command input with --command
     bucket_name: str
     project_id: int
     project_name: str
@@ -126,6 +129,11 @@ class BcftoolsQueryKwargs(SharedBaseModel):
     samples: list[str] | None = None
     all_samples: bool = False
 
+    @field_validator("command")
+    @classmethod
+    def validate_command(_cls, value: str) -> str:
+        return validate_command_not_empty(value)
+
     @model_validator(mode="after")
     def validate_sample_selection_mode(self) -> Self:
         tsv_filter = self.tsv_filter

packages/divbase-lib/src/divbase_lib/utils.py

@@ -1,3 +1,8 @@
+from typing import Literal, TypeAlias
+
+QuoteChar: TypeAlias = Literal["'", '"']
+
+
 def format_file_size(size_bytes: int | float | None, decimals: int = 2) -> str:
     """
     Converts a file size in bytes to a human-readable format.
@@ -15,3 +20,57 @@ def format_file_size(size_bytes: int | float | None, decimals: int = 2) -> str:
         size_bytes /= power
         n += 1
     return f"{size_bytes:.{decimals}f} {power_labels[n]}B"
+
+
+def split_semicolon_bcftools_command_segments(command: str) -> list[str]:
+    """
+    Split a user provided bcftools command string on semicolons while respecting quoted substrings.
+
+    bcftools view allows semicolons for certain options (e.g. -i 'FILTER="A;B"'), so we need to ensure that we only
+    split on semicolons that are not inside quotes.
+
+    --command "view -i 'FILTER=\"A;B\"'; view -r 1:1-1000"
+
+    Slides character by character through the command string to identify the true semicolon delimiters, whilst ignoring semicolons inside quotes.
+    The command strings are not that long, so we can afford to do this on a character level.
+    """
+    command_segments: list[str] = []
+    segment_chars: list[str] = []
+
+    single_quote = "'"
+    double_quote = '"'
+
+    # Possible states None = outside quotes, "'" = inside single-quoted substring, '"' = inside double-quoted substring.
+    open_quote_char: QuoteChar | None = None
+    is_escaped = False
+
+    for char in command:
+        if is_escaped:
+            segment_chars.append(char)
+            is_escaped = False
+            continue
+
+        # Handle backslash-escaped characters in the received command text.
+        # In single-quoted regions, backslashes are treated as literal chars.
+        if char == "\\" and open_quote_char != single_quote:
+            segment_chars.append(char)
+            is_escaped = True
+            continue
+
+        if char in (double_quote, single_quote):
+            segment_chars.append(char)
+            if open_quote_char is None:
+                open_quote_char = char
+            elif open_quote_char == char:
+                open_quote_char = None
+            continue
+
+        if char == ";" and open_quote_char is None:
+            command_segments.append("".join(segment_chars))
+            segment_chars = []
+            continue
+
+        segment_chars.append(char)
+
+    command_segments.append("".join(segment_chars))
+    return command_segments

tests/e2e_integration/cli_commands/conftest.py

@@ -10,6 +10,7 @@
 import logging
 from pathlib import Path
 
+import boto3
 import pytest
 from typer.testing import CliRunner
 
@@ -164,3 +165,28 @@ def factory(CONSTANTS):
 def fixtures_dir():
     """Path to the fixtures directory."""
     return Path(__file__).parent.parent.parent / "fixtures"
+
+
+@pytest.fixture
+def cleaned_project_bucket(CONSTANTS):
+    """
+    Ensure cleaned-project bucket is empty before and after test.
+
+    Use this for tests that require deterministic project bucket contents.
+    """
+    s3_resource = boto3.resource(
+        "s3",
+        endpoint_url=CONSTANTS["MINIO_URL"],
+        aws_access_key_id=CONSTANTS["BAD_ACCESS_KEY"],
+        aws_secret_access_key=CONSTANTS["BAD_SECRET_KEY"],
+    )
+
+    project_name = CONSTANTS["CLEANED_PROJECT"]
+    bucket_name = CONSTANTS["PROJECT_TO_BUCKET_MAP"][project_name]
+    # pylance does not understand boto3 resource return types.
+    bucket = s3_resource.Bucket(bucket_name)  # type: ignore
+    bucket.object_versions.delete()
+
+    yield project_name, bucket_name
+
+    bucket.object_versions.delete()

tests/e2e_integration/cli_commands/test_file_cli.py

@@ -12,7 +12,6 @@
 from io import StringIO
 from pathlib import Path
 
-import boto3
 import pytest
 from typer.testing import CliRunner
 
@@ -38,7 +37,7 @@
 
 
 @pytest.fixture(autouse=True)
-def start_with_clean_project(CONSTANTS):
+def start_with_clean_project(cleaned_project_bucket):
     """
     For tests that require a project with a clean bucket, this fixture will
     ensure that the CONSTANTS["CLEANED_PROJECT"]'s bucket is empty before and after running the test.
@@ -47,23 +46,8 @@ def start_with_clean_project(CONSTANTS):
     If you modify the approach make sure your implementation does not just add delete markers.
     The files need to be actually deleted.
     """
-    s3_resource = boto3.resource(
-        "s3",
-        endpoint_url=CONSTANTS["MINIO_URL"],
-        aws_access_key_id=CONSTANTS["BAD_ACCESS_KEY"],
-        aws_secret_access_key=CONSTANTS["BAD_SECRET_KEY"],
-    )
-    # pylance does not understand boto3 resource returns types, hence ignore below
-    cleaned_project_bucket_name = CONSTANTS["PROJECT_TO_BUCKET_MAP"][CONSTANTS["CLEANED_PROJECT"]]
-
-    bucket = s3_resource.Bucket(cleaned_project_bucket_name)  # type: ignore
-    bucket.object_versions.delete()
-
     yield
 
-    bucket = s3_resource.Bucket(cleaned_project_bucket_name)  # type: ignore
-    bucket.object_versions.delete()
-
 
 @pytest.fixture(scope="module")
 def large_file_for_multipart(tmp_path_factory):

tests/e2e_integration/cli_commands/test_query_cli.py

@@ -7,6 +7,7 @@
 """
 
 import datetime
+import gzip
 import logging
 import os
 import subprocess
@@ -197,6 +198,86 @@ def test_bcftools_pipe_query(
     )
 
 
+@pytest.mark.parametrize(
+    "arg_command,expected_records,expected_view_command_filter_fragment",
+    [
+        (
+            r"view -i 'FILTER~\"q10\"'",
+            [("17330", ".", "q10"), ("1234567", "microsat1", "q10;s50")],
+            'FILTER~"q10"',
+        ),
+        (
+            r"view -i 'FILTER=\"q10\"'",
+            [("17330", ".", "q10")],
+            'FILTER="q10"',
+        ),
+        (
+            r"view -i 'FILTER=\"q10;s50\"'",
+            [("1234567", "microsat1", "q10;s50")],
+            'FILTER="q10;s50"',
+        ),
+    ],
+    ids=[
+        "filter-subset-match-q10",
+        "filter-exact-match-q10",
+        "filter-exact-match-q10-s50",
+    ],
+)
+def test_bcftools_pipe_query_supports_semicolon_in_filter_expression_e2e(
+    CONSTANTS,
+    logged_in_edit_user_with_existing_config,
+    run_update_dimensions,
+    project_map,
+    fixtures_dir,
+    cleaned_project_bucket,
+    arg_command,
+    expected_records,
+    expected_view_command_filter_fragment,
+):
+    """
+    Test that VCF queries can support bcftools view -i FILTER expressions (including semicolon values)
+    """
+    project_name, bucket_name = cleaned_project_bucket
+    project_id = project_map[project_name]
+    user_id = 1
+    fixture_name = "vcf_specification_v45_example11.vcf.gz"
+    fixture_path = (fixtures_dir / fixture_name).resolve()
+    upload_result = runner.invoke(app, f"files upload {fixture_path} --project {project_name}")
+    assert upload_result.exit_code == 0, f"Upload failed: {upload_result.stdout}"
+
+    run_update_dimensions(bucket_name=bucket_name, project_id=project_id, project_name=project_name, user_id=user_id)
+
+    query_result = runner.invoke(
+        app,
+        f'query vcf --samples "NA00001,NA00002,NA00003" --command "{arg_command}" --project {project_name}',
+    )
+    assert query_result.exit_code == 0, f"Query submission failed: {query_result.stdout}"
+    assert "Job submitted" in query_result.stdout
+
+    user_task_id = query_result.stdout.strip().split()[-1]
+    task_result = wait_for_task_complete(user_task_id=user_task_id)
+    assert task_result.status == "SUCCESS", f"Task failed: {task_result.result}"
+    assert hasattr(task_result.result, "output_file"), f"Missing output_file in task result: {task_result.result}"
+    output_file = task_result.result.output_file
+
+    stream_result = runner.invoke(
+        app,
+        f"files stream {output_file} --project {project_name}",
+    )
+    assert stream_result.exit_code == 0, f"Streaming query result failed: {stream_result.stdout}"
+
+    streamed_vcf_content = gzip.decompress(stream_result.stdout_bytes).decode("utf-8")
+    assert "##bcftools_viewCommand=" in streamed_vcf_content
+    assert expected_view_command_filter_fragment in streamed_vcf_content
+
+    records = [line for line in streamed_vcf_content.splitlines() if line and not line.startswith("#")]
+    parsed_records = [(cols[1], cols[2], cols[6]) for cols in (record.split("\t") for record in records)]
+
+    assert parsed_records == expected_records, (
+        f"Unexpected records for command '{arg_command}'. Expected {expected_records}, got {parsed_records}"
+    )
+
+
 def test_bcftools_pipe_query_direct_samples_mode(
     CONSTANTS,
     logged_in_edit_user_with_existing_config,

tests/fixtures/vcf_specification_v45_example11.vcf.gz

@@ -52,6 +52,7 @@ class TestValidateUserSubmittedBcftoolsCommand:
             "view -s",
             "view -s -r 21:15000000-25000000",
             "view --samples -i 'QUAL>20'",
+            "view -i 'FILTER=\"A;B\"'; view -r 1:1-1000",
         ],
     )
     def test_validate_user_submitted_bcftools_command_accepts_valid_commands(self, command):