From b42b8bc3bb388a89651f3da907d407d91c0e62dc Mon Sep 17 00:00:00 2001 From: Brandur Date: Thu, 5 Apr 2018 12:52:33 -0700 Subject: [PATCH] Reflect an object's primary ID from a path back in the response When requesting a path from `stripe-mock` with an object's primary ID in the URL: ``` GET /v1/charges/ch_idFromURL ``` Previously, we'd return the ID from the bundled fixtures. This patch changes the behavior to try and detect when the path contains an object's primary ID and reflect that back into the respond instead: ``` { "id": "ch_idFromURL", } ``` (Previously, this would have been `ch_123` or whatever the fixtures contained.) We also tweak things in a few other places for consistency. If we find a string value in our generated response with the same value as the ID we replaced, we replace it as well. This allows an embedded resource with an ID field that references its parent to reference it with a sane ID (e.g., the `charge` field on a refund that's embedded in a charge's `refunds` sublist). We also modify URLs in embedded sublists to contain the right ID. e.g., ``` { "url": "/v1/charges/ch_idFromURL" } ``` This is necessary because those URLs are used by our client libraries to determine where they should make calls to (e.g., in `stripe-ruby` calling `charge.refunds.create(...)` uses that `url` values to know where to post). All of this is designed to get our test suite working again with the latest OpenAPI fixtures. In particular, this will allow us to return the right `id` for any deleted object, which will allow `stripe-ruby`'s web mock checks to pass correctly. --- generator.go | 91 ++++++++++++++++++++++++++++++++++++++++------- generator_test.go | 39 +++++++++++++++++--- main_test.go | 21 +++++++++++ server.go | 56 ++++++++++++++++++++++++++--- server_test.go | 40 ++++++++++++++++++--- 5 files changed, 219 insertions(+), 28 deletions(-) diff --git a/generator.go b/generator.go index 02ffe0bc..221b779a 100644 --- a/generator.go +++ b/generator.go @@ -19,8 +19,8 @@ type DataGenerator struct { } // Generate generates a fixture response. -func (g *DataGenerator) Generate(schema *spec.Schema, requestPath string, expansions *ExpansionLevel) (interface{}, error) { - return g.generateInternal(schema, requestPath, expansions, nil, fmt.Sprintf("Responding to %s:\n", requestPath)) +func (g *DataGenerator) Generate(schema *spec.Schema, requestPath string, id *string, expansions *ExpansionLevel) (interface{}, error) { + return g.generateInternal(schema, requestPath, id, nil, true, expansions, nil, fmt.Sprintf("Responding to %s:\n", requestPath)) } // The purpose of this simple wrapper is so that we can write "example = nil" @@ -30,7 +30,7 @@ type Example struct { value interface{} } -func (g *DataGenerator) generateInternal(schema *spec.Schema, requestPath string, expansions *ExpansionLevel, example *Example, context string) (interface{}, error) { +func (g *DataGenerator) generateInternal(schema *spec.Schema, requestPath string, id *string, replacedID *string, doReplaceID bool, expansions *ExpansionLevel, example *Example, context string) (interface{}, error) { // This is a bit of a mess. We don't have an elegant fully-general approach to // generating examples, just a bunch of specific cases that we know how to // handle. If we find ourselves in a situation that doesn't match any of the @@ -70,14 +70,16 @@ func (g *DataGenerator) generateInternal(schema *spec.Schema, requestPath string if expansions != nil { // We're expanding this specific object result, err := g.generateInternal( - schema.XExpansionResources.OneOf[0], requestPath, expansions, nil, + schema.XExpansionResources.OneOf[0], + requestPath, id, replacedID, false, expansions, nil, fmt.Sprintf("%sExpanding optional expandable field:\n", context)) return result, err } else { // We're not expanding this specific object. Our example should be of the // unexpanded form, which is the first branch of the AnyOf result, err := g.generateInternal( - schema.AnyOf[0], requestPath, expansions, example, + schema.AnyOf[0], + requestPath, id, replacedID, false, expansions, example, fmt.Sprintf("%sNot expanding optional expandable field:\n", context)) return result, err } @@ -91,7 +93,8 @@ func (g *DataGenerator) generateInternal(schema *spec.Schema, requestPath string } else { // Since there's only one subschema, we can confidently recurse into it result, err := g.generateInternal( - schema.AnyOf[0], requestPath, expansions, example, + schema.AnyOf[0], + requestPath, id, replacedID, doReplaceID, expansions, example, fmt.Sprintf("%sChoosing only branch of anyOf:\n", context)) return result, err } @@ -102,7 +105,8 @@ func (g *DataGenerator) generateInternal(schema *spec.Schema, requestPath string // in any example, even if we have an example available, because we don't // know which branch of the AnyOf the example corresponds to. result, err := g.generateInternal( - schema.AnyOf[0], requestPath, expansions, nil, + schema.AnyOf[0], + requestPath, id, replacedID, doReplaceID, expansions, nil, fmt.Sprintf("%sChoosing first branch of anyOf:\n", context)) return result, err } @@ -111,7 +115,7 @@ func (g *DataGenerator) generateInternal(schema *spec.Schema, requestPath string // We special-case list resources and always fill in the list with at least // one item of data, regardless of what was present in the example listData, err := g.generateListResource( - schema, requestPath, expansions, example, context) + schema, requestPath, id, replacedID, expansions, example, context) return listData, err } @@ -129,6 +133,20 @@ func (g *DataGenerator) generateInternal(schema *spec.Schema, requestPath string return nil, nil } + // If we replaced a primary object ID, then also replace any of values that + // we happen to find which had the same value as it. These will usually be + // IDs in child objects that reference the parent. + // + // For example, a charge may contain a sublist of refunds. If we replaced + // the charge's ID, we also want to replace that charge ID in every one of + // the child refunds. + if replacedID != nil && schema.Type == "string" { + valStr, ok := example.value.(string) + if ok && valStr == *replacedID { + example = &Example{value: *id} + } + } + if schema.Type == "boolean" || schema.Type == "integer" || schema.Type == "number" || schema.Type == "string" { return example.value, nil @@ -156,7 +174,38 @@ func (g *DataGenerator) generateInternal(schema *spec.Schema, requestPath string resultMap := make(map[string]interface{}) + // We might have obtained an ID for the object from an extracted path + // parameter. If we did, fill it in. Note that this only occurs at the + // top level of recursion because any ID fields we find at other levels + // are likely for other objects. + // + // If we do replace an ID, extract the old one so that we can inject it + // into list URLs from our fixtures. + // + // This replacement must occur before iterating through the loop below + // because we might also use the new ID to replace other values in the + // object as well. + if doReplaceID && id != nil { + _, ok := schema.Properties["id"] + if ok { + idValue, ok := exampleMap["id"] + if ok { + idValueStr := idValue.(string) + replacedID = &idValueStr + resultMap["id"] = *id + + if verbose { + fmt.Printf("Found ID to replace; previous: '%s' new: '%s'\n", *replacedID, *id) + } + } + } + } + for key, subSchema := range schema.Properties { + // If these conditions are met this was handled above. Skip it. + if doReplaceID && key == "id" && replacedID != nil { + continue + } var subExpansions *ExpansionLevel if expansions != nil { @@ -185,7 +234,8 @@ func (g *DataGenerator) generateInternal(schema *spec.Schema, requestPath string } subValue, err := g.generateInternal( - subSchema, requestPath, subExpansions, subExample, + subSchema, + requestPath, id, replacedID, false, subExpansions, subExample, fmt.Sprintf("%sIn property '%s' of object:\n", context, key)) if err != nil { return nil, err @@ -216,7 +266,7 @@ func (g *DataGenerator) maybeDereference(schema *spec.Schema, context string) (* return schema, context, nil } -func (g *DataGenerator) generateListResource(schema *spec.Schema, requestPath string, expansions *ExpansionLevel, example *Example, context string) (interface{}, error) { +func (g *DataGenerator) generateListResource(schema *spec.Schema, requestPath string, id *string, replacedID *string, expansions *ExpansionLevel, example *Example, context string) (interface{}, error) { var itemExpansions *ExpansionLevel if expansions != nil { @@ -226,6 +276,9 @@ func (g *DataGenerator) generateListResource(schema *spec.Schema, requestPath st itemData, err := g.generateInternal( schema.Properties["data"].Items, requestPath, + id, + replacedID, + false, itemExpansions, nil, fmt.Sprintf("%sPopulating list resource:\n", context)) @@ -249,16 +302,28 @@ func (g *DataGenerator) generateListResource(schema *spec.Schema, requestPath st case "total_count": val = 1 case "url": + var url string if strings.HasPrefix(subSchema.Pattern, "^") { // Many list resources have a URL pattern of the form "^/v1/whatevers"; // we cut off the "^" to leave the URL. - val = subSchema.Pattern[1:] + url = subSchema.Pattern[1:] } else if example != nil { // If an example was provided, we can assume it has the correct format example := example.value.(map[string]interface{}) - val = example["url"] + url = example["url"].(string) + } else { + url = requestPath + } + + // Potentially replace a primary ID in the URL of a list so that + // requests against it may be consistent. For example, if + // `/v1/charges/ch_123` was requested, we'd want the refunds list + // within the returned object to have a URL like + // `/v1/charges/ch_123/refunds`. + if replacedID != nil { + val = strings.Replace(url, *replacedID, *id, 1) } else { - val = requestPath + val = url } default: val = nil diff --git a/generator_test.go b/generator_test.go index 31ea67d2..c8c9a99c 100644 --- a/generator_test.go +++ b/generator_test.go @@ -52,7 +52,7 @@ func TestConcurrentAccess(t *testing.T) { go func() { defer wg.Done() _, err := generator.Generate( - &spec.Schema{Ref: "#/components/schemas/subscription"}, "", nil) + &spec.Schema{Ref: "#/components/schemas/subscription"}, "", nil, nil) assert.NoError(t, err) }() } @@ -68,7 +68,7 @@ func TestGenerateResponseData(t *testing.T) { // basic reference generator = DataGenerator{testSpec.Components.Schemas, &testFixtures} data, err = generator.Generate( - &spec.Schema{Ref: "#/components/schemas/charge"}, "", nil) + &spec.Schema{Ref: "#/components/schemas/charge"}, "", nil, nil) assert.Nil(t, err) assert.Equal(t, testFixtures.Resources["charge"].(map[string]interface{})["id"], @@ -84,6 +84,7 @@ func TestGenerateResponseData(t *testing.T) { data, err = generator.Generate( &spec.Schema{Ref: "#/components/schemas/charge"}, "", + nil, &ExpansionLevel{expansions: map[string]*ExpansionLevel{"customer": {expansions: map[string]*ExpansionLevel{}}}}) assert.Nil(t, err) assert.Equal(t, @@ -95,6 +96,7 @@ func TestGenerateResponseData(t *testing.T) { data, err = generator.Generate( &spec.Schema{Ref: "#/components/schemas/charge"}, "", + nil, &ExpansionLevel{expansions: map[string]*ExpansionLevel{"id": {expansions: map[string]*ExpansionLevel{}}}}) assert.Equal(t, err, errExpansionNotSupported) @@ -103,6 +105,7 @@ func TestGenerateResponseData(t *testing.T) { data, err = generator.Generate( &spec.Schema{Ref: "#/components/schemas/charge"}, "", + nil, &ExpansionLevel{expansions: map[string]*ExpansionLevel{"customer.id": {expansions: map[string]*ExpansionLevel{}}}}) assert.Equal(t, err, errExpansionNotSupported) @@ -111,6 +114,7 @@ func TestGenerateResponseData(t *testing.T) { data, err = generator.Generate( &spec.Schema{Ref: "#/components/schemas/charge"}, "", + nil, &ExpansionLevel{wildcard: true}) assert.Nil(t, err) assert.Equal(t, @@ -119,7 +123,7 @@ func TestGenerateResponseData(t *testing.T) { // list generator = DataGenerator{testSpec.Components.Schemas, &testFixtures} - data, err = generator.Generate(listSchema, "/v1/charges", nil) + data, err = generator.Generate(listSchema, "/v1/charges", nil, nil) assert.Nil(t, err) assert.Equal(t, "list", data.(map[string]interface{})["object"]) assert.Equal(t, "/v1/charges", data.(map[string]interface{})["url"]) @@ -148,7 +152,7 @@ func TestGenerateResponseData(t *testing.T) { "charges_list": listSchema, }, XResourceID: "with_charges_list", - }, "", nil) + }, "", nil, nil) assert.Nil(t, err) chargesList := data.(map[string]interface{})["charges_list"] assert.Equal(t, "list", chargesList.(map[string]interface{})["object"]) @@ -156,6 +160,31 @@ func TestGenerateResponseData(t *testing.T) { assert.Equal(t, testFixtures.Resources["charge"].(map[string]interface{})["id"], chargesList.(map[string]interface{})["data"].([]interface{})[0].(map[string]interface{})["id"]) + + // injected ID + generator = DataGenerator{testSpec.Components.Schemas, &spec.Fixtures{ + Resources: map[spec.ResourceID]interface{}{ + spec.ResourceID("charge"): map[string]interface{}{ + // This is contrived, but we inject the value we expect to be + // replaced into `customer` as well so that we can verify the + // secondary behavior that replaces all values that look like a + // replaced ID (as well as the ID). + "customer": "ch_123", + + "id": "ch_123", + }, + }, + }} + id := "ch_123_InjectedFromURL" + data, err = generator.Generate( + &spec.Schema{Ref: "#/components/schemas/charge"}, "", &id, nil) + assert.Nil(t, err) + assert.Equal(t, + id, + data.(map[string]interface{})["id"]) + assert.Equal(t, + id, + data.(map[string]interface{})["customer"]) } func TestValidFixtures(t *testing.T) { @@ -196,7 +225,7 @@ func testCanGenerate(t *testing.T, path spec.Path, schema *spec.Schema, expand b var example interface{} var err error assert.NotPanics(t, func() { - example, err = generator.Generate(schema, string(path), expansions) + example, err = generator.Generate(schema, string(path), nil, expansions) }) assert.NoError(t, err) diff --git a/main_test.go b/main_test.go index 0fd26bc7..c24cb238 100644 --- a/main_test.go +++ b/main_test.go @@ -6,10 +6,13 @@ import ( "github.com/stripe/stripe-mock/spec" ) +var applicationFeeRefundCreateMethod *spec.Operation +var applicationFeeRefundGetMethod *spec.Operation var chargeAllMethod *spec.Operation var chargeCreateMethod *spec.Operation var chargeDeleteMethod *spec.Operation var chargeGetMethod *spec.Operation +var invoicePayMethod *spec.Operation // Try to avoid using the real spec as much as possible because it's more // complicated and slower. A test spec is provided below. If you do use it, @@ -54,6 +57,11 @@ func initRealSpec() { } func initTestSpec() { + // These are basically here to give us a URL to test against that has + // multiple parameters in it. + applicationFeeRefundCreateMethod = &spec.Operation{} + applicationFeeRefundGetMethod = &spec.Operation{} + chargeAllMethod = &spec.Operation{} chargeCreateMethod = &spec.Operation{ RequestBody: &spec.RequestBody{ @@ -85,6 +93,10 @@ func initTestSpec() { chargeDeleteMethod = &spec.Operation{} chargeGetMethod = &spec.Operation{} + // Here so we can test the relatively rare "action" operations (e.g., + // `POST` to `/pay` on an invoice). + invoicePayMethod = &spec.Operation{} + testFixtures = spec.Fixtures{ Resources: map[spec.ResourceID]interface{}{ @@ -127,6 +139,12 @@ func initTestSpec() { }, }, Paths: map[spec.Path]map[spec.HTTPVerb]*spec.Operation{ + spec.Path("/v1/application_fees/{fee}/refunds"): { + "get": applicationFeeRefundCreateMethod, + }, + spec.Path("/v1/application_fees/{fee}/refunds/{id}"): { + "get": applicationFeeRefundGetMethod, + }, spec.Path("/v1/charges"): { "get": chargeAllMethod, "post": chargeCreateMethod, @@ -135,6 +153,9 @@ func initTestSpec() { "get": chargeGetMethod, "delete": chargeDeleteMethod, }, + spec.Path("/v1/invoices/{id}/pay"): { + "post": invoicePayMethod, + }, }, } } diff --git a/server.go b/server.go index ebe6d160..399e8191 100644 --- a/server.go +++ b/server.go @@ -92,6 +92,7 @@ type StubServer struct { // pattern to match an incoming path and a description of the method that would // be executed in the event of a match. type stubServerRoute struct { + endsWithID bool pattern *regexp.Regexp operation *spec.Operation requestBodyValidator *jsval.JSVal @@ -113,7 +114,7 @@ func (s *StubServer) HandleRequest(w http.ResponseWriter, r *http.Request) { // Every response needs a Request-Id header except the invalid authorization w.Header().Set("Request-Id", "req_123") - route := s.routeRequest(r) + route, id := s.routeRequest(r) if route == nil { message := fmt.Sprintf(invalidRoute, r.Method, r.URL.Path) stripeError := createStripeError(typeInvalidRequestError, message) @@ -137,6 +138,7 @@ func (s *StubServer) HandleRequest(w http.ResponseWriter, r *http.Request) { } if verbose { + fmt.Printf("ID extracted from route: %+v\n", id) fmt.Printf("Response schema: %s\n", responseContent.Schema) } @@ -203,6 +205,7 @@ func (s *StubServer) HandleRequest(w http.ResponseWriter, r *http.Request) { responseData, err := generator.Generate( responseContent.Schema, r.URL.Path, + id, expansions) if err != nil { fmt.Printf("Couldn't generate response: %v\n", err) @@ -258,7 +261,18 @@ func (s *StubServer) initializeRouter() error { numValidators++ } + // We use whether the route ends with a parameter as a heuristic as + // to whether we should expect an object's primary ID in the URL. + var endsWithID bool + for _, suffix := range endsWithIDSuffixes { + if strings.HasSuffix(string(path), suffix) { + endsWithID = true + break + } + } + route := stubServerRoute{ + endsWithID: endsWithID, pattern: pathPattern, operation: operation, requestBodyValidator: requestBodyValidator, @@ -277,18 +291,50 @@ func (s *StubServer) initializeRouter() error { return nil } -func (s *StubServer) routeRequest(r *http.Request) *stubServerRoute { +// routeRequest tries to find a matching route for the given request. If +// successful, it returns the matched route and where possible, an extracted ID +// which comes from the last capture group in the URL. An ID is only returned +// if it looks like it's supposed to be the primary identifier of the returned +// object (i.e., the route's pattern ended with a parameter). A nil is returned +// as the second return value when no primary ID is available. +func (s *StubServer) routeRequest(r *http.Request) (*stubServerRoute, *string) { verbRoutes := s.routes[spec.HTTPVerb(r.Method)] for _, route := range verbRoutes { - if route.pattern.MatchString(r.URL.Path) { - return &route + matches := route.pattern.FindAllStringSubmatch(r.URL.Path, -1) + + if len(matches) < 1 { + continue + } + + // There will only ever be a single match in the string (this match + // contains the entire match plus all capture groups). + firstMatch := matches[0] + + // This route doesn't appear to contain the ID of the primary object + // being returned. Return the route only. + if !route.endsWithID { + return &route, nil } + + // Return the route along with the likely ID. + return &route, &firstMatch[len(firstMatch)-1] } - return nil + return nil, nil } // --- +// Suffixes for which we will try to exact an object's ID from the path. +var endsWithIDSuffixes = [...]string{ + // The general case: we're looking for the end of an OpenAPI URL parameter. + "}", + + // These are resource "actions". They don't take the standard form, but we + // can expect an object's primary ID to live right before them in a path. + "/close", + "/pay", +} + func getRequestBodySchema(operation *spec.Operation) *spec.Schema { if operation.RequestBody == nil { return nil diff --git a/server_test.go b/server_test.go index b0dfa1b0..13b3b8cd 100644 --- a/server_test.go +++ b/server_test.go @@ -76,31 +76,61 @@ func TestStubServer_FormatsForCurl(t *testing.T) { func TestStubServer_RoutesRequest(t *testing.T) { server := getStubServer(t) + var id *string var route *stubServerRoute - route = server.routeRequest( + route, id = server.routeRequest( &http.Request{Method: "GET", URL: &url.URL{Path: "/v1/charges"}}) assert.NotNil(t, route) assert.Equal(t, chargeAllMethod, route.operation) + assert.Nil(t, id) - route = server.routeRequest( + route, id = server.routeRequest( &http.Request{Method: "POST", URL: &url.URL{Path: "/v1/charges"}}) assert.NotNil(t, route) assert.Equal(t, chargeCreateMethod, route.operation) + assert.Nil(t, id) - route = server.routeRequest( + route, id = server.routeRequest( &http.Request{Method: "GET", URL: &url.URL{Path: "/v1/charges/ch_123"}}) assert.NotNil(t, route) assert.Equal(t, chargeGetMethod, route.operation) + assert.Equal(t, "ch_123", *id) - route = server.routeRequest( + route, id = server.routeRequest( &http.Request{Method: "DELETE", URL: &url.URL{Path: "/v1/charges/ch_123"}}) assert.NotNil(t, route) assert.Equal(t, chargeDeleteMethod, route.operation) + assert.Equal(t, "ch_123", *id) - route = server.routeRequest( + route, id = server.routeRequest( &http.Request{Method: "GET", URL: &url.URL{Path: "/v1/doesnt-exist"}}) assert.Equal(t, (*stubServerRoute)(nil), route) + assert.Nil(t, id) + + // Route with a parameter, but not an object's primary ID + route, id = server.routeRequest( + &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, "in_123", *id) + + // Route with a parameter, but not an object's primary ID + route, id = server.routeRequest( + &http.Request{Method: "GET", + URL: &url.URL{Path: "/v1/application_fees/fee_123/refunds"}}) + assert.NotNil(t, route) + assert.Equal(t, invoicePayMethod, route.operation) + assert.Nil(t, id) + + // Route with multiple parameters in its URL + route, id = server.routeRequest( + &http.Request{Method: "GET", + URL: &url.URL{Path: "/v1/application_fees/fee_123/refunds/fr_123"}}) + assert.NotNil(t, route) + assert.Equal(t, chargeDeleteMethod, route.operation) + assert.Equal(t, "fr_123", *id) } // ---