pact-foundation / pact-reference

Reference implementations for the pact specifications
https://pact.io
MIT License
91 stars 46 forks source link

`application/xml` body contents sometimes not returned from mock_server #305

Closed YOU54F closed 3 months ago

YOU54F commented 1 year ago

๐Ÿ‘‹๐Ÿพ

I've been building out a PactV3 interface over in pact-python and come across a few snags.

Given the following test,

If i provide

  1. only lib.pactffi_with_header_v2(interaction_handle, 1,b'Content-Type', 0, content_type.encode('ascii')) it fails
  2. only lib.pactffi_with_header_v2(interaction_handle, 1,b'content-type', 0, content_type.encode('ascii')) it fails
  3. both of the above it passes, around 60% of the time. Sometimes body contents aren't returned from the provider.

    My example test is here.

import pytest
import requests
# from matchersv3 import EachLike, Integer, Like, AtLeastOneLike
import xml.etree.ElementTree as ET
from cffi import FFI
from register_ffi import get_ffi_lib
import json
import requests
ffi = FFI()

@pytest.fixture
def lib():
    lib = get_ffi_lib(ffi) # loads the entire C namespace
    lib.pactffi_logger_init()
    lib.pactffi_logger_attach_sink(b'stdout', 3)
    lib.pactffi_logger_apply()
    version_encoded = lib.pactffi_version()
    lib.pactffi_log_message(b'pact_python_ffi', b'INFO', b'hello from pact python ffi, using Pact FFI Version: '+ ffi.string(version_encoded))
    return lib

# TODO:- This test in unreliable, sometimes xml is not returned from the mock provider
def test_with_xml_requests(lib):

    expected_response_body = '''<?xml version="1.0" encoding="UTF-8"?>
        <projects>
        <item>
        <id>1</id>
        <tasks>
            <item>
                <id>1</id>
                <name>Do the laundry</name>
                <done>true</done>
            </item>
            <item>
                <id>2</id>
                <name>Do the dishes</name>
                <done>false</done>
            </item>
            <item>
                <id>3</id>
                <name>Do the backyard</name>
                <done>false</done>
            </item>
            <item>
                <id>4</id>
                <name>Do nothing</name>
                <done>false</done>
            </item>
        </tasks>
        </item>
        </projects>'''
    format = 'xml'
    content_type =  'application/' + format     
    pact_handle = lib.pactffi_new_pact(b'consumer',b'provider')
    lib.pactffi_with_pact_metadata(pact_handle, b'pact-python', b'version', b'1.0.0')
    interaction_handle = lib.pactffi_new_interaction(pact_handle, b'description')
    lib.pactffi_given(interaction_handle, b'i have a list of projects')
    lib.pactffi_upon_receiving(interaction_handle, b'a request for projects in XML')
    lib.pactffi_with_request(interaction_handle, b'GET', b'/projects')
    lib.pactffi_with_header_v2(interaction_handle, 0,b'Accept', 0, content_type.encode('ascii'))

    # I can only seem to get this to pass, if I set these two headers
    #         "headers": {
    #   "Content-Type": "application/xml",
    #   "content-type": ", application/xml"
    # },
    lib.pactffi_with_header_v2(interaction_handle, 1,b'Content-Type', 0, content_type.encode('ascii'))
    lib.pactffi_with_header_v2(interaction_handle, 1,b'content-type', 1, content_type.encode('ascii'))
    lib.pactffi_with_body(interaction_handle, 1, content_type.encode('ascii'), expected_response_body.encode('ascii'))

    mock_server_port = lib.pactffi_create_mock_server_for_transport(pact_handle, b'127.0.0.1', 0, b'http', b'{}')
    print(f"Mock server started: {mock_server_port}")
    try:
        uri = f"http://127.0.0.1:{mock_server_port}/projects"
        response = requests.get(uri,
                        headers={'Accept': content_type})
        response.raise_for_status()
    except requests.HTTPError as http_err:
        print(f'Client request - HTTP error occurred: {http_err}')  # Python 3.6
    except Exception as err:
        print(f'Client request - Other error occurred: {err}')  # Python 3.6

    # Check the client made the right request

    result = lib.pactffi_mock_server_matched(mock_server_port)
    print(f"Pact - Got matching client requests: {result}")
    if result == True:
        PACT_FILE_DIR='./pacts'
        print(f"Writing pact file to {PACT_FILE_DIR}")
        res_write_pact = lib.pactffi_write_pact_file(mock_server_port, PACT_FILE_DIR.encode('ascii'), False)
        print(f"Pact file writing results: {res_write_pact}")
    else:
        print('pactffi_mock_server_matched did not match')
        mismatches = lib.pactffi_mock_server_mismatches(mock_server_port)
        result = json.loads(ffi.string(mismatches))
        print(json.dumps(result, indent=4))
        native_logs = lib.pactffi_mock_server_logs(mock_server_port)
        logs = ffi.string(native_logs).decode("utf-8").rstrip().split("\n")
        print(logs)

    ## Cleanup

    lib.pactffi_cleanup_mock_server(mock_server_port)
    assert result == True
    print(f"Client request - matched: {response.text}")
    # Check our response came back from the provider ok

    assert response.text != '' # This should always have a response
    projects = ET.fromstring(response.text)
    assert len(projects) == 1
    assert projects[0][0].text == '1'
    tasks = projects[0].findall('tasks')[0]
    assert len(tasks) == 4
    assert tasks[0][0].text == '1'
    # assert tasks[0][1].text == 'Do the laundry'
    print(f"Client response - matched: {response.text}")
    print(f"Client response - matched: {response.text == expected_response_body}")

I believe I am mis-using pactffi_with_header_v2 according to the docs

To setup a header with multiple values, you can either call this function multiple times with a different index value, i.e. to create x-id=2, 3

pactffi_with_header_v2(handle, InteractionPart::Request, "x-id", 0, "2");
pactffi_with_header_v2(handle, InteractionPart::Request, "x-id", 1, "3");

So if I wanted to separate headers with a different case, I would use the following (but in reality I assume I should only care about passing one of these and the core should deal with case matches - should that be configurable?)

pactffi_with_header_v2(handle, InteractionPart::Request, "Content-Type", 0, "application/xml");
pactffi_with_header_v2(handle, InteractionPart::Request, "content-type", 0, "application/xml");

This works for the test

pactffi_with_header_v2(handle, InteractionPart::Request, "Content-Type", 0, "application/xml");
pactffi_with_header_v2(handle, InteractionPart::Request, "content-type", 1, "application/xml");

but that results in the following pact which shows content-type as , "application/xm" as it was index at position 1.

{
  "consumer": {
    "name": "consumer"
  },
  "interactions": [
    {
      "description": "a request for projects in XML",
      "providerStates": [
        {
          "name": "i have a list of projects"
        }
      ],
      "request": {
        "headers": {
          "Accept": "application/xml"
        },
        "method": "GET",
        "path": "/projects"
      },
      "response": {
        "body": "<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n        <projects>\n        <item>\n        <id>1</id>\n        <tasks>\n            <item>\n                <id>1</id>\n                <name>Do the laundry</name>\n                <done>true</done>\n            </item>\n            <item>\n                <id>2</id>\n                <name>Do the dishes</name>\n                <done>false</done>\n            </item>\n            <item>\n                <id>3</id>\n                <name>Do the backyard</name>\n                <done>false</done>\n            </item>\n            <item>\n                <id>4</id>\n                <name>Do nothing</name>\n                <done>false</done>\n            </item>\n        </tasks>\n        </item>\n        </projects>",
        "headers": {
          "Content-Type": "application/xml",
          "content-type": ", application/xml"
        },
        "status": 200
      }
    }
  ],
  "metadata": {
    "pact-python": {
      "version": "1.0.0"
    },
    "pactRust": {
      "ffi": "0.4.7",
      "mockserver": "1.2.3",
      "models": "1.1.9"
    },
    "pactSpecification": {
      "version": "3.0.0"
    }
  },
  "provider": {
    "name": "provider"
  }
}

I couldn't seem to get a combo that worked reliably.

I also noted that the with_body docs states

content_type - The content type of the body. Defaults to text/plain. Will be ignored if a content type header is already set.

but doesn't say which casing, or if casing matters (or what happens if there are multiple cases ๐Ÿคฏ )

I know we can't document all the things, and I haven't looked at any other sources to compare yet.

Screenshot 2023-08-02 at 20 07 27

The example is pushed up to the branch demo/python_xml_bug

I'll do some more digging but thought it would be good to make up a repro without any more python wrapper stuff around it :)

YOU54F commented 1 year ago

So I can now get it consistently passing on the consumer side

Setting only either one of these will allow the mock_server to return XML, every time

However the header is encoded to the pact as "content-type": ", application/xml" due to using an index of 1.

Setting it to 0 causes the test to fail

    lib.pactffi_with_header(interaction_handle, 1,b'content-type', 1, content_type.encode('ascii'))
    # lib.pactffi_with_header(interaction_handle, 1,b'Content-Type', 1, content_type.encode('ascii'))

Pact file

{
  "consumer": {
    "name": "consumer"
  },
  "interactions": [
    {
      "description": "a request for projects in XML",
      "providerStates": [
        {
          "name": "i have a list of projects"
        }
      ],
      "request": {
        "headers": {
          "Accept": "application/xml"
        },
        "method": "GET",
        "path": "/projects"
      },
      "response": {
        "body": "<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n        <projects>\n        <item>\n        <id>1</id>\n        <tasks>\n            <item>\n                <id>1</id>\n                <name>Do the laundry</name>\n                <done>true</done>\n            </item>\n            <item>\n                <id>2</id>\n                <name>Do the dishes</name>\n                <done>false</done>\n            </item>\n            <item>\n                <id>3</id>\n                <name>Do the backyard</name>\n                <done>false</done>\n            </item>\n            <item>\n                <id>4</id>\n                <name>Do nothing</name>\n                <done>false</done>\n            </item>\n        </tasks>\n        </item>\n        </projects>",
        "headers": {
          "content-type": ", application/xml"
        },
        "status": 200
      }
    }
  ],
  "metadata": {
    "pact-python": {
      "version": "1.0.0"
    },
    "pactRust": {
      "ffi": "0.4.7",
      "mockserver": "1.2.3",
      "models": "1.1.9"
    },
    "pactSpecification": {
      "version": "3.0.0"
    }
  },
  "provider": {
    "name": "provider"
  }
}

Passing CI run

https://cirrus-ci.com/build/5375617600847872

Failing CI run

https://cirrus-ci.com/build/5721911821336576

rholshausen commented 1 year ago

Looks like it might be an issue with setting up the body. I am seeing in the logs

2023-08-07T05:43:04.082802Z DEBUG tokio-runtime-worker pact_mock_server::hyper_server: 
          ----------------------------------------------------------------------------------------
           status: 200
           headers: Some({"Content-Type": ["application/xml"]})
           body: Present(0 bytes, application/xml)
          ----------------------------------------------------------------------------------------

You can see the body is 0 bytes.

As an aside, I have updated pactffi_with_header_v2 to deal with different case keys, so if people do silly things like:

lib.pactffi_with_header_v2(interaction_handle, 1,b'Content-Type', 0, content_type.encode('ascii'))
lib.pactffi_with_header_v2(interaction_handle, 1,b'content-type', 1, content_type.encode('ascii'))

they now get more sensible results

         ----------------------------------------------------------------------------------------
           status: 200
           headers: Some({"Content-Type": ["application/xml", "application/xml"]})
           body: Present(0 bytes, application/xml)
          ----------------------------------------------------------------------------------------

Note the Content-Type header, that is what happens if you give different values for the index. image

I have also added a pactffi_set_header function for the simple case where you would just want to set KEY=VALUE, like with the Content-Type header.

rholshausen commented 1 year ago

I think the non-determinism was being caused by a combination of the empty body as well as the multiple content type headers with different case. After the change, it looks like the test always gives the same result (for me at least).

rholshausen commented 1 year ago

Hmm, looking into the XML support, it seems the code is expecting the input to be in JSON (the intermediate format) and then it generates XML off of that.

Huh! Looks like for once, you can't blame this on me :-D image

rholshausen commented 1 year ago

Ok, I've updated the pactffi_with_body function to sniff at the contents when the content type is XML, and if it is JSON process it as it currently does, otherwise just set the raw body. And then:

2023-08-07T06:31:58.537594Z DEBUG tokio-runtime-worker pact_mock_server::hyper_server: 
          ----------------------------------------------------------------------------------------
           status: 200
           headers: Some({"Content-Type": ["application/xml"]})
           body: Present(627 bytes)
          ----------------------------------------------------------------------------------------

Pact - Got matching client requests: True
Writing pact file to ./pacts
Client request - matched: <?xml version="1.0" encoding="UTF-8"?>
    <projects>
    <item>
    <id>1</id>
    <tasks>
        <item>
            <id>1</id>
            <name>Do the laundry</name>
            <done>true</done>
        </item>
        <item>
            <id>2</id>
            <name>Do the dishes</name>
            <done>false</done>
        </item>
        <item>
            <id>3</id>
            <name>Do the backyard</name>
            <done>false</done>
        </item>
        <item>
            <id>4</id>
            <name>Do nothing</name>
            <done>false</done>
        </item>
    </tasks>
    </item>
    </projects>
Client response - matched: <?xml version="1.0" encoding="UTF-8"?>
    <projects>
    <item>
    <id>1</id>
    <tasks>
        <item>
            <id>1</id>
            <name>Do the laundry</name>
            <done>true</done>
        </item>
        <item>
            <id>2</id>
            <name>Do the dishes</name>
            <done>false</done>
        </item>
        <item>
            <id>3</id>
            <name>Do the backyard</name>
            <done>false</done>
        </item>
        <item>
            <id>4</id>
            <name>Do nothing</name>
            <done>false</done>
        </item>
    </tasks>
    </item>
    </projects>
Client response - matched: True
mefellows commented 1 year ago

Huh! Looks like for once, you can't blame this on me :-D

If you like, I can try?

YOU54F commented 1 year ago

As an aside, I have updated pactffi_with_header_v2 to deal with different case keys, so if people do silly things like:

๐Ÿ˜… nice one! that helps!

ahh do love a good git blame, cheers for the investigation and fix

YOU54F commented 1 year ago

Initial tests look really good Ron thank you! Might have found something nice we could address about the mismatch header index. (built it manually from the repo ๐Ÿ‘๐Ÿพ )

As an aside, I have updated pactffi_with_header_v2 to deal with different case keys, so if people do silly things like:

lib.pactffi_with_header_v2(interaction_handle, 1,b'Content-Type', 0, content_type.encode('ascii'))
lib.pactffi_with_header_v2(interaction_handle, 1,b'content-type', 1, content_type.encode('ascii'))

they now get more sensible results

         ----------------------------------------------------------------------------------------
           status: 200
           headers: Some({"Content-Type": ["application/xml", "application/xml"]})
           body: Present(0 bytes, application/xml)
          ----------------------------------------------------------------------------------------

Note the Content-Type header, that is what happens if you give different values for the index. image

I have also added a pactffi_set_header function for the simple case where you would just want to set KEY=VALUE, like with the Content-Type header.

So hopefully we wouldn't do this now, but if we did this and sent the pact for verification with the following

        "headers": {
          "Content-Type": "application/xml, application/xml"
        },

Should this error message be telling us the index at which the value was not found. Otherwise I have to check the test description/pact or pact-core logs

Expected header 'Content-Type'[1] to have value 'application/xml' but was ''

This gives me a bit of a clue as to w

  a request for projects in XML (0s loading, 397ms verification)
     Given i have a list of projects
    returns a response which
      has status code 200 (OK)
      includes headers
        "Content-Type" with value "application/xml, application/xml" (FAILED)
      has a matching body (OK)

screenshot

Screenshot 2023-08-07 at 12 45 28
rholshausen commented 1 year ago

Hmm, I thought I dealt with issues matching multiple header values. Maybe that's in Pact-JVM.

rholshausen commented 1 year ago

Ah, it does: https://github.com/pact-foundation/pact-reference/blob/master/rust/pact_matching/src/headers.rs#L96

Except content-type is a special header, so is compared differently.