upbit / pixivpy

Pixiv API for Python
https://pypi.org/project/PixivPy3/#files
The Unlicense
1.79k stars 149 forks source link

Check parameters with fixed choices before requesting #215

Closed eggplants closed 2 years ago

eggplants commented 2 years ago

For example, content_type of illust_recommended should be 'illust', 'manga', or ''. To reduce invalid requests, I recommend to check if content_type and other parameters with fixed choices is valid before posting a request and raise error if invalid.

content_type = 'test'
content_types = ['illust', 'manga', '']
if content_type not in content_types:
    raise ValueError(f"'{content_type}' is not included in {content_types}")
else:
    # post a request

Parameters with fixed choices:

upbit commented 2 years ago

Good advice, parameter checking is indeed necessary, but the implementation needs to be carefully considered.

Is it possible to check the type and value of incoming parameters through features such as Decorator? e.g:

from functools import wraps

def checkparams(name, values):
    def decorate(func):
        @wraps(func)
        def wrapper(*args, **kwargs):
            # if not func.content_type in values: raise Exception(...)
            return func(*args, **kwargs)
        return wrapper
    return decorate

    @checkparams('content_type', ['illust', 'manga', ''])
    def funcion(self, content_type):
        # ...
Xdynix commented 2 years ago

Good advice, parameter checking is indeed necessary, but the implementation needs to be carefully considered.

Is it possible to check the type and value of incoming parameters through features such as Decorator? e.g:

from functools import wraps

def checkparams(name, values):
    def decorate(func):
        @wraps(func)
        def wrapper(*args, **kwargs):
            # if not func.content_type in values: raise Exception(...)
            return func(*args, **kwargs)
        return wrapper
    return decorate

    @checkparams('content_type', ['illust', 'manga', ''])
    def funcion(self, content_type):
        # ...

Using decorator can be hard to locate the passed values from *args and **kwargs. E.g.: _Is content_type the first argument in args, or is it in kwargs._

I would suggest something like:

def user_detail(self, user_id, filter="for_ios", req_auth=True):
    check_param("user_id", user_id, types=(int, str))
    check_param("filter", filter, values=("for_ios",))
    check_param("req_auth", req_auth, values=(True,))
   # ...
eggplants commented 2 years ago
from functools import wraps
from inspect import getfullargspec, unwrap
from typing import Any, Dict, List

def checkparams(d: Dict[str, List[Any]]):
    def decorator(func):
        fullargspec = getfullargspec(func)
        args = fullargspec.args
        idxs = [args.index(name) for name in d.keys()]
        default_kwargs = dict(
            zip(args[-len(fullargspec.defaults) :], fullargspec.defaults)
        )

        @wraps(func)
        def wrapper(*args, **kwargs):
            default_kwargs.update(kwargs)
            for i, (name, values) in enumerate(d.items()):
                if name not in func.__code__.co_varnames:
                    raise ValueError(
                        f"parameter {repr(name)} is missing "
                        "in given function's arguments"
                    )
                try:
                    value = args[idxs[i]]
                except IndexError:
                    value = default_kwargs[name]
                if value not in values:
                    raise ValueError(
                        f"parameter {repr(name)} was expected to "
                        f"be one of {repr(values)}, but got {repr(value)}"
                    )
            return func(*args, **kwargs)

        return wrapper

    return decorator
@checkparams({"a": [1], "b": [2], "c": [3], "d": [4, "test"]})
def a(a, b, c, d="foo", e=""):
    pass

a(1, 2, 3) # ValueError: parameter 'd' was expected to be one of [4, 'test'], but got 'foo'

a(1, 2, 3, d="bar") # ValueError: parameter 'd' was expected to be one of [4, 'test'], but got 'bar'
invobzvr commented 2 years ago

I suggest using typing.Literal and decorator to achieve it.

from typing import Literal
from inspect import signature

def validate(func):
    params = [  # get all Literal parameters
        (iParam.name, iParam.default, annotation.__args__) for iParam in signature(func).parameters.values()
        if getattr(annotation := iParam.annotation, '__origin__', None) == Literal
    ]
    def wrapper(*args, **kwargs):
        for key, default, vals in params:
            value = kwargs.get(key, default)
            assert value in vals, f'{key!r} should be one of {vals!r}, but got {value!r}'
        return func(*args, **kwargs)
    return wrapper

class AppPixivAPI:
    @validate
    def user_illusts(
        self,
        user_id,
        type: Literal['illust', 'manga'] = 'illust',  # Literal values here
        filter='for_ios',
        offset=None,
        req_auth=True,
    ): ...

AppPixivAPI().user_illusts(type='illu')  # AssertionError
AppPixivAPI().user_illusts(type='illust')  # No exception

But as @Xdynix mentioned, it's hard to determine whether the value is *args or **kwargs, or we should force parameters to be keywords-only-parameters

eggplants commented 2 years ago

Note: typing.Literal is introduced in Python 3.8. If pixivpy adopts @invobzvr's plan, we have to drop Python 3.7.

invobzvr commented 2 years ago

If not using typing.Literal, I got a method inspired by dataclasses. But it also depends on keywords-only-parameters.

from inspect import signature

def validate(func):
    params = [
        (iParam.name, lv.default, lv.values)
        for iParam in signature(func).parameters.values()
        if isinstance(lv := iParam.default, LiteralVals)
    ]

    def wrapper(*args, **kwargs):
        for key, default, vals in params:
            if value := kwargs.get(key):
                assert value in vals, f'{key!r} should be one of {vals!r}, but got {value!r}'
            else:
                kwargs[key] = default
        return func(*args, **kwargs)
    return wrapper

class LiteralVals:
    def __init__(self, values, default_index=0, default=None):
        self.values = values
        if isinstance(default_index, int):
            self.default = values[default_index]
        elif default:
            self.default = default
        else:
            raise

class AppPixivAPI:
    @validate
    def user_illusts(
        self,
        user_id,
        type=LiteralVals(['illust', 'manga']),
        filter='for_ios',
        offset=None,
        req_auth=True,
    ): ...

Actually, decorator-based methods all depend on keywords-only-parameters. And to support old python versions, @upbit 's method is just enough, while :=, inspect.getfullargspec, inspect.signature is not available. It requires runtime arguments' name and value to get rid of keywords-only-parameters limit. But I didn't find a way to achieve that. So then, it seems that @Xdynix 's method is the most suitable.

eggplants commented 2 years ago

One suggestion: Literal + agronholm/typeguard

from typing import Literal
from typeguard import check_argument_types, check_return_type

class AppPixivAPI:
    ...
    def user_illusts(
        self, user_id, type="illust", filter="for_ios", offset=None, req_auth=True
    ):
        # type: (Union[int, str], Literal['illust', 'manga', ''], Literal['for_ios', ''], Optional[Union[int, str]], bool) -> ParsedJson
        assert check_argument_types()
        url = "%s/v1/user/illusts" % self.hosts
        params = {
            "user_id": user_id,
            "filter": filter,
        }
        if type is not None:
            params["type"] = type
        if offset:
            params["offset"] = offset
        r = self.no_auth_requests_call("GET", url, params=params, req_auth=req_auth)
        retval = self.parse_result(r)
        assert check_return_type(retval)
        return retval
invobzvr commented 2 years ago

I've found a really interesting method that uses sys.setprofile to get runtime arguments. Only inspect.signature need to be adapted to support python 2.

from inspect import signature
from sys import setprofile

def validate(func):
    params = [
        (iParam.name, lv.default, lv.values)
        for iParam in signature(func).parameters.values()
        if isinstance(lv := iParam.default, LiteralVals)
    ]

    def tracer(frame, event, arg):
        if event == 'call':
            arguments = frame.f_locals
            for key, default, vals in params:
                value = arguments[key]
                if isinstance(value, LiteralVals):
                    arguments[key] = value.default
                else:
                    assert value in vals, f'{key!r} should be one of {vals!r}, but got {value!r}'

    def wrapper(*args, **kwargs):
        setprofile(tracer)
        ret = func(*args, **kwargs)
        setprofile(None)
        return ret
    return wrapper

class LiteralVals:
    def __init__(self, values, default_index=0, default=None):
        self.values = values
        if isinstance(default_index, int):
            self.default = values[default_index]
        elif default:
            self.default = default
        else:
            raise

class AppPixivAPI:
    @validate
    def user_illusts(
        self,
        user_id,
        type=LiteralVals(['illust', 'manga']),
        filter='for_ios',
        offset=None,
        req_auth=True,
    ): ...
upbit commented 2 years ago

As an API, ease of use may be the primary consideration. Like Xdynix said, using decorator can be hard to locate the passed values from *args and **kwargs

Using typing.Literal in Python 3.8 or sys.setprofile is both OK, we can maintain an experimental branch to support this feature, until Python 3.7 is no longer officially maintained.