Skip to content

Parameters defined on an entire path are not included in operation parameters #527

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

Closed
henhal opened this issue Mar 18, 2021 · 7 comments
Closed

Comments

@henhal
Copy link
Contributor

henhal commented Mar 18, 2021

If a document has common parameters defined outside of the method->operation map, those parameters are not included in the operations types.

Example paths with some common and some local parameters:

paths:
  /foo/{id}:
    parameters:
      - in: path
        name: id
        schema:
          type: string
        required: true
      - in: query
        name: x
        schema:
          type: string
        required: false
    get:
      operationId: getFoo
      ...
    put:
      operationId: putFoo
      parameters:
        - in: query
          name: y
          schema:
            type: number
          required: false
      ...

This currently results in:

export interface paths {
  "/foo/{id}": {
    get: operations["getFoo"];
    put: operations["putFoo"];
    parameters: {
      path: {
        id: string;
      };
      query: {
        x?: string;
      };
    }
  }
}

export interface operations {
  getFoo: {
    responses: { ... }
  },
  putFoo: {
    parameters: {
      query: {
        y?: number;
      }
    },
    responses: { ... }
  }
}

For operations to accurately describe the getFoo and putFoo operations, I think parameters that are inherited from the path should be included there as well.

Note that if an operation has both local parameters and common parameters for the entire path, the operations interface will contain the local parameters, but not the common ones. I think it should contain both.

Desired output:

export interface operations {
  getFoo: {
    parameters: {
      path: {
        id: string;
      };
      query: {
        x?: string;
      };
    };
    responses: { ... };
  },
  putFoo: {
    parameters: {
      path: {
        id: string;
      };
      query: {
        x?: string;
        y?: number;
      };
    };
    responses: { ... };
  }
}

Thoughts about this?

It would be nice to be able to reference paths["/foo/{id}"]["parameters"] to merge the common and local parameters, but that would require deep merging two types, and also conditionally dealing with no common parameters existing etc, so while it is probably possible to use a recursive DeepMerge utility type, perhaps copying the parameters statically is simpler/preferable?

Background: I'm using openapi-typescript from my library https://www.npmjs.com/package/openapi-ts-backend, which generates types and adds an additional layer of types to map to requests/operations, for a fully typed OpenAPI backend implementation with very little effort. The fact that each operations["..."] type is incomplete breaks this though. I could fix it on my side during code generation, but it just seems to make sense to me that openapi-typescript would also provide all parameters for an operation?
Ref: henhal/openapi-ts-backend#3

@drwpow
Copy link
Contributor

drwpow commented Mar 18, 2021

Thanks for using the library!

The operations object is a convenience method; it only kicks in if a operationId is specified. Those are supposed to be unique within a schema, so we can pull those out.

However, we can’t pull out parameters from local methods into a global object because we can’t guarantee there won’t be conflicts across local methods. We don’t want to impose restrictions on your schema, and doing so would break type generation for many people.

If, however, you want to reuse parameters and make them global, I’d recommend putting them instead in components and using $refs within local methods.

@drwpow
Copy link
Contributor

drwpow commented Mar 18, 2021

Actually the more I think about it, you’re right—even though those operations have an ID, they’re only defined in one place. So maybe a conflict is not a problem like I originally thought. And also to your generated code, we aren’t changing the existing structure; we’re just adding more properties.

@drwpow
Copy link
Contributor

drwpow commented Mar 18, 2021

OK. I think I’m on board with this!

@henhal
Copy link
Contributor Author

henhal commented Mar 18, 2021

Thanks, I was just about to reply to your first comment when you beat me to it. :)

I didn't really understand what you mean with "pull out parameters from local methods into a global object"; what I'm talking about is to copy parameters from the common section of a path into each operation under that path.

If there were to be conflicts there such as a parameter being declared with one schema in the common part and another schema in the operation part it would probably be ambiguous in the original definition as well; however I would suspect that more local should have preference (that's the way it works with other overridden parameters such as security, which can be defined globally and overriden per operation). In other words, a "deep merge" of parameters from the operation's parent path object and the operation's own parameters should work?

Example where there's a conflict of query.x, it's an optional string in the path, but a mandatory number in the putFoo operation under that path.

paths:
  /foo/{id}:
    parameters:
      - in: path
        name: id
        schema:
          type: string
        required: true
      - in: query
        name: x
        schema:
          type: string
        required: false
    put:
      operationId: putFoo
      parameters:
        - in: query
          name: x
          schema:
            type: number
          required: true
        - in: query
          name: y
          schema:
            type: number
          required: false
      ...

This is of course an edge case, but I'd say the correct way here is likely to merge the two parameters sections with the operation having priority (unless the document should be considered invalid with such a conflict):

parameters: {
  path: {
    id: string;
  },
  query: {
    x: number;
    y?: number;
  }
}

@henhal
Copy link
Contributor Author

henhal commented Mar 18, 2021

Another thought: Strictly with my own library in mind, I'm not dependent on inherited parameters being included in operations that don't have any operationId defined, i.e., operation objects inlined under the path object. But for consistency, perhaps that's also best?

I also want to mention something else I thought of: if you're hesitant to the idea of merging inherited parameters, operations could have a path pointing to their parent path object. That might be sufficient for my library, but I would have to perform the deep merge on my end.

Personally I think it would be of general value to inherit parameters though, but just wanted to mention it. I'm not sure if your ambition with these types is for them to be a strict "mirror" of an OpenAPI spec with no inherited data resolved, or if it should aim to be "cross-complete" with data relations such as inherited data being resolved, which is very useful for use-cases like my own - validating requests and creating typed requests/responses. If you don't want to copy inherited data, adding a path reference from operations would at least make it possible for users of your library to find inherited parameters in the path object.

@henhal
Copy link
Contributor Author

henhal commented Mar 18, 2021

@drwpow I whipped up a PR for this: #530. Please let me know what you think. :)

@drwpow
Copy link
Contributor

drwpow commented Mar 24, 2021

Released in v3.2.0! 🎉 Thanks again.

@drwpow drwpow closed this as completed Mar 24, 2021
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

2 participants