Allow empty content type on DELETE
#73
Merged
+101
−54
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We recently added checks in `stripe-mock` to make sure that the right `Content-Type` was being sent. Unfortunately, the logic was not quite right for deletes, where you occasionally see a `Content-Type` in rare cases, but which usually have empty request bodies, and therefore an empty `Content-Type` is allowed. This patch brings in two fixes: 1. The OpenAPI spec has been tweak so as to not incorrectly include `expand` parameters on `DELETE` operations. This means that most `DELETE` operations have no defined request schema at all. 2. Special cases incoming `DELETE` operations so that they can be checked against a schema if they provide parameters, but are also allowed to specify an empty `Content-Type` as well. Between the two improvements, all edges should be covered. The impetus here is to resolve [1] where a `stripe-mock` upgrade is incompatible with the current code. I've verified that with this patch included, `stripe-python`'s test suite runs successfully. [1] stripe/stripe-python#426
brandur-stripe
commented
May 10, 2018
| @@ -83,14 +83,26 @@ func initTestSpec() { | |||
| Content: map[string]spec.MediaType{ | |||
| "application/json": { | |||
| Schema: &spec.Schema{ | |||
| Ref: "#/components/schemas/customer", | |||
| Ref: "#/components/schemas/charge", | |||
There was a problem hiding this comment.
This tweaks is an unrelated fix.
brandur-stripe
commented
May 10, 2018
| // Firstly, `Content-Type` is checked against the schema's media type, then | ||
| // string-encoded parameters are coerced to expected types (where possible). | ||
| // Finally, we validate the incoming payload against the schema. | ||
| func validateAndCoerceRequest( |
There was a problem hiding this comment.
Most of this code is the same, but for cleanliness I've pulled it into its own function.
brandur-stripe
commented
May 10, 2018
| @@ -178,7 +186,7 @@ func TestStubServer_RoutesRequest(t *testing.T) { | |||
| &http.Request{Method: "POST", | |||
| URL: &url.URL{Path: "/v1/invoices/in_123/pay"}}) | |||
| assert.NotNil(t, route) | |||
| assert.Equal(t, chargeDeleteMethod, route.operation) | |||
| assert.Equal(t, invoicePayMethod, route.operation) | |||
There was a problem hiding this comment.
My tweaks to the test fixtures revealed that these were previously wrong, so they've also been fixed.
|
Going to self-approve this one so we can start closing out these upgrades: LGTM. |
brandur-stripe
pushed a commit
to stripe/stripe-python
that referenced
this pull request
May 10, 2018
See the fix here: stripe/stripe-mock#73
brandur-stripe
pushed a commit
to stripe/stripe-python
that referenced
this pull request
May 10, 2018
See the fix here: stripe/stripe-mock#73
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We recently added checks in
stripe-mockto make sure that the rightContent-Typewas being sent. Unfortunately, the logic was not quiteright for deletes, where you occasionally see a
Content-Typein rarecases, but which usually have empty request bodies, and therefore an
empty
Content-Typeis allowed.This patch brings in two fixes:
expandparameters onDELETEoperations. This means that mostDELETEoperations have no defined request schema at all.DELETEoperations so that they can bechecked against a schema if they provide parameters, but are also
allowed to specify an empty
Content-Typeas well.Between the two improvements, all edges should be covered.
The impetus here is to resolve [1] where a
stripe-mockupgrade isincompatible with the current code. I've verified that with this patch
included,
stripe-python's test suite runs successfully.[1] stripe/stripe-python#426