Fix param $refs

[drwpow]

Changes

Fixes #984

How to Review

How can a reviewer review your changes? What should be kept in mind for this review?

Checklist

• Unit tests updated
• README updated
• examples/ directory updated (if applicable)

[wolfy1339]

Hi @drwpow,
Do you have an update for this PR?
It's currently blocking octokit/octokit-next.js#104

Thanks

[jhunterkohler]

Are there any updates? I know @wolfy1339 already asked, but this undermines a lot of use cases for this project, its a major bug. Thanks again.

[drwpow]

@HunterKohler Will pick this back up again this week. I’m still looking for help with maintainers (#961) and only have limited capacity to work on it. But I think this can be finished and shipped soon.

[AplusKminus]

Hi @drwpow,

since this bug affects us as well, I looked at the code and the PR to try and figure out the problem. I will try to explain my findings:

The Goal

Given for example this API specification:

openapi: 3.1.0
info:
  version: 1.2.3
  title: Example Service
paths:
  /data/{foo}:
    get:
      parameters:
        - $ref: '#/components/parameters/foo'
        - $ref: '#/components/parameters/bar'
      responses:
        default:
          description: A response
          content:
            text/plain:
              schema:
                type: string
components:
  parameters: 
    foo:
      name: foo
      schema:
        type: string
      in: path
      required: true
    bar:
      name: bar
      schema:
        type: string
      in: query
      required: true

The expected output should be, as far as I understand, the following (shortened and ignoring formatting):

export interface paths {
  "/data/{foo}": {
    get: {
      parameters: {
        path: {
          foo: components["parameters"]["foo"];
        }
        query: {
          bar: components["parameters"]["bar"];
        }
      }
    };
  };
}

export interface components {
  parameters: {
    foo: string;
    bar: string;
  };
}

What is actually generated (again shortened for clarity):

export interface paths {
  "/data/{foo}": {
    get: {
    };
  };
}

export interface components {
  parameters: {
    foo: string;
    bar: string;
  };
}

The parameters are entirely missing from the operation (GET /data/{foo}).

The Current Implementation

Debugging into operation-object.ts#67 for this example shows parts to be ["components", "parameters", "foo"]. Since parts[paramI + 2] is undefined (paramI is the index of the word "parameters", i.e. 1), in line 67 the continue; will be executed. This will skip the parameter being registered in the operation object and thus leads to the case above.

The Difficulty of the Problem

Before I can explain why the PR doesn't yet fix the problem, I first have to explain why this particular case of resolving $ref is so difficult. Operation parameters can exist in one of four locations according to the spec (path, query, header, cookie). This location information is specified within the Parameter Object in its in field (see here). The location cannot be deduced from the $ref URI alone.

The Proposed Solution in this PR

This PR proposes to replace the line referenced before with:

const partsParamIn = parts.find((p) => p === "query" || p === "header" || p === "path" || p === "cookie");
if (paramI === -1 || (partsParamIn && partsParamIn !== paramIn)) continue;

This is an attempt to match the parameter currently examined (p) with the correct counterpart via its location. However, this is not possible, as the parts array does not and cannot (as currently implemented) contain the location. The information sought after (i.e. "query", "path", etc) is only contained in the referenced object, not in the reference. The paths object is, however, only constructed from the $ref itself, i.e. ["components", "parameters", "foo"].

So as the PR currently stands, the if statement will be true for both example parameters (because partsParamIn is always undefined) and thus the execution continues beyond these lines. The result from the change will be (shortened for clarity):

export interface paths {
  "/data/{foo}": {
    get: {
      parameters: {
        query: Pick<NonNullable<components["parameters"]>, "foo" | "bar">;
        header: Pick<NonNullable<components["parameters"]>, "foo" | "bar">;
        path: Pick<components["parameters"], "foo" | "bar">;
        cookie: Pick<NonNullable<components["parameters"]>, "foo" | "bar">;
      };
    };
  };
}

export interface components {
  parameters: {
    foo: string;
    bar: string;
  };
}

This now picks all ref'd parameters for all locations.

A somewhat unrelated problem can also be identified from this:

The ternary operator in operation-object.ts#L92 needs to be swapped, since path parameters must be non-nullable, while others may be nullable. See also my related PR #1053 for optional parameters.

Digging Deeper

Parameter Object Names

I assume it is an intended feature of openapi-typescript to provide convenient objects for parameters.query, .path and so on.
So the challenge faced here is, how to match a specific parameter with its location when resolving a $ref.

It seems important to mention (I wasn't aware of this before experimenting), that the parameter reference is not resolved by
parameter name, but rather by parameter object name.
So the following API spec is functionally identical to the one above:

openapi: 3.1.0
info:
  version: 1.2.3
  title: Example Service
paths:
  /data/{foo}:
    get:
      parameters:
        - $ref: '#/components/parameters/foo-object-name'
        - $ref: '#/components/parameters/bar'
      responses:
        default:
          description: A response
          content:
            text/plain:
              schema:
                type: string
components:
  parameters:
    foo-object-name:
      name: foo
      schema:
        type: string
      in: path
      required: true
    bar:
      name: bar
      schema:
        type: string
      in: query
      required: true

This generates (shortened):

export interface paths {
  "/data/{foo}": {
    get: {
      parameters: {
        query: Pick<NonNullable<components["parameters"]>, "foo-object-name" | "bar">;
        header: Pick<NonNullable<components["parameters"]>, "foo-object-name" | "bar">;
        path: Pick<components["parameters"], "foo-object-name" | "bar">;
        cookie: Pick<NonNullable<components["parameters"]>, "foo-object-name" | "bar">;
      };
    };
  };
}

export interface components {
  parameters: {
    "foo-object-name": string;
    bar: string;
  };
}

Since a user of the API would have to fill a parameter with the name foo, but the generated model would only have a parameter named foo-object-name, the resulting code would not meet the API spec.

Parameter References Including the Location

When the (wrong) assumption of the proposed code change in this PR is fulfilled by doing this:

paths:
  /data/{foo}:
    get:
      parameters:
        - $ref: '#/components/parameters/path/foo'
        - $ref: '#/components/parameters/query/bar'

This is wrong since the components/parameters object of the spec does not allow a location object like path inside of it.

The following code is generated from the modified spec:

export interface paths {
  "/data/{foo}": {
    get: {
      parameters: {
        query: Pick<NonNullable<components["parameters"]["query"]>, "bar">;
        path: Pick<components["parameters"]["path"], "foo">;
      };
    };
  };
}

export interface components {
  parameters: {
    foo: string;
    bar: string;
  };
}

The selector components["parameters"]["query"] shows the idea of generating location-specific sub-objects in the component["parameters"].
This idea aligns well with the discovery from above that parameter names and parameter object names do not need to match.

The Actually Expected Output

From all this I deduct that the actually expected output should rather look something like this:

export interface paths {
  "/data/{foo}": {
    get: {
      parameters: {
        query: Pick<components["parameters"]["query"], "bar">;
        path: Pick<components["parameters"]["path"], "foo">;
      };
      responses: {
        default: {
          content: {
            "text/plain": string;
          };
        };
      };
    };
  };
}

export interface components {
  parameters: {
    path: {
      foo: string;
    }
    query: {
      bar: string;
    }
  };
}

Summary

1. Parameters are uniquely identified by their name+location (e.g. foo in path).
2. Parameter $ref strings do not contain the parameter location.
3. The parameter location is only contained in the referenced component's in field.
4. Parameter $ref strings do not contain the parameter name, but rather the parameter object name.
5. Parameter names do not have to match parameter object names.
6. Parameters (in components) should be generated by parameter name.
7. Parameters (in components) should be generated in a location-specific enclosing object.
8. Parameters (in an operation) should be referenced via the corresponding selector, using the parameter's location and name.
9. The NonNullable is not needed as path parameters MUST be required: true.

I really hope my insights help you.