Support redirects #7

dimamo5 · 2019-05-13T13:35:22Z

This PR deprecates Volley in favor of OkHttp.

With this new HTTP client, we can handle redirects just by setting .followRedirects(true) and .followSslRedirects(true). I also noticed that only GET/HEAD requests follow the redirect with the same method. Any request with a different method is changed to a GET request. square/okhttp#2262
To avoid running into some problems in the future, I changed the way we make tracking requests. Previously, the track request was a POST with the event in the body, now the request is a GET and the event is serialized to query parameters (similar to JS tag).

I also updated the changelog with some entries from previous PRs.

codecov · 2019-05-13T13:42:22Z

Codecov Report

Merging #7 into master will increase coverage by 0.3%.
The diff coverage is 90.69%.

@@             Coverage Diff             @@
##             master       #7     +/-   ##
===========================================
+ Coverage     69.17%   69.48%   +0.3%     
  Complexity        1        1             
===========================================
  Files             8        8             
  Lines           146      154      +8     
  Branches         19       21      +2     
===========================================
+ Hits            101      107      +6     
- Misses           37       38      +1     
- Partials          8        9      +1

Impacted Files	Coverage Δ	Complexity Δ
...cidi-sdk/src/main/kotlin/com/velocidi/Constants.kt	`0% <ø> (ø)`	`0 <0> (ø)`	⬇️
...ocidi-sdk/src/main/kotlin/com/velocidi/Velocidi.kt	`76.27% <100%> (+0.4%)`	`0 <0> (ø)`	⬇️
velocidi-sdk/src/main/kotlin/com/velocidi/Util.kt	`68.75% <100%> (-1.84%)`	`0 <0> (ø)`
...elocidi-sdk/src/main/kotlin/com/velocidi/Config.kt	`90.9% <100%> (+5.19%)`	`0 <0> (ø)`	⬇️
...idi-sdk/src/main/kotlin/com/velocidi/HttpClient.kt	`83.33% <82.6%> (-1.29%)`	`1 <0> (ø)`
...c/main/kotlin/com/velocidi/GetAdvertisingIdTask.kt	`6.66% <0%> (-0.48%)`	`0% <0%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 162199a...32f7209. Read the comment docs.

thyandrecardoso · 2019-05-15T11:03:23Z

velocidi-sdk/src/main/kotlin/com/velocidi/HttpClient.kt


 /**
 * Http Client based on Android Volley
 *
 */
-internal class HttpClient {
-    private val cache = DiskBasedCache(File(Constants.CACHE_DIR), 1024 * 1024) // 1MB cap


This CACHE_DIR constant is no longer required it seems.

Removed in 211a8b1

thyandrecardoso · 2019-05-15T11:05:41Z

velocidi-sdk/src/main/kotlin/com/velocidi/HttpClient.kt


    companion object {
+
+        val JSON_MEDIA_TYPE = MediaType.get("application/json; charset=utf-8")
+
        val defaultListener = object : ResponseListener {


Can this be made private?

Improved in 211a8b1

thyandrecardoso · 2019-05-15T11:06:39Z

velocidi-sdk/src/main/kotlin/com/velocidi/Util.kt

@@ -75,17 +74,12 @@ object Util {
     * @return URL with the new parameters
     */
    @Throws(URISyntaxException::class)


Does it still throw this type?

Now, this function doesn't throw. Removed in e34a461

thyandrecardoso · 2019-05-15T11:13:38Z

velocidi-sdk/src/main/kotlin/com/velocidi/HttpClient.kt

-        start()
-    }
+internal class HttpClient {
+    var client =


Can't this be a val?

It can. Changed in fdfa6fe

velocidi-sdk/src/main/kotlin/com/velocidi/HttpClient.kt

thyandrecardoso · 2019-05-15T11:21:24Z

velocidi-sdk/src/main/kotlin/com/velocidi/events/TrackingEvent.kt

@@ -16,10 +12,39 @@ abstract class TrackingEvent(
    /**
     * Serializes the current data model to JSON
     */
-    open fun serialize(): String {
+    open fun toJson(): String {


Why do we still need this?

In my opinion, this method gives more flexibility to SDK users. E.g., they can create a pageView, convert it to JSON and add custom properties and convert it back to a CustomTrackingEvent.

I don't have a strong opinion about this. If you think it's best to be stricter on this matter, I'll remove it.

If that is the use case, can't we just provide a method to append custom properties directly from a CustomTrackingEvent?

I removed the toJson operation in da4ca9c. I also added the possibility to append proprieties to customTrackingEvents in f929a22.

thyandrecardoso · 2019-05-15T11:22:26Z

velocidi-sdk/src/test/kotlin/com/velocidi/HttpClientTest.kt

-    @Rule
-    @JvmField
-    val globalTimeout = Timeout.seconds(10) // 10 seconds max per method tested
+    // @Rule


Are these supposed to be left here?

Readded in fdfa6fe

thyandrecardoso · 2019-05-20T18:47:21Z

Previously, the track request was a POST with the event in the body, now the request is a GET and the event is serialized to query parameters (similar to JS tag)

From what I understood from here the 302 allows the client to change from POST to GET...?

Did you entertain the possibility of changing our server behavior to reply with 308s?

dimamo5 · 2019-05-21T14:36:00Z

From what I understood from here the 302 allows the client to change from POST to GET...?

That's right.

Did you entertain the possibility of changing our server behavior to reply with 308s?

As discussed offline, 308 is a recent HTTP code and may not be supported by some HTTP clients. https://serverfault.com/questions/609872/is-it-safe-to-use-http-status-308-permanent-redirect

thyandrecardoso · 2019-05-22T12:40:33Z

velocidi-sdk/src/main/kotlin/com/velocidi/events/CustomTrackingEvent.kt

@@ -14,6 +14,9 @@ class CustomTrackingEvent(
    @Transient
    override val gson = defaultGson

+    fun appendProperty(key: String, value: String) =


Shouldn't the value be of type Object?

Improved in 32f7209.

thyandrecardoso · 2019-05-22T13:00:16Z

velocidi-sdk/src/main/kotlin/com/velocidi/events/CustomTrackingEvent.kt

@@ -11,10 +11,14 @@ class CustomTrackingEvent(
    val extraAttributes: JSONObject = JSONObject()


I know this is not from this PR, but if we don't have a concrete use case, I would prefer not to include the functionality to get a CustomTrackingEvent from JSON. Also, I think the exceptions thrown should be custom (like if a given field cannot be extracted from the input JSON) and are, at this moment, generic JSON exceptions.

I think we can iterate on this later. I'll create an issue with this.

👍 Please leave that issue link here.

dimamo5 added 3 commits May 13, 2019 12:26

Migrate from volley to okhttp and change URL instances to Android Uri

4d01c20

Update sample app

bcffefe

Update changelog

72adee5

dimamo5 requested a review from thyandrecardoso May 13, 2019 13:35

dimamo5 assigned thyandrecardoso May 13, 2019

dimamo5 force-pushed the migrate-to-okhttp branch from ddc92a8 to b1524cc Compare May 13, 2019 14:38

Remove unnecessary prints

4f9e808

dimamo5 force-pushed the migrate-to-okhttp branch from b1524cc to 4f9e808 Compare May 13, 2019 14:55

thyandrecardoso requested changes May 15, 2019

View reviewed changes

dimamo5 added 3 commits May 15, 2019 14:42

Remove unused constants and improve JSON_MEDIA_TYPE visibility

211a8b1

Apply suggestions from code review

fdfa6fe

Remove unnecessary annotation

e34a461

dimamo5 force-pushed the migrate-to-okhttp branch from ad1423f to e34a461 Compare May 15, 2019 14:16

dimamo5 requested a review from thyandrecardoso May 17, 2019 11:03

dimamo5 added 2 commits May 21, 2019 15:08

Remove serialization to Json

da4ca9c

Add function to append proprieties to customTrackingEvents

f929a22

dimamo5 force-pushed the migrate-to-okhttp branch from 932e100 to f929a22 Compare May 21, 2019 14:32

thyandrecardoso requested changes May 22, 2019

View reviewed changes

Change parameter from string to Any

32f7209

thyandrecardoso approved these changes May 28, 2019

View reviewed changes

dimamo5 mentioned this pull request May 28, 2019

Improve exception handling in CustomTrackingEvent #8

Open

thyandrecardoso merged commit 03d8961 into master May 28, 2019

thyandrecardoso deleted the migrate-to-okhttp branch May 28, 2019 17:15

dimamo5 mentioned this pull request Jul 10, 2019

Dokka support #10

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support redirects #7

Support redirects #7

dimamo5 commented May 13, 2019

codecov bot commented May 13, 2019 •

edited

Loading

thyandrecardoso May 15, 2019

dimamo5 May 15, 2019

thyandrecardoso May 15, 2019

dimamo5 May 15, 2019

thyandrecardoso May 15, 2019

dimamo5 May 15, 2019

thyandrecardoso May 15, 2019

dimamo5 May 15, 2019

thyandrecardoso May 15, 2019

dimamo5 May 15, 2019

thyandrecardoso May 20, 2019

dimamo5 May 21, 2019 •

edited

Loading

thyandrecardoso May 15, 2019

dimamo5 May 15, 2019

thyandrecardoso commented May 20, 2019

dimamo5 commented May 21, 2019

thyandrecardoso May 22, 2019

dimamo5 May 27, 2019

thyandrecardoso May 22, 2019

dimamo5 May 27, 2019

thyandrecardoso May 28, 2019

dimamo5 May 28, 2019

		@@ -11,10 +11,14 @@ class CustomTrackingEvent(
		val extraAttributes: JSONObject = JSONObject()

Support redirects #7

Support redirects #7

Conversation

dimamo5 commented May 13, 2019

codecov bot commented May 13, 2019 • edited Loading

Codecov Report

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

dimamo5 May 21, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thyandrecardoso commented May 20, 2019

dimamo5 commented May 21, 2019

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

codecov bot commented May 13, 2019 •

edited

Loading

dimamo5 May 21, 2019 •

edited

Loading