Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug 5417: An empty annotation value does not match #1896

Conversation

rousskov
Copy link
Contributor

Helpers may return annotations with empty values:

OK team_=""

A note ACL may be configured to match an annotation with an empty value:

configuration_includes_quoted_values on
acl emptyTeam note team_ ""

However, that emptyTeam ACL did not match the above helper annotation:

  • AppendTokens() split an empty annotation value into an empty vector
    instead of a vector with a single empty entry. That "never match an
    empty value received from the helper" bug was probably introduced in
    2017 commit 75d4734 when it replaced an "always try to match an empty
    value, even when it was not received from the helper" bug in
    ACLNoteStrategy::matchNotes().

  • ACLStringData::match(SBuf v) never matched an empty value v. That bug
    was probably introduced in 2015 commit 76ee67a that mistook a nil
    c-string pointer for an empty c-string.

Helpers may return annotations with empty values:

    OK team_=""

A note ACL may be configured to match an annotation with an empty value:

    configuration_includes_quoted_values on
    acl emptyTeam note team_ ""

However, that emptyTeam ACL did not match the above helper annotation:

* AppendTokens() split an empty annotation value into an empty vector
  instead of a vector with a single empty entry. That "never match an
  empty value received from the helper" bug was probably introduced in
  2017 commit 75d4734 when it replaced an "always try to match an empty
  value, even when it was not received from the helper" bug in
  ACLNoteStrategy::matchNotes().

* ACLStringData::match(SBuf v) never matched an empty value v. That bug
  was probably introduced in 2015 commit 76ee67a that mistook a nil
  c-string pointer for an empty c-string.
src/Notes.cc Outdated Show resolved Hide resolved
@rousskov rousskov added the S-waiting-for-author author action is expected (and usually required) label Aug 27, 2024
eduard-bagdasaryan and others added 2 commits August 29, 2024 01:39
Adjusted AppendTokens() to include empty-tokens into the result list,
for strings such as ",", "a,", ",a", "a,,b".
Also polished comment to avoid implying that we may
push_back() multiple tokens at once.
Copy link
Contributor Author

@rousskov rousskov left a comment

Choose a reason for hiding this comment

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

Thank you, @eduard-bagdasaryan. It looks like the bug reporter may use a workaround instead of testing these changes, so our own lab testing may be the best we can do for now. I am removing the draft flag from this PR and plan to clear it for merging in a few days.

@rousskov rousskov marked this pull request as ready for review August 29, 2024 13:07
@rousskov rousskov added S-could-use-an-approval An approval may speed this PR merger (but is not required) and removed S-waiting-for-author author action is expected (and usually required) labels Aug 29, 2024
v = tok.remaining();
if (!v.isEmpty())
entries.push_back(new NotePairs::Entry(key, v));
const auto tokenCharacters = delimiters.complement("non-delimiters");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Computing this complement is a fairly expensive operation. Its result cannot be cached because delimiters may change from one call to another. If we want to optimize this and similar code, I see two options:

  1. Add Tokenizer::prefixUntil() or similar method that takes delimiters instead of their complement.
  2. Make CharacterSet::compliment() fast, either by always storing and maintaining complement (doubling memory usage requirement) or by storing a "reverse every comparison" flag (slowing down every comparison a little). In either case, we would have to make CharacterSet::Storage shareable across CharacterSets (to make CharacterSet copies cheap).

The first option looks significantly better to me because it does not slow down every CharacterSet test. In other words, the first option does not hurt any caller performance, while the second option hurts every caller performance (a little). The first option is also much easier to implement, and it does not make the rest of the affected class code more complex.

I consider this optimization, regardless of the implementation option, outside this bug-fixing PR scope.

@yadij yadij added M-cleared-for-merge https://github.com/measurement-factory/anubis#pull-request-labels and removed S-could-use-an-approval An approval may speed this PR merger (but is not required) labels Sep 9, 2024
squid-anubis pushed a commit that referenced this pull request Sep 9, 2024
Helpers may return annotations with empty values:

    OK team_=""

A note ACL may be configured to match an annotation with an empty value:

    configuration_includes_quoted_values on
    acl emptyTeam note team_ ""

However, that emptyTeam ACL did not match the above helper annotation:

* AppendTokens() split an empty annotation value into an empty vector
  instead of a vector with a single empty entry. That "never match an
  empty value received from the helper" bug was probably introduced in
  2017 commit 75d4734 when it replaced an "always try to match an empty
  value, even when it was not received from the helper" bug in
  ACLNoteStrategy::matchNotes().

* ACLStringData::match(SBuf v) never matched an empty value v. That bug
  was probably introduced in 2015 commit 76ee67a that mistook a nil
  c-string pointer for an empty c-string.
@squid-anubis squid-anubis added the M-waiting-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels label Sep 9, 2024
@squid-anubis squid-anubis added M-merged https://github.com/measurement-factory/anubis#pull-request-labels and removed M-waiting-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels M-cleared-for-merge https://github.com/measurement-factory/anubis#pull-request-labels labels Sep 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
M-merged https://github.com/measurement-factory/anubis#pull-request-labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants