preliminary minimal changes

[zjmarlow]

draft pull request with minimal changes to handle eol. also updates is-chunked check to include the case where multiple Transfer Encodings are present (chunked should always be last).

requesting feedback before final changes.

[lizmat]

Appears to be failing 2 tests for me on MacOS:

=== t/020-message.rakutest
1..21
ok 1 - new 1/4
ok 2 - new 2/4
ok 3 - new 3/4
ok 4 - new 4/4
ok 5 - push-field 1/2
ok 6 - push-field 2/2
ok 7 - add-content 1/2
ok 8 - add-content 2/2
ok 9 - remove-field 1/1
ok 10 - parse 1/4
ok 11 - parse 2/4
ok 12 - parse 3/4
ok 13 - parse 4/4
not ok 14 - Str 1/2
# Failed test 'Str 1/2'
# at t/020-message.rakutest line 41
# expected: 'a: b, c, d
# 
# line
# '
#      got: 'a: b, c, d
# Content-Length: 4
# 
# line'
not ok 15 - Str 2/2
# Failed test 'Str 2/2'
# at t/020-message.rakutest line 42
# expected: 'a: b, c, d

line
'
#      got: 'a: b, c, d
Content-Length: 4

line'
ok 16 - clear 1/2
ok 17 - clear 2/2
ok 18 - parse complex 1/3
ok 19 - parse complex 2/3
ok 20 - parse complex 3/3
# Subtest: charset
    ok 1 - dumb default charset
    ok 2 - default text charset
    ok 3 - default "non-text" charset
    ok 4 - explicity charset
    1..4
ok 21 - charset
# You failed 2 tests of 21

[zjmarlow]

Should HTTP::UserAgent be producing valid Messages, or is it up to the users of the library to use it in such a way that it only produces valid messages? Some of the test messages are themselves invalid, so what needs changing will depend on the library's purpose.

[lizmat]

Well, that is a good question. What this module really needs, is a maintainer.

Would you be willing to take on that role? You apparently have a need to fix things in it :-)

[zjmarlow]

Yes, but I'll need guidance and feedback as I maintain it, if that's okay?

Some first questions:

• should the module or the module users be responsible for making sure messages generated are valid, and
• what should be done when an invalid message is received:
  • nothing / ignore
  • parse as much as possible and silently ignore the rest
  • warning
  • exception
  • optional error handler callback that takes the message as an argument
  • other?

I'll start looking through the other open issues, too.

[zjmarlow]

What would the best channel / medium be for discussing this module?

[lizmat]

Either #raku on libera.chat or on Discord

[zjmarlow]

There are enough changes to get things conformant that it would be difficult not to break existing behavior. My plan is to provide alternate classes that can be imported with:

use HTTP::UserAgent 'strict';

if that is acceptable, though I'm open to other options.

[lizmat]

That would make it opt-in, and as such backward compatible. 👍

[zjmarlow]

I wasn't able to get exporting to work how I was hoping, so instead, the strict versions are just named after the originals with "-Strict" attached. They are subclasses of the originals. The new classes are:

• HTTP::UserAgent-Strict
• HTTP::Message-Strict
• HTTP::Request-Strict
• HTTP::Response-Strict
• HTTP::Header-Strict
• HTTP::Header::ETag (HTTP::Header::Field with a "weak" attribute)

They are all made available with

use HTTP::UA-Strict;

so that it is easy to just import the strict classes. Behavior when mixing strict and original classes is "undefined".

The only change to the original should be the is-chunked multi methods in the HTTP::Message class. One checks the message's own headers, the other takes an arbitrary header object.

The following new testcases were added:

• 011-headers-strict.rakutest
• 021-message-issue-226.rakutest
• 042-request-issue-226.rakutest
• 051-response-issue-226.rakutest

I still need to do some testing with the HTTP::UserAgent-Strict class before turning this draft request into a full pull request, but this should be a good place for review and verifying testcases (the new behavior passed for me locally across Windows, Linux, and Mac).

[zjmarlow]

need to fix body-less messages. will comment again once fixed.
fix committed.

[jonathanstowe]

I think these were qualified like this because there are un-related packages that provide similarly named modules, so I'd be careful about removing that.

[zjmarlow]

My apologies. I am now reading through the META6.json and distributions documentation and committed some fixes. Will the version's patch number eventually need to be updated?

[zjmarlow]

Okay, the META6.json should be fixed. I did not change the version number.

[zjmarlow]

Should this draft be marked ready for review / become a regular pull request? Are there any other changes that would be good before doing so?

[jonathanstowe]

The only major suggestion I have is really a matter of personal taste: I'm not keen on the -Strict naming. I'm not sure I can completely explain why, but to my eye it would be more regular and consistent if the new classes were named HTTP::UserAgent::Strict, HTTP::Message::Strict and so forth. If nothing else it may make it easier to adapt existing code.

[jonathanstowe]

This could just be named Grammar as it will get a name based on the enclosing package anyway. As it stands if someone wanted to refer to it outside the class they'd have to use HTTP::Header-Strict::HTTP::Header-Strict::Grammar

[jonathanstowe]

I'm not sure whether it makes any difference in this case but having this as a my scoped variable in the class body means that every instance in the same process will get the same Lock object. A program might want multiple UA instances with different parameters for some reason.

It could be an attribute that is populated in TWEAK or even with a custom accessor to populate it lazily I suppose.

[jonathanstowe]

Strictly speaking it doesn't matter here (the try require ::("IO::Socket::SSL") can't throw an exception,) but generally :

$https_lock.protect: {
    try require ::("IO::Socket::SSL");
};

might be preferred: if nothing else it is less typing and another developer doesn't have to scan the code to find the matching unlock.