hashicorp / terraform-plugin-codegen-openapi

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

Add configuration option to match parameters to root schema attributes #47

Closed austinvalle closed 1 year ago

austinvalle commented 1 year ago

Closes #33

This PR introduces a new generator config option for both resources and data_sources:

resources:
  pet:
    # create, read, update, delete configuration
    # ...
    schema:
      attributes:
        aliases:
          # Match 'petId' parameter to 'id'
          petId: id

data_sources:
  order:
    # read configuration
    # ...
    schema:
      attributes:
        aliases:
          # Match 'orderId' parameter to 'id'
          orderId: id

The implementation of this does introduce some "quirks" that could be solved with naming or by expanding/refactoring the merge functionality (which I chose to punt for this PR).

You'll just change the name of the resulting parameter to be mapped, no errors or combinations are made:

{
    "name": "doesntexist",
    "int64": {
        "computed_optional_required": "required",
        "description": "ID of order that needs to be fetched"
    }
},
{
    "name": "id",
    "int64": {
        "computed_optional_required": "computed"
    }
},

It's a bug not a featureℒ️

bflad commented 1 year ago

I'd love some bikeshedding on the name/structure of the config, my initial idea was to provide merge_options as a section for other tweak/config options in the future, with param_matches being the first.

Since you asked! πŸ˜„ I think it might be worth considering tailoring the configuration naming for the end goal, rather than potentially the internal implementation details. One way you might be able to do this is:

resources:
  pet:
    # create, read, update, delete configuration
    # ...
    schema:
      attributes:
        # or remapping, etc.
        aliases:
          petId: id

I think this has a nice benefit of not "caring" about whether it is a path parameter, request/response schema, etc. for the aliasing/remapping/whatever-we-want-to-call-this. If we take this further, you might envision other "attributes" configurations, such as "overrides":

resources:
  pet:
    # create, read, update, delete configuration
    # ...
    schema:
      description: Manages a pet resource
      attributes:
        aliases:
          petId: id
        overrides:
          id:
            description: My more important description of the identifier attribute

What do you think?


Aside: I'm a little doubtful about this further use case in the real world, but its theoretically possible that individual API operations could need this configuration, if for some awful reason the API definition had conflicting usage of the same name but mapped in two ways. I would hope that underneath the operation, we could also set up this aliasing/remapping/whatever-we-want-to-call-this configuration. I think a similar "attributes" property at that level would hopefully make sense. Let's hope we don't need that though!

austinvalle commented 1 year ago
resources:
  pet:
    # create, read, update, delete configuration
    # ...
    schema:
      description: Manages a pet resource
      attributes:
        aliases:
          petId: id
        overrides:
          id:
            description: My more important description of the identifier attribute

What do you think?

That's a good call out re: "exposing internal implementation details", I like moving towards the end result in our config, so I'll update the PR with that πŸ‘πŸ»

Slightly adjacent tangent

One conversation @bendbennett and I had this morning was around the complexities of mirroring nested attributes in our configuration vs. creating some form of path system for describing how to apply these overrides and aliases (fortunately not a problem now, as the parameters are always in the root).

Any thoughts on which way we should be leaning when the time comes for something like "overriding a description" for a single nested attribute with say, an id field?

Mirroring the nested object

resources:
  pet:
    # create, read, update, delete configuration
    # ...
    schema:
      description: Manages a pet resource
      attributes:
        overrides:
          nested_obj:
            # description of the nested object?
            id:
              description: My more important description of the identifier attribute

Path referencing the id in a nested object

resources:
  pet:
    # create, read, update, delete configuration
    # ...
    schema:
      description: Manages a pet resource
      attributes:
        overrides:
          - name: id
            # path is optional? If not included it's assumed to be in the root
            path: nested_obj.id
            description: My more important description of the identifier attribute
bflad commented 1 year ago

Any thoughts on which way we should be leaning when the time comes for something like "overriding a description" for a single nested attribute with say, an id field?

Great question! I'm guessing the nesting of objects per "attribute path step" in YAML raises the possibility for collisions, e.g. this is ambiguous:

resources:
  pet:
    # create, read, update, delete configuration
    # ...
    schema:
      description: Manages a pet resource
      attributes:
        overrides:
          nested_obj:
            description: Et tu, YAML property?

Is there a description attribute under nested_obj or is that updating the description of the nested attribute itself? You could probably guess based on object versus string value, but that seems awfully messy or hard to explain. It also means that you might not be able to configure a top-level description and a nested description attribute. πŸ˜„

Personal opinion: I think it would be fine to support a synthetic, period-separated string for this purpose, e.g.

resources:
  pet:
    # create, read, update, delete configuration
    # ...
    schema:
      description: Manages a pet resource
      attributes:
        overrides:
          "nested_obj.id":
            description: Then fall id description!

We don't have to worry about differences of collection types, etc, because in terms of an "attribute path" those indices/keys wouldn't exist (e.g. there's no list_nested_attr[0].attr awkwardness to worry about). Then nothing needs to change other than a future feature saying "let's support parsing a period-separated value in those YAML keys now" instead of just top-level attributes.

bendbennett commented 1 year ago

Personal opinion: I think it would be fine to support a synthetic, period-separated string for this purpose, e.g.

resources:
  pet:
    # create, read, update, delete configuration
    # ...
    schema:
      description: Manages a pet resource
      attributes:
        overrides:
          "nested_obj.id":
            description: Then fall id description!

I like this approach as it removes all ambiguity and provides a clear path to the exact attribute field that is being "overridden" πŸ‘

austinvalle commented 1 year ago

I like this approach as it removes all ambiguity and provides a clear path to the exact attribute field that is being "overridden" πŸ‘

Agreed! I will compile the thoughts about overriding descriptions and create an issue for future consideration πŸ‘πŸ»


@bendbennett @bflad - I have made the recommended updates to the config structure in this PR, take a look at let me know what you think!

austinvalle commented 1 year ago

I created #49 to introduce this dot-separated syntax and override functionality πŸ‘πŸ»

github-actions[bot] commented 3 months ago

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