Make validate_params work with the function's parametters instead of a dict

[Seluj78]

Instead of having

@validate_params(
    {
        "name": str,
        "age": int,
        "is_student": bool,
        "courses": List[str],
        "grades": Dict[str, int],
    }
)
def example():

It would make more sense to have the decorator read the function arguments directly like so :

@validate_params
def example(name: str, age: int, is_student: bool, courses: List[str], grades: Dict[str, int]):

/!\ One attention point is to see how Flask interacts with the function and where it places it's variables when doing a function with some kind of slug (@app.route("/users/<user_id>"))

[Seluj78]

Credit for this idea goes to @JulienPalard

[JulienPalard]

I'd add: choose another name for this decorator. Both for backward compatibility and to try to stuff more meaning in it?

[Seluj78]

I'd add: choose another name for this decorator. Both for backward compatibility and to try to stuff more meaning in it?

@JulienPalard that's a good idea, do you have any recommendations ? :) I was thinking validate_arguments but I'm not sure

[Mews]

I think I might have an idea on how to accomplish this ngl. Are there tests in place to test the decorator so I can make sure I dont break anything?

[Seluj78]

Yes there are ! See the tests folder, they run with pytest and tox

Check the readme (I think I detailed it here) or the contributing file and if it's not detailed let me know and I'll add it

[Mews]

Alright thanks 👍 I know how to run the tests haha I was just wondering if this particular feature was accounted for in them. Btw, another question, since I'm not too familiar with flask. Take, for example the example you provided:

@validate_params
def example(name: str, age: int, is_student: bool, courses: List[str], grades: Dict[str, int]):
    ...

Would the arguments name, age etc be used inside the function? Or would they be fetched from the json? (Again, this is based only on the context I got from looking at the source code so I might be misinterpreting what it's doing)

[Mews]

Also I seem to be running into some issues using tox to run the tests, but I can run them fine with pytest. Is it fine to run the tests with only pytest or should I look into fixing tox.

[Seluj78]

No worries if your tests work with pytest they should work with tox.

FYI tox is a utility that allows you to run commands with multiple environments, and in this case it allows me to run the tests (pytest) for python 3.8 up to 3.12 for many versions of flask !

[Seluj78]

As for your question about the function, it's a little more complicated than that.

Flask can also inject some variables in the function when using slugs (for example https://stackoverflow.com/a/40460690)

And then we want to validate the JSON (and maybe one day form data) params that would be inputted along the potentials slugs

(Sorry I'm on my phone I can't give you a better example right now)

[Seluj78]

@all-contributors please add @JulienPalard for ideas

[allcontributors[bot]]

@Seluj78

I've put up a pull request to add @JulienPalard! :tada:

[Seluj78]

@Mews let me know if you have more questions for this :)

[Mews]

Oh yeah sorry I still do plan on contributing to this issue I was just busy with something else. So is there a reason to pass *args, **kwargs to the decorated function here? https://github.com/Seluj78/flask-utils/blob/main/flask_utils/decorators.py#L274

[Seluj78]

Oh yeah sorry I still do plan on contributing to this issue I was just busy with something else.

No worries, take your time :)

So is there a reason to pass *args, **kwargs to the decorated function here? main/flask_utils/decorators.py#L274

Yes there is :) Because of the way flask work, there already are args and kwargs to the function so we need to pass them along :)

[Mews]

Okay and the name, age etc arguments should also be passed to the function yes? If so I can probably get to working on it right away.

[Seluj78]

Probably yes. Try it and we'll see once you get the PR I'll test it and see if it does what I imagine :)

[Mews]

Okay, but I'll probably need to change the tests too right? To use the new syntax you proposed

[Seluj78]

Yes that's correct ! But don't worry too much about tests and documentation for now, at least not before we validated it does what we want

[Mews]

Also, taking a look at the tests I found this: https://github.com/Seluj78/flask-utils/blob/main/tests/test_validate_params.py#L247 However, for function arguments you can't use type hints in the same way

# Not a valid type hint
def example(name: (str, int)):
    etc...

Python does evaluate it but it's not a very good way to do it. The way to indicate it would be through typing.Tuple[str, int] or through typing.Union[str, int]. My question is just what would you prefer to be used? I'm leaning more to typing.Tuple because its easier to tell what it means, and also because Union's purpose is generally to indicate the argument can be one of a few types, but idk.´

You could probably expand the _check_type function to allow for tuples like this:

    elif origin is tuple:
        if not isinstance(value, tuple) or len(value) != len(args):
            return False
        return all(
            _check_type(item, arg, allow_empty, (curr_depth + 1)) for item, arg in zip(value, args)
        )

Where we check if a tuple was passed, if the tuple has the correct amount of elements, and if the elements of the tuple match the types declared inside Tuple[...]

[Mews]

Also I have a question about a test that's being ran, here: https://github.com/Seluj78/flask-utils/blob/3ec12cf62f6112cbd16ef2a656993f7e337ee976/tests/test_validate_params.py#L243-L256 Maybe I'm wrong, but in test_valid_request aren't you passing a string, which would be an invalid type? Feels like it should be like:

 class TestTupleUnion: 
     @pytest.fixture(autouse=True) 
     def setup_routes(self, flask_client): 
         @flask_client.post("/tuple-union") 
         @validate_params({"name": (str, int)}) 
         def union(): 
             return "OK", 200 

     def test_valid_request(self, client): 
         response = client.post("/tuple-union", json={"name":("this is a string", 123)})
         assert response.status_code == 200 

Still, as far as I can tell, the _check_types doesn't account for tuples as of right now, so I'm a bit confused.

[Mews]

Sorry for spamming this thread but I've began implementing a few of the changes necessary and I have a question related to pull requests and stuff. I'm making many changes on the same file, on many parts of the code. My question is, should I make a commit for each change individually, or make a single big commit when I'm done editing the file? And if it's the former, should I be pushing the commits as I make them or only at the end?

[JulienPalard]

My question is, should I make a commit for each change individually, or make a single big commit when I'm done editing the file?

My point of view for this is: each commit should work. Or not break the whole project. Read: tests shall pass.

Idea is that I see git commits like saving in a video game: do you save a single time at the end of a hard dungeon, or do you save each time you complete one room? (Or do you save when you're in a half-backed/bad-position? NO? Same with code, don't commit it).

Another more distant argument is: is someone want to use git bisect, he does not want to hit commit known to be fully broken.

I also abuse of git rebase -i in case I have "too small" commits like "ok I need to sleep, this is half backed, but still I'm like: git commit -m wip; git push in case of fire, so I often have to "clean the mess up for the history", you don't need to play like this ;)

An approach that is slowly maturing in me is to write the commit message before doing the work, it's a clean way to delimit the task to be done AND to write good commit messages, let's call it "commit message driven development".

[Mews]

Alright that makes sense I guess. Still, I think in this case it might make sense to change the tests on a separate commit. @JulienPalard btw I was wondering if its normal to open a pull request where you might still make changes and discuss stuff there or if I should ask stuff in this issue and only open the pull request when I've done everything

[Mews]

Hi @Seluj78 Apart from that one TupleUnion test, this feature is practically done. There's only one issue left, which is about optional arguments. Currently, I'm passing the arguments from the json to the decorated function like this:

for key in data:
    kwargs[key] = data[key]

return fn(*args, **kwargs)

However, this causes an obvious issue, which is that if a parameter is optional and is not present in data, no error will be raised, since its optional, but it also wont be added to kwargs and as such not passed to the wrapped function, which would raise TypeError: missing x required positional arguments.

I was thinking you could get around this by doing:

for key in parameters:
    if _is_optional(parameters[key]) and key not in data:
        kwargs[key] = None

    else:
        kwargs[key] = data[key]

So, when an optional parameter isn't passed in the json, it gets passed to the function as None. This passes all the tests (after changing them to the new syntax). What do you think of this?

[JulienPalard]

What about making optional arguments optional in the prototype too?

àla :

@validate_params
def example(name: str, age : Optional[int] = None):
    ...

[Mews]

What about making optional arguments optional in the prototype too?

àla :

@validate_params
def example(name: str, age : Optional[int] = None):
    ...

Yeah that's the same as what my example does, but the user doesn't need to type = None

[JulienPalard]

for key in data: kwargs[key] = data[key]

I don't have the context, but please take extra care (by adding a test or two) of not having a query string argument clobbered by a value from the request body.

I mean (I'm no flask user forgive inconsistencies):

@validate_params
@app.route("/user/{user_id}")
def get_user(user_id, is_admin: bool):
    ...

If someone sends:

curl -XPOST -H "Content-Type: application/json" -d '{"user_id": 666, "is_admin": False}' localhost:8000/user/42

I except to get user_id equal to 42, not 666.

[Mews]

for key in data: kwargs[key] = data[key]

I don't have the context, but please take extra care (by adding a test or two) of not having a query string argument clobbered by a value from the request body.

I mean (I'm no flask user forgive inconsistencies):

@validate_params
@app.route("/user/{user_id}")
def get_user(user_id, is_admin: bool):
    ...

If someone sends:

curl -XPOST -H "Content-Type: application/json" -d '{"user_id": 666, "is_admin": False}' localhost:8000/user/42

I except to get user_id equal to 42, not 666.

I mean I'm not a flask expert either, that's why I asked @Seluj78 if the way I was doing it wasn't gonna cause issues. I guess we'll have to wait until he responds.

[Seluj78]

(I'm not dead, just was really busy these past few days. I will respond ASAP, probably tomorrow)

[Mews]

No worries haha

[Seluj78]

For the tuple and union question, I do agree with you.

We shouldn't allow the usage of

# Not a valid type hint
def example(name: (str, int)):
    etc...

What we can probably do, I don't know how though, is raise a DeprecationWarning when we see this usage.

As for the choice between Tuple and Union, I agree with you. typing.Tuple should be used because it means that a tuple is passed (for example Tuple[str, int] -> (42, 'hello')) whereas typing.Union means that one of the types is passed (for example Union[str, int] -> 42 or 'hello').

For your question about the test_valid_request for the TestTupleUnion class, I agree with you. It should raise an error if we implement the above thing where it's either a tuple passed to the function (works in Python functions, not with JSON data)

So the test should test for typing.Tuple, typing.Union and the normal Python tuple (Any, Any, ...)

As for the commit messages question, I agree with Julien. I also do try to keep my commit messages clean and each commit working when I can !

Do let me know if you have more questions. In any case I will test your PR and code once you have it pushed so we can check stuff together.

[Mews]

I was looking into that test and I think I interpreted what its for incorrectly lol. Its not for validating that the argument is a tuple that contains a string and an int, it indicates an Union type where the argument can be either a string or a int. Still, when defining type hints it would be interpreted differently, so in my opinion it should just be removed.

My proposal for allowing validating tuples still stands though and I'd be happy to introduce it too :)

[Seluj78]

My proposal for allowing validating tuples still stands though and I'd be happy to introduce it too :)

Works for me !