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

obfuscate: add exception when parsing empty-string identifiers #514

Merged
merged 4 commits into from
Nov 5, 2018

Conversation

gbbr
Copy link
Contributor

@gbbr gbbr commented Nov 5, 2018

Fixes #316

@gbbr gbbr requested review from ufoot and palazzem November 5, 2018 13:25
Copy link

@ufoot ufoot left a comment

Choose a reason for hiding this comment

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

Sounds good to me, fixes the problem while keeping all other quantizations "just as before".

},
{
`SELECT * FROM users WHERE firstname=""`,
`SELECT * FROM users WHERE firstname = ""`,
Copy link

Choose a reason for hiding this comment

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

Sounds to me the problem is solved here, indeed.

// This string is an empty or white-space only identifier.
// We should keep the start and end delimiters in order to
// avoid creating invalid queries.
// See: https://github.com/DataDog/datadog-trace-agent/issues/316
Copy link

Choose a reason for hiding this comment

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

👍

@gbbr
Copy link
Contributor Author

gbbr commented Nov 5, 2018

FYI Minor impact on time:

name                      old time/op    new time/op    delta
Tokenizer/Escaping/512-4    8.35µs ± 0%    8.47µs ± 0%  +1.53%
Tokenizer/Grouping/199-4    4.78µs ± 0%    4.92µs ± 0%  +2.82%

name                      old alloc/op   new alloc/op   delta
Tokenizer/Escaping/512-4    4.40kB ± 0%    4.40kB ± 0%   0.00%
Tokenizer/Grouping/199-4    2.72kB ± 0%    2.72kB ± 0%   0.00%

name                      old allocs/op  new allocs/op  delta
Tokenizer/Escaping/512-4      87.0 ± 0%      87.0 ± 0%   0.00%
Tokenizer/Grouping/199-4      68.0 ± 0%      68.0 ± 0%   0.00%

@gbbr
Copy link
Contributor Author

gbbr commented Nov 5, 2018

Added another commit improving the white-space verification:

name                      old time/op    new time/op    delta
Tokenizer/Escaping/512-4    8.36µs ± 0%    8.37µs ± 0%  +0.13%
Tokenizer/Grouping/199-4    4.79µs ± 0%    4.71µs ± 0%  -1.69%

name                      old alloc/op   new alloc/op   delta
Tokenizer/Escaping/512-4    4.40kB ± 0%    4.40kB ± 0%   0.00%
Tokenizer/Grouping/199-4    2.72kB ± 0%    2.72kB ± 0%   0.00%

name                      old allocs/op  new allocs/op  delta
Tokenizer/Escaping/512-4      87.0 ± 0%      87.0 ± 0%   0.00%
Tokenizer/Grouping/199-4      68.0 ± 0%      68.0 ± 0%   0.00%

@gbbr gbbr merged commit 5e69c6a into master Nov 5, 2018
@gbbr gbbr deleted the christian/issue316-unit-test branch November 5, 2018 14:07
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