Skip to content

Use the private setter pattern on SessionGenerator.currentSession #4785

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Mar 16, 2023
Merged

Use the private setter pattern on SessionGenerator.currentSession #4785

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1ef87bf
Use the private setter pattern on SessionGenerator.currentSession
mrober Mar 15, 2023
6b6ab5f
Comments
mrober Mar 16, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 20 additions & 26 deletions firebase-sessions/src/main/kotlin/com/google/firebase/sessions/SessionGenerator.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,35 +36,29 @@ internal data class SessionDetails(
*
* @hide
*/
internal class SessionGenerator(private var collectEvents: Boolean) {
private var firstSessionId = ""
private var sessionIndex: Int = -1
internal class SessionGenerator(
private val collectEvents: Boolean,
private val uuidGenerator: () -> UUID = UUID::randomUUID
) {
private val firstSessionId = generateSessionId()
private var sessionIndex = -1

private var thisSession: SessionDetails =
SessionDetails(
sessionId = "",
firstSessionId = "",
collectEvents,
sessionIndex,
)
/** The current generated session, must not be accessed before calling [generateNewSession]. */
lateinit var currentSession: SessionDetails
private set
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this not associated with any var/val? Felt like a hanging private set.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the setter for currentSession, I think it looks more clear now that we removed the default instance.


// Generates a new Session ID. If there was already a generated Session ID
// from the last session during the app's lifecycle, it will also set the last Session ID
/** Generates a new session. The first session's sessionId will match firstSessionId. */
fun generateNewSession(): SessionDetails {
val newSessionId = UUID.randomUUID().toString().replace("-", "").lowercase()

// If firstSessionId is set, use it. Otherwise set it to the
// first generated Session ID
firstSessionId = firstSessionId.ifEmpty { newSessionId }

sessionIndex += 1

thisSession =
SessionDetails(sessionId = newSessionId, firstSessionId, collectEvents, sessionIndex)

return thisSession
sessionIndex++
currentSession =
SessionDetails(
sessionId = if (sessionIndex == 0) firstSessionId else generateSessionId(),
firstSessionId,
collectEvents,
sessionIndex,
)
return currentSession
}

val currentSession: SessionDetails
get() = thisSession
private fun generateSessionId() = uuidGenerator().toString().replace("-", "").lowercase()
}
120 changes: 87 additions & 33 deletions firebase-sessions/src/test/kotlin/com/google/firebase/sessions/SessionGeneratorTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
package com.google.firebase.sessions

import com.google.common.truth.Truth.assertThat
import java.util.UUID
import org.junit.Test

class SessionGeneratorTest {
Expand All @@ -34,66 +35,119 @@ class SessionGeneratorTest {
}

// This test case isn't important behavior. Nothing should access
// currentSession before generateNewSession has been called. This test just
// ensures it has consistent behavior.
@Test
fun currentSession_beforeGenerateReturnsDefault() {
// currentSession before generateNewSession has been called.
@Test(expected = UninitializedPropertyAccessException::class)
fun currentSession_beforeGenerate_throwsUninitialized() {
val sessionGenerator = SessionGenerator(collectEvents = false)

assertThat(sessionGenerator.currentSession.sessionId).isEqualTo("")
assertThat(sessionGenerator.currentSession.firstSessionId).isEqualTo("")
assertThat(sessionGenerator.currentSession.collectEvents).isFalse()
assertThat(sessionGenerator.currentSession.sessionIndex).isEqualTo(-1)
sessionGenerator.currentSession
}

@Test
fun generateNewSessionID_generatesValidSessionDetails() {
val sessionGenerator = SessionGenerator(collectEvents = true)
fun generateNewSession_generatesValidSessionIds() {
val sessionGenerator = SessionGenerator(collectEvents = true) // defaults to UUID::randomUUID

sessionGenerator.generateNewSession()

assertThat(isValidSessionId(sessionGenerator.currentSession.sessionId)).isTrue()
assertThat(isValidSessionId(sessionGenerator.currentSession.firstSessionId)).isTrue()
assertThat(sessionGenerator.currentSession.firstSessionId)
.isEqualTo(sessionGenerator.currentSession.sessionId)
assertThat(sessionGenerator.currentSession.collectEvents).isTrue()
assertThat(sessionGenerator.currentSession.sessionIndex).isEqualTo(0)

// Validate several random session ids.
repeat(16) {
assertThat(isValidSessionId(sessionGenerator.generateNewSession().sessionId)).isTrue()
}
}

// Ensures that generating a Session ID multiple times results in the fist
// Session ID being set in the firstSessionId field
@Test
fun generateNewSessionID_incrementsSessionIndex_keepsFirstSessionId() {
val sessionGenerator = SessionGenerator(collectEvents = true)
fun generateNewSession_generatesValidSessionDetails() {
val sessionGenerator = SessionGenerator(collectEvents = true, uuidGenerator = UUIDs()::next)

sessionGenerator.generateNewSession()

val firstSessionDetails = sessionGenerator.currentSession
assertThat(isValidSessionId(sessionGenerator.currentSession.sessionId)).isTrue()
assertThat(isValidSessionId(sessionGenerator.currentSession.firstSessionId)).isTrue()

assertThat(sessionGenerator.currentSession)
.isEqualTo(
SessionDetails(
sessionId = SESSION_ID_1,
firstSessionId = SESSION_ID_1,
collectEvents = true,
sessionIndex = 0,
)
)
}

// Ensures that generating a Session ID multiple times results in the fist
// Session ID being set in the firstSessionId field
@Test
fun generateNewSession_incrementsSessionIndex_keepsFirstSessionId() {
val sessionGenerator = SessionGenerator(collectEvents = true, uuidGenerator = UUIDs()::next)

val firstSessionDetails = sessionGenerator.generateNewSession()

assertThat(isValidSessionId(firstSessionDetails.sessionId)).isTrue()
assertThat(isValidSessionId(firstSessionDetails.firstSessionId)).isTrue()
assertThat(firstSessionDetails.firstSessionId).isEqualTo(firstSessionDetails.sessionId)
assertThat(firstSessionDetails.sessionIndex).isEqualTo(0)

sessionGenerator.generateNewSession()
val secondSessionDetails = sessionGenerator.currentSession
assertThat(firstSessionDetails)
.isEqualTo(
SessionDetails(
sessionId = SESSION_ID_1,
firstSessionId = SESSION_ID_1,
collectEvents = true,
sessionIndex = 0,
)
)

val secondSessionDetails = sessionGenerator.generateNewSession()

assertThat(isValidSessionId(secondSessionDetails.sessionId)).isTrue()
assertThat(isValidSessionId(secondSessionDetails.firstSessionId)).isTrue()
// Ensure the new firstSessionId is equal to the first Session ID from earlier
assertThat(secondSessionDetails.firstSessionId).isEqualTo(firstSessionDetails.sessionId)
// Session Index should increase
assertThat(secondSessionDetails.sessionIndex).isEqualTo(1)

// Ensure the new firstSessionId is equal to the first sessionId, and sessionIndex increased
assertThat(secondSessionDetails)
.isEqualTo(
SessionDetails(
sessionId = SESSION_ID_2,
firstSessionId = SESSION_ID_1,
collectEvents = true,
sessionIndex = 1,
)
)

// Do a third round just in case
sessionGenerator.generateNewSession()
val thirdSessionDetails = sessionGenerator.currentSession
val thirdSessionDetails = sessionGenerator.generateNewSession()

assertThat(isValidSessionId(thirdSessionDetails.sessionId)).isTrue()
assertThat(isValidSessionId(thirdSessionDetails.firstSessionId)).isTrue()
// Ensure the new firstSessionId is equal to the first Session ID from earlier
assertThat(thirdSessionDetails.firstSessionId).isEqualTo(firstSessionDetails.sessionId)
// Session Index should increase
assertThat(thirdSessionDetails.sessionIndex).isEqualTo(2)

assertThat(thirdSessionDetails)
.isEqualTo(
SessionDetails(
sessionId = SESSION_ID_3,
firstSessionId = SESSION_ID_1,
collectEvents = true,
sessionIndex = 2,
)
)
}

private class UUIDs(val names: List<String> = listOf(UUID_1, UUID_2, UUID_3)) {
var index = -1

fun next(): UUID {
index = (index + 1).coerceAtMost(names.size - 1)
return UUID.fromString(names[index])
}
}

@Suppress("SpellCheckingInspection") // UUIDs are not words.
companion object {
const val UUID_1 = "11111111-1111-1111-1111-111111111111"
const val SESSION_ID_1 = "11111111111111111111111111111111"
const val UUID_2 = "22222222-2222-2222-2222-222222222222"
const val SESSION_ID_2 = "22222222222222222222222222222222"
const val UUID_3 = "CCCCCCCC-CCCC-CCCC-CCCC-CCCCCCCCCCCC"
const val SESSION_ID_3 = "cccccccccccccccccccccccccccccccc"
}
}