apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Joe Schaefer <joe+gm...@sunstarsys.com>
Subject Re: [PATCH] table patch using mergesort Re: Frankentables
Date Tue, 24 Jun 2003 04:10:54 GMT
Cliff Woolley <jwoolley@virginia.edu> writes:

> On Mon, 23 Jun 2003, Joe Schaefer wrote:


> > To get the mod_headers tests to pass, here is a patch to
> > httpd-test/perl-framework/t/modules/headers.t
> Whoa, hang on... -1 to changing the test framework.  If we've had a
> semantic change in APR and the test suite caught it, that's a bug in APR
> and *not* a bug in the test suite, IMO (unless of course the original
> semantics were "wrong" in some way... but even then a semantic change
> in a library is not good).
> Why the semantic change?

Here is the section of headers.t (line 76) in question :

                ## should 'append' work this way?
                ## currently, if there are multiple headers
                ## with the same name, 'append' appends the value
                ## to the FIRST instance of that header.
                if (@expected_value) {

The documentation for apr_table_mergen gives no indication that
such behavior is intentional.  Moreover, besides being counterintuitive, 
IMO the current merge behavior violates both the letter and spirit of
RFC 2616 Section 4.2:

  Multiple message-header fields with the same field-name MAY be present 
  in a message ... It MUST be possible to combine the multiple header 
  fields into one "field-name: field-value" pair, without changing the 
  semantics of the message, by appending each subsequent field-value to 
  the first, each separated by a comma. The order in which header fields 
  with the same field-name are received is therefore significant to the 
  interpretation of the combined field value, and thus a proxy MUST NOT 
  change the order of these field values when a message is forwarded.

The current mergen code takes a subsequent header and effectively
*inserts* it ahead of all previous headers save for the first.
With the callback patch (which Brian hasn't applied yet), mergen
really does append the header to the end, and then combines those
headers into one.  The relative ordering of the headers is preserved.

However, I can see the value in not changing the current semantics
if people are concerned that the change may cause problems for third 
party modules that rely on the current behavior.  Either way, it's good 
to discuss this issue before adopting the callback API, so I can make 
sure the correct behavior, whatever that turns out to be, is both 
documented and tested prior to acceptance.

As far as Brian's current patch goes, it does not change the  mergen 
semantics.  However, it may have an impact on what apr_table_overlap 
is "supposed" to do in the case where a multivalued key appears in 
the first table, but not the second.  The documented behavior for this 
case is to do nothing; but the patches under discussion in this thread 
no longer have that behavior (apr_table_compress will always result in 
a table free of multivalued keys).

Joe Schaefer

View raw message