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

Swagger fixes #609

Merged
merged 7 commits into from
Nov 4, 2024
Merged

Swagger fixes #609

merged 7 commits into from
Nov 4, 2024

Conversation

crummy
Copy link
Contributor

@crummy crummy commented Oct 26, 2024

I use liblab to generate SDK clients from swagger. It complained about a few things that I had to fix. Some seemed like legitimate bugs, others I just added some placeholders.

  1. Reaction and receipt endpoints were missing descriptions for their path parameters
  2. Search endpoint had a description for its path parameter, but path parameter was not present in path
  3. Host and scheme were missing (not sure if they are genuinely required in the spec but Liblab won't generate an SDK without them so hoping we can get them in too!)

@bbernhard
Copy link
Owner

bbernhard commented Oct 26, 2024

Thanks for the PR!

The problem is, that the Swagger file gets generated by a code generator (https://github.com/swaggo/swag) based on some annotations in the code. So, while your changes fix the issue temporarily, the problem would unfortunately be back once the swag is run again.

I haven't had time to look at the swag documentatiom right now, but I guess there should be some settings to accomplish the same.

@crummy
Copy link
Contributor Author

crummy commented Oct 28, 2024

Thanks, I've updated the source files and re-ran swag.

edit: I see this has broken 54252a3 again. Any idea why this might have happened?

@bbernhard
Copy link
Owner

bbernhard commented Oct 31, 2024

edit: I see this has broken 54252a3 again. Any idea why this might have happened?

Do you see any errors when running swag init?

In case of 54252a3, swag initwas logging an error (which I didn't spot at first glance).

@crummy
Copy link
Contributor Author

crummy commented Nov 1, 2024

I see this error in the output:

2024/11/01 08:16:31 Error parsing type definition 'api.SendMessageV2': [mentions]: cannot find type definition: ds.MessageMention
2024/11/01 08:16:31 Skipping 'api.SendMessageV2', recursion detected.

Unfortunately, I was unable to resolve the error. I don't know enough Go, I suppose. I copy and pasted this missing lines from an older commit.

src/api/api.go Outdated
@@ -1464,6 +1465,7 @@ func (a *Api) SendReaction(c *gin.Context) {
// @Success 204 {string} OK
// @Failure 400 {object} Error
// @Param data body Reaction true "Reaction"
// @Param id path string true "Phone number"
Copy link
Owner

Choose a reason for hiding this comment

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

I am not 100% sure, but I think the parameter name needs to match the parameter name in the annotated URI.

As annotated URI is // @Router /v1/reactions/{number} [delete], I think the line needs to be:

// @Param number path string true "Phone number"

At least that's how I understood the swaggo documentation: https://github.com/swaggo/swag?tab=readme-ov-file#use-multiple-path-params. To be honest, I am not sure what happens when one chooses a different name - I guess maybe some interactive stuff breaks in the swagger UI then? Not sure 😅 But I think it might be better to play it safe here and change the name :)

Don't worry about the generation of the swagger files, I'll run swag after the PR is merged :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, fixed that up.

@crummy crummy mentioned this pull request Nov 4, 2024
2 tasks
@bbernhard
Copy link
Owner

looks good, thanks a lot!

@bbernhard bbernhard merged commit 6b08159 into bbernhard:master Nov 4, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants