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

OAS Generation for APIV2 #589

Merged
merged 13 commits into from
Oct 23, 2024
Merged

OAS Generation for APIV2 #589

merged 13 commits into from
Oct 23, 2024

Conversation

Dashron
Copy link
Contributor

@Dashron Dashron commented Oct 21, 2024

Addresses #586.

Complete

  • Switches to drf-spectacular for oas generation
  • Works for APIV2
  • Trim down the oas file to only include certain paths
  • All errors fixed
  • Most warnings fixed

Todo

  • Some warnings
  • Set up spectacular to be installed only when the --dev flag is set
  • Update the github action to use this new method.

@augustjohnson
Copy link
Collaborator

Adding to todo:
Update the github action to use this new method.

@Dashron
Copy link
Contributor Author

Dashron commented Oct 21, 2024

@augustjohnson I just pushed an update that eliminates most warnings and all errors. It adds annotations to clarify some types.

I left todo's for any type that tripped up my IDE. There are many fields VS code interpreted as any or | none. I think it's more of an IDE issue though, e.g. HasSpeed clearly types walk as float in the definition, but as any inside get_speed. 🤷 .

I don't know which of these can be improved via code, and which need to be overridden via annotations, but I need to at the very least test each one to make sure my typing is correct.

I'll work on the final warnings and todo's tonight, then QA tomorrow.

@Dashron
Copy link
Contributor Author

Dashron commented Oct 21, 2024

When I run this, I get...

[truncated]/lib/python3.11/site-packages/rest_framework/fields.py:990: UserWarning: max_value should be a Decimal instance.
  warnings.warn("max_value should be a Decimal instance.")
[truncated]/lib/python3.11/site-packages/rest_framework/fields.py:992: UserWarning: min_value should be a Decimal instance.
  warnings.warn("min_value should be a Decimal instance.")

The error kind of looks like a warning that there are model.DecimalFields without min and max values set? I'm going to ignore this for now, but can create a ticket if you would like.

@Dashron
Copy link
Contributor Author

Dashron commented Oct 21, 2024

I also get a warning for this line: https://github.com/open5e/open5e-api/blob/staging/api_v2/serializers/spell.py#L23. Every other field I've worked with had a similarly named function or model attribute. This seems to be the only use of slot_expended in the code. Am I overlooking something or is it a bug?

@Dashron
Copy link
Contributor Author

Dashron commented Oct 21, 2024

It doesn't like the way damage die types are created. Because multiple identical enums are created, it gets confused in naming them. This warning is more about highlighting a "possible" bug, not an issue in the output schema, so it's likely safe to ignore.

@Dashron
Copy link
Contributor Author

Dashron commented Oct 22, 2024

Ok, I just squashed the final warning by adding a manual postprocessing step to fix the search object.

@augustjohnson let me know if you want me to try to tackle the decimal min/max issue, or remove slot_expended. With those two wrapped up I would be comfortable with marking this ready for review.

@augustjohnson
Copy link
Collaborator

Doing a bit of research into the decimal issue, I believe it's a DRF serializer throwing the error, and it's not clear which.

The following places are all places where a decimal (or float) field are defined with mins that are integers instead of decimal/floats. Adding a .0 to these would be easy enough.
https://github.com/open5e/open5e-api/blob/staging/api_v2/models/creature.py#L72
https://github.com/open5e/open5e-api/blob/staging/api_v2/models/abstracts.py#L52
https://github.com/open5e/open5e-api/blob/staging/api_v2/models/object.py#L29.
I have a related ticket here: #564

You have my blessing to add those changes in to see if it fixes the error.

Regarding the Extra Damage thing: I have no strong attachment to the current method, but it's a fairly established concept. See Deva as an example of a creature who's attack has "extra" damage.
https://open5e.com/monsters/deva. Feel free to simply put your concerns into an issue in the backlog and push as-is.

@augustjohnson
Copy link
Collaborator

Also, regarding slot_expended, looks like that's an old unused field. Feel free to remove.

@Dashron
Copy link
Contributor Author

Dashron commented Oct 22, 2024

  • removed slot_expended
  • Figured out decimal, I have to wrap the number in decimal.Decimal(x.x)
  • the extra damage warning went away? 🤷
  • Unfortunately I don't think I can make this dev because it depends on annotations in the code.
  • ironing out the github action update now

@Dashron Dashron marked this pull request as ready for review October 22, 2024 20:16
@augustjohnson
Copy link
Collaborator

Hey, here's a thought. Can we replace the current HTML UI here: https://api-beta.open5e.com/ with something nice generated by spectacular? I understand that competes with the readme featureset, but it's easier to find at our own url.

@Dashron
Copy link
Contributor Author

Dashron commented Oct 22, 2024

This comes with swagger-ui and redocly built in. I thought open source readme had access to custom domains, but I just verified, it doesn't. I totally understand if that's a blocker.

Here are screenshots:

Redocly

Screenshot 2024-10-22 at 19-39-00 Open5e

Swagger-UI

Screenshot 2024-10-22 at 19-39-14 Open5e

I can't find a good way to customize their style when we use drf-spectacular, but you could always spin up your own redocly, swagger-ui, or other service if you want more control.

And for my reference, this is the code we need to add to server/urls.py.

from drf_spectacular.views import SpectacularAPIView, SpectacularRedocView, SpectacularSwaggerView

urlpatterns+=[
    path('api/schema/', SpectacularAPIView.as_view(), name='schema'),
    path('api/schema/swagger-ui/', SpectacularSwaggerView.as_view(url_name='schema'), name='swagger-ui'),
    path('api/schema/redoc/', SpectacularRedocView.as_view(url_name='schema'), name='redoc'),
]

@augustjohnson
Copy link
Collaborator

Wow, amazing work here, I didn't realize you were manually forcing the properties to be one of the field types. Yikes. Lots of work. I think this will be a fantastic approach moving forward.

@augustjohnson augustjohnson merged commit ed28901 into open5e:staging Oct 23, 2024
2 checks passed
@Dashron
Copy link
Contributor Author

Dashron commented Oct 23, 2024

Thanks! Looking at the OAS file, there's a lot to clean up. I'll poke away at a couple more prs to work on that.

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