Skip to content
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

feat: Add support for handling refs within OpenAPI path items #276

Merged
merged 1 commit into from
Dec 27, 2023

Conversation

DerDackel
Copy link
Contributor

@DerDackel DerDackel commented Dec 15, 2023

  • Please check if the PR fulfills these requirements
  • The commit message describes your change
  • Tests for the changes have been added if possible (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)
  • Changes are mentioned in the changelog (for bug fixes / features)
  • What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

Fixes a bug due to a recently addressed gap in the OpenAPI spec implementation of openapi-typescript where refs to path items within the paths section of the API defnition weren't supported. This resulted in a type signature change on openapi-typescripts PathsObject that broke the RestApi projen aspect.

I took the liberty of adding some error handling for processing malformed OpenAPI definitions (read below), so some definitions where older versions of cdk-serverless would have ignored e.g. invalid refs will now throw an error upon project synthesiziation.

  • What is the current behavior? (You can also link to an open issue here)

Project won't compile with recent versions of openapi-typescript due to a type signature change in the dependency.
Current behavior would alreadys have guarded against accidentally creating superfluous lambdas for refs otherwise as the RestApi component only follows paths in the definition where the child has keys that correspond to the keys of a PathItemObject.

Refs introduce two error states though: Cyclical refs and refs to nonexistant or malformed nodes in the definition. Currently openapi-typescript will terminate on cyclical references to avoid endless loops, but will gladly generate types that reference paths that don't exist, or that are not path items at all. This might probably occur in other parts of the code generation too.

While someone can probably come up with a case that can hack this into some nifty feature, I couldn't and would personally prefer it the synthesizer stops and delivers a specific error rather than wondering what the type checker is complaining about. So I have the RestApi component check whether any path item refs in the definition file point to valid path items. Cycle checks are a required necessity for this.

If this behavior is explicitly not desired, please close this and I'll submit a minimal fix to satisfy the compiler as a new PR.

  • What is the new behavior (if this is a feature change)?

cdk-serverless will compile again with newer versions of openapi-typescript. Runtime behavior will now throw errors upon encountering OpenAPI definitions with invalid or cyclical path item references.

  • Does this PR introduce a breaking change? (What changes might users need to make in their setup due to this PR?)

No API changes were introduced, but abovementioned changes may affect existing projects making use of existing unspecified/error-tolerant behavior.

  • Other information:

This will hopefully be enough to make cdk-serverless build again with updated dependencies

@@ -43,10 +48,10 @@ export class RestApi extends pj.Component {
}
}
}
if (!fs.existsSync('./src/generated')) {
fs.mkdirSync('./src/generated');
if (!fs.existsSync(`${this.project.outdir}/src/generated`)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, completely forgot to mention this! I stumbled over these hardcoded paths when writing unit tests. I hope this doesn't break something else!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will check

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Datastore and GraphQL aspects share the same relative paths, my gut says if it breaks something, I'd rather fix that than leave the old paths, even if currently unit testing was the first time I ran into problems with this.

@hoegertn
Copy link
Contributor

Thanks I will review it and merge it afterwards

@hoegertn hoegertn self-assigned this Dec 15, 2023
@hoegertn hoegertn added bug Something isn't working enhancement New feature or request labels Dec 15, 2023
@hoegertn hoegertn changed the title fix: Add support for handling refs within OpenAPI path items feat: Add support for handling refs within OpenAPI path items Dec 15, 2023
@DerDackel
Copy link
Contributor Author

DerDackel commented Dec 15, 2023

Oh, another thing I forgot: The synthSnapshotForAspect function I wrote for testing utility as based on a similar utility function within projen, as I basically looked up how they test project synthesis. I basically took their synthSnapshot function and amended it so I could do a two-step synthesizing a base project and synthesizing again after applying some side-effects to it that require an existing synthesized state.

Shouldn't matter, but as this is adapted from a copypaste from another project, I feel obligated to mention this.

@hoegertn
Copy link
Contributor

@DerDackel please rebase onto merged PR #277

@DerDackel
Copy link
Contributor Author

Done!

@mergify mergify bot merged commit 825ede6 into open-constructs:main Dec 27, 2023
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants