Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,89 @@ describe('AdvancedQueryFilter', () => {
await expect.element(getByTitle('Validating')).toBeVisible();
});

it('allowedFields - shows error when query references a disallowed metadata field', async ({ routeMockers }) => {
const onInput = vi.fn();

routeMockers.lapis.mockPostQueryParse(
{ queries: ['host:Human'], doFullValidation: true },
{ data: [{ type: 'success', filter: { type: 'StringEquals', column: 'host', value: 'Human' } }] },
);

const { getByRole, getByLabelText, getByText } = render(
<AdvancedQueryFilterWithProvider
onInput={onInput}
enabled
lapisUrl={DUMMY_LAPIS_URL}
allowedFields={['nextcladePangoLineage']}
/>,
);

await userEvent.type(getByRole('textbox'), 'host:Human');

await expect.element(getByLabelText('Error')).toBeVisible();
await expect.element(getByText(/"host"/)).toBeVisible();
await expect.element(getByText(/nextcladePangoLineage/)).toBeVisible();
expect(onInput).not.toHaveBeenCalled();
});

it('allowedFields - does not show error when all referenced fields are in the allowed list', async ({
routeMockers,
}) => {
const onInput = vi.fn();

routeMockers.lapis.mockPostQueryParse(
{ queries: ['BA.1*'], doFullValidation: true },
{
data: [
{
type: 'success',
filter: {
type: 'Lineage',
column: 'nextcladePangoLineage',
value: 'BA.1',
includeSublineages: true,
},
},
],
},
);

const { getByRole, getByTitle } = render(
<AdvancedQueryFilterWithProvider
onInput={onInput}
enabled
lapisUrl={DUMMY_LAPIS_URL}
allowedFields={['nextcladePangoLineage']}
/>,
);

await userEvent.type(getByRole('textbox'), 'BA.1*');

await expect.element(getByTitle('Advanced query is valid')).toBeVisible();
await expect.poll(() => onInput).toHaveBeenCalledWith('BA.1*');
});

it('allowedFields - mutation-only query passes even with a restrictive allowedFields list', async ({
routeMockers,
}) => {
routeMockers.lapis.mockPostQueryParse(
{ queries: ['A123T'], doFullValidation: true },
{ data: [{ type: 'success', filter: { type: 'NucleotideEquals', position: 123, symbol: 'A' } }] },
);

const { getByRole, getByTitle } = render(
<AdvancedQueryFilterWithProvider
enabled
lapisUrl={DUMMY_LAPIS_URL}
allowedFields={['nextcladePangoLineage']}
/>,
);

await userEvent.type(getByRole('textbox'), 'A123T');

await expect.element(getByTitle('Advanced query is valid')).toBeVisible();
});

it('shows error icon with network error tooltip when LAPIS is unreachable', async ({ routeMockers }) => {
routeMockers.lapis.mockLapisDown();

Expand Down
22 changes: 21 additions & 1 deletion website/src/components/genspectrum/AdvancedQueryFilter.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { type FC, type InputEvent, useEffect, useRef, useState } from 'react';

import { getClientLogger } from '../../clientLogger.ts';
import { parseQuery } from '../../lapis/parseQuery.ts';
import { extractMetadataFields } from '../../lapis/siloFilterExpression.ts';

const logger = getClientLogger('AdvancedQueryFilter');

Expand All @@ -19,9 +20,16 @@ type AdvancedQueryFilterProps = {
onInput?: (newValue: string | undefined) => void;
enabled: boolean;
lapisUrl: string;
allowedFields?: string[];
};

export const AdvancedQueryFilter: FC<AdvancedQueryFilterProps> = ({ value, onInput, enabled, lapisUrl }) => {
export const AdvancedQueryFilter: FC<AdvancedQueryFilterProps> = ({
value,
onInput,
enabled,
lapisUrl,
allowedFields,
}) => {
const [inputValue, setInputValue] = useState(value);
const [validationState, setValidationState] = useState<ValidationState>({ type: 'idle' });
const userEditedRef = useRef(false);
Expand All @@ -31,6 +39,18 @@ export const AdvancedQueryFilter: FC<AdvancedQueryFilterProps> = ({ value, onInp
onSuccess: (results, query) => {
const result = results[0];
if (result.type === 'success') {
if (allowedFields !== undefined) {
const usedFields = extractMetadataFields(result.filter);
const disallowed = usedFields.filter((col) => !allowedFields.includes(col));
if (disallowed.length > 0) {
const listed = disallowed.map((col) => `"${col}"`).join(', ');
setValidationState({
type: 'error',
message: `Field ${listed} is not allowed. Allowed fields: ${allowedFields.join(', ')}.`,
});
Comment on lines +42 to +50
return;
}
Comment on lines +42 to +52
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AI: ❓ allowedFields is captured in useMutation's onSuccess closure. useMutation doesn't re-create callbacks when props change — if allowedFields ever becomes dynamic (e.g. derived from state), stale closure would use the old list. Fine for now since it comes from static config, but worth noting. Consider using a ref or adding allowedFields to the mutation key if this ever becomes dynamic.

}
setValidationState({ type: 'valid' });
onInput?.(query);
} else {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,14 +34,18 @@ export type NumberRangeFilterConfig = {
sliderStep?: number;
};

export type AdvancedQueryFilterConfig = {
allowedFields?: string[];
};

export type BaselineFilterConfig =
| ({
type: 'date';
} & DateRangeFilterConfig)
| ({ type: 'text' } & TextInputConfig)
| ({ type: 'location' } & LocationFilterConfig)
| ({ type: 'number' } & NumberRangeFilterConfig)
| { type: 'advancedQuery' };
| ({ type: 'advancedQuery' } & AdvancedQueryFilterConfig);

export function BaselineSelector({
baselineFilterConfigs,
Expand Down Expand Up @@ -177,6 +181,7 @@ export function BaselineSelector({
value={datasetFilter.advancedQuery ?? ''}
enabled={enableAdvancedQueryFilter}
lapisUrl={lapisUrl}
allowedFields={config.allowedFields}
/>
);
}
Expand Down
40 changes: 39 additions & 1 deletion website/src/lapis/siloFilterExpression.spec.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
import { describe, expect, test } from 'vitest';

import { siloFilterExpressionSchema } from './siloFilterExpression.ts';
import {
type SiloFilterExpression,
extractMetadataFields,
siloFilterExpressionSchema,
} from './siloFilterExpression.ts';

describe('siloFilterExpressionSchema', () => {
test('should parse StringEquals', () => {
Expand Down Expand Up @@ -229,7 +233,41 @@ describe('siloFilterExpressionSchema', () => {

expect(result.success).toBe(false);
});
});

describe('extractMetadataFields', () => {
test('extracts column names from nested And/Or expressions', () => {
const expr: SiloFilterExpression = {
type: 'And',
children: [
{ type: 'Lineage', column: 'nextcladePangoLineage', value: 'BA.1', includeSublineages: true },
{
type: 'Or',
children: [
{ type: 'StringEquals', column: 'country', value: 'Germany' },
{ type: 'DateBetween', column: 'date', from: '2024-01-01', to: null },
],
},
],
};

expect(extractMetadataFields(expr)).toEqual(['nextcladePangoLineage', 'country', 'date']);
});

test('returns no columns for mutation-only expressions', () => {
const expr: Parameters<typeof extractMetadataFields>[0] = {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AI: 🔵 nit: This uses Parameters<typeof extractMetadataFields>[0] but the test above uses SiloFilterExpression directly (already imported). Inconsistent — prefer SiloFilterExpression for clarity.

type: 'And',
children: [
{ type: 'NucleotideEquals', position: 123, symbol: 'A' },
{ type: 'HasAminoAcidMutation', sequenceName: 'S', position: 501 },
],
};

expect(extractMetadataFields(expr)).toEqual([]);
});
});

describe('siloFilterExpressionSchema (parse)', () => {
test('should accept nullable values', () => {
const data = {
type: 'StringEquals',
Expand Down
29 changes: 29 additions & 0 deletions website/src/lapis/siloFilterExpression.ts
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,35 @@ const nOfSchema: z.ZodType<{
}),
);

/**
* Given an expression, returns a list of all the metadata fields that are referenced
* in the expression.
*/
export function extractMetadataFields(expr: SiloFilterExpression): string[] {
switch (expr.type) {
case 'StringEquals':
case 'BooleanEquals':
case 'Lineage':
case 'DateBetween':
case 'IntEquals':
case 'IntBetween':
case 'FloatEquals':
case 'FloatBetween':
case 'StringSearch':
case 'PhyloDescendantOf':
return [expr.column];
case 'And':
case 'Or':
case 'N-Of':
return expr.children.flatMap(extractMetadataFields);
case 'Not':
case 'Maybe':
return extractMetadataFields(expr.child);
default:
return [];
}
}

// Combined union for all SiloFilterExpression types.
// This schema was initially LLM generated from the LAPIS code.
export const siloFilterExpressionSchema = z.union([
Expand Down