HTTP Paginated Response #783

ricardopereira · 2018-08-23T15:58:12Z

No description provided.

This slipped through the cracks of this PR https://github.com/ably/ably-ios/pull/777/files

Fix RSC7b test

Source/ARTHttp.m

funkyboy

I left some observations. Nothing major, but I think it's worth making those clear before merging.

Source/ARTConstants.m

funkyboy · 2018-09-05T15:10:27Z

Source/ARTHTTPPaginatedResponse.m

+                userCallback(result, error);
+            });
+        };
+    }


This if looks the same as implemented in first:. Worth extracting it in separate method?

funkyboy · 2018-09-05T15:13:56Z

Source/ARTHTTPPaginatedResponse.m

+
+            ARTHTTPPaginatedResponse *result = [[ARTHTTPPaginatedResponse alloc] initWithResponse:response items:items rest:rest relFirst:firstRel relCurrent:currentRel relNext:nextRel responseProcessor:responseProcessor];
+
+            callback(result, [ARTErrorInfo createFromNSError:error]);


I don't understand why the error is passed along here. Shouldn't it be nil?

I changed it 861f32b. Thanks

Source/ARTStatus.m

Source/Ably.modulemap

Spec/RestPaginated.swift

funkyboy · 2018-09-05T15:28:40Z

Spec/RestPaginated.swift

+                    fail("First link isn't a valid URL"); return
+                }
+
+                expect(firstRequest.url?.absoluteString).to(equal("https://sandbox-rest.ably.io:443/channels/foo/messages?start=0&end=1535035746063&limit=100&direction=backwards&format=msgpack&firstEnd=1535035746063&fromDate=1535035746063&mode=all"))


funkyboy · 2018-09-05T15:29:07Z

Spec/RestPaginated.swift

+            }
+        }
+    }
+}


Can we have also tests for next and last?

Hmm, that's a good point but we need something to respond with the ref next and last links.

funkyboy

The major one is likely the direct reference to "sandbox"

paddybyers

See comment

paddybyers · 2018-09-06T09:54:06Z

Spec/RestClient.swift

                                    guard let error = error else {
-                                        fail("Error is nil"); done(); return
+                                        fail("Error is empty"); done(); return


I don't understand why you expect there to be an error in this case. The semantics of this method is that the request was made, and there was a response, and that's that. There was no error.

The response includes a statusCode that is an "error" result, but the request itself didn't fail. It succeeded, and returned a result.

Also, pls can we have tests for requests that do indeed fail - for example it times out, fails to resolve the host, returns a response body that cannot be parsed, etc.

@paddybyers this is making a GET /feature which doesn't exist, so the request fails and the error should not be nil. This guard simply makes the test fail if the error is nil.

No, that's not right. If the server responds to the request, and that response is well-formed (even though it is a 404), then that counts as success in this API. There is no error, but the response, encapsulated in the HttpPaginatedResponse, will indicate that the content of the response was an error, appropriately populating success, statusCode, errorCode and errorMessage.

RSC19e states that this method does result in an error if the server did not respond, or its response was not well-formed in some way, so it was not possible to return a HttpPaginatedResponse.

Oh I see now. So the error in nested in the pagination block of the response, not on the first level of the returned object.

No, that's not right. If the server responds to the request, and that response is well-formed (even though it is a 404), then that counts as success in this API. There is no error, but the response, encapsulated in the HttpPaginatedResponse, will indicate that the content of the response was an error, appropriately populating success, statusCode, errorCode and errorMessage.

That makes it clear, yes. Thanks. I'll fix it.

Fixed 63ebbf5.

paddybyers · 2018-09-13T18:10:06Z

Spec/RestClient.swift

                                    guard let paginatedResponse = paginatedResponse else {
                                        fail("PaginatedResult is empty"); done(); return
                                    }
-                                    expect(error.statusCode) == 404


Why are there no corresponding assertions for the properties of paginatedResponse ?

@paddybyers There already exist at the bottom. Did you miss them? https://github.com/ably/ably-ios/blob/63ebbf5a24d5807a19e07ddbfb2de0e9e65def24/Spec/RestClient.swift#L1642-L1647

@paddybyers 😄 So we're good?

paddybyers

LGTM, thanks

funkyboy and others added 7 commits August 23, 2018 11:35

Fix RSC7b test

cff4ce9

This slipped through the cracks of this PR https://github.com/ably/ably-ios/pull/777/files

Merge pull request #782 from ably/fix-RSC7b-test

f46bcaa

Fix RSC7b test

ARTHTTPPaginatedResponse

a506086

Update RSC19a with the use of ARTHTTPPaginatedResponse

60cc6c4

Implement ARTHTTPPaginatedResponse

3e04100

Test: response links extraction

3b14620

fixup! Update RSC19a with the use of ARTHTTPPaginatedResponse

6f716c4

ricardopereira mentioned this pull request Aug 23, 2018

RSC19a #754

Merged

ricardopereira requested a review from paddybyers August 23, 2018 16:00

ricardopereira commented Aug 23, 2018

View reviewed changes

Source/ARTHttp.m Show resolved Hide resolved

Merge branch 'develop' into http-paginated-response

85711c6

funkyboy reviewed Sep 5, 2018

View reviewed changes

funkyboy suggested changes Sep 6, 2018

View reviewed changes

paddybyers requested changes Sep 6, 2018

View reviewed changes

ricardopereira added 2 commits September 13, 2018 16:06

fixup! Update RSC19a with the use of ARTHTTPPaginatedResponse

63ebbf5

fixup! Implement ARTHTTPPaginatedResponse

861f32b

paddybyers reviewed Sep 13, 2018

View reviewed changes

paddybyers approved these changes Sep 13, 2018

View reviewed changes

ricardopereira merged commit d57d60f into rsc19a Sep 21, 2018

ricardopereira deleted the http-paginated-response branch September 21, 2018 13:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HTTP Paginated Response #783

HTTP Paginated Response #783

ricardopereira commented Aug 23, 2018

funkyboy left a comment

funkyboy Sep 5, 2018

funkyboy Sep 5, 2018

ricardopereira Sep 13, 2018

funkyboy Sep 5, 2018

funkyboy Sep 5, 2018

ricardopereira Sep 13, 2018

funkyboy left a comment

paddybyers left a comment

paddybyers Sep 6, 2018

funkyboy Sep 6, 2018

paddybyers Sep 6, 2018

funkyboy Sep 6, 2018

ricardopereira Sep 6, 2018

ricardopereira Sep 13, 2018

paddybyers Sep 13, 2018

ricardopereira Sep 13, 2018

paddybyers Sep 13, 2018

ricardopereira Sep 13, 2018

paddybyers left a comment


		ARTHTTPPaginatedResponse *result = [[ARTHTTPPaginatedResponse alloc] initWithResponse:response items:items rest:rest relFirst:firstRel relCurrent:currentRel relNext:nextRel responseProcessor:responseProcessor];

		callback(result, [ARTErrorInfo createFromNSError:error]);

HTTP Paginated Response #783

HTTP Paginated Response #783

Conversation

ricardopereira commented Aug 23, 2018

funkyboy 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

funkyboy left a comment

Choose a reason for hiding this comment

paddybyers 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

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

paddybyers left a comment

Choose a reason for hiding this comment