Skip to content

Fix nested length-constrained array generation #2330

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Jamie5
Copy link

@Jamie5 Jamie5 commented May 21, 2025

Changes

This fixes a bug when doing nested length-constrained array generation, specifically the situation added in options > arrayLength: true > minItems: 1, maxItems: 1; minItems: 1, maxItems: 1. Without the change, the code would generate the following (note the extra layer of nesting):

[
    [
        string
    ][]
]

What went wrong in the old code? I think the main issue is that itemType could either be the actual item type, or the type of the entire array (in the case of a tuple). In the problematic nesting situation, in the outer nesting, we run the code itemType = ts.factory.createArrayTypeNode(transformSchemaObject(schemaObject.items, options));. The call to transformSchemaObject returns [string], and then createArrayTypeNode turns it into [string][]. Finally, we go through the logic of arrayLength, which wraps that double nested array again as a singleton array, to become a triply-nested array.

This also likely fixes a situation where length-constrained arrays were not being declared as readonly when the immutable flag was set - notice the old code early-returns when computing length-constrained arrays.

The obvious first change is to stop doing itemType = ts.factory.createArrayTypeNode(transformSchemaObject(schemaObject.items, options)); completely and always just do itemType = transformSchemaObject(schemaObject.items, options);, but this runs into a problem with basic nested arrays. In the basic nested array case, in the inner run itemType is string[], which makes sense, and then we would like to in the outer run turn that it into string[][]. However, this does not happen - when computing finalType, we will skip the array-wrapping in this case, as itemType is indeed an array (it is an array because the type of the item is a nested array, but we do not know that).

Fundamentally, I think the problem is that sometimes itemType is the type of a single item in the array, and sometimes it is the entire array, and we cannot simply check whether itemType is an array/tuple or not to detect that, so in the length-unconstrained array case we added a hack that then caused problems with the nested length-constrained array case.

Hence, this diff changes things so that itemType is truly the type of the item, and hence is applicable only with arrays. Instead, we have arrayType, which is the type of the array/tuple before potentially applying immutability. In this flow of logic, for tuple types, we simply create the tuple and stop there in the arrayType computation. It is only for arrays that we compute the itemType, which is truly the type of a single item in the array. Then, for length-constrained arrays we use that itemType to generate the array, whiule for length-unconstrained arrays we simply make an array of itemType.

How to Review

The main thing is - are there additional missing tests, which would catch issues overlooked in this change? Also, the change in logic should hopefully be clearer in addition to fixing this specific problem.

(The main code is also easier to review when hiding whitespace)

Checklist

  • Unit tests updated
  • [N/A] docs/ updated (if necessary)
  • pnpm run update:examples run (only applicable for openapi-typescript)

@Jamie5 Jamie5 requested a review from a team as a code owner May 21, 2025 22:28
@Jamie5 Jamie5 requested a review from duncanbeevers May 21, 2025 22:28
Copy link

netlify bot commented May 21, 2025

👷 Deploy request for openapi-ts pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit 9749add

Copy link

changeset-bot bot commented May 21, 2025

⚠️ No Changeset found

Latest commit: 9749add

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@Jamie5 Jamie5 force-pushed the array branch 2 times, most recently from fafb525 to 19be4e4 Compare May 21, 2025 22:33
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant