1
\$\begingroup\$

What I have here is a relatively simple endpoint for a small site I'm making. Idea is to take:

  • title
  • content
  • tags

and create an entry in the database. It works properly.

I threw this together as something that should work and tried to make it clean (like with the error list that gets returned) but clearly I'm doing this all by hand. Are there best practices for writing these API-style endpoints that I'm not following?

I looked under the hood at a few sites (such as SO) and noticed they are using JSON as encoding for the response. That's why I used it too.

On the input validation front, is my system decently robust or is it too flimsy?

I also tried to make the code safe by catching any exceptions that might pop up. If there's something I've overlooked please let me know.

@main.route('/createpost', methods=['POST'])
@login_required
def createpost():
resp = {
    'success': False
}

err = []

u = current_user.id  # 2458017363
title = request.values.get('title')
_tags = request.values.get('tags')  # JSON btw
content = request.values.get('content')
# _attachments = request.files.getlist('file')
# attachments = []

# for f in _attachments:
#     if f.filename.rsplit('.', 1)[1].lower() not in ALLOWED_EXTENSIONS:
#         filepath = os.path.join(UPLOAD_DIR, secure_filename(f.filename))
#         f.save(filepath)
#         attachments.append(filepath)
#     else:
#         err.append('File ' + f.filename + "is not permitted!")

if not title or len(title) > 100:
    err.append('Your title must exist and be less than 100 characters.')

try:
    tags = json.loads(_tags)
    if not tags or len(tags) > 3:
        err.append('Choose between 1-3 tags so people know what your post is about!')
except Exception:
    err.append('Choose between 1-3 tags so people know what your post is about!')

if not content or len(content) < 50:
    err.append('Your content must be at least 50 characters.')

if err:
    resp['error'] = err
    print('err')
    return Response(json.dumps(resp), mimetype='text/json')

# PROVIDED EVERYTHING IS CORRECT

while True:
    try:
        dbentry = Post(id=snowflake(),
                       author_id=u,
                       title=bleach.clean(str(title)),
                       tags=bleach.clean(str(_tags)),
                       content=bleach.clean(str(content)).encode(),
                       )

        db.session.add(dbentry)
        db.session.commit()
    except IntegrityError:
        continue
    break

resp['success'] = True
return Response(json.dumps(resp), mimetype='text/json')

imports are as follows:

# main.py
import json
import os

import bleach
from sqlalchemy.exc import IntegrityError

from .models import Post # sqlalchemy model for posts
from flask import Blueprint, render_template, request, Response
from flask_login import login_required, current_user
from werkzeug.utils import secure_filename

from . import db
from .utils import snowflake  # random number generator 
# (hence why i have a while loop around the db entry creation since there is a 
# miniscule chance it will give the same number again)
\$\endgroup\$
1
  • \$\begingroup\$ I changed the title so that it describes what the code does per site goals: "State what your code does in your title, not your main concerns about it.". Please check that I haven't misrepresented your code, and correct it if I have. \$\endgroup\$ Mar 3 at 7:49
1
\$\begingroup\$

Some quicky remarks:

  • a well-behaved API should return specific HTTP codes depending on the outcome, that is: 200 for success, or 201 for resource created. 204 is a possibility for an empty but successful response too but I would not advise using the full panoply available. Just be aware of what codes exist and when to use them.
  • likewise, use 401/403 for permission issues or authentication errors but here you have the @login_required that should take care of that
  • accordingly, an error should return a meaningful status code to the client - 400 would be a good candidate for an invalid request
  • the bottom line, always return an appropriate status code, never return HTTP/200 if the request failed. API calls are routinely automated, and the client will rely on the status code returned by the server to determine if the call was successful or not.
  • useless variable: u = current_user.id # 2458017363. You could just write: author_id=current_user.id, instead of author_id=u. u is not an intuitive variable name anyway
  • I don't understand the logic of generating a random ID in your code, the database backend should generate an incremented record ID for you (simply update your data model). I don't see why you need a random value here...
  • When you decide to handle certain exceptions like except IntegrityError, you can indeed decide to ignore them or hide them to the client but you should still log them for later review. IntegrityError suggests there is something wrong with your handling of incoming data. It should not happen if data is inspected and sanitized properly.
  • except Exception is too broad when all you're doing is json.loads. The error that you can expect here should be ValueError. Try submitting empty JSON and malformed JSON to verify.
  • to sum I don't think you should have resp['error'] and resp['success'], just use the same name in all cases eg: 'response'. It is the status code that will make the difference between success and failure. Whatever you do, the API should respond in a way that is consistent and predictable.
\$\endgroup\$
1
  • \$\begingroup\$ Thank you! Great idea with the status codes. I will be sure to implement it. I also realized the random ID was not ideal (was using sqlite and I couldn't find such a feature except autoincrementing integers 1 ,2 ,3...) Will also fix the broad exception clauses. Really appreciate it! \$\endgroup\$
    – atulw
    Mar 3 at 22:32

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.