Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Some public Paho client properties and methods are not exposed - example username_pw_set() #191

Closed
skewty opened this issue Feb 12, 2023 · 5 comments

Comments

@skewty
Copy link
Contributor

skewty commented Feb 12, 2023

Please either expose the Paho "Client" as a property (less preferred) or expose all its public methods and properties (preferred - also offers ability to place type hints).

Example of missing method is username_pw_set().

def username_pw_set(self, username: str | None = None, password: str | None) -> None:
    self._client.username_pw_set(username, password)

Otherwise accessing them must be done via protected "_client" which leads to linting warnings
unless something like this getattr(client, "_client").username_pw_set(username=token) is used.

@frederikaalund
Copy link
Collaborator

Hi Scott, thanks for opening this issue. Let me have a look. :)

This is a really good point. 👍 There are other issues around (as you've found; thanks for the PR!) about specific properties/methods already. It is clearly something that our users want. :)

We (asyncio-mqtt team) won't expose the underlying paho-mqtt client. Through the paho client, the user can, e.g., change the callbacks and break the higher-level API. It's a footgun.

Instead, we (asyncio-mqtt team) welcome PRs that expose properties/methods in an API-compatible way. 👍 It's actually a really good "first PR" since it's mostly just calling the underlying method on the paho-mqtt client.

Let me know if anything is unclear, or, if you have questions. 👍

~Frederik

@skewty
Copy link
Contributor Author

skewty commented Feb 13, 2023

Part of the difficulty with this is how the Paho client keeps many values protected. I spent some time yesterday looking into cleanest solution for many of the fields. In the end I came up with the following subclass of the Paho client with more fields exposed. Then the async Client class just instantiates this more exposed subclass.

class PahoClient(mqtt.Client):
    """Paho client with some exposed protected values"""

    @property
    def client_id(self) -> str:
        return self._client_id.decode()

    @client_id.setter
    def client_id(self, value: str) -> None:
        if value == "":
            if self._protocol == ProtocolVersion.V31:
                self._client_id = base62(uuid4().int, padding=22)  # copied from paho code
            else:
                self._client_id = b""
        else:
            self._client_id = value.encode()

    @property
    def host(self) -> str:
        return self._host

    @host.setter
    def host(self, value: str):
        self._host = str(value)

    @property
    def port(self) -> int:
        return self._port

    @port.setter
    def port(self, value: int) -> None:
        value = int(value)
        if 0x0000 <= value <= 0xFFFF:
            self._port = value
        raise ValueError("port number out of range")

    @property
    def username(self) -> str | None:
        return None if self._username is None else self._username.decode()

    @username.setter
    def username(self, value: str | None):
        self._username = None if value is None else value.encode()

    @property
    def password(self) -> str | None:
        return None if self._password is None else self._password.decode()

    @password.setter
    def password(self, value: str | None):
        self._password = None if value is None else value.encode()

    @property
    def connect_timeout(self) -> float:
        return self._connect_timeout

    @connect_timeout.setter
    def connect_timeout(self, value: float) -> None:
        value = float(value)
        if value <= 0.0:
            raise ValueError("timeout must be a positive number")
        self._connect_timeout = value

    @property
    def keep_alive(self) -> int:
        return self._keepalive

    @keep_alive.setter
    def keep_alive(self, value: int) -> None:
        value = int(value)
        if value <= 0:
            raise ValueError("keepalive must be a positive number")
        self._keepalive = value

    @property
    def max_inflight_messages(self) -> int:
        return self._max_inflight_messages

    @max_inflight_messages.setter
    def max_inflight_messages(self, value: int) -> None:
        if value < 0:
            raise ValueError("value must not be negative")
        self._max_inflight_messages = value

    @property
    def max_queued_messages(self) -> int:
        return self._max_queued_messages

    @max_queued_messages.setter
    def max_queued_messages(self, value: int) -> None:
        if value < 0:
            raise ValueError("value must not be negative")
        self._max_queued_messages = value

    @property
    def will_topic(self) -> str:
        return self._will_topic.decode()

    @property
    def will_payload(self) -> bytes:
        return self._will_payload

Your thoughts on an approach like this?

@frederikaalund
Copy link
Collaborator

Thanks for taking your time to look into this issue. 👍

Your thoughts on an approach like this?

In short: I fear the maintenance burden if we start to rely on private/protected member variables/methods. :)

paho-mqtt chose not to expose these variables/methods. There may or may not be a good reason for this. In any case, I vote that we don't depend on the non-public part of the paho-mqtt library.

As a side note, I know that we already do this for _client_id. That was a mistake on my part. I'd rather get rid of that than go further down that road. 😅


Alternative 1

Expose all the keyword-only parameters of Client.__init__. Basically make a bunch of propertys so that the user gets read-only access. That doesn't cover all the underlying API surface but it's a start.

Alternative 2

Move this issue to paho-mqtt first. That is, first convince paho-mqtt to expose these member variables in their public API (basically copy-paste of your property declarations 👍). Then, we can forward the very same properties in asyncio-mqtt as well.


Personally, I lean towards Alternative 2. What do you think?

I hope that makes sense. :) Again, thanks for looking into this!

@skewty
Copy link
Contributor Author

skewty commented Dec 13, 2023

Also: eclipse-paho/paho.mqtt.python#765

@skewty
Copy link
Contributor Author

skewty commented Mar 28, 2024

I tried Alternative 2 and didn't get anywhere. Closing for now.

@skewty skewty closed this as completed Mar 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants