Skip to content
Merged
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
121 changes: 104 additions & 17 deletions webui/src/__tests__/query.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,49 +7,136 @@ it('parses a simple query', () => {
});

it('parses a query with multiple filters', () => {
expect(parse('foo:bar baz:foo-bar')).toEqual({
expect(parse(`foo:bar baz:foo-bar`)).toEqual({
foo: ['bar'],
baz: ['foo-bar'],
});

expect(parse(`label:abc freetext`)).toEqual({
label: [`abc`],
freetext: [''],
});

expect(parse(`label:abc with "quotes" 'in' freetext`)).toEqual({
label: [`abc`],
with: [''],
'"quotes"': [''],
"'in'": [''],
freetext: [''],
});
});

it('parses a quoted query', () => {
expect(parse('foo:"bar"')).toEqual({
foo: ['bar'],
expect(parse(`foo:"bar"`)).toEqual({
foo: [`"bar"`],
});

expect(parse("foo:'bar'")).toEqual({
foo: ['bar'],
expect(parse(`foo:'bar'`)).toEqual({
foo: [`'bar'`],
});

expect(parse(`label:'multi word label'`)).toEqual({
label: [`'multi word label'`],
});

expect(parse(`label:"multi word label"`)).toEqual({
label: [`"multi word label"`],
});

expect(parse(`label:'multi word label with "nested" quotes'`)).toEqual({
label: [`'multi word label with "nested" quotes'`],
});

expect(parse(`label:"multi word label with 'nested' quotes"`)).toEqual({
label: [`"multi word label with 'nested' quotes"`],
});

expect(parse(`label:"with:quoated:colon"`)).toEqual({
label: [`"with:quoated:colon"`],
});

expect(parse(`label:'name ends after this ->' quote'`)).toEqual({
label: [`'name ends after this ->'`],
"quote'": [``],
});

expect(parse('foo:\'bar "nested" quotes\'')).toEqual({
foo: ['bar "nested" quotes'],
expect(parse(`label:"name ends after this ->" quote"`)).toEqual({
label: [`"name ends after this ->"`],
'quote"': [``],
});

expect(parse("foo:'escaped\\' quotes'")).toEqual({
foo: ["escaped' quotes"],
expect(parse(`label:'this ->"<- quote belongs to label name'`)).toEqual({
label: [`'this ->"<- quote belongs to label name'`],
});

expect(parse(`label:"this ->'<- quote belongs to label name"`)).toEqual({
label: [`"this ->'<- quote belongs to label name"`],
});

expect(parse(`label:'names end with'whitespace not with quotes`)).toEqual({
label: [`'names end with'whitespace`],
not: [``],
with: [``],
quotes: [``],
});

expect(parse(`label:"names end with"whitespace not with quotes`)).toEqual({
label: [`"names end with"whitespace`],
not: [``],
with: [``],
quotes: [``],
});
});

it('should not escape nested quotes', () => {
expect(parse(`foo:'do not escape this ->'<- quote'`)).toEqual({
Copy link
Contributor

Choose a reason for hiding this comment

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

This behavior is also different in the go parse I believe. The first corresponding quote would terminate the quoted section. It looks like the regex based parser is too greedy and try to match as much as possible?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's fine for now if the JS parser is not matching exactly the go one, just write the correct test and comment the ones that don't pass. That way it's explicit were the mismatch are and we don't have tests that mislead the reader into thinking that the code is correct.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should be fixed. Added also additional test cases to catch greediness.
Let me now if something is missing and before you merge, so that I can get rid of the eslint hotfixes. :-)

foo: [`'do not escape this ->'<-`],
"quote'": [``],
});

expect(parse(`foo:'do not escape this ->"<- quote'`)).toEqual({
foo: [`'do not escape this ->"<- quote'`],
});

expect(parse(`foo:"do not escape this ->"<- quote"`)).toEqual({
foo: [`"do not escape this ->"<-`],
'quote"': [``],
});

expect(parse(`foo:"do not escape this ->'<- quote"`)).toEqual({
foo: [`"do not escape this ->'<- quote"`],
});
});

it('parses a query with repetitions', () => {
expect(parse('foo:bar foo:baz')).toEqual({
expect(parse(`foo:bar foo:baz`)).toEqual({
foo: ['bar', 'baz'],
});
});

it('parses a complex query', () => {
expect(parse('foo:bar foo:baz baz:"foobar" idont:\'know\'')).toEqual({
expect(parse(`foo:bar foo:baz baz:"foobar" idont:'know'`)).toEqual({
foo: ['bar', 'baz'],
baz: ['foobar'],
idont: ['know'],
baz: [`"foobar"`],
idont: [`'know'`],
});
});

it('parses a key:value:value query', () => {
expect(parse(`meta:github:"https://github.com/MichaelMure/git-bug"`)).toEqual(
{
meta: [`github:"https://github.com/MichaelMure/git-bug"`],
}
);
});

it('quotes values', () => {
expect(quote('foo')).toEqual('foo');
expect(quote('foo bar')).toEqual('"foo bar"');
expect(quote('foo "bar"')).toEqual(`'foo "bar"'`);
expect(quote(`foo "bar" 'baz'`)).toEqual(`"foo \\"bar\\" 'baz'"`);
expect(quote(`foo`)).toEqual(`foo`);
expect(quote(`foo bar`)).toEqual(`"foo bar"`);
expect(quote(`foo "bar"`)).toEqual(`"foo "bar""`);
expect(quote(`foo 'bar'`)).toEqual(`"foo "bar""`);
expect(quote(`'foo'`)).toEqual(`"foo"`);
expect(quote(`foo "bar" 'baz'`)).toEqual(`"foo "bar" "baz""`);
});

it('stringifies params', () => {
Expand Down
44 changes: 21 additions & 23 deletions webui/src/pages/list/Filter.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -35,52 +35,50 @@ const ITEM_HEIGHT = 48;
export type Query = { [key: string]: string[] };

function parse(query: string): Query {
// TODO: extract the rest of the query?
const params: Query = {};

// TODO: support escaping without quotes
const re = /(\w+):([A-Za-z0-9-]+|(["'])(([^\3]|\\.)*)\3)+/g;
let re = new RegExp(/([^:\s]+)(:('[^']*'\S*|"[^"]*"\S*|\S*))?/, 'g');
let matches;
while ((matches = re.exec(query)) !== null) {
if (!params[matches[1]]) {
params[matches[1]] = [];
}

let value;
if (matches[4]) {
value = matches[4];
if (matches[3] !== undefined) {
params[matches[1]].push(matches[3]);
} else {
value = matches[2];
params[matches[1]].push('');
}
value = value.replace(/\\(.)/g, '$1');
params[matches[1]].push(value);
}
return params;
}

function quote(value: string): string {
const hasSingle = value.includes("'");
const hasDouble = value.includes('"');
const hasSpaces = value.includes(' ');
if (!hasSingle && !hasDouble && !hasSpaces) {
return value;
}
const isSingleQuotedRegEx = RegExp(/^'.*'$/);
const isDoubleQuotedRegEx = RegExp(/^".*"$/);
const isQuoted = () =>
isDoubleQuotedRegEx.test(value) || isSingleQuotedRegEx.test(value);

if (!hasDouble) {
return `"${value}"`;
//Test if label name contains whitespace between quotes. If no quoates but
//whitespace, then quote string.
if (!isQuoted() && hasSpaces) {
value = `"${value}"`;
}

if (!hasSingle) {
return `'${value}'`;
//Convert single quote (tick) to double quote. This way quoting is always
//uniform and can be relied upon by the label menu
const hasSingle = value.includes(`'`);
if (hasSingle) {
value = value.replace(/'/g, `"`);
}

value = value.replace(/"/g, '\\"');
return `"${value}"`;
return value;
}

function stringify(params: Query): string {
const parts: string[][] = Object.entries(params).map(([key, values]) => {
return values.map((value) => `${key}:${quote(value)}`);
return values.map((value) =>
value.length > 0 ? `${key}:${quote(value)}` : key
);
});
return new Array<string>().concat(...parts).join(' ');
}
Expand Down
13 changes: 11 additions & 2 deletions webui/src/pages/list/FilterToolbar.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,16 @@ function CountingFilter({ query, children, ...props }: CountingFilterProps) {
);
}

function quoteLabel(value: string) {
const hasUnquotedColon = RegExp(/^[^'"].*:.*[^'"]$/);
if (hasUnquotedColon.test(value)) {
//quote values which contain a colon but are not quoted.
//E.g. abc:abc becomes "abc:abc"
return `"${value}"`;
}
return value;
}

type Props = {
query: string;
queryLocation: (query: string) => LocationDescriptor;
Expand Down Expand Up @@ -87,7 +97,7 @@ function FilterToolbar({ query, queryLocation }: Props) {
labelsData.repository.validLabels.nodes
) {
labels = labelsData.repository.validLabels.nodes.map((node) => [
node.name,
quoteLabel(node.name),
node.name,
node.color,
]);
Expand Down Expand Up @@ -131,7 +141,6 @@ function FilterToolbar({ query, queryLocation }: Props) {
[key]: [],
});

// TODO: author/label filters
return (
<Toolbar className={classes.toolbar}>
<CountingFilter
Expand Down