Skip to content

WebUI: Fix quoted filter strings - #658

Merged
MichaelMure merged 4 commits into
git-bug:masterfrom
GlancingMind:fix-webui-quoted-filter-parameters
May 24, 2021
Merged

MichaelMure merged 4 commits into
git-bug:masterfrom
GlancingMind:fix-webui-quoted-filter-parameters

Conversation

@GlancingMind

@GlancingMind GlancingMind commented Apr 29, 2021 •

Copy link
Copy Markdown
Contributor

Fix #655 (comment)
The parse function dropped user given quotes. This resulted into a wrong formated query to the backend, which further lead to an error. This error then prevented the WebUI from displaying the proper bug count.

  • Fix query
  • Update unit tests
  • Make sure, that labels containing a ':' are quoted. (Must convert user quotes to one specific quoting style, which then can also be used by label value. Otherwise label must match 'value' as well as "value".)

@GlancingMind
GlancingMind force-pushed the fix-webui-quoted-filter-parameters branch from 6451734 to 1cbf0cd Compare May 1, 2021 13:17
@GlancingMind
GlancingMind marked this pull request as ready for review May 1, 2021 13:23
@GlancingMind

GlancingMind commented May 1, 2021 •

Copy link
Copy Markdown
Contributor Author

Okey the counter on the open/close filters for quoted (multi : strings) is fixed.
For the time being, this should work and is ready for merge. But for the future, the WebUI must find a better solution to the current query handling.

@MichaelMure MichaelMure left a comment

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.

Looks good but could you add the two extra tests?

But for the future, the WebUI must find a better solution to the current query handling.

Agree on that. The lexer/parser in go are 100 (simple) lines each. Re-implementing that in js would 1) ensure feature parity and 2) allow for simpler query manipulation (dedup, reordering, simplification, quote transformation ...).

Comment thread webui/src/__tests__/query.ts Outdated
Comment thread webui/src/__tests__/query.ts
@GlancingMind

Copy link
Copy Markdown
Contributor Author

@MichaelMure I have nothing to add. If you are fine with this changes, this can be merged. :-)

});

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

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.

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
Copy Markdown
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
Copy Markdown
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. :-)

The parse function dropped user given quotes. This resulted into a wrong query
to the backend, which lead to an error. This error prevented the webui from
displaying the proper bug count.
Key:Value parsing test for quoated colon. E.g. label:"foo:bar"
Key:Value:Value parsing test. E.g. metadata:key:"https://www.example.com/"
@GlancingMind
GlancingMind force-pushed the fix-webui-quoted-filter-parameters branch from 2b01d64 to 7446a20 Compare May 20, 2021 15:13
@GlancingMind

Copy link
Copy Markdown
Contributor Author

Rebased, to get rid of the fix-eslint commits and a fix-typo commit.

@MichaelMure
MichaelMure merged commit 9ded45f into git-bug:master May 24, 2021
@MichaelMure

Copy link
Copy Markdown
Contributor

good to go!

@GlancingMind
GlancingMind deleted the fix-webui-quoted-filter-parameters branch May 25, 2021 12:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Label naming

2 participants