fix: values escaping bugs #727
Conversation
ywwg
force-pushed
the
owilliams/underscores
branch
from
bf1fba0
to
7d51d16
Compare
ywwg
force-pushed
the
owilliams/underscores
branch
from
7d51d16
to
0cf7f55
Compare
ywwg
changed the title
ywwg
force-pushed
the
owilliams/underscores
branch
from
0cf7f55
to
14d8b64
Compare
model/metric.go
Outdated
| @@ -391,7 +382,7 @@ func UnescapeName(name string, scheme EscapingScheme) string { | |||
| var utf8Val uint | |||
| for j := 0; i < len(escapedName); j++ { | |||
| // This is too many characters for a utf8 value. | |||
| if j > 4 { | |||
| if j > 8 { | |||
There was a problem hiding this comment.
Where does this come from?
There was a problem hiding this comment.
the max UTF codepoint is '\U0010FFFF', therefore the max number of characters is actually 6, not 8.
There was a problem hiding this comment.
Sorry I dived a bit and found a few constants, have a look at this https://go.dev/play/p/PoHxOOeoyQu
The maximum number of bytes needed to represent unicode.MaxRune is: 4
There is also utf8.UTFMAX: : 4
Shouldnt this remain as 4?
There was a problem hiding this comment.
Wait, it should be 7, right? The 4 bytes in the longest possible rune encode as 8 hexadecimal digits, so j can legally be between 0 and 7.
There was a problem hiding this comment.
well, maxrune is \U0010FFFF so maybe 6 is too many characters?
There was a problem hiding this comment.
Sorry for slow thinking. I think I got it now:
utf8.UTFMAX is about the bytes needed in UTF-8 encoded form. But what we put into the U__ encoding is the code point, which is different from the encoded byte sequence. The codepoint \U0010FFFF that @ywwg mentioned would be written as U___10FFFF_. As UTF-8 string, it would take the byte pattern F4 8F BF BF (the 4 bytes max length that utf8.UTFMAX refers to).
But this means the correct number here is 5, don't you think so?
There was a problem hiding this comment.
aha yes -- or as you suggest, >=6
| @@ -318,21 +315,15 @@ func EscapeName(name string, scheme EscapingScheme) string { | |||
| } | |||
| escaped.WriteString("U__") | |||
| for i, b := range name { | |||
| if isValidLegacyRune(b, i) { | |||
| if b == '_' { | |||
| escaped.WriteString("__") | |||
There was a problem hiding this comment.
Something rings a bell that this was decided when there is an underscore put two underscores but could we add a reference to where this was agreed on?
There was a problem hiding this comment.
This is necessary so that we can know to start parsing a unicode value if we see only 1 underscore. this is similar to dots escaping where dots become _dot_ and we double underscores.
ywwg
force-pushed
the
owilliams/underscores
branch
from
9acceae
to
95defda
Compare
ywwg
force-pushed
the
owilliams/underscores
branch
from
b35c146
to
0288113
Compare
Issues with underscores and large unicode value conversion Signed-off-by: Owen Williams <[email protected]>
ywwg
force-pushed
the
owilliams/underscores
branch
from
0288113
to
febd997
Compare
I missed some test cases the first time around. Confirmed that without this fix the test cases fail the round trip.