AutosoftDMS / SignalR-Swift

SignalR client library written in pure Swift
MIT License
60 stars 55 forks source link

HttpTransport encodedRequest call is unnecessary #34

Closed kamrankhan07 closed 6 years ago

kamrankhan07 commented 6 years ago

In HttpTransport class, this line of code is executing this url using get method. As far as I could figure out, this line is there just to generate the url with query string. If that is the case then we should be just generating the url with query string and not execute it like this.

let encodedRequest = connection.sessionManager.request(url, method: .get, parameters: parameters.toJSON(), encoding: URLEncoding.default, headers: nil)

The reason, when SignalR tries to connect to a hub using Server Sent Events or Long Polling, the call with the above code always fail and we were getting a lot of errors being generated in the log, such as

Value cannot be null. Parameter name: s

Can anyone please let me know if that is the case, I checked the change log for HttpTransport class and was confused by one of the commits ("a self-signed SSL certificate support", 63b657f6913a314c301128d558b13d4d6b513aa9).

@vldalx @jordancamara can you please explain if this call is necessary to be executed, or are we just creating url with query string and missed the point that it is also executing the request with GET method. I already have a fix, if that is the case, I can make a pull request.

vldalx commented 6 years ago

There is a pull request. Take a look at the description. If you connect through HTTPS protocol and have a not valid SSL certificate, all requests will be rejected. In order to get a valid certificate, you have to pay for it. So, it is common practice to create a self-signed certificate for a testing purpose. Using Alamofire.Manager it is possible to fine-tune http requests outside the library.

By the way, you are not the one who got that error. There is an issue in the official repository. It seems like there is something wrong with you environment.

kamrankhan07 commented 6 years ago

@vldalx thanks for quick reply. I checked the pull request and I understand what you did, you re-routed all the requests via custom manager. The issue I am facing is with that single line in HttpTransport. Even if I switch it back to Alamofire.request it won't solve the issue. There are two lines of code that are making requests in the method

public func send(connection: ConnectionProtocol, data: Any, connectionData: String?, completionHandler: ((Any?, Error?) -> ())?)

The first one's response object is just used to extract url (with query string)

let encodedRequest = connection.sessionManager.request(url, method: .get, parameters: parameters, encoding: URLEncoding.default, headers: nil) -- 1

let request = connection.getRequest(url: encodedRequest.request!.url!.absoluteString, httpMethod: .post, encoding: URLEncoding.httpBody, parameters: requestParams) -- 2

In my opinion, first call (1) is not necessary, and there was mistake in code from the start as it executes the url when request() method is called, either using Alamofire.request or connection.sessionManager.request. It doesn't look like the first call has any other task than to create the url for the next call (2), which is doing the real work.

vldalx commented 6 years ago

Seems like you are right. It's strange I don't have this issue in my application. Does you backend have [Authorize] attribute on a Hub?. Take a look at this issue.

kamrankhan07 commented 6 years ago

No, we don't have any [Authorize] attribute on the backend. I think the other issue might be because of this too. But for us the first call was failing because the client on server side couldn't parse the response, it was telling us that parameter 's' is null. I tried adding dummy value for 's' but it didn't work even then (may be I made a mistake adding it the proper way). So, I guess when they add [Authorize] parameter, the backend client parses the request correctly and then connects, thats why they have two connections.

vldalx commented 6 years ago

You have found definitely a mistake. We can check it here. This library is based on SignalR-ObjC.

kamrankhan07 commented 6 years ago

yes, you are right. I checked the code and its generating the url using AFHTTPRequestSerializer to use with AFHTTPRequestOperation. However, on Swift with Alamofire it also executes the request. @jordancamara can you please check this issue and merge the pull request.

 //TODO: this is a little strange but SignalR Expects the parameters in the queryString and fails if in the body.
    //So we let AFNetworking Generate our URL with proper encoding and then create the POST url which will encode the data in the body.
    NSMutableURLRequest *url = [[AFHTTPRequestSerializer serializer] requestWithMethod:@"GET" URLString:[connection.url stringByAppendingString:@"send"] parameters:parameters error:nil];
NSMutableURLRequest *request = [[AFHTTPRequestSerializer serializer] requestWithMethod:@"POST" URLString:[[url URL] absoluteString] parameters:@{ @"data" : data } error:nil];