-
Notifications
You must be signed in to change notification settings - Fork 21
fix: updated parseRequestUnion for union requests #1055
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
fix: updated parseRequestUnion for union requests #1055
Conversation
22d4ff2
to
b2731e4
Compare
Path parameters were being dropped when the first schema in a union request didn't contain params. Now it iterates through all schemas to find the first one with path parameters.
8651c8c
to
9396a0e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few small comments, but looks good overall
path: '/internal/api/policy/v1/{applicationName}/touchpoints/{touchpoint}/rules/evaluation', | ||
method: 'POST', | ||
request: t.union([ | ||
// First schema has NO path parameters - this was causing the bug |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to keep these comments around? It doesn't mean anything without context
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed parts of the code base had comments that pointed out small things, so I didn't know if it was better or worse to leave tidbits. Your comment makes sense though, I'll delete the comment
}, | ||
); | ||
|
||
const REAL_WORLD_POLICY_EVALUATION_ROUTE = ` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this variable name can be better
226e512
to
d64cd6a
Compare
d64cd6a
to
d2836f4
Compare
🎉 This PR is included in version @api-ts/openapi-generator@5.10.2 🎉 The release is available on npm package (@latest dist-tag) Your semantic-release bot 📦🚀 |
Ticket: DX-1437
The error happened in this case:
If Type A didn't have the params defined (due to inconsistency in source code), the generator would immediately stop looking without checking for param specifications in the other types.
With the changes it iterates through all types in the union (Type A, then Type B, etc.) until it finds the first one that contains the params definition. Once it finds the parameter, it uses it for the route and stops searching, since the URL parameters should be the same for the entire route.
I also made a few new test cases that now pass. One of them were if the first schema in a union did not have params, but the rest did, it now looks through the rest of them instead of breaking after the first.