Tests: REQUEST_URI variable test with rewrite

[andrey-zelenkov]

Tests for the #1162

[hongzhidao]

Hi @andrey-zelenkov,
This PR looks good, but could you separate it into two patches?
The first one is to test "rewrite" together with "proxy" without changing any rules, I mean $requst_uri is still changeable, it doesn't depend on #1162.
Then I'll commit a patch separated from #1162, it will rework the "proxy" module.
Let me know if you are confused with the idea.

[andrey-zelenkov]

Dropped the first patch to submit it as a separate pull request.

% git range-diff 0852b615...51cdaa9c
1:  5030a900 < -:  -------- Tests: added $rewrite_uri test with proxy
2:  0852b615 = 1:  51cdaa9c Tests: added REQUEST_URI test with rewrite rule

[ac000]

Is this not an issue?

=================================== FAILURES ===================================
__________________ test_python_application_variables[3.10.12] __________________
date_to_sec_epoch = <function date_to_sec_epoch.<locals>._date_to_sec_epoch at 0x7fe46df140d0>
sec_epoch = 1712839114.0
    def test_python_application_variables(date_to_sec_epoch, sec_epoch):
        client.load('variables')
    
        body = 'Test body string.'
    
        resp = client.http(
            f"""POST / HTTP/1.1
    Host: localhost
    Content-Length: {len(body)}
    Custom-Header: blah
    Custom-hEader: Blah
    Content-Type: text/html
    Connection: close
    custom-header: BLAH
    
    {body}""".encode(),
            raw=True,
        )
    
        assert resp['status'] == 200, 'status'
        headers = resp['headers']
        header_server = headers.pop('Server')
        assert re.search(r'Unit/[\d\.]+', header_server), 'server header'
        assert (
            headers.pop('Server-Software') == header_server
        ), 'server software header'
    
        date = headers.pop('Date')
        assert date[-4:] == ' GMT', 'date header timezone'
        assert abs(date_to_sec_epoch(date) - sec_epoch) < 5, 'date header'
    
        assert headers == {
            'Connection': 'close',
            'Content-Length': str(len(body)),
            'Content-Type': 'text/html',
            'Request-Method': 'POST',
            'Request-Uri': '/',
            'Http-Host': 'localhost',
            'Server-Protocol': 'HTTP/1.1',
            'Custom-Header': 'blah, Blah, BLAH',
            'Wsgi-Version': '(1, 0)',
            'Wsgi-Url-Scheme': 'http',
            'Wsgi-Multithread': 'False',
            'Wsgi-Multiprocess': 'True',
            'Wsgi-Run-Once': 'False',
        }, 'headers'
        assert resp['body'] == body, 'body'
    
        # REQUEST_URI unchanged
    
        path = f'{option.test_dir}/python/variables'
        assert 'success' in client.conf(
            {
                "listeners": {"*:8080": {"pass": "routes"}},
                "routes": [
                    {
                        "action": {
                            "rewrite": "/foo",
                            "pass": "applications/variables",
                        }
                    }
                ],
                "applications": {
                    "variables": {
                        "type": client.get_application_type(),
                        "processes": {'spare': 0},
                        "path": path,
                        "working_directory": path,
                        "module": "wsgi",
                    }
                },
            }
        )
    
        resp = client.http(
            f"""POST /bar HTTP/1.1
    Host: localhost
    Content-Length: 1
    Custom-Header: blah
    Content-Type: text/html
    Connection: close
    
    a""".encode(),
            raw=True,
        )
>       assert resp['headers']['Request-Uri'] == '/bar', 'REQUEST_URI unchanged'
E       AssertionError: REQUEST_URI unchanged
E       assert '/foo' == '/bar'
E         
E         - /bar
E         + /foo
test/test_python_application.py:105: AssertionError
=========================== short test summary info ============================
FAILED test/test_python_application.py::test_python_application_variables[3.10.12] - AssertionError: REQUEST_URI unchanged
assert '/foo' == '/bar'
  
  - /bar
  + /foo

[andrey-zelenkov]

Is this not an issue?

This should be fixed once #1162 is merged.

[andrey-zelenkov]

Rebased:

% git range-diff 51cdaa9c...e04be85a
-:  -------- > 1:  d494d2eb Wasm-wc: Bump the h2 crate from 0.4.2 to 0.4.4
-:  -------- > 2:  e6d8fc66 njs (lowercase) is more preferred way to mention
-:  -------- > 3:  6e79da47 Docs: njs (lowercase) is more preferred way to mention
-:  -------- > 4:  5f606742 Tests: added $request_uri tests with proxy
-:  -------- > 5:  a625a0b1 Tests: compatibility with OpenSSL 3.2.0
-:  -------- > 6:  2d7a8468 HTTP: Added variable validation to the response_headers option
-:  -------- > 7:  62697773 Tests: error report corrected for unknown variables in "response_headers"
1:  51cdaa9c = 8:  e04be85a Tests: added REQUEST_URI test with rewrite rule

[ac000]

Bit of a chicken and egg problem...

I think I'd wait for the code change to be merged first, then we won't
get failing tests all the time...

[ac000]

The other option would be to have Zhidao merge it via his prll-request,
which would be a completely reasonable thing to do...

[andrey-zelenkov]

The two latest commits, 2d7a846 and 6269777, had the exact same problem. So I was planning to wait for #1162 to be merged before pushing this one (as we did yesterday).

Another option is to commit this test under pytest.skip() call and add skip removal in #1162, but I am personally would prefer just to wait.

[ac000]

Yes, waiting's good...

[hongzhidao]

Hi @andrey-zelenkov @ac000,
Does it make sense to release #1162 and #1197 in a single commit?
If released separately, the tests won't pass.

The other option would be to have Zhidao merge it via his prll-request,
which would be a completely reasonable thing to do...

LGTM.

[hongzhidao]

Hi @andrey-zelenkov,
It seems you missed reworking test/test_rewrite.py.

[ac000]

Does it make sense to release #1162 and #1197 in a single commit?
If released separately, the tests won't pass.

Not the same commit. But they could go via the same pull-request...

Once Andrei and you are happy with the test. You could pull the patch,
e.g

$ wget https://github.com/nginx/unit/commit/e04be85ab5af4ef4ff5da89750ef1b487bc32a91.patch

(Don't just use that URL as the commit id will change if the commit is
modified)

You can then apply it to your tree with

$ git am e04be85ab5af4ef4ff5da89750ef1b487bc32a91.patch

[andrey-zelenkov]

Hi @andrey-zelenkov,
It seems you missed reworking test/test_rewrite.py.

Last time when we discussed that it was necessary to fix relevant log message https://github.com/nginx/unit/blob/master/src/nxt_http_rewrite.c#L110.

Was noted here: #1162 (comment)
No plans to fix this anymore?

[hongzhidao]

Hi @andrey-zelenkov,
It seems you missed reworking test/test_rewrite.py.

Last time when we discussed that it was necessary to fix relevant log message https://github.com/nginx/unit/blob/master/src/nxt_http_rewrite.c#L110.

Was noted here: #1162 (comment) No plans to fix this anymore?

Fixed, thanks.

[hongzhidao]

Hi Andrei,
Take a look at the test, it's broken with #1162.

assert (
            wait_for_record(fr'^::1 /bar /bar\?a=b$', 'access.log') is not None
        ), 'req 8081 (proxy) rewrite'
E       AssertionError: req 8081 (proxy) rewrite
E       assert None is not None
E        +  where None = <function wait_for_record.<locals>._wait_for_record at 0x7f62e92daaf0>('^::1 /bar /bar\\?a=b$', 'access.log')

FAILED test/test_variables.py::test_variables_request_uri - AssertionError: re...

I think it's because of the rule change of request uri which is switched from changeable to constant.

[hongzhidao]

Hi @andrey-zelenkov,
If you don't mind, I'll close this PR :)

[andrey-zelenkov]

Sure!

For history: this PR was merged as part of #1162