Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Content and Content-Type Specification #3

Open
JP-Ellis opened this issue Nov 9, 2023 · 1 comment
Open

Content and Content-Type Specification #3

JP-Ellis opened this issue Nov 9, 2023 · 1 comment

Comments

@JP-Ellis
Copy link

JP-Ellis commented Nov 9, 2023

Motivation

I have been working on implementing the compatibility suite for Pact Python, and have run into issues with the content/content-type combination.

The below description outlines some of the issues I have come across, and some suggestions as to how it can be resolved. This issue is primarily meant to gather feedback before any work goes into updating the compatibility suite.

Note that this is primarily feedback on the V1/http_consumer.feature file, as that is what I have been working on.

body Column

The interactions are defined at the top of the file with:

Given the following HTTP interactions have been defined:
| No | method | path | query | headers | body | response | response content | response body |
| 1 | GET | /basic | | | | 200 | application/json | file: basic.json |
| 2 | GET | /with_params | a=1&b=2 | | | 200 | | |
| 3 | GET | /with_headers | | 'X-TEST: Compatibility' | | 200 | | |
| 4 | PUT | /basic | | | file: basic.json | 200 | | |
| 5 | PUT | /plain | | | file: text-body.xml | 200 | | |
| 6 | PUT | /xml | | | file: xml-body.xml | 200 | | |
| 7 | PUT | /bin | | | file: rat.jpg | 200 | | |
| 8 | PUT | /form | | | file: form-post-body.xml | 200 | | |
| 9 | PUT | /multipart | | | file: multipart-body.xml | 200 | | |

The body column serves to define the payload to be sent along with a PUT request. It is also used later in the feature file to define changes to the main definition:

| body |
| JSON: {"one": "a", "two": "c"} |

| body |
| Hello Mars! |

| body |
| file: spider.jpg |

After making some mistakes in implementation, I realised that I skipped over the part of the README explaining that the *-body.xml files are special and must be parsed, unlike the other files.

Proposed Change

My first proposal would be to make the above explicit. Specifically, I would suggest the following prefixes:

  • fixture: (new) for a file which contains additional metadata to be parsed
  • file: (modified) for a file which is to be passed as the payload as-is
  • JSON: (unchanged) for a JSON string
  • XML: (unchanged) for an XML string
  • Otherwise, default to a string literal.
    • There is a question here around whitespaces. I would propose that whitespaces be trimmed by default, and also allow for literal strings to be quoted in either ' or " (e.g., | 'some string ' | vs | some string | ).

Request Content Type

The second big question is around the request content type. At the moment, it is either implicitly defined (for example, through trial and error, interaction 4 above is implicitly an application/json), or defined explicitly in the *-body.xml fixtures.

The implicitly defined content type is an issue for a few reasons:

  • The Pact FFI allows for a null pointer to be specified for the content type. For example, with_body states that it defaults to text/plain if a null pointer is passed.
  • Based on a discussion with Ron, the Pact library may also intelligently try and determine the content type.
  • Without an explicit content type, it is unclear what content type should be used when making the HTTP request to the mock server. Some HTTP client libraries perform their own automated content-type detection and add the heading as the library deems appropriate (if not otherwise specified), while some other libraries send the HTTP request with no Content-Type header.
  • There is no guarantee that the Pact library's logic or defaults align with the logic or defaults from the HTTP client library.

Proposed Change

The crux of the proposed change is that I think we need to make the content type explicit and/or clearly defined. I propose the following:

  1. We add a new content type column which would be analogous to the response content column. When specified, this column is the ultimate source of truth and will override any and all other definitions of content type. This explicitly allows this column to override other values, allowing for example:

    | content type     | body                |
    | application/json | XML: <test></hello> |
    
  2. If the body is a file: {value}, then the content type is set based on the extension of the file. The README can be updated with a table to make it clear as there are sometimes multiple (perhaps non-standard) matches to a given extension. As an example, .pdf appears to match:

    • application/acrobat
    • application/pdf
    • application/x-pdf
    • applications/vnd.pdf
    • text/pdf
    • text/x-pdf

    Though application/pdf is, I believe, the most common.

  3. If the body is a fixture: {value}, then the content type must be defined in the fixture.

  4. If the body is one of the other prefixes, then the mapping is:

    Prefix Content Type
    JSON: application/json
    XML: application/xml
    string literal text/plain

This proposed change means that there should be relatively few changes to the existing feature file, while still making it clear what the content type ought to be.

Possible Ambiguity

Based on the above changes, I think there is still one risk of ambiguity which arises when we wish to override an existing definition. Consider for example an interaction defined initially with:

| body               |
| XML: <test></test> |

If a scenario wished to test the passing of an invalid XML string by overriding the body:

| body         |
| <test></test |

there is some ambiguity here whether the override also updates the content type to text/plain (as defined above). Without introducing a lot of other changes to the existing feature file, this would be avoided by simply making use of the new content type column (i.e., avoid the ambiguity by being explicit):

| content type    | body         |
| application/xml | <test></test |

A concrete example of this ambiguity can be seen with the following two scenarios:

Scenario: Request with the incorrect type of body contents
When the mock server is started with interaction 4
And request 4 is made to the mock server with the following changes:
| body |
| XML: <?xml version="1.0" encoding="UTF-8"?><alligator name="Mary" feet="4"/> |
Then a 500 error response is returned
When the pact test is done
Then the mock server status will NOT be OK
And the mock server status will be mismatches
And the mismatches will contain a "body-content-type" mismatch with error "Expected a body of 'application/json' but the actual content type was 'application/xml'"
And the mock server will NOT write out a Pact file for the interaction when done

Scenario: Request with a form post body (negative case)
When the mock server is started with interaction 8
And request 8 is made to the mock server with the following changes:
| body |
| a=1&b=2&c=33&d=4 |
Then a 500 error response is returned
And the mismatches will contain a "body" mismatch with error "Expected form post parameter 'c' with value '3' but was '33'"

In the first scenario, the content type is meant to be updated, which is in contrast to the second scenario whereby the content type is meant to stay the same.

@JP-Ellis
Copy link
Author

JP-Ellis commented Nov 9, 2023

Tagging @rholshausen, @mefellows, @YOU54F and @tienvx for feedback. Let me know what you all think 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant