therealadityashankar / flask-value-checker

easy form or query parameter checking with flask
MIT License
1 stars 1 forks source link

Add support for args in POST/PUT requests #2

Closed therealadityashankar closed 4 years ago

therealadityashankar commented 4 years ago

flask-value-checker does not support checking query parameters for POST/PUT requests yet - add support for that

also see https://github.com/therealadityashankar/flask-value-checker/pull/1

therealadityashankar commented 4 years ago

proposed idea

from flask_value_checker import Invigilator
from flask import Flask, request

app = Flask(__name__)
invigilator = Invigilator()

@app.route('/abc', methods=['POST'])
@invigilator.check(
   'POST form-only',
   '''
   username : str/lenlim(5, 15)
   password : str/lenlim(8, inf)
   stayLoggedIn : str/accept(['on'])/optional
   '''
)
def abc():
    stay_logged_in = request.form.get('stayLoggedIn', 'off')
    return f'hi {request.form['username']}, stay logged in: {stay_logged_in}'

app.run()

and for arguments

@app.route('/abc', methods=['POST'])
@invigilator.check(
   'POST args-only',
   '''
   username : str/lenlim(5, 15)
   password : str/lenlim(8, inf)
   stayLoggedIn : str/accept(['on'])/optional
   '''
)
def abc():
    stay_logged_in = request.form.get('stayLoggedIn', 'off')
    return f'hi {request.form['username']}, stay logged in: {stay_logged_in}'

app.run()

and for both

@app.route('/abc', methods=['POST'])
@invigilator.check(
   'POST args-or-form',
   '''
   username : str/lenlim(5, 15)
   password : str/lenlim(8, inf)
   stayLoggedIn : str/accept(['on'])/optional
   '''
)
def abc():
    stay_logged_in = request.form.get('stayLoggedIn', 'off')
    return f'hi {request.form['username']}, stay logged in: {stay_logged_in}'

app.run()

moreover

@app.route('/abc', methods=['POST'])
@invigilator.check(
   'POST form-only',

I think should be synonymous with

@app.route('/abc', methods=['POST'])
@invigilator.check(
   'POST',

since that's most probably the use-case

cbcoutinho commented 4 years ago

This is a great improvement compared to my initial PR. My two initial thoughts when going over this:

The first is a nitpick: the args-only example should reference args in the function:

@app.route('/abc', methods=['POST'])
@invigilator.check(
   'POST args-only',
   '''
   username : str/lenlim(5, 15)
   password : str/lenlim(8, inf)
   stayLoggedIn : str/accept(['on'])/optional
   '''
)
def abc():
    stay_logged_in = request.args.get('stayLoggedIn', 'off')
    return f'hi {request.args['username']}, stay logged in: {stay_logged_in}'

app.run()

And secondly, how do you reference in the request where the different variables are coming from in the args-or-form case? Should it possible to note in the docstring that the argument comes from the form/args to force on or the either? Should there be default if unset or require that user checks both?

@app.route('/abc', methods=['POST'])
@invigilator.check(
   'POST args-or-form',
   '''
   username : str/lenlim(5, 15) : form
   password : str/lenlim(8, inf) : form
   stayLoggedIn : str/accept(['on'])/optional # Unset means it could be both
   '''
)
def abc():
    # This is slightly exagerated
    stay_logged_in = request.form.get(
        'stayLoggedIn', 
        request.args.get(
            'stayLoggedIn', 'off'
        )
    )
    return f'hi {request.form['username']}, stay logged in: {stay_logged_in}'

app.run()
therealadityashankar commented 4 years ago

Nice nitpick lol, I didn't think much before writing that :)

Here's what is in my mind, umm

@app.route('/abc', methods=['POST'])
@invigilator.check(
   'POST args-only',
   '''
   username : str/lenlim(5, 15)
   password : str/lenlim(8, inf)
   '''
)
@invigilator.check(
   'POST form-only',
   '''
   stayLoggedIn : str/accept(['on'])/optional
   '''
)
def abc():
    stay_logged_in = request.form.get('stayLoggedIn', 'off')
    return f'hi {request.form['username']}, stay logged in: {stay_logged_in}'

app.run()

I no longer think "args-or-form" is a good Idea, however, If implimented, the user could just do,

data = {**request.args, **request.form}

": form" is something I don't wanna do cause I'd have to modify the parser

cbcoutinho commented 4 years ago

I agree the ambiguity is not ideal. Explicitly setting where the variables are required/optional is better

I think there also needs to be an option on the request.files portion. See the test I included in #1

therealadityashankar commented 4 years ago

fixed issue, see https://github.com/therealadityashankar/flask-value-checker/commit/143782accf450be507d4ab94afcb606623150bfc

cbcoutinho commented 4 years ago

@therealadityashankar I've done some research regarding this issue and came across this SO answer with associated RFC spec.

Apparently all data required during a POST request should be in the body (ie. form) part of the request instead of the query parameters. Flask allows the use of args when POSTing, which why I ran into the problem and raised this issue; however, when I read the SO answer and the attached HTTP spec I come to the conclusion that my API was 'incorrect'. I should have used forms instead of query parameters when POSTing to my API.

If I switch to using the form fields instead of args, then the previous implementation works as expected.

therealadityashankar commented 4 years ago

deprecating new changes