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

New properties for protected attributes so downstream can depend on them #764

Closed
skewty opened this issue Dec 13, 2023 · 6 comments · Fixed by #794
Closed

New properties for protected attributes so downstream can depend on them #764

skewty opened this issue Dec 13, 2023 · 6 comments · Fixed by #794
Labels
Status: Available No one has claimed responsibility for resolving this issue. Status: In Progress This issue is being worked on, and has someone assigned. Type: Enhancement A new feature for a minor or major release.

Comments

@skewty
Copy link

skewty commented Dec 13, 2023

I am hoping the PAHO team would be open to exposing some currently protected attributes as properties so downstream projects can / will couple to them.

Reference: empicano/aiomqtt#191

Example of what is desired:

class NewClient(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
@github-actions github-actions bot added the Status: Available No one has claimed responsibility for resolving this issue. label Dec 13, 2023
@PierreF
Copy link
Contributor

PierreF commented Jan 6, 2024

I think that read properties are fine and will be done.

Write properties are an issue to me. What means changing the client_id when you are connected ? Should we disconnect and reconnect with new client_id ? Only wait for next reconnection to use the new ID (which could happen days later) ?

@PierreF
Copy link
Contributor

PierreF commented Jan 6, 2024

I've added with write for most properties. Some will only apply on next reconnection (as described in docstring).
Some write are unsupported if the connection is established due to possible unexpected condition. In that case it will a warning.
Finally some few their is not write because it's pretty sure to broken if the connection is established (e.g. protocol)

@MattBrittan MattBrittan added Type: Enhancement A new feature for a minor or major release. Status: In Progress This issue is being worked on, and has someone assigned. labels Jan 8, 2024
@akx
Copy link
Contributor

akx commented Jan 11, 2024

As discussed in the pull request, I'm not convinced it's necessarily the best idea to add setters for all properties, especially when they won't take effect immediately.

@skewty (and @frederikaalund from the aiomqtt issue): what would be the use case for setting e.g. a host or port for a client that's already connected? Could one not just create a new client in that case?

@skewty
Copy link
Author

skewty commented Jan 16, 2024

Creating a new client requires, of course, re-binding all the callbacks as well. If all my code knows about is the new host + port to use that becomes an issue.

My use case is somewhat complicated because it uses asyncio and paho is a hidden requirement. Downstream developers are reluctant to couple to protected fields (as they should be). Small decisions can have larger impacts due to this.

What is likely to happen is other projects just keep their own copy of values that are already in paho. I guess I was hoping paho would make these changes to be a little bit more open for extension (open-closed principle in SOLID) even if it doesn't need the function itself.

@PierreF
Copy link
Contributor

PierreF commented Jan 16, 2024

I don't get the use-case for writing host property with aiomqtt. It's API is made such as when the Client (paho-mqtt & aiomqtt) is created, the host is already decided and immutable or did I miss something ?

I believe only two options are valid (and prefer the first):

  • either don't allow to modify at all host, port or other options that only apply on future connection. host, port and keepalive are settable before (re)establishing the connection, using connect()
  • raise error if trying to set them while connection is open or being currently being opened

If we don't do that, the following would be nondeterministic:

client = Client()
client.connect_async("host1")
client.loop_start()
client.host = "host2"

I think we might be able to write similar case in aiomqtt (we only need to have concurrency between the connection & the client.host = "host2". The result in not deterministic, it could connect to host "host1" or "host2".

It seems much easier to just don't allow writing host, port and keepalive, especially since those 3 value are overwrite by connect() anyway.

@PierreF
Copy link
Contributor

PierreF commented Jan 20, 2024

As commented in the PR, I'll disallow changed connection related properties while connected or connecting. It'll only be allowed to change them before first connect/connect_async/reconnect or after disconnect()

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Available No one has claimed responsibility for resolving this issue. Status: In Progress This issue is being worked on, and has someone assigned. Type: Enhancement A new feature for a minor or major release.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants