-
Notifications
You must be signed in to change notification settings - Fork 38
Use ContentProvider instead of hashmap for numberMatch data, AB#3308440, Fixes AB#3308440 #2692
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.
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
Conversation
❌ Work item link check failed. Description does not contain AB#{ID}. Click here to Learn more. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR replaces in-memory storage of number match data with a ContentProvider for proper interprocess communication.
- Refactors
NumberMatchHelper
to use a ContentProvider for storing, retrieving, and clearing number match entries. - Adds
NumberMatchDbHelper
andNumberMatchContentProvider
to manage a SQLite-backed table. - Updates tests and the JavaScript interface/WebView client to inject and use
Context
for ContentResolver operations.
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
File | Description |
---|---|
common/src/test/java/com/microsoft/identity/common/internal/numberMatch/NumberMatchHelperTest.kt | Tests updated to mock ContentResolver and drive new store /get methods with Context |
common/src/test/java/com/microsoft/identity/common/internal/broker/AuthUxJavaScriptInterfaceTest.kt | Injects a mocked Context into the JS interface constructor |
common/src/main/java/com/microsoft/identity/common/internal/ui/webview/AzureActiveDirectoryWebViewClient.java | Passes application context into the JS interface (incorrectly via getActivity() ) |
common/src/main/java/com/microsoft/identity/common/internal/numberMatch/NumberMatchHelper.kt | Refactored helper methods to use ContentResolver and ContentProvider URIs |
common/src/main/java/com/microsoft/identity/common/internal/numberMatch/NumberMatchDbHelper.kt | New SQLiteOpenHelper creating the number_match table |
common/src/main/java/com/microsoft/identity/common/internal/numberMatch/NumberMatchContentProvider.kt | New ContentProvider implementation with query/insert/delete/update |
common/src/main/java/com/microsoft/identity/common/internal/broker/AuthUxJavaScriptInterface.kt | JS interface now takes Context and calls NumberMatchHelper.storeNumberMatch(context,…) |
Comments suppressed due to low confidence (1)
common/src/main/java/com/microsoft/identity/common/internal/numberMatch/NumberMatchHelper.kt:95
- There’s no unit test covering
clearNumberMatchMap(context)
. Add a test to verify that calling it deletes all entries and subsequentgetNumberMatch
returns null.
fun clearNumberMatchMap(context: Context) {
...on/src/test/java/com/microsoft/identity/common/internal/numberMatch/NumberMatchHelperTest.kt
Outdated
Show resolved
Hide resolved
common/src/main/java/com/microsoft/identity/common/internal/numberMatch/NumberMatchHelper.kt
Outdated
Show resolved
Hide resolved
...c/main/java/com/microsoft/identity/common/internal/numberMatch/NumberMatchContentProvider.kt
Show resolved
Hide resolved
...ava/com/microsoft/identity/common/internal/ui/webview/AzureActiveDirectoryWebViewClient.java
Show resolved
Hide resolved
...ava/com/microsoft/identity/common/internal/ui/webview/AzureActiveDirectoryWebViewClient.java
Show resolved
Hide resolved
✅ Work item link check complete. Description contains link AB#3308440 to an Azure Boards work item. |
common/src/main/java/com/microsoft/identity/common/internal/numberMatch/NumberMatchDbHelper.kt
Show resolved
Hide resolved
...c/main/java/com/microsoft/identity/common/internal/numberMatch/NumberMatchContentProvider.kt
Show resolved
Hide resolved
*/ | ||
fun storeNumberMatch(sessionId: String?, numberMatch: String?) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did not understand. This is called as part of the javascript API call made by the AuthUx. Are you saying we should be calling ContentProvider directly?
* | ||
* @param context The context to use for locating paths to the the database | ||
*/ | ||
class NumberMatchDbHelper(context: Context) : SQLiteOpenHelper(context, "number_match.db", null, 1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HashMap will not work for IPC. That is the main reason of this PR and change. Let me know if I am missing something.
* | ||
* The provider uses NumberMatchDbHelper for database management and exposes its data via a content URI. | ||
*/ | ||
class NumberMatchContentProvider : ContentProvider() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NumberMatchHelper.clearNumberMatchMap() | ||
MockitoAnnotations.openMocks(this) | ||
Mockito.reset(context, contentResolver, cursor) | ||
`when`(context.contentResolver).thenReturn(contentResolver) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AB#3308440
In the current implementation for number matching feature, the data is stored in the hashmap. On testing, I found out that the data in the hashmap was not accessible since this is an interprocess communication. This PR uses ContentProvider to enable interprocess communication.