Re: Helpers and Notes

On 26/01/2013 10:39 a.m., Tsantilas Christos wrote:
> Hi all,
>
> While we are working in a project we seen that the helpers do wrong
> usage of the Notes interface.
>
> This is maybe it is our mistake (and mostly my mistake) because we do
> not have make good documentation of this interface.
> In any case I believe that we should fix some thinks.
>
> In the Notes.h file there are the following classes:
>
> class Note
> -----------
>
> This is a class used to store a note configuration not a note it self.
> It is a note name plus a list of possible values with an ACL list in
> each value.
> It is used to store configurations for the following cfg lines:
> note key value acl1 acl2
>
>
> class Notes
> ------------
> It is used to store a list of Notes. A list like the following:
> note key valueA acl1a acl2a
> note key valueB acl1b acl2b
>
> This is used to add custom notes (or metaHeaders) in an HTTP request

With the ACL list being optional and the word "note" being dropped by
the config parser what is actually stored is:

  NotesList{
       Note{key,value,NULL},
       Note{key,value,NULL}
       ...
}

... which is clearly a list of kv-pairs without any specific information
about delimiter, display semantics, or input source location. Nowhere
in ths implementation or documentation is it clear that this list
structure is *only* ever to be used for parsing the config file. Rather
the simplicity of the information stored implies strongly that it is
created as re-usable storage structure.
  ***As was requested when you were planning the Notes feature.***

> class NotePairs
> -----------------
> This is an HttpHeader kid class used to store the notes (or
> MetaHeaders). We select HttpHeader class because it has some interesting
> features. For example assume that two different subsystems add the same
> note:
> X-SQUID-ProcessedBy: ReWriter
> X-SQUID-ProcessedBy: Adaptation
>
> Then this is will be merged to
> X-SQUID-ProcessedBy: ReWriter, Adaptation

Yes NotePairs is clearly and specifically a Mime syntax version of kv-pairs.

The interesting and useful fetures are *only* useful when outputting
kv-pairs as ICAP or HTTP headers in Mime format. Those features are not
useful in any other context Squid uses kv-pairs. ... which is why the
helpers avoid that type until the final logging stage where it has no
choice.

> Squid helper Notes problems
> -------------------------------
> The changes made in trunk revno:12490 and revno:12495
>
> Problems I am seeing:
> - it uses class Notes to store the notes/metaHeaders not the NotePairs.
> It adds the "Notes HelperReply:notes" and the "Notes
> HttpRequest::helperNotes" members.

This is intentional. see above.

The first implementation attempt used a NotePairs list for helpers. The
exact same behaviour of merging keys together which make NotePair
wonderful for ICAP header display and logging display make this NotePair
type difficult to use as a kv-pair list for helpers and any other use
where HTTP field-value syntax is needed.
  The other difficulty was the whole or at least a large segment of the
HTTP parser becomes a dependency, which was not worth the cost and
effort for the simplicity of NotePairs. This was probably not noticable
when integrating with ICAP due to it already having that dependency in
place for ICAP header parsing.

The need for multiple de-references ( A->value[X].value()->value ) when
accessing NotesList is accepted as a cost (for now) in exchange for the
benefit of *not* merging key values together and not having to link in
the HTTP parser.
  If you are solving problems in the Notes structure systems please
resolve that dereference issue.

>
> - It adds some extra methods which does not make sense and confuse more
> the Notes interface:
> * NotePairs::append(const Notes::NotesList &src)
> * NotePairs::append(const NotePairs *src)
> These functions used to add a Notes list to a NotePairs List. The
> HttpRequest::helperNotes to AccessLogEntry::notes.

By 'add' you mean 'append list S to this list'. Which is why it is
called 'append()'. Where is the problem? naming, semantics and operation
conform to standard conventions.

The first above converts a NotesList into a NotesPairs list.
Specifically because the NotePairs list is hard-coded to utilize the
MIME format key-value pairs with colon (:) delimiter - which in Squid
are stored in HttpHeader class types.

The second is added such that any existing AccessLogEntry list can be
appended to by other sources producing NotePairs lists (ie RESPMOD).

> But It is easier to use NotePairs for HttpRequest::helperNotes member
> and just use the (pre-)existing NotePairs::append (the HttpHeaders::append)

Causing the need to covert a key=value string pair into an HttpHeader
object. Causing the need to define said HttpHeader as a custom header
and fill out its value set. All of which adds HTTP protocol operations
(and validity checks) for the sake of parsing the very non-HTTP input
"OK foo=hello".

  Not to mention that "foo_=hello" (_ being defined for use by custom
key names) is an invalid HTTP header name rejected by the HttpHeader
parser infrastructure, and I have no intention of altering that
HTTP-specific parser (potentially affecting on-wire HTTP validity
checks) for the sake of unrelated code that does not even use HTTP or
Mime syntax.

> * Notes::add(const String &noteKey, const String &noteValue);
> * Notes::add(const Notes &src);
> * Notes::find(const String &noteKey)
> To add and search for a note in Notes List. But it is wrong. We do
> not store such Notes here. We are storing Notes configuration, while
> parsing the cfg file.

So we have a linked list of key-value pairs writen with an generic
interface which is not allowed to be used anywhere outside of Parsing?
That goes against the design requirements I requested back at the
beginning. That Notes and NotesList be a generic list of kv-pairs we
could add to arbitrarily and output with various format delimiters.

We have four desired uses for NotesList in Squid:

  1) [A-Z]key ':' ' ' value - I/O for ICAP in Mime header format
  2) key '=' value - I/O for ICAP or HTTP in HTTP header field-value syntax
  3) key '=' value - helper I/O syntax
  4) key ' ' value - squid.conf 'notes' directive syntax

NP: (1) has limitations on key character set, and optional (desired 1
SP) whitespace after the ':' delimiter. The others do not.

> We should use NotePairs for HttpRequest::helperNotes and
> HelperReply::notes members and use the NotePairs::putExt or
> NotePairs::addEntry(new HttpHeaderEntry(HDR_OTHER, name, value)); to
> adding note/value pairs.
>
> - Initially we had select the AccessLogEntry to store notes. The
> AccessLogEntry selected because it considered as good structure to hold
> HTTP request X-data. (But we had problem on this).
> I am not sure if the HttpRequest::helperNotes is the correct position to
> add Notes. We should consider if we can add notes to
> AccessLogEntry::notes, or select an other X-data structure.

HttpRequest is currently abused as a transport mechanism to relay the
notes through to AccessLogEntry, which is not directly accessible from
the helper handlers.
If ALE can be made accessible to both helpers and ACL match system in a
useful way the HttpRequest member can be removed.

NP: adding straight to the ALE list of NotePairs adds an extra problem,
that the kv-pair is now *copied* into Mime syntax. Once the note has
been converted to Mime header syntax it becomes much more difficult to
process the list and match a single kv-pair out of a set of identical
ones. ACLs for example do not have access to the ALE object to test
particular key values and even if they did they can only retrieve the
entire list of all values associated with each key name.

> I am working on changing the above problems. Also maybe we need to
> change the names of some classes to avoid confusion in the future, for
> example rename Note and Notes to NoteRule and NoteRules or similar.
>
> Any opinions on the above?

When writing the helper interface use of Notes the problem was only that
Notes* classes were hard-coded for a specific use-case output.

We had long discussions during the design phase and auditing about using
Notes as both header format "Key:value" and field format "key=value" - I
was under the impression that you had taken those usage requirements on
board and designed Note class which clearly takes a Key and Value
separately (without any additional details like ACLs!).

Other than the link with ACLs, class Note is fine for use as a generic
kv-pair node.
Other than your documentation of it, class NotesList is reasonably okay
for use as a generic list of Note kv-pair. One problem is the use of
std::list<Node::Pointer> which causes a double-indirection layer when
trying to access any node position of the list.
  For example: list->values[0]->value().keyValue() instead of just
list[0]->keyValue().

To resolve what you see as an API abuse "problem" I think all that is
needed:
  1) make Note the base type representing just a kv-pair
  2) make a sub-class associating ACLs as a config specific sub-type of it.
  2) make a "Packer" for NotesList that outputs in each of the earlier
mentioned syntax formats.

To resolve the indirection problems in NotesList I propose adding a
template to Squid sources which presents the *next pointer and
linked-list accessors and iterators. Such that any class can inherit
from it to be a node in a linked-list.
  We can then unify all the classes and structures which are performing
their own linked-list implementations into inheritence from this template.
  - NotesList would then be a class inheriting from ListNode<> and Note.

Sound reasonable?

Amos
Received on Sat Jan 26 2013 - 04:05:24 MST