Remove the unnecessary code line because of fix in common #2135

Yuki-YuXin · 2024-07-01T15:43:04Z

Company PR: AzureAD/microsoft-authentication-library-common-for-android#2435

Remove the conditional setting of the correlation ID in AccountState since it's a quick fix to pass the tests.
Add try catch block for the UUID to String

…n AccountState.kt

Yuki-YuXin · 2024-07-03T09:56:07Z

azure-pipelines/pull-request-validation/pr-msal.yml

@@ -30,7 +30,7 @@ resources:
  - repository: common
    type: github
    name: AzureAD/microsoft-authentication-library-common-for-android
-    ref: dev
+    ref: robert/empty-correlation-id-fixes


Revert it back before the merge.

Why do we have this here?

I cannot change the targe branch in the DevOps portal thus change the command here.

SammyO · 2024-07-15T09:45:20Z

msal/src/main/java/com/microsoft/identity/client/internal/CommandParametersAdapter.java

-                // Set it to a default value.
-                .correlationId(DiagnosticContext.INSTANCE.getThreadCorrelationId())
+                // If default value "UNSET", generate a new UUID as correlationId.
+                .correlationId(Objects.equals(DiagnosticContext.INSTANCE.getThreadCorrelationId(), "UNSET") ? UUID.randomUUID().toString() : DiagnosticContext.INSTANCE.getThreadCorrelationId())


This logic doesn't belong here. it's the responsibility of DiagnosticContext to generate an ID, and do additional null checks to ensure an actual ID is returned.

Update the logic inside DiagnosticContext getThreadCorrelationId()

…' into yuki/company-with-robert-PR-test

…ializeDiagnosticContext final String correlationId = StringUti?.isNullOrEmpty(requestCorrelationId)?UUID.randomUUID().toString():requestCorrelationId;

SammyO · 2024-07-19T08:40:56Z

msal/src/main/java/com/microsoft/identity/nativeauth/NativeAuthPublicClientApplication.kt

@@ -262,7 +262,7 @@ class NativeAuthPublicClientApplication(
                    GetAccountResult.AccountFound(
                        resultValue = AccountState.createFromAccountResult(
                            account = account,
-                            correlationId = DiagnosticContext.INSTANCE.threadCorrelationId,
+                            correlationId = "",


Could you check what MSAL does here?

Also, what was your thinking behind changing this from the thread ID to an empty string?

This path generate AccountState from the SDK cache (authClient.getCurrentAccount())

The default value of DiagnosticContext.INSTANCE.threadCorrelationId is UNSET. And currently, SDK won't transform “UNSET" into UUID.

If use DiagnosticContext.INSTANCE.threadCorrelationId,

authClient.getCurrentAccount() before signIn. correlation id would be "UNSET"

signIn. correlation id should be UUID

authClient.getCurrentAccount() after signIn. When getting correlation id again then becomes UUID as the sign in flow.

I am not sure if the difference between the two correlation ids before and after is what we want

SammyO · 2024-07-19T08:41:10Z

msal/src/main/java/com/microsoft/identity/nativeauth/NativeAuthPublicClientApplication.kt

@@ -341,7 +341,7 @@ class NativeAuthPublicClientApplication(
                    return@withContext SignInError(
                        errorType = ErrorTypes.INVALID_USERNAME,
                        errorMessage = "Empty or blank username",
-                        correlationId = "UNSET"
+                        correlationId = ""


Why not set it at all, meaning the value will be null?

The correlationId parameter is mandatory. Setting it to “” would be much easier than dealing with if null.

SammyO · 2024-07-19T08:46:15Z

msal/src/main/java/com/microsoft/identity/nativeauth/statemachine/states/AccountState.kt

@@ -77,7 +77,7 @@ class AccountState private constructor(

    constructor (parcel: Parcel) : this (
        account = parcel.serializable<IAccount>() as IAccount,
-        correlationId = parcel.readString() ?: "UNSET",
+        correlationId = parcel.readString() ?: "",


Doesn't UUID.fromString("") also produce an exception?

That's a good point. How about generating random UUID here?

Keep the original "UNSET" string.

# Conflicts: # common

… the common PR made changes in getThreadCorrelationId(), if it is null or UNSET, will generate a random UUID

# Conflicts: # common

GBakolas · 2024-07-26T14:46:44Z

msal/src/main/java/com/microsoft/identity/nativeauth/statemachine/states/AccountState.kt

@@ -309,12 +309,17 @@ class AccountState private constructor(
                            correlationId = correlationId
                        )

-                val privateCorrelationId = if (correlationId == "UNSET") { UUID.randomUUID().toString() } else { correlationId }
+                try {
+                    UUID.fromString(correlationId)


nit: maybe assign the result to a variable and use it in the builder to avoid calling the fromString() method twice

@Yuki-YuXin let's run the tests without this fix here to ensure it works not. But let's keep this fix here as well, to avoid any production crashes (but clean it up a bit, e.g. call DiagnosticContext.INSTANCE.getThreadCorrelationId() so that a new UUID is generated in there, rather than here at the call site).

Local GetAccessTokenTests passed with and without the changes here.

# Conflicts: # common

Yuki-YuXin added 2 commits July 1, 2024 16:22

Comment the code to see the test

87e051c

Update the pr for test. Revert it back later.

44e0f07

github-actions bot added the msal label Jul 1, 2024

Yuki-YuXin added 2 commits July 1, 2024 17:07

Decomment because of the test failed

5d8f15c

Modify CommandParametersAdapter.java to move the temporary solution i…

304ba3b

…n AccountState.kt

Yuki-YuXin mentioned this pull request Jul 3, 2024

Add check for unset correlation ID when configuring request headers AzureAD/microsoft-authentication-library-common-for-android#2435

Merged

Yuki-YuXin marked this pull request as ready for review July 3, 2024 09:55

Yuki-YuXin requested review from a team as code owners July 3, 2024 09:55

Yuki-YuXin commented Jul 3, 2024

View reviewed changes

Merge branch 'dev' into yuki/company-with-robert-PR-test

2780cc6

SammyO reviewed Jul 15, 2024

View reviewed changes

Yuki-YuXin added 7 commits July 16, 2024 09:53

Merge branch 'dev' into yuki/company-with-robert-PR-test

e6614e3

Merge remote-tracking branch 'origin/yuki/company-with-robert-PR-test…

a4ae188

…' into yuki/company-with-robert-PR-test

Revert changes to trigger error log

11605bb

update submodule reference

b07663c

Use empty string "" to instead "UNSET" because inside Dispatcher.init…

862c126

…ializeDiagnosticContext final String correlationId = StringUti?.isNullOrEmpty(requestCorrelationId)?UUID.randomUUID().toString():requestCorrelationId;

Update the comment

f87a447

update submodule reference

f799c82

SammyO reviewed Jul 19, 2024

View reviewed changes

Yuki-YuXin added 8 commits July 19, 2024 14:15

Merge branch 'dev' into yuki/company-with-robert-PR-test

73c241c

# Conflicts: # common

Revert back to DiagnosticContext.INSTANCE.threadCorrelationId because…

2a024b5

… the common PR made changes in getThreadCorrelationId(), if it is null or UNSET, will generate a random UUID

update submodule reference

c10ec23

Use "UNSET"

0ce6946

Merge branch 'dev' into yuki/company-with-robert-PR-test

74d3658

# Conflicts: # common

Tidy up imports

1262d2c

Revert ref branch

b392bda

ref: dev

0b1cd98

Yuki-YuXin added 2 commits July 24, 2024 14:35

Add try catch block according to AuthenticationResult

2050eb2

Add try catch block according to AuthenticationResult

319bd43

GBakolas reviewed Jul 26, 2024

View reviewed changes

GBakolas approved these changes Jul 26, 2024

View reviewed changes

Yuki-YuXin added 5 commits July 29, 2024 09:37

Address comment

c6f864a

Update submodule reference

856c013

Merge branch 'dev' into yuki/company-with-robert-PR-test

d2f0162

# Conflicts: # common

Merge branch 'dev' into yuki/company-with-robert-PR-test

43be7bf

# Conflicts: # common

Add changelog entry

f056483

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove the unnecessary code line because of fix in common #2135

Remove the unnecessary code line because of fix in common #2135

Yuki-YuXin commented Jul 1, 2024 •

edited

Loading

Yuki-YuXin Jul 3, 2024

SammyO Jul 19, 2024

Yuki-YuXin Jul 22, 2024

SammyO Jul 15, 2024

Yuki-YuXin Jul 18, 2024

SammyO Jul 19, 2024

SammyO Jul 19, 2024

Yuki-YuXin Jul 19, 2024 •

edited

Loading

SammyO Jul 19, 2024

Yuki-YuXin Jul 22, 2024

SammyO Jul 19, 2024

Yuki-YuXin Jul 22, 2024

Yuki-YuXin Jul 24, 2024

GBakolas Jul 26, 2024

SammyO Jul 29, 2024

Yuki-YuXin Jul 29, 2024

Remove the unnecessary code line because of fix in common #2135

Are you sure you want to change the base?

Remove the unnecessary code line because of fix in common #2135

Conversation

Yuki-YuXin commented Jul 1, 2024 • edited Loading

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

Yuki-YuXin Jul 19, 2024 • edited Loading

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

Yuki-YuXin commented Jul 1, 2024 •

edited

Loading

Yuki-YuXin Jul 19, 2024 •

edited

Loading