🙌 - Added option for custom extra headers in Websocket Transport #484

Adrxx · 2018-07-07T19:03:24Z

No description provided.

MikeWeller

I'm not involved in MQTT-Client-Framework at all, but I'm leaving a few comments as my team was in the process of making similar changes.

MikeWeller · 2018-07-09T14:44:35Z

MQTTClient.podspec

 	mqttc.summary      = "iOS, macOS and tvOS native ObjectiveC MQTT Client Framework"
 	mqttc.homepage     = "https://github.com/novastone-media/MQTT-Client-Framework"
 	mqttc.license      = { :type => "EPLv1", :file => "LICENSE" }
 	mqttc.author       = { "novastonemedia" => "ios@novastonemedia.com" }
 	mqttc.source       = {
-		:git => "https://github.com/novastone-media/MQTT-Client-Framework.git",
-		:tag => "0.14.0",
+		:git => "https://github.com/ment-mx/MQTT-Client-Framework.git",


I'm guessing this change should not go in

I agree to Mike!

MikeWeller · 2018-07-09T14:45:06Z

MQTTClient.podspec

@@ -75,15 +75,15 @@ Pod::Spec.new do |mqttc|
 	end

 	mqttc.subspec 'Manager' do |manager|
-		manager.source_files =	"MQTTClient/MQTTClient/MQTTSessionManager.{h,m}", 
+		manager.source_files =	"MQTTClient/MQTTClient/MQTTSessionManager.{h,m}",


Probably best to keep these whitespace-only changes out of this diff

MikeWeller · 2018-07-09T14:51:34Z

MQTTClient/MQTTClient/MQTTWebsocketTransport/MQTTWebsocketTransport.h

+/** customHeaders an NSMutableDictionary containing extra headers sent when stablishing the websocket connection. Useful for custom authorization protocols. e.g. AWS IoT Custom Auth.
+ * defaults to an empty dictionary
+ */
+@property (nonatomic, strong) NSMutableDictionary<NSString *, NSString*> *extraHeaders;


It may be better to just make this a regular NSDictionary * with copy semantics, rather than have clients dive into a mutable data type and add/remove items. Although I don't see any existing threading/synchronization code, it makes things a little easier to reason about from a concurrency point of view. It will also bridge into Swift better: the property as is will currently bridge into Swift as an NSMutableDictionary reference type, whereas using copy and NSDictionary it will bridge as [String: String] which would be better IMO.

Then again if this is made copy I would also want to change the other array and string properties here copy which is generally good practice anyway to avoid people setting an instance of a mutable type and changing it from underneath us. But that probably doesn't make sense as part of this review.

MikeWeller · 2018-07-09T14:55:17Z

MQTTClient/MQTTClient/MQTTWebsocketTransport/MQTTWebsocketTransport.h

+/** customHeaders an NSMutableDictionary containing extra headers sent when stablishing the websocket connection. Useful for custom authorization protocols. e.g. AWS IoT Custom Auth.
+ * defaults to an empty dictionary
+ */
+@property (nonatomic, strong) NSMutableDictionary<NSString *, NSString*> *extraHeaders;


Also a slight inconsistency with the property name and the doc comment here. I would probably name this additionalHeaders which resembles the httpAdditionalHeaders on URLSessionConfiguration (just to pick an example of something similar from Apple themselves), or maybe even more explicitly additionalConnectHeaders.

Also stablishing should be establishing

MikeWeller · 2018-07-09T15:09:42Z

MQTTClient.podspec

@@ -1,13 +1,13 @@
 Pod::Spec.new do |mqttc|
 	mqttc.name         = "MQTTClient"
-	mqttc.version      = "0.14.0"
+	mqttc.version      = "0.14.1"


I'm guessing this should be an increment of the minor version, i.e. bump to 0.15.0 (or maybe that would be another PR for a release) since this is a feature enhancement rather than a bug fix.

Adrxx · 2018-07-12T19:25:05Z

I have fixed all the problems now, what do you think? ready to merge?

Adrxx · 2018-07-12T19:50:03Z

By the way. For some reason if I use the pod like this locally, it crashes with:
-[MQTTWebsocketTransport setUrl:]: unrecognized selector sent to instance I assume it has something to do with another commit #461. What happens in this case? should I go back to tag 0.14.0 and commit my changes there to another branch? Sorry, I'm really new to pull requests hahaha.

jcavar · 2018-07-18T07:08:44Z

@Adrxx can you just merge master to your branch? I think that should fix it.

jcavar · 2018-07-18T07:09:42Z

MQTTClient/MQTTClient/MQTTWebsocketTransport/MQTTWebsocketTransport.h

@@ -49,6 +49,12 @@
 * defaults to nil
 */
 @property (strong, nonatomic) NSArray *pinnedCertificates;
+
+/** additionalHeaders an NSMutableDictionary containing extra headers sent when stablishing the websocket connection. Useful for custom authorization protocols. e.g. AWS IoT Custom Auth.


This should be NSDictionaryto reflect actual type.

Adrxx · 2018-07-26T21:06:32Z

Done! @jcavar Also now everything is merged to master, I think I have finally fixed everything 😄

jcavar · 2018-07-27T07:16:18Z

MQTTClient/MQTTClient/MQTTWebsocketTransport/MQTTWebsocketTransport.h

@@ -44,6 +44,12 @@
 * defaults to nil
 */
 @property (strong, nonatomic) NSArray *pinnedCertificates;
+
+/** additionalHeaders an NSDictionary containing extra headers sent when stablishing the websocket connection. Useful for custom authorization protocols. e.g. AWS IoT Custom Auth.


I think another typo here: "stablishing"

But that is fine, I am merging this!

jcavar · 2018-07-27T07:21:12Z

Thank you @Adrxx!

Adrxx · 2018-07-27T14:07:40Z

You're welcome @jcavar! Sorry, I missed that one. Are you gonna make the correction of the typo? do you have final control of what gets merged? Let me know If I can help. By the way, I did this because the AWS IoT sampled lacked this functionality, then I realized their code https://github.com/awslabs/aws-sdk-ios-samples/tree/master/IoT-Sample/Swift is kind of a copy-paste of yours. Did you already know that?

jcavar · 2018-07-30T09:21:16Z

No, don't worry I will do it.

Which part is copy paste?

Adrxx · 2018-08-01T18:45:00Z

@jcavar Ok so turns out the copy-paste is actually from SocketRocket. Apparently AWS made its own SRWebSocket Class called AWSSRWebSocket. You can see it's copy-pasted because the comments are all the same. Comments like:

    NSAssert(self.readyState != SR_CONNECTING, @"Invalid State: Cannot call send: until connection is open");
    // TODO: maybe not copy this for performance
    data = [data copy];

I mean, I just thought it was interesting. I don't think there's anything wrong with it.

MikeWeller reviewed Jul 9, 2018

View reviewed changes

temporaryFix

4fd297c

jcavar reviewed Jul 18, 2018

View reviewed changes

📄 - Fixed docs to match the code in "additionalHeaders" property

dc89fd5

Adrxx force-pushed the master branch from 03cf44e to dc89fd5 Compare July 26, 2018 21:04

jcavar reviewed Jul 27, 2018

View reviewed changes

jcavar merged commit bc34a9f into novastone-media:master Jul 27, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🙌 - Added option for custom extra headers in Websocket Transport #484

🙌 - Added option for custom extra headers in Websocket Transport #484

Adrxx commented Jul 7, 2018

MikeWeller left a comment

MikeWeller Jul 9, 2018

mendirattanishant Jul 9, 2018

MikeWeller Jul 9, 2018

MikeWeller Jul 9, 2018

MikeWeller Jul 9, 2018

MikeWeller Jul 9, 2018

Adrxx commented Jul 12, 2018

Adrxx commented Jul 12, 2018 •

edited

Loading

jcavar commented Jul 18, 2018

jcavar Jul 18, 2018

Adrxx commented Jul 26, 2018

jcavar Jul 27, 2018

jcavar Jul 27, 2018

jcavar commented Jul 27, 2018

Adrxx commented Jul 27, 2018

jcavar commented Jul 30, 2018

Adrxx commented Aug 1, 2018 •

edited

Loading

🙌 - Added option for custom extra headers in Websocket Transport #484

🙌 - Added option for custom extra headers in Websocket Transport #484

Conversation

Adrxx commented Jul 7, 2018

MikeWeller left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Adrxx commented Jul 12, 2018

Adrxx commented Jul 12, 2018 • edited Loading

jcavar commented Jul 18, 2018

Choose a reason for hiding this comment

Adrxx commented Jul 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcavar commented Jul 27, 2018

Adrxx commented Jul 27, 2018

jcavar commented Jul 30, 2018

Adrxx commented Aug 1, 2018 • edited Loading

Adrxx commented Jul 12, 2018 •

edited

Loading

Adrxx commented Aug 1, 2018 •

edited

Loading