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

Some codegen improvements #3090

Merged

Conversation

hughsimpson
Copy link
Contributor

@hughsimpson hughsimpson commented Aug 2, 2023

Mixing up a few things in this pr:

  • Fail codegen if parameters can't be decoded
  • Added slightly more detail to a few of the error messages
  • Support defining maps directly under schemas
  • Make endpoint definitions lazy, to reduce occurrence of 'method too large' errors

Happy to put any of these changes into a separate pr if they're contentious, but figured there wasn't too much harm in putting a few little things all in the same pr

@hughsimpson
Copy link
Contributor Author

hughsimpson commented Aug 2, 2023

@kciesielski any thoughts on this? I feel like it's pretty uncontentious except maybe for the lazy val stuff. I made the mistake of checking whether I could codegen from a much larger openAPI spec than I currently need, and found that this was a surprising blocker - indeed, even with this change I can't generate tapir for some endpoints with some... idiosyncratic quirks (I suspect the huge data model is the problem there, but I digress). If there are any more general best practice approaches to avoiding the 'Method too large' issues I'd love to know.

Edit: For the record, the more problematic endpoints (that remain unaddressed by this pr) the culprit seems to be sprawling API models, which honestly I need to get under control on my side... However even with adjustments there, an issue remains in that the single generated class becomes too large (i.e. Class too large error from javac). I wonder if perhaps we could wrap the endpoint definitions in an object based on, say, the first defined tag...

It might also be worth splitting out the jsonBody decorations into their own lazy vals... Would permit declaration reuse, and go some way towards increasing the complexity limit of what can be generated...

@kciesielski
Copy link
Member

Thanks a lot for this contribution, @hughsimpson! Looks good to me, and I agree it's fine to have all these changes in a scope of one PR.
As for the further improvements for increasing the complexity limit - wrapping endpoint definitions in an object might be helpful. If you could propose a PR with such changes that would be very much appreciated :)

@kciesielski kciesielski merged commit 425d65c into softwaremill:master Aug 7, 2023
@kciesielski kciesielski added the enhancement New feature or request label Aug 7, 2023
@hughsimpson hughsimpson deleted the more_error_information branch August 7, 2023 07:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants