Skip to content
This repository has been archived by the owner on Aug 23, 2023. It is now read-only.

Add unit test to test various special characters in tag values #1217

Closed
wants to merge 3 commits into from

Conversation

replay
Copy link
Contributor

@replay replay commented Feb 6, 2019

This is a regression test to ensure that we continue supporting special characters in the tag values

Related #1216

Note: Github does not display all of it correctly. For some of the values to be shown correctly it is necessary to git fetch the commit and look at it locally.

@Dieterbe
Copy link
Contributor

Dieterbe commented Feb 7, 2019

wouldn't it be simpler and more robust to iterate the ascii table (or a subrange of it) ?

@replay
Copy link
Contributor Author

replay commented Feb 8, 2019

Yeah, I guess that would work. I wanted to test some UTF-8 as well. But i can replace the ASCII chars with some loop that just iterates over the table.

@replay
Copy link
Contributor Author

replay commented Feb 8, 2019

This is now iterating over the ascii table and tests all characters except those on a short blacklist: 9d93f6d


blackList := []int{
59, // ; not allowed in values
126, // ~ can't query values starting with ~ because =~ looks like a regular expression query
Copy link
Contributor

Choose a reason for hiding this comment

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

can't you just specify the actual characters like ':'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, that works too: 42c75fa

for i := 0; i < 128; i++ {
firstChar := i
for isBlacklisted(rune(firstChar)) {
firstChar = (firstChar + 1) % 128
Copy link
Contributor

Choose a reason for hiding this comment

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

seems like % 128 is redundant given the check below

Copy link
Contributor

@Dieterbe Dieterbe Feb 11, 2019

Choose a reason for hiding this comment

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

maybe change to for isBlacklisted(rune(firstChar)) && i < 128 {

data := []testCase{}

for i := 0; i < 6; i++ {
data = append(data, testCase{ids[i], 1, []string{}})
Copy link
Contributor

Choose a reason for hiding this comment

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

what is the purpose of setting these very low lastUpdate values?
seems it has nothing to do with what is being tested, and i would emit this from the testCase types

Copy link
Contributor

Choose a reason for hiding this comment

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

also, having all these predeclared id's complicates things needlessly. just generate them as you need them

@Dieterbe
Copy link
Contributor

I'm not sure how useful this PR is.
It basically confirms that if in Go you create a string using any ascii char or any unicode characters, and you copy that string, that equality between them works.
This is something the runtime provides and not something we should be dedicating 100 lines of code for, or even test at all, I think.

It seems more useful to think about where special characters may be problematic and either test those or analyze and document them.
E.g. when:

  • parsing incoming carbon or MetricData input
  • when generating messagepack and json responses. (wether intra-cluster or to clients) will characters that have a meaning in json be roperly escaped? This could use a unit test for our custom json encoder. For msgp we use a 3rd party library so we should look at the docs and/or contribute unit tests to the msgp project.
  • when parsing user queries that come in over the http api.

@Dieterbe
Copy link
Contributor

@replay what's the status of this?

@replay
Copy link
Contributor Author

replay commented Mar 18, 2019

@Dieterbe Ok, in that case we can close this PR and I'll make another one with unit tests which test via the APIs and the MetricData input, instead of directly testing on the index interface.

@Dieterbe Dieterbe closed this May 28, 2019
@replay replay mentioned this pull request Jun 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants