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

Generation ignore a query parameter #942

Closed
Kalshu opened this issue Aug 16, 2024 · 8 comments
Closed

Generation ignore a query parameter #942

Kalshu opened this issue Aug 16, 2024 · 8 comments
Labels
bug 🔥 Something isn't working prioritized 🚚 This issue has been prioritized and will be worked on soon

Comments

@Kalshu
Copy link

Kalshu commented Aug 16, 2024

Description

Hello,
You can retrieve in this repro a base to reproduce the bug.
https://github.com/Kalshu/heyapi/tree/main

I have a swagger file in this repro how declare several endpoint with common model but one model have a query parameter to filter the result
https://github.com/Kalshu/heyapi/blob/b6f333b03f044ffcda973117aabdfc7279e5c5fa/swagger.json#L222
, { "name": "fromStore", "in": "query", "schema": { "type": "boolean", "default": false } }

But this parameter is ignored by the generation and we can retrieve it in the final method
https://github.com/Kalshu/heyapi/blob/b6f333b03f044ffcda973117aabdfc7279e5c5fa/src/_generated/test/services.gen.ts#L91
https://github.com/Kalshu/heyapi/blob/b6f333b03f044ffcda973117aabdfc7279e5c5fa/src/_generated/test/types.gen.ts#L66

Reproducible example or configuration

https://github.com/Kalshu/heyapi/tree/main

OpenAPI specification (optional)

No response

System information (optional)

No response

@Kalshu Kalshu added the bug 🔥 Something isn't working label Aug 16, 2024
Copy link

stackblitz bot commented Aug 16, 2024

Fix this issue in StackBlitz Codeflow Start a new pull request in StackBlitz Codeflow.

@mrlubos mrlubos added the prioritized 🚚 This issue has been prioritized and will be worked on soon label Aug 16, 2024
@mrlubos
Copy link
Member

mrlubos commented Aug 16, 2024

Thanks @Kalshu, will check!

@mrlubos
Copy link
Member

mrlubos commented Aug 17, 2024

@Kalshu do you want to separately talk about how you're using Hey API? I see your methodNameBuilder() function, can you describe what's missing/the desired behaviour that would simplify your setup?

@mrlubos
Copy link
Member

mrlubos commented Aug 17, 2024

@Kalshu this is due to duplicate operation IDs, these are meant to be unique if provided. Not following that may produce unexpected results as you found out

@mrlubos mrlubos closed this as not planned Won't fix, can't repro, duplicate, stale Aug 17, 2024
@Kalshu
Copy link
Author

Kalshu commented Aug 19, 2024

@Kalshu do you want to separately talk about how you're using Hey API? I see your methodNameBuilder() function, can you describe what's missing/the desired behaviour that would simplify your setup?

Hi @mrlubos, where do you want to speak about the name generation ?

About the invalid swagger specification, maybe it will be a good feature to call https://validator.swagger.io/ during the generation to warn the user if the spec is not fully validate

@mrlubos
Copy link
Member

mrlubos commented Aug 19, 2024

You can comment here.

RE validation, yes at some point, but not any time soon. It's easy enough to validate on your own compared to what it would take to build into the library

@Kalshu
Copy link
Author

Kalshu commented Aug 19, 2024

About the name generator,

Initially, there was a bug with the use of the word “delete” in the name of certain methods. So I started by overloading the name generation method to bypass the bug.

Then I must have had a problem with operationsIDs (which I'm working on).

Now it's more a question of standardization in the code, as we use different api managed by different teams.
The operations ids won't always follow the same logic and nomenclature. I therefore prefer to define the names of my methods with the path of the endpoints, which are normally explicit. What's more, using the http verb makes it easier to understand api actions (Post vs Patch vs Put).

Ideally, I'd find it interesting to have an option that lets you choose between different name generation algorithms.

Translated with DeepL.com (free version)

@mrlubos
Copy link
Member

mrlubos commented Aug 19, 2024

Yep! That will need to be added, probably as part of #926. Would need to gather the possible approaches there, but it makes total sense the way you put it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🔥 Something isn't working prioritized 🚚 This issue has been prioritized and will be worked on soon
Projects
None yet
Development

No branches or pull requests

2 participants