hashicorp / terraform-plugin-codegen-openapi

OpenAPI to Terraform Provider Code Generation Specification
Mozilla Public License 2.0
50 stars 9 forks source link

Consider how best to handle array response mapping (like data source query endpoints) #16

Closed austinvalle closed 10 months ago

austinvalle commented 1 year ago

Problem

A great example of this problem can be found in the petstore OAS, GET /pet/findByStatus

{
    // ... Rest of OAS
    "/pet/findByStatus": {
        "get": {
            "tags": [
                "pet"
            ],
            "summary": "Finds Pets by status",
            "description": "Multiple status values can be provided with comma separated strings",
            "operationId": "findPetsByStatus",
            "parameters": [
                {
                    "name": "status",
                    "in": "query",
                    "description": "Status values that need to be considered for filter",
                    "required": false,
                    "explode": true,
                    "schema": {
                        "type": "string",
                        "default": "available",
                        "enum": [
                            "available",
                            "pending",
                            "sold"
                        ]
                    }
                }
            ],
            "responses": {
                "200": {
                    "description": "successful operation",
                    "content": {
                        "application/json": {
                            "schema": {
                                "type": "array",
                                "items": {
                                    "$ref": "#/components/schemas/Pet"
                                }
                            }
                        }
                    }
                }
            }
        }
    }
}

Generator config:

provider:
  name: petstore

data_sources:
  pets:
    read:
      path: /pet/findByStatus
      method: GET

The response schema #/components/schemas/Pet is not picked up because currently, the mapping of resources and datasources always assumes the top level JSON schema is type: object. So you end up with a framework IR that doesn't have the response body mapping at all 😞

IR output:

{
    "datasources": [
        {
            "name": "pets",
            "schema": {
                "attributes": [
                    {
                        "name": "status",
                        "string": {
                            "computed_optional_required": "computed_optional",
                            "description": "Status values that need to be considered for filter",
                            "sensitive": false
                        }
                    }
                ]
            }
        }
    ],
    "provider": {
        "name": "petstore"
    }
}

Solution

I'm not sure the best way to represent this in a TF Schema, but I would guess that we might want to end up with a list nested attribute in some way? One problem will be that the array here doesn't have a "name" as it's not a property, so maybe we could default to something like responses?

Potential IR output

{
    "datasources": [
        {
            "name": "pets",
            "schema": {
                "attributes": [
                    {
                        "name": "status",
                        "string": {
                            "computed_optional_required": "computed_optional",
                            "description": "Status values that need to be considered for filter",
                            "sensitive": false
                        }
                    }
                    {
                        // Defaulted name?
                        "name": "responses",
                        "list_nested": {
                            "computed_optional_required": "computed_optional",
                            "nested_object": {
                                "attributes": [
                                    // ... mapping of #/components/schemas/Pet
                                ]
                            }
                        }
                    }
                ]
            }
        }
    ],
    "provider": {
        "name": "petstore"
    }
}
bflad commented 1 year ago

I think your proposed solution here is on the right track -- returning the array of objects as a list/set of objects -- best represented in the schema as a list/set nested attribute. Naming the collection I think we could bikeshed between some "standard" attribute name, e.g. responses or results, or potentially using the suffix of the data source name to hint at the attribute naming, e.g. xxx_pets using a pets attribute name.

austinvalle commented 1 year ago

What do we think about resources/future provider schemas? Should they follow the same logic of:

Request body's can technically be arrays 😆

bflad commented 1 year ago

Sorry I feel like I'm being very dense, what does an array of request bodies actually mean in practice? Do you have an example handy?

austinvalle commented 1 year ago

No worries, the actual use-case is obtuse and hopefully not common. This is a technically valid, but weird API example:

{
    "/pet": {
        "post": {
            "summary": "Add new pets to the store",
            "description": "Add new pets to the store",
            "operationId": "addPets",
            "requestBody": {
                "description": "Create new pets in the store",
                "content": {
                    "application/json": {
                        "schema": {
                            "type": "array",
                            "items": {
                                "$ref": "#/components/schemas/Pet"
                            }
                        }
                    }
                },
                "required": true
            },
            "responses": {
                "200": {
                    "description": "Successful operation",
                    "content": {
                        "application/json": {
                            "schema": {
                                "type": "array",
                                "items": {
                                    "$ref": "#/components/schemas/Pet"
                                }
                            }
                        }
                    }
                }
            }
        }
    }
}

That would be an array of Pet objects, but you could "technically" have an array of primitives too:

{
    "/pet": {
        "post": {
            "summary": "Add new strings to something?",
            "description": "Add new strings to something?",
            "operationId": "addStrings",
            "requestBody": {
                "description": "Create new strings",
                "content": {
                    "application/json": {
                        "schema": {
                            "type": "array",
                            "items": {
                                "type": "string"
                            }
                        }
                    }
                },
                "required": true
            },
            "responses": {
                "200": {
                    "description": "Successful operation",
                    "content": {
                        "application/json": {
                            "schema": {
                                "type": "array",
                                "items": {
                                    "type": "string"
                                }
                            }
                        }
                    }
                }
            }
        }
    }
}
austinvalle commented 1 year ago

Best use-case I can think of is some type of "bulk" create API? Doesn't seem like a valid use-case really

bflad commented 1 year ago

Terraform provider design suggests that resources should be a 1:1 mapping from Terraform resource to external system "object", so bulk creation feels outside that model. 👍 https://developer.hashicorp.com/terraform/plugin/best-practices/hashicorp-provider-design-principles#resources-should-represent-a-single-api-object

bflad commented 1 year ago

If necessary because an OAS only specifies that bulk creation method, we could consider a "collapse" configuration to drop the array handling to a single object.

austinvalle commented 1 year ago

Sounds good, data source only sounds good to me 👍🏻

austinvalle commented 1 year ago

For future reference, another great "production" example of this issue can also be seen on the GET /gists endpoint of GitHub's REST API:

"/gists": {
  "get": {
    "summary": "List gists for the authenticated user",
    "description": "Lists the authenticated user's gists or if called anonymously, this endpoint returns all public gists:",
    "tags": [
      "gists"
    ],
    "operationId": "gists/list",
    "externalDocs": {
      "description": "API method documentation",
      "url": "https://docs.github.com/rest/reference/gists#list-gists-for-the-authenticated-user"
    },
    "parameters": [
      {
        "$ref": "#/components/parameters/since"
      },
      {
        "$ref": "#/components/parameters/per-page"
      },
      {
        "$ref": "#/components/parameters/page"
      }
    ],
    "responses": {
      "200": {
        "content": {
          "application/json": {
            "schema": {
              "type": "array",
              "items": {
                "$ref": "#/components/schemas/base-gist"
              }
            }
          }
        }
      }
    }
  }
}
austinvalle commented 10 months ago

This issue will be released with v0.2.0 soon 🎉

https://github.com/hashicorp/terraform-plugin-codegen-openapi/milestones

github-actions[bot] commented 3 months ago

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.