vaphes / pocketbase

PocketBase client SDK for python
https://pypi.org/project/pocketbase/
MIT License
331 stars 38 forks source link

Remove class variables #70

Closed simon-lund closed 9 months ago

simon-lund commented 11 months ago

In the client class (and some other classes) I have seen variables defined in the class body. Those are class variables, and a client instance per se should not share any variables with another client instance. This would, for example, be really bad for APIs security-wise).

class Foo:
    x: int = 1    # class variable

class Bar:
    def __init__():
        x = 1    # instance variable

You don't use the class variables as you define instance variables in the init shadowing the class variables. However, this still feels a bit insecure to me, leaving room for error.

So, I'd like to suggest two things:

  1. Remove all class variables that should not be shared between client instances (e.g. AuthStore)
  2. Check for certain class variables if they are defined and do not initialize them with default value. For example, setting Client.base_url="..." would be neat, but is currently shadowed by instance variable.

Further Explanation: https://stackoverflow.com/a/43921843

m29h commented 11 months ago

looks like you have a valid point. as you already mention, more or less all the static variables are "shadowed" already during the object init. The class variables do not hold any value in any scenario. I am also not really aware of a use-case of this library where this could even theoretically be a security issue.

maybe @vaphes can comment on why the static variables were defined in the first place. If i would make a guess this originally may have been done for type hinting purposes. (there are obviously other/better ways to do the type hinting directly on the instance variables.)

@simon-lund feel free to propose a Pull Request on this issue if you have some time to work on this

simon-lund commented 11 months ago

Sure, I can make the changes.

Would like to know beforehand whether @vaphes agrees.

vaphes commented 11 months ago

@simon-lund I agree, this was probably done when I was translating the pocketbase js lib to python and went I bit braindead on the perks of each lang.

If you propose a pull request I will promptly verify it.

As always @m29h thx a lot for the work on this lib.