OneZoom / OZtree

OneZoom Tree of Life Explorer
Other
87 stars 18 forks source link

`otts2vns` API endpoint considers 0 to be true. #866

Open fwimp opened 1 month ago

fwimp commented 1 month ago

When hitting the ott to common name endpoint (https://www.onezoom.org/API/otts2vns), the query all=0 is the same as all=on or all=true.

More specifically I actually cannot find any way to provide this argument that does not switch it on.

Requests that count as "on"

https://www.onezoom.org/API/otts2vns?key=0&otts=247341&lang=en&all=false https://www.onezoom.org/API/otts2vns?key=0&otts=247341&lang=en&all=0 https://www.onezoom.org/API/otts2vns?key=0&otts=247341&lang=en&all=none https://www.onezoom.org/API/otts2vns?key=0&otts=247341&lang=en&all=off

It looks like this issue comes from https://github.com/OneZoom/OZtree/blob/889026e68377ae306e8f3ffc60e16223803420cc/controllers/API.py#L719 where the truthiness of a filled string is always checked rather than trying to cast said string first.

In the popularity endpoint it looks like there is a custom function that could be duplicated over to solve this issue: https://github.com/OneZoom/OZtree/blob/889026e68377ae306e8f3ffc60e16223803420cc/controllers/popularity.py#L296-L297

lentinj commented 1 month ago

Omitting it (https://www.onezoom.org/API/otts2vns?key=0&otts=247341&lang=en) or an empty string (https://www.onezoom.org/API/otts2vns?key=0&otts=247341&lang=en&all=) will turn it off, but you're right, this is a bit confusing.

There may be a helper in gluon/web2py to use here rather than rolling our own.

Thanks!

fwimp commented 1 month ago

Yeah that's my fix for the moment is to check whether it the argument is false and then not include it in the query if that is the case.

I actually believe that gluon does not have IS_BOOL or similar as a validator.

I remember rolling our own years ago for another project. This would not do the conversion directly, but leverages strtobool() from the distutils package, which could be a viable approach here too :)

lentinj commented 1 month ago

distutils.strtobool is getting deprecated sadly: https://github.com/drgarcia1986/simple-settings/issues/273

Rolling our own would also allow us to consider doing without the lower(), which would be nice if we could.

fwimp commented 1 month ago

Ahh that's frustrating.

Looks like the original code of strtobool was just this:

def strtobool(val):
    """Convert a string representation of truth to true (1) or false (0).

    True values are 'y', 'yes', 't', 'true', 'on', and '1'; false values
    are 'n', 'no', 'f', 'false', 'off', and '0'.  Raises ValueError if
    'val' is anything else.
    """
    val = val.lower()
    if val in ('y', 'yes', 't', 'true', 'on', '1'):
        return 1
    elif val in ('n', 'no', 'f', 'false', 'off', '0'):
        return 0
    else:
        raise ValueError(f"invalid truth value {val!r}")

Wouldn't get rid of lower(), though honestly given that bools are inconsistently capitalised between languages when cast to strings, this could actually be required. (e.g. R converts as FALSE while python converts as False)