Broker hostname needs to start with http or https otherwise an error is raised

[jimbobbennett]

I'm working against the latest version of this with the breaking changes from #21. I've made the required changes to set the socket and changed the call to __init__.

I'm hitting a bug with connecting. When I call connect I get an error:

Traceback (most recent call last):
  File "code.py", line 120, in <module>
  File "azureiotmqtt.py", line 523, in connect
  File "azureiotmqtt.py", line 219, in _loopAssign
  File "azureiotmqtt.py", line 439, in _mqttConnect
  File "azureiotmqtt.py", line 121, in _createMQTTClient
  File "adafruit_minimqtt.py", line 220, in connect
ValueError: need more than 1 values to unpack

Digging into the code I see this at line 215:

try:
    proto, dummy, self.broker, path = self.broker.split("/", 3)
    # replace spaces in path
    path = path.replace(" ", "%20")
except ValueError:
    proto, dummy, self.broker = self.broker.split("/", 2)
    path = ""
if proto == "http:":
    self.port = MQTT_TCP_PORT
elif proto == "https:":
    self.port = MQTT_TLS_PORT
else:
    raise ValueError("Unsupported protocol: " + proto)

It looks like the code is checking for http:// or https:// at the start of the broker name, and using this to get the port. This looks wrong to me:

• I've already set the port in the call to __init__ so there is no need to look it up.
• I'm using MQTT, not over http so my broker name will never start with http:// or 'https://`, so this code will always throw an error.

My workaround is to manually prepend https:// to my broker name, which is less than ideal.

Looking at the previous version of the code, this wasn't there and there was no checking for the port, assuming the user had set it. I guess this is to work around the user not setting the port.

[brentru]

Looking at the previous version of the code, this wasn't there and there was no checking for the port, assuming the user had set it. I guess this is to work around the user not setting the port.

Yep, I updated the code to avoid the user from having to set their port.

Will look into this, assigned.

[dhalbert]

Another user hit this; they specified a numeric IP 'xxx.xxx.xxx.xxx' as the broker string, which got parsed into four octets, and then the split() failed because it wasn't a string. self.broker ends up as either octets or a string, due to this code:

Adafruit_CircuitPython_MiniMQTT/adafruit_minimqtt.py

Lines 122 to 125 in dd1e3f2

	try: # set broker IP
	self.broker = _the_interface.unpretty_ip(broker)
	except ValueError: # set broker URL
	self.broker = broker

[brendanm720]

Hello! I'm the user that Dan H just mentioned.

I think in trying to make it easier for beginners, we may have made it more difficult for advanced users who know that MQTT is not HTTP or HTTPS. I would never have dreamed that I'd have had to put in http or https in my broker address, but then again, I'm a network engineer. Also, I think it's wrong to teach beginners this particular syntax -- when they go to start using other products that use MQTT, they're going to have the the reverse of the surprise I had today.

I personally am okay with inputting the IP Address or FQDN and relying on the is_ssl option to determine if the port is 1833 or 8833 unless a custom port number attribute is defined.

[brentru]

I personally am okay with inputting the IP Address or FQDN and relying on the is_ssl option to determine if the port is 1833 or 8833 unless a custom port number attribute is defined.

I'm using MQTT, not over http so my broker name will never start with http:// or 'https://`, so this code will always throw an error.

@brendannm720 @jimbobbennett OK, I agree with both of these points. Thanks for the further clarification and both of your opinions on this. I'll be looking at MiniMQTT this upcoming week with cellular sockets, will roll back to relying on the is_ssl option to determine the port (if not already set in __init__)

[bangell]

issue #26 and this issue is the same. one self.broker = _the_interface.unpretty_ip returns 4 octets, but the remainder of the code expects a string.. I would just remove that try/except block. also you next issue will be down in connect in the first try block. in the except block it doesn't handle just a xx.xx.xx.xx for broker. so i would just check for '/' there and not do the split. Now the assumption is proto, as its not specified, do you default HTTP? Ive played around and got both xxx.xx.xx.xx and my reconnect issue working by just that little change.

[bangell]

I also think proto should be saved as it will be needed during reconnect. reconnect will need proto, port, path, and broker as they were already parsed out.