CM: WIP for android http client #127

carlo-backblaze · 2020-09-21T19:13:12Z

WIP, do not merge
Will most likely be split into it's own repository
Creating separate gradle project including an Android application for instrumented tests and examples httpclient_android/app
Creating a httpclient library targeting Android frameworks in httpclient_android/http_client
Creating base library unit tests
Removing the Apache HTTP Client framework dependencies
Using URLConnection, HttpUrlConnection, and HttpsUrlConnection
Will be outputting .aar library for publishing

...ava/com/backblaze/b2/client/webapihttpclient/android/http_client/B2WebApiHttpClientImpl.java

malaysf · 2020-10-15T04:47:17Z

...ava/com/backblaze/b2/client/webapihttpclient/android/http_client/B2WebApiHttpClientImpl.java

+            URL clientUrl = new URL(url);
+            URLConnection head;
+            if (clientUrl.getProtocol().equals("https")) {
+                 head = (HttpsURLConnection) clientUrl.openConnection();
+            } else {
+                head = (HttpURLConnection) clientUrl.openConnection();
+            }


this looks similar to code above, perhaps factor it out into something that take a url string and returns the URLConnection instance for it?

Now that I look more closely at it, is the if statement necessary? It looks like we make an openConnection on the clientUrl and then case it to a specific Http/Https flavor of UrlConnection, but then assign it to a variable of type URLConnection.

The difference is that each HttpsURLConnection has all of the SSL methods. I can create a convenience builder/class if you think that is necessary.

...ava/com/backblaze/b2/client/webapihttpclient/android/http_client/B2WebApiHttpClientImpl.java

malaysf

there are some empty files like
httpclient_android/TODO

can these be removed?

Also, there are some image files in the diff, are those needed?

...a/com/backblaze/b2/client/webApiHttpClient/android/UrlConnection/B2WebApiHttpClientImpl.java

...roid/app/src/test/java/com/backblaze/b2/client/webApiHttpClient/android/ExampleUnitTest.java

...a/com/backblaze/b2/client/webApiHttpClient/android/UrlConnection/B2WebApiHttpClientImpl.java

malaysf · 2020-11-19T05:21:40Z

...backblaze/b2/client/webapihttpclient/android/http_client/B2StorageHttpClientBuilderTest.java

+public class B2StorageHttpClientBuilderTest {
+
+    @Test
+    public void B2StorageHttpClientBuilder_builder() {


are there any real tests we can put in here?

Yes. of course

...va/com/backblaze/b2/client/webapihttpclient/android/http_client/ExampleInstrumentedTest.java

malaysf · 2020-11-19T05:27:16Z

...m/backblaze/b2/client/webApiHttpClient/android/UrlConnection/B2StorageHttpClientBuilder.java

+ * Copyright 2020, Backblaze Inc. All Rights Reserved.
+ * License https://www.backblaze.com/using_b2_code.html
+ */
+package com.backblaze.b2.client.webApiHttpClient.android.UrlConnection;


the webApiHttpClient name refers to a webApi implementation using the Apache HttpClient library.

I think the correct package name here would be:
com.backblaze.b2.client.webApiUrlConnection

I would also advocate for making this a regular java module as there is nothing Android specific about this. If needed, create a second Gradle file to allow it to be built for Android.

malaysf · 2020-11-19T05:27:57Z

...android/app/src/main/java/com/backblaze/b2/client/webApiHttpClient/android/MainActivity.java

+
+import android.os.Bundle;
+
+public class MainActivity extends AppCompatActivity {


is this file needed?

It's an example Android App where integration tests/example can be added.

malaysf · 2020-11-19T05:28:13Z

...droidTest/java/com/backblaze/b2/client/webApiHttpClient/android/ExampleInstrumentedTest.java

+ * @see <a href="http://d.android.com/tools/testing">Testing documentation</a>
+ */
+@RunWith(AndroidJUnit4.class)
+public class ExampleInstrumentedTest {


looks like an example file that can be removed.

…ava into cmogavero/url-cxn-wip

malaysf · 2021-03-22T04:22:20Z

...ackblaze/b2/client/webapiurlconnection/android/UrlConnection/B2StorageHttpClientBuilder.java

@@ -0,0 +1,101 @@
+/*


Thanks for renaming the package, but please also move all these files into the core project.

malaysf · 2021-03-22T04:27:09Z

...com/backblaze/b2/client/webapiurlconnection/android/UrlConnection/HttpClientFactoryImpl.java

+            connection.disconnect();
+        } catch (Error e) {
+            // restore the interrupt because we're not acting on it here.
+            if (e.getMessage() != null) {


should this swallow the error here or rethrow?

malaysf · 2021-03-22T04:34:30Z

...laze/b2/client/webapiurlconnection/android/UrlConnection/B2StorageHttpClientBuilderTest.java

+    public void B2StorageHttpClientBuilder_setHttpClientFactory() {
+        final HttpClientFactory clientFactory = HttpClientFactoryImpl.build();
+        final B2StorageClient factory = new B2StorageHttpClientBuilder.build();
+        final B2StorageHttpClientBuilder factoryTwo = factory.setHttpClientFactory(clientFactory);


I'm confused, B2StorageClient doesn't have a method setHttpClientFactory. Am I missing something?

malaysf · 2021-03-22T04:35:22Z

...laze/b2/client/webapiurlconnection/android/UrlConnection/B2StorageHttpClientBuilderTest.java

+ *
+ * @see <a href="http://d.android.com/tools/testing">Testing documentation</a>
+ */
+public class B2StorageHttpClientBuilderTest {


These tests need to be more detailed, more than just asserting non null. Check default values and that specified values are set in the built B2StorageClient instance.

malaysf · 2021-03-22T04:36:02Z

...laze/b2/client/webapiurlconnection/android/UrlConnection/B2StorageHttpClientFactoryTest.java

+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertTrue;
+
+public class B2StorageHttpClientFactoryTest {


like above, tests need to do more than assert non null.

malaysf · 2021-03-22T04:44:43Z

...com/backblaze/b2/client/webapiurlconnection/android/UrlConnection/HttpClientFactoryImpl.java

+            connection.setConnectTimeout(this.connectTimeoutSeconds);
+            return connection;
+        } catch (Error e) {
+            // Log


missing log statement? Should this also turn around and throw a B2Exception?

malaysf · 2021-03-22T04:54:04Z

...om/backblaze/b2/client/webapiurlconnection/android/UrlConnection/B2WebApiHttpClientImpl.java

+                final Scanner input = new Scanner(new InputStreamReader(client.getInputStream()));
+                final StringBuilder response = new StringBuilder();
+                String line;
+                while ((line = input.hasNext())) {
+                    response.append(line);
+                }
+                input.close();


this should be factored out into a helper method since it is used below as well

malaysf · 2021-03-22T04:57:12Z

...om/backblaze/b2/client/webapiurlconnection/android/UrlConnection/B2WebApiHttpClientImpl.java

+        if (clientUrl.getProtocol().equals("https")) {
+            client = (HttpsURLConnection) clientUrl.openConnection();
+        } else {
+            client = (HttpURLConnection) clientUrl.openConnection();
+        }


is this necessary? clientUrl.openConnection returns a URLConnection instance already so downcasting to HttpsURLConnection or HttpURLConnection and then storing in a URLConnection instance is unnecessary.

I think this could be reduced down to:

final URLConnection client = clientUrl.openConnection();

This applies to the several occurrences of this code.

malaysf · 2021-03-22T05:01:21Z

...om/backblaze/b2/client/webapiurlconnection/android/UrlConnection/B2WebApiHttpClientImpl.java

+     * @param request the object to be json'ified.
+     * @return a new ByteArrayEntity with the json representation of request in it.
+     */
+    private static String parseToStringUsingBzJson(Object request) throws B2Exception {


I think you meant to name this parseToStringUsingB2Json instead of parseToStringUsingBzJson?

malaysf · 2021-03-22T05:03:21Z

...om/backblaze/b2/client/webapiurlconnection/android/UrlConnection/B2WebApiHttpClientImpl.java

+     * @return the body of the response.
+     * @throws B2Exception if there's any trouble
+     */
+    private String postAndReturnString(String url, B2Headers headersOrNull, String requestString)


can requestString be wrapped as an InputStream and call the other overload of postAndReturnString so that there's only one core implementation?

caffed added 5 commits September 21, 2020 12:12

CM: WIP for android http client

81aafcb

CM: WIP

13ca830

CM: WIP

43c5656

CM: WIP

5157de3

CM: WIP

38e8a3b

malaysf requested changes Oct 15, 2020

View reviewed changes

caffed added 3 commits November 16, 2020 13:52

CM: PR review

3a76ac5

CM: pr feedback

92e1ab0

CM: preliminary READEME

2706ade

malaysf requested changes Nov 19, 2020

View reviewed changes

caffed and others added 6 commits February 4, 2021 13:37

CM: PR feedback and reorg

610d657

CM: Addressing additonal feedback

8e4b42c

CM: removing TODO

e7724c0

Merge branch 'cmogavero/url-cxn-wip' of github.com:Backblaze/b2-sdk-j…

67ce33f

…ava into cmogavero/url-cxn-wip

CM: removing example test

b492506

CM: dev-5605

4c819f8

malaysf requested changes Mar 22, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CM: WIP for android http client #127

CM: WIP for android http client #127

carlo-backblaze commented Sep 21, 2020 •

edited

Loading

malaysf Oct 15, 2020

carlo-backblaze Mar 1, 2021

malaysf left a comment

malaysf Nov 19, 2020

carlo-backblaze Mar 1, 2021

malaysf Nov 19, 2020

malaysf Nov 19, 2020

carlo-backblaze Mar 19, 2021

malaysf Nov 19, 2020

malaysf Mar 22, 2021

malaysf Mar 22, 2021

malaysf Mar 22, 2021

malaysf Mar 22, 2021

malaysf Mar 22, 2021

malaysf Mar 22, 2021

malaysf Mar 22, 2021

malaysf Mar 22, 2021

malaysf Mar 22, 2021

malaysf Mar 22, 2021


		import android.os.Bundle;

		public class MainActivity extends AppCompatActivity {

CM: WIP for android http client #127

Are you sure you want to change the base?

CM: WIP for android http client #127

Conversation

carlo-backblaze commented Sep 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

malaysf 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

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

carlo-backblaze commented Sep 21, 2020 •

edited

Loading