Skip to content

Commit 24f7ddd

Browse files
committed
Address PR review (shahzaibj): eliminate double deserialization + nullability nit
- MsalBrokerResultAdapter.getAcquireTokenResultFromResultBundle: deserialize BrokerResult once at the top of the success branch instead of letting authenticationResultFromBundle and getOnboardingBlobFromBundle each deserialize the same bundle. Reuse the BrokerResult for both consumers. - Add authenticationResultFromBrokerResult(BrokerResult) overload that builds ILocalAuthenticationResult from a pre-deserialized BrokerResult; the existing authenticationResultFromBundle now delegates to it (no behavior change for other callers). - Add getOnboardingBlobFromBundle(BrokerResult) overload that returns the blob from a pre-deserialized BrokerResult without doing I/O. The original Bundle-taking overload is preserved for callers that don't already have a BrokerResult. - BrokerResult.Builder.onboardingBlob: add 'final' to the parameter and '@nullable' annotation, matching the surrounding builder method style.
1 parent 978eb83 commit 24f7ddd

2 files changed

Lines changed: 31 additions & 5 deletions

File tree

common/src/main/java/com/microsoft/identity/common/internal/broker/BrokerResult.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -727,7 +727,7 @@ public Builder exceptionType(String exceptionType) {
727727
return this;
728728
}
729729

730-
public Builder onboardingBlob(String onboardingBlob) {
730+
public Builder onboardingBlob(@Nullable final String onboardingBlob) {
731731
this.mOnboardingBlob = onboardingBlob;
732732
return this;
733733
}

common/src/main/java/com/microsoft/identity/common/internal/result/MsalBrokerResultAdapter.java

Lines changed: 30 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -465,9 +465,16 @@ public Bundle bundleFromBaseExceptionForWebApps(@NonNull final BaseException exc
465465
@NonNull
466466
@Override
467467
public ILocalAuthenticationResult authenticationResultFromBundle(@NonNull final Bundle resultBundle) throws ClientException {
468-
final String methodTag = TAG + ":authenticationResultFromBundle";
469-
final BrokerResult brokerResult = brokerResultFromBundle(resultBundle);
468+
return authenticationResultFromBrokerResult(brokerResultFromBundle(resultBundle));
469+
}
470470

471+
/**
472+
* Overload that builds the authentication result from an already-deserialized
473+
* {@link BrokerResult}. Use this when the caller has the [BrokerResult] in hand
474+
* to avoid a redundant deserialization of the result bundle.
475+
*/
476+
public ILocalAuthenticationResult authenticationResultFromBrokerResult(@NonNull final BrokerResult brokerResult) throws ClientException {
477+
final String methodTag = TAG + ":authenticationResultFromBrokerResult";
471478
Logger.info(methodTag, "Broker Result returned from Bundle, constructing authentication result");
472479

473480
final List<ICacheRecord> tenantProfileCacheRecords = brokerResult.getTenantProfileData();
@@ -570,6 +577,9 @@ public BrokerPerformanceMetrics getBrokerPerformanceMetricsFromBundle(@NonNull f
570577
* Best-effort: returns null if the bundle cannot be deserialized into a BrokerResult
571578
* or if no blob is present. Telemetry failures must never fail an otherwise-successful
572579
* auth result. Blob contents are not logged (may carry sessionCorrelationId).
580+
*
581+
* If the caller has already deserialized the [BrokerResult], prefer the overload
582+
* [getOnboardingBlobFromBundle(BrokerResult)] to avoid a second deserialization.
573583
*/
574584
@Nullable
575585
@VisibleForTesting(otherwise = VisibleForTesting.PRIVATE)
@@ -584,6 +594,17 @@ public String getOnboardingBlobFromBundle(@NonNull final Bundle resultBundle) {
584594
}
585595
}
586596

597+
/**
598+
* Overload that reads the onboarding telemetry blob from a {@link BrokerResult}
599+
* that has already been deserialized by the caller. Use this when you already
600+
* have a [BrokerResult] in hand to avoid a second deserialization of the bundle.
601+
*/
602+
@Nullable
603+
@VisibleForTesting(otherwise = VisibleForTesting.PRIVATE)
604+
public String getOnboardingBlobFromBundle(@NonNull final BrokerResult brokerResult) {
605+
return brokerResult.getOnboardingBlob();
606+
}
607+
587608
@NonNull
588609
@Override
589610
public AcquirePrtSsoTokenResult getAcquirePrtSsoTokenResultFromBundle(Bundle resultBundle) {
@@ -1036,9 +1057,14 @@ public AcquireTokenResult getDeviceCodeFlowTokenResultFromResultBundle(@NonNull
10361057
AcquireTokenResult getAcquireTokenResultFromResultBundle(@NonNull final Bundle resultBundle) throws BaseException {
10371058
final MsalBrokerResultAdapter resultAdapter = new MsalBrokerResultAdapter();
10381059
if (resultBundle.getBoolean(AuthenticationConstants.Broker.BROKER_REQUEST_V2_SUCCESS)) {
1060+
// Deserialize BrokerResult once and reuse for both the local authentication result
1061+
// and the onboarding telemetry blob, instead of letting authenticationResultFromBundle
1062+
// and getOnboardingBlobFromBundle each pay the deserialization cost.
1063+
final BrokerResult brokerResult = resultAdapter.brokerResultFromBundle(resultBundle);
1064+
10391065
final AcquireTokenResult acquireTokenResult = new AcquireTokenResult();
10401066
acquireTokenResult.setLocalAuthenticationResult(
1041-
resultAdapter.authenticationResultFromBundle(resultBundle)
1067+
resultAdapter.authenticationResultFromBrokerResult(brokerResult)
10421068
);
10431069
// Set broker performance metrics if available
10441070
final BrokerPerformanceMetrics metrics = resultAdapter.getBrokerPerformanceMetricsFromBundle(resultBundle);
@@ -1059,7 +1085,7 @@ AcquireTokenResult getAcquireTokenResultFromResultBundle(@NonNull final Bundle r
10591085
}
10601086

10611087
// Set onboarding telemetry blob if present (best-effort; never fails the result).
1062-
final String onboardingBlob = resultAdapter.getOnboardingBlobFromBundle(resultBundle);
1088+
final String onboardingBlob = resultAdapter.getOnboardingBlobFromBundle(brokerResult);
10631089
if (onboardingBlob != null) {
10641090
acquireTokenResult.setOnboardingBlob(onboardingBlob);
10651091
}

0 commit comments

Comments
 (0)