Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#4090 closed enhancement (fixed)

Implement TokenSubstring class

Reported by: sar Owned by: sar
Priority: high Milestone: Kea1.0-beta
Component: dhcp 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: 16 Internal?: no

Description

Implement the TokenSubstring? class as part of the client classification work.

Subtickets

Change History (10)

comment:1 Changed 4 years ago by sar

  • Keywords classification added

comment:2 Changed 4 years ago by sar

  • Owner set to sar
  • Status changed from new to assigned

comment:3 Changed 4 years ago by sar

  • Owner changed from sar to UnAssigned
  • Status changed from assigned to reviewing

This is now ready for review.

I've chosen to go with substring(string, start, length) where the third parameter is the length of the desired substring.

Start and length maybe positive or negative. In the negative case start counts from the end and length means to go from the start position towards the front. So substr("foobar", -2, 2) will get the last two characters 'ar', substr("foobar", -2, -2) will get the the second two from the end 'ob'.

Length may also be "all" in which case you get all of the characters from the start point to the end.

comment:4 Changed 4 years ago by stephen

  • Owner changed from UnAssigned to stephen

comment:5 follow-up: Changed 4 years ago by stephen

  • Owner changed from stephen to sar

Reviewed commit e5be011922ec7c808ae1102feb98600debdcd6a5

src/lib/eval/eval_messages.mes
EVAL_SUBSTRING_BAD_PARAM_CONVERSION: the explanatory text should make clean that if this message is output, the substring operation results in an empty string.

src/lib/eval/token.h
Line 156: typo: "poped" (I know that line is not part of the ticket, but...)

Line 173-175: Suggest that the first paragraph mention that as well as consuming the top three (prefer "top three" to "last three") elements on the stack, the resulting substring is pushed back on the stack.

Line 176-178: Precede each element name with a "- " (i.e. list elements) if they are to appear vertically in the Doxygen documentation (at present they appear in the sentence one after the other).

Line 180: s/empty an empty/empty, an empty/

Line 184: "0 is before the first character" and "-1 is before the last character". This is a strange way of putting it as "position" usually indicates the starting character. Clearer would be to say that position 0 is the first character; if the index is negative, -1 is the last character, -2 the last-but-one character etc.

Line 201/202: Use "@param" for method parameters. (The same goes for lines 155/156 - which aren't part of this change).

Some more examples in the header would be helpful.

src/lib/eval/token.cc
Lines 79, 81: typo: "legnth" (a couple of places)

Line 83: Declare one variable per line (rather than multiple variables per line)

Line 86: I think that len_string == "all" is clearer than putting "all" on the left-hand side.

Line 105: bit-wise OR used instead of logical OR.

Line 105: As we know that string_str.length() is greater than zero at this point (if it was zero, an empty string would have been pushed onto the stack and the method exited before now), a simpler test is

((start_pos < -string_str.length()) || (start_pos >= string_str.length()))

Line 112: typo: "staring"

Line 113: typo: "depnding"

src/lib/eval/tests/token_unittest.cc
Line 236 and 355: typo: "excpetion"

substringStartingEmpty: strictly speaking, testing that an empty string is returned if the input string is not blank and the length field is zero is not part of a test of the incoming string being empty. The test is needed, but perhaps in a different test case?

Other
As logging is being used, I think this ticket should include the files that enable the logging.

(Steps are:

  • Create the files eval_log.{cc,h} (copy the hooks_log files and edit)
  • Include eval_log.h in token.cc
  • Update Makefile.am to include eval_log.* in the dependencies
  • Update Makefile.am to include the logger and utilities library in the link command line
  • Update the administration guide to list the "eval" logger.

)

A ChangeLog entry is required for this. Something as simple as

Added client classification substring token.

should do.

comment:6 in reply to: ↑ 5 Changed 4 years ago by sar

All done, comments below

Replying to stephen:

Reviewed commit e5be011922ec7c808ae1102feb98600debdcd6a5

src/lib/eval/token.h

Comments updated with some new text and more examples.

src/lib/eval/token.cc

Line 105: As we know that string_str.length() is greater than zero at this point (if it was zero, an empty string would have been pushed onto the stack and the method exited before now), a simpler test is

((start_pos < -string_str.length()) || (start_pos >= string_str.length()))

I had problems with this and with casting the string_str.length() directly. I re-wrote the code to get the length once and to use that during the rest of the work.

substringStartingEmpty: strictly speaking, testing that an empty string is returned if the input string is not blank and the length field is zero is not part of a test of the incoming string being empty. The test is needed, but perhaps in a different test case?

I renamed the test case to be substringReturnEmpty - which checks what happens if we pass in good arguments but that call for no string to be returned.

Other
As logging is being used, I think this ticket should include the files that enable the logging.

(Steps are:

  • Create the files eval_log.{cc,h} (copy the hooks_log files and edit)
  • Include eval_log.h in token.cc
  • Update Makefile.am to include eval_log.* in the dependencies
  • Update Makefile.am to include the logger and utilities library in the link command line
  • Update the administration guide to list the "eval" logger.

)

Logging has now been enabled.

A ChangeLog entry is required for this. Something as simple as

Added client classification substring token.

should do.

Per chat I think Stephen agrees the ChangeLog? isn't necessary for this feature but I've included it in the current patch in case I misunderstood. I can remove it before merging.

comment:7 Changed 4 years ago by sar

  • Owner changed from sar to stephen

comment:8 Changed 4 years ago by stephen

  • Owner changed from stephen to sar

Reviewed commits b36a613740284c5197e99b8fdd3a78bcad3202b3 through 358927b0a2616be496002777dad09250202339b4

doc/guide/logging.xml
In the description for the kea{4,6}.eval logger, I suggest using the term "client classification expression evaluation code" (instead of "evaluation code"). This will more clearly identify the part of Kea from which the messages originate.

src/lib/eval/Makefile.am
Add $(top_builddir)/src/lib/util/libkea-util.la to the list of link libraries. We have had problems in the past on some operating systems when indirectly-referenced libraries (i.e. user-libraries not directly referenced by the code but referenced by user-libraries against which the code is linked) are not specified in the link command.

ChangeLog
Agree that this is not required for this change.

I don't need to see this ticket again, please merge after making the changes suggested.

comment:9 Changed 4 years ago by sar

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

changes done

merged with commit 4f44fea70c65c286861b4607c7ace66cbfdacfdf

comment:10 Changed 4 years ago by tomek

  • Milestone changed from Kea1.0 to Kea1.0-beta

Milestone renamed

Note: See TracTickets for help on using tickets.