Opened 4 years ago

Closed 4 years ago

#4231 closed enhancement (complete)

new boolean operators for classification expression

Reported by: fdupont Owned by: fdupont
Priority: medium Milestone: Kea1.1
Component: classification Version: git
Keywords: classification Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DHCP Feature Depending on Ticket:
Estimated Difficulty: 0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

In order:

  • parentheses
  • NOT
  • .exists
  • AND and OR

Subtickets

Change History (18)

comment:1 Changed 4 years ago by fdupont

  • Owner set to fdupont
  • Status changed from new to accepted

comment:2 Changed 4 years ago by fdupont

  • Owner changed from fdupont to UnAssigned
  • Status changed from accepted to reviewing

Done. Ready for review when 1.0 will be released (or a particular item, each was individually committed).

comment:3 Changed 4 years ago by fdupont

Related to #4255 and #4256.

comment:4 Changed 4 years ago by tomek

  • Milestone changed from Kea-proposed to Kea1.1

Moving to 1.1 as it's in scope of the 1.1 charter.

comment:5 Changed 4 years ago by tomek

  • Owner changed from UnAssigned to tomek

comment:6 Changed 4 years ago by tomek

  • Component changed from Unclassified to classification

comment:7 Changed 4 years ago by tomek

  • Owner changed from tomek to fdupont

token.cc

The changes in TokenOption::evaluate(), TokenNot::evaluate(),
TokenAnd::evaluate(), TokenOr::evaluate() do not follow the design:
http://kea.isc.org/wiki/ClientClassificationDesign#Tokenvaluesarestrings

You should implement Token::toBool() method and do the conversion
there. This approach was reviewed and the conversion rules is what
everyone agreed to. It's essential that the conversion is done once
and every time a conversion to boolean is needed, Token::toBool() is
called. With this approach, the TokenOr::evaluate could look like this:

    string op1 = values.top();
    values.pop();
    string op2 = values.top();
    values.pop(); // Dammit, std::stack interface is awkward.

    return (toBool(op1) || toBool(op2));

Similar applies to other evaluate methods. This code is much more
understandable.

During the design review, everyone agreed to the conversion rules.
In particular, that everything other than "true" and "1" evaluates
to false. This is less strict strict approach that you propose
(throw if not equal to "true" or "false"). If you really think
that we have to throw, I propose to accept the following values:
"true", "1", "false", "0" and throw for any other value. If you
choose this approach, please also update the design document.

token_unittest.cc

The TokenTest.optionNotInvalid checks whether the code will throw
if an attempt to use TokenNot on "foo" is done.

    // CASE 2: The top value is not a boolean
    values_.push("foo");
    EXPECT_THROW(t_->evaluate(*pkt4_, values_), EvalTypeError);

If you reimplement the code as the current rules say, this will
have to be updated. If you decide to go with the true/1/false/0
approach, the code could stay, but we would need extra test
for toBool() method.


Please update copyright years in the headers of every file you
modified.


I'd like to bring to your attention the ticket #4264. It updates
the TokenOption class slightly. Right now it extracts options
from packet, but with #4264 it will extract the data from other
places, like RAI. There's also upcoming ticket that will extract
options from the v6 relays. I'm not sure how to proceed here.
I think this ticket can go in first, and #4264 could go after
it.


This ticket does not update User's Guide. It should. There is no
ChangeLog proposal.

I tested that this ticket builds and unit-tests pass on Ubuntu 15.10
x64.

comment:8 Changed 4 years ago by tomek

One more thing. There should be a unit-test that verifies parentheses better and boolean logic in general. It should evaluate the following expressions:

  • true and (false or false)
  • (true and false) or false
  • not true
  • not false
  • true and true and true and false
  • false or false or false or true
  • true or false or false or false
  • not (true or false)
  • not (true and false)
  • (not true) and false

comment:9 Changed 4 years ago by fdupont

  • Owner changed from fdupont to UnAssigned

Comments addressed. I propose for the !Change log entry: "Added Not, And and Or logical operators, parentheses around logical expressions and option[code].exist logical predicate (to check the presence of an empty option). [#4231, git xxx).
2 notes:

  • #4264 could get exist too
  • please improve the doc (I am not very satisfied by my new text).

comment:10 Changed 4 years ago by tomek

  • Owner changed from UnAssigned to tomek

comment:11 follow-up: Changed 4 years ago by tomek

  • Owner changed from tomek to fdupont

Thanks for addressing those comments. Your proposed changelog is ok.

There's one last thing outstanding, though. I can't find any tests that verify parsing of expressions with parentheses. Did I miss them or are there none at all? I think adding one or two tests in context_unittest.cc will do the trick.

comment:12 in reply to: ↑ 11 Changed 4 years ago by fdupont

  • Owner changed from fdupont to tomek

Replying to tomek:

There's one last thing outstanding, though. I can't find any tests that verify parsing of expressions with parentheses. Did I miss them or are there none at all? I think adding one or two tests in context_unittest.cc will do the trick.

=> there are some in the parse errors but I believe you want a few in a logical test set. I'll add the parentheses complement of the logicalPrecedence set.

comment:13 follow-up: Changed 4 years ago by fdupont

Done in 0c554b2..0923372 trac4231 -> trac4231 commit.

comment:14 in reply to: ↑ 13 Changed 4 years ago by tomek

  • Owner changed from tomek to fdupont

Replying to fdupont:

Done in 0c554b2..0923372 trac4231 -> trac4231 commit.

Thanks. The code is now ready. Please merge.

comment:15 Changed 4 years ago by fdupont

Merged.

comment:16 Changed 4 years ago by fdupont

  • Resolution set to complete
  • Status changed from reviewing to closed

comment:17 Changed 4 years ago by fdupont

  • Resolution complete deleted
  • Status changed from closed to reopened

Reopened because #4264 was incorrectly merged within this ticket, cf #4313

comment:18 Changed 4 years ago by fdupont

  • Resolution set to complete
  • Status changed from reopened to closed
Note: See TracTickets for help on using tickets.