Repository navigation
refactor(android SDK): convert four B1 networking classes to Kotlin - #985
nickolas-dimitrakas merged 1 commit into
Conversation
Kotlin migration progressThis pull request converts 228 lines of Java to Kotlin.
Conversion progress: 7.6% (15,300 of 16,550 baseline Java LOC left to convert). 110 Java files and 58 Kotlin files in scope. |
Binary compatibility
Compares this branch with the last version published to Maven Central, both built without R8, over the public classes and members that release keeps under their own names. It also checks that this branch's release AAR still keeps them (see the script docstring). Full japicmp HTML/XML reports are attached as the |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. It converts classes on the upload and certificate-pinning network path, and it changes the declared shape of the public MParticleBaseClient and MParticleApiClient interfaces, so a look from the SDK team is still worthwhile.
What was reviewed:
MPUrlImplandMPConnectionImplagainst the deleted Java. Each!!dereferences at the point where the Java first used the value, and the SSL socket factory cast still throws for non-HTTPS connections, as it did before.MParticleApiClient/AliasNetworkResponse: the constructor andsetErrorMessagekeep their JVM names, and@ NonNull/@ Nullableare kept onsendAliasRequestandgetErrorMessage.requestHandleras a non-nullvar: Kotlin callers (PinningTestHelper,UserAudiencesRetriever,UploadHandlerTest) are compatible. No Kotlin class implements either interface with non-null overrides that the change would break.
Extended reasoning...
The change converts four android-core networking types from Java to Kotlin in place (MParticleApiClient, MParticleBaseClient, MPUrlImpl, MPConnectionImpl) and adds about 300 lines of new unit tests. It touches the HTTP connection wrapper, including where the SSL socket factory is set for certificate pinning, but it adds no new logic there. Two public interfaces change their nullability and accessor declarations: requestHandler becomes a non-null property, and the message and uploadSettings parameters, which were @ NonNull, become nullable. The PR author says these are deliberate choices. CODEOWNERS assigns every path to the SDK team, and these API-surface decisions belong to them, so this is deferred rather than approved.
📦 SDK Size Impact ReportWhat the SDK adds to a minified release APK. mParticle Core SDKMeasured against an empty baseline app. Unlike the Rokt kit, android-core ships no Compose and no resources, so there is nothing here that a host app would already provide.
mParticle Core + Rokt kitMeasured against a Compose + Material3 reference app, so these are the costs on top of an app that already ships Compose. The reference app's dependencies are a documented convention, not a measured average: see
Rokt SDK+ umbrella (adds the payment extension)
➡️ SDK size impact change is minimal. Raw measurementscore, Target branch: {"baseline_dex_bytes": 0, "baseline_download_bytes": 2513, "baseline_install_bytes": 7530, "core_dex_bytes": 221948, "core_download_bytes": 124034, "core_install_bytes": 131286}core, This PR: {"baseline_dex_bytes": 0, "baseline_download_bytes": 2510, "baseline_install_bytes": 7526, "core_dex_bytes": 222180, "core_download_bytes": 124076, "core_install_bytes": 131327}rokt, Target branch: {"baseline_dex_bytes": 1829108, "baseline_download_bytes": 1240062, "baseline_install_bytes": 1300934, "kit_dex_bytes": 4171332, "kit_download_bytes": 2548131, "kit_install_bytes": 2623275, "sdkplus_dex_bytes": 9293096, "sdkplus_download_bytes": 7985735, "sdkplus_install_bytes": 8165359}rokt, This PR: {"baseline_dex_bytes": 1829108, "baseline_download_bytes": 1240062, "baseline_install_bytes": 1300934, "kit_dex_bytes": 4171552, "kit_download_bytes": 2548145, "kit_install_bytes": 2623275, "sdkplus_dex_bytes": 9293196, "sdkplus_download_bytes": 7985757, "sdkplus_install_bytes": 8165359} |
Why
Risk class L.
This pull request converts four of the five classes in row B1 of the Kotlin migration tracker from Java to Kotlin:
MParticleBaseClient,MPUrlImpl,MPConnectionImplandMParticleApiClient. None of them had a unit test, so it first adds unit tests that pin what each class does. Apps and kits do not see it: the classes are internal, R8 renames them in the published AAR, and no call site in the repository needed an edit.NetworkOptionsManagerwaits for an API review, for the reason below.Programme
The Java-to-Kotlin migration of android-core, TRACKER.md row B1 (Stack B, networking), converted under PLAYBOOK.md.
What changes
The four classes become Kotlin, each in its Java file's directory and package.
scripts/check_api_dump.pyagainst main finds no difference in either API dump.Characterization tests, added first:
networking/MPUrlImplTest, 8 tests: the file, authority and path each constructor reads,toStringas the whole URL, a malformed or null string, equality against anMPUrlImpl, aURLand other objects, andopenConnectionfor HTTPS, HTTP and afile:URL.networking/MPConnectionImplTest, 10 tests: each setter and getter reaching theHttpURLConnection, null boxed values, the response code read on every call,IOException, the SSL socket factory on HTTPS and HTTP connections, and a null connection.internal/MParticleApiClientTest, 6 tests: theAliasNetworkResponseconstructors, getters and setter, its JVM name and modifiers, each interface method's checked exceptions, and therequestHandleraccessors.Decisions made by hand:
MPUrlImplandMPConnectionImpl. Package-private, sointernal class. Both become final. Nothing extends or mocks either one.@Throws: 4 onMParticleApiClient, 2 onMPUrlImpland 5 onMPConnectionImpl.AliasNetworkResponse. Anopen classwith opengetResponseCodeandgetErrorMessage. Its two-argument constructor isinternal.setErrorMessage. Aninternalfunction with@JvmName("setErrorMessage"), becauseUploadHandler, in Java, calls it by that name, and Kotlin would otherwise add a module suffix.getCookiesand theAliasNetworkResponsegetters stay functions. No Kotlin code calls them with property syntax.@NonNullstays onsendAliasRequestand@NullableongetErrorMessage, becauseproguard.prokeeps members that carry them.@JvmStatic,@JvmField,@Synchronizedor@Volatile, no default arguments, no companion objects and no top-level declarations.Nullability. The playbook's default is a nullable type for every unannotated parameter and return. Three choices depart from it or from the Java's annotations:
MParticleBaseClient.requestHandleris a non-nullvar.PinningTestHelperandMParticleApiClientImplTestread it with property syntax and no null check.messageanduploadSettingsparameters ofsendMessageBatchandsendAliasRequestare nullable, where the Java said@NonNull.UploadHandlerTeststubssendAliasRequestthrough the interface withMockito.any(), which returns null, and a non-null Kotlin parameter would make those calls throwIllegalStateException.MParticleApiClientImplstill carries@NonNullon its own parameters.sendAliasRequestreturns a non-nullAliasNetworkResponse, as the Java's@NonNullsays.MPUrlImplandMPConnectionImplis nullable, read with!!at the step where the Java first used it. Their returns are nullable, excepttoString,isHttpsandgetResponseCode.What a strict client could notice:
NullPointerExceptionat the step where the Java threw, but the exception from!!has no message.MPUrlImpl,MPConnectionImpl, the internal constructor andsetErrorMessageare public in bytecode, where the Java was package-private. R8 renames them in the published AAR.messageanduploadSettingsparameters through the interface.NetworkOptionsManagerstays Java.scripts/check_api_dump.pytreats every class outsidecom.mparticle.internalas frozen. A Kotlin form adds a publicCompanionfield, and@JvmStaticmarksvalidateAndResolvefinal, so the check fails without theapi-change-approvedlabel. No ProGuard rule keeps the class, so R8 renames it in the published AAR. Its conversion belongs in a pull request of its own, with that label.Linked work
Rows A2 and A3 are separate pull requests. This pull request shares no file with either.
Rollout
This merges with no release, and it reaches apps in the next SDK release. No flag applies, because a library conversion has no runtime switch. To undo it, revert the squash commit.
The playbook checklist. Done before opening:
scripts/check_api_dump.pyagainst main: the dumps are unchanged.Waiting for this pull request's jobs:
./gradlew apiCheck: Unit Tests../gradlew test ktlintCheck lint, with 24 more unit tests than main: Unit Tests, Lint Checks and Kotlin Lint Checks.PinningTest,UploadMessageTestandMParticleBaseClientImplTest, which drive these classes on a device: the Instrumented Tests jobs.trunk check: Trunk code check.Risks
!!sits where the Java first used the value.Who
An AI coding agent wrote the tests and the conversion, as the next row of the tracker. The SDK team reviews it on this pull request. What ran and what waits for this pull request's jobs is under Rollout.
Size
Cost: not yet read.
4 Kotlin files, 147 lines, replacing 4 Java files of 229 lines. 3 new unit test files, 299 lines. The API dump does not change. Expect a few hundred bytes in SDK Size Impact.