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

Add crud utilities #243

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from
Draft

Conversation

wookie184
Copy link
Contributor

Not ready yet, just opening as a draft. Sorry for the large PR, I think it's worth it though.

This makes a lot of logic shared to reduce the amount of code there is and fix and help prevent some issues:

  • Are pastes being reaped properly? #182: RE "Are pastes being reaped on all show pages?", the repaste endpoint doesn't currently, but this fixes that and ensures it can't be forgotten in the future
  • The previous logic for checking if a paste with multiple files went over the file size when the files were combined only checked the total size once all the files were processed. This meant you could have e.g. 100 files just under the size limit individually and DoS the server because it has to process them all with pygments. Now an error is raised as soon as the combined size goes above the max allowed.
  • Some validation logic was inconsistent across different endpoints (most endpoints consider whitespace an empty paste which isn't allowed, but api_v1 didn't). Centralising the logic ensures all endpoints are consistent when they should be.

To Do:

  • Add tests
  • Add docstrings
  • Tidy up code
  • Make sure error responses are in the correct format for each endpoint respectively.

It might be nicer to separate the validation and formatted code generation from the model __init__s in the future
@supakeen
Copy link
Owner

There's quite a lot of stuff here that I like :)

@wookie184
Copy link
Contributor Author

Just adding a note that i've not forgotten about this, I'll fix the conflicts and finish this off when I get a chance 👍

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