Add unsafe metadata support to clerk-android #355
Conversation
Co-authored-by: sam <[email protected]>
|
Cursor Agent can help with this pull request. Just |
📝 WalkthroughWalkthroughAdds an optional Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Possibly related issues
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)**/*⚙️ CodeRabbit configuration file
Files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (9)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
source/api/src/main/kotlin/com/clerk/api/user/User.kt(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*
⚙️ CodeRabbit configuration file
If there are no tests added or modified as part of the PR, please suggest that tests be added to cover the changes.
Files:
source/api/src/main/kotlin/com/clerk/api/user/User.kt
| /** JSON string containing unsafe metadata to update. */ | ||
| @SerialName("unsafe_metadata") val unsafeMetadata: String? = null, |
There was a problem hiding this comment.
🧩 Analysis chain
Implementation looks good and follows existing patterns.
The addition of unsafeMetadata to UpdateParams correctly follows the same pattern as publicMetadata and privateMetadata. The type String? is appropriate for the update parameters, matching the existing metadata fields.
However, the AI summary incorrectly states that unsafeMetadata was added to the User data class. The field already exists at line 165, so this appears to be a documentation inconsistency rather than a code change.
As per coding guidelines, please add tests to cover the new unsafeMetadata parameter. The tests should verify:
- Updating a user with
unsafeMetadatasucceeds - The field is properly serialized in API requests
- The updated metadata is correctly reflected in the returned User object
Would you like me to help generate test cases for this functionality?
Additionally, verify that the @AutoMap annotation properly handles the new field:
🏁 Script executed:
#!/bin/bash
# Description: Verify that unsafeMetadata is properly integrated with the UpdateParams serialization
# Search for how UpdateParams.toMap() is implemented or generated by @AutoMap
rg -n "toMap" --type=kotlin -A 5 -B 2
# Search for any existing tests of UpdateParams to understand the test pattern
rg -n "UpdateParams" --type=kotlin -g "*test*" -C 3
# Search for any existing usage of publicMetadata or privateMetadata updates to see the pattern
rg -n "publicMetadata|privateMetadata" --type=kotlin -g "**/*test*" -C 3Length of output: 41447
---
🏁 Script executed:
```shell
#!/bin/bash
# Better search for test files related to User.kt and UpdateParams
# Search for test files by common naming patterns
echo "=== Searching for User-related test files ==="
fd -e kt -path "*test*" | grep -i user | head -20
echo -e "\n=== Searching for UpdateParams in any Kotlin files (broader search) ==="
rg -n "UpdateParams" --type=kotlin -B2 -A2 | head -50
echo -e "\n=== Checking for test directory structure ==="
find . -type d -name "test" -o -name "tests" 2>/dev/null | head -10
echo -e "\n=== Search for any test involving metadata updates ==="
rg -n "metadata" --type=kotlin -g "*test*" -C2 | head -40
Length of output: 3857
🏁 Script executed:
#!/bin/bash
# Search in test directory specifically
echo "=== List files in test directory ==="
find ./source/api/src/test -type f -name "*.kt" 2>/dev/null | head -20
echo -e "\n=== Search for User in test files ==="
find ./source/api/src/test -type f -name "*.kt" -exec grep -l "User" {} \; 2>/dev/null
echo -e "\n=== Search for update or Update in test files ==="
find ./source/api/src/test -type f -name "*.kt" -exec grep -l "update\|Update" {} \; 2>/dev/null | head -10
echo -e "\n=== Check for any metadata tests ==="
find ./source/api/src/test -type f -name "*.kt" | xargs grep -l "metadata" 2>/dev/nullLength of output: 1622
🏁 Script executed:
#!/bin/bash
# Check what User-related tests exist in ClerkTest.kt
echo "=== Inspect ClerkTest.kt for update-related tests ==="
grep -n "update\|Update" ./source/api/src/test/java/com/clerk/api/sdk/ClerkTest.kt
echo -e "\n=== Check if ClerkTest.kt tests User.update() ==="
grep -n -A5 -B5 "User\|user" ./source/api/src/test/java/com/clerk/api/sdk/ClerkTest.kt | head -50
echo -e "\n=== Verify the PR changes - check if any test files were modified ==="
# Look at the actual file structure to confirm no test changes
find . -name "*.kt" -path "*test*" -newer ./source/api/src/main/kotlin/com/clerk/api/user/User.kt 2>/dev/null | head -5 || echo "Cannot determine file modification times"Length of output: 2795
Add tests for the new unsafeMetadata field in User.update().
The implementation correctly follows the existing pattern for publicMetadata and privateMetadata fields, with proper @AutoMap and @SerialName annotations. However, per coding guidelines, tests must be added to cover this new functionality since none exist in the codebase for User.update() operations.
Add tests to verify:
- Updating a user with
unsafeMetadatasucceeds - The field is properly serialized in API requests via
toMap() - Updated metadata is correctly reflected in the returned User object
Co-authored-by: sam <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
source/api/src/test/java/com/clerk/api/user/UserTest.kt (1)
113-148: Good edge case coverage for empty and complex metadata.The tests properly validate that empty JSON objects and complex nested structures are preserved correctly.
Consider adding optional tests for additional edge cases:
@Test fun `UpdateParams with invalid JSON should pass through to server validation`() { // Given - invalid JSON val invalidJson = """{"incomplete":""" val params = User.UpdateParams(unsafeMetadata = invalidJson) // When val paramsMap = params.toMap() // Then - should preserve the string even if invalid assertEquals("Should preserve even invalid JSON", invalidJson, paramsMap["unsafe_metadata"]) } @Test fun `UpdateParams with empty string unsafeMetadata should be handled`() { // Given val params = User.UpdateParams(unsafeMetadata = "") // When val paramsMap = params.toMap() // Then assertEquals("Should include empty string", "", paramsMap["unsafe_metadata"]) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
source/api/src/test/java/com/clerk/api/user/UserTest.kt(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*
⚙️ CodeRabbit configuration file
If there are no tests added or modified as part of the PR, please suggest that tests be added to cover the changes.
Files:
source/api/src/test/java/com/clerk/api/user/UserTest.kt
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (4)
source/api/src/test/java/com/clerk/api/user/UserTest.kt (4)
15-48: Excellent coverage of inclusion and exclusion behavior.The tests properly validate that
unsafeMetadatais included when provided and excluded when null, with correct key mapping tounsafe_metadatain the map representation.
50-94: Thorough validation of metadata field combinations.These tests properly verify that multiple metadata types can coexist and that
unsafeMetadatacan be used independently. The size assertion in the solo test is particularly valuable for catching unintended field inclusions.
1-149: Comprehensive test coverage for the unsafe metadata feature.The test suite provides excellent coverage of the new
unsafeMetadatafunctionality, including:
- Basic inclusion/exclusion behavior
- Interaction with other metadata fields
- JSON serialization
- Edge cases (empty and complex structures)
The tests follow good practices with clear Given-When-Then structure and descriptive assertion messages.
12-12: Consider removing Robolectric from UserTest.kt if not required project-wide.The test code demonstrates no Android-specific functionality.
UpdateParamsis a pure Kotlin data class, and the test uses only JUnit assertions and kotlinx.serialization. ThetoMap()method is generated by the@AutoMapannotation processor and doesn't require Android emulation.However, Robolectric is used consistently across multiple test files in the project. Verify:
- Whether this is a project-wide testing convention or infrastructure requirement
- If removing it from UserTest.kt aligns with the project's testing strategy
- Whether any other tests or build configuration depend on this runner
If not required, use
@RunWith(JUnit4::class)or the default runner to reduce test overhead and improve execution speed.
| @Test | ||
| fun `UpdateParams serialization should include unsafe_metadata field`() { | ||
| // Given | ||
| val unsafeMetadataJson = """{"custom":"data"}""" | ||
| val params = User.UpdateParams(username = "testuser", unsafeMetadata = unsafeMetadataJson) | ||
|
|
||
| // When | ||
| val json = Json.encodeToString(params) | ||
|
|
||
| // Then | ||
| assertTrue("Serialized JSON should contain unsafe_metadata", json.contains("unsafe_metadata")) | ||
| assertTrue( | ||
| "Serialized JSON should contain the metadata value", | ||
| json.contains(unsafeMetadataJson), | ||
| ) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Verify JSON escaping in serialization test.
The assertion on lines 107-110 checks if the serialized JSON contains the unescaped unsafeMetadataJson string. However, when kotlinx.serialization encodes a String field containing JSON, it should escape the quotes. For example:
- Input:
unsafeMetadata = """{"custom":"data"}""" - Expected serialized:
{"username":"testuser","unsafe_metadata":"{\"custom\":\"data\"}"}
The test json.contains(unsafeMetadataJson) might fail because it's searching for the unescaped string {"custom":"data"} within escaped JSON.
Please run this test to confirm it passes, or consider updating the assertion to verify proper JSON escaping:
If the test passes, the assertion is working as intended. If it fails, consider this alternative assertion:
// Verify proper JSON escaping
assertTrue(
"Serialized JSON should contain escaped metadata value",
json.contains("\\\"custom\\\":\\\"data\\\"") || json.contains("{\\\"custom\\\":\\\"data\\\"}")
)🏁 Script executed:
#!/bin/bash
# Run the specific serialization test to verify it passes
./gradlew :api:testDebugUnitTest --tests "com.clerk.api.user.UserTest.UpdateParams serialization should include unsafe_metadata field"Length of output: 189
---
🏁 Script executed:
```shell
#!/bin/bash
# Find the User.UpdateParams class definition
rg -n "class UpdateParams|data class UpdateParams" --type=kt source/api/src
Length of output: 26
🏁 Script executed:
#!/bin/bash
# Search for unsafeMetadata field definition and usage
rg -n "unsafeMetadata" --type=kt source/api/src -C 3Length of output: 26
🏁 Script executed:
#!/bin/bash
# Check if there are custom serializers for UpdateParams
rg -n "@Serializable|KSerializer" --type=kt source/api/src -C 2 | grep -A 5 -B 5 "UpdateParams"Length of output: 26
---
🏁 Script executed:
```shell
#!/bin/bash
# Find User.kt file in the codebase
fd -e kt User.kt source/api/src
Length of output: 53
🏁 Script executed:
#!/bin/bash
# Search for UpdateParams class without type restriction
rg -n "UpdateParams" source/api/src -C 5 | head -100Length of output: 8221
🏁 Script executed:
#!/bin/bash
# Search for unsafeMetadata without type restriction
rg -n "unsafeMetadata" source/api/src -C 3 | head -100Length of output: 8692
---
🏁 Script executed:
```shell
#!/bin/bash
# View the User.kt file to find UpdateParams definition
cat source/api/src/main/kotlin/com/clerk/api/user/User.kt
Length of output: 26371
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for @AutoMap annotation and toMap method references
rg -n "toMap|AutoMap" source/api/src -C 3 | head -80
Length of output: 6826
🏁 Script executed:
#!/bin/bash
# Check for custom serializers or Json configuration
rg -n "kotlinx.serialization|Json\\.encodeToString|KSerializer" source/api/src -C 2 | head -60Length of output: 4541
---
🌐 Web query:
```web
kotlinx.serialization String field JSON escaping behavior quote characters
💡 Result:
Short answer: kotlinx.serialization follows the JSON spec — string values are emitted in double quotes and any interior double-quote characters are escaped with a backslash (" ) when you serialize (e.g., Json.encodeToString). If you need an unquoted/raw JSON literal (not ordinary JSON string escaping), you can use JsonUnquotedLiteral (experimental) but that can produce invalid JSON if misused. [1][2][3]
Sources:
- Json.encodeToString behavior (kotlinx.serialization docs). [1]
- JsonUnquotedLiteral (creates unquoted/raw literal). [2]
- Example showing library escaping double quotes and other characters when encoding strings. [3]
Fix the JSON escaping assertion in the serialization test.
The assertion at lines 107-110 checks for an unescaped JSON substring, but kotlinx.serialization escapes double quotes when serializing String fields per the JSON specification. When unsafeMetadata = """{"custom":"data"}""" is serialized, it becomes "{\"custom\":\"data\"}" in the JSON output, so the substring {"custom":"data"} will not be found.
Update the assertion to verify the escaped version:
assertTrue(
"Serialized JSON should contain escaped metadata value",
json.contains("\\\"custom\\\":\\\"data\\\"") || json.contains("{\\\"custom\\\":\\\"data\\\"}")
)🤖 Prompt for AI Agents
In source/api/src/test/java/com/clerk/api/user/UserTest.kt around lines 96 to
111, the test asserts the raw JSON substring {"custom":"data"} which won't
appear because kotlinx.serialization escapes quotes in string fields; update the
assertion (lines ~107-110) to check for the escaped form of the metadata value
(e.g., look for \"custom\":\"data\" or include the surrounding escaped braces
{\"custom\":\"data\"}) so the test verifies the serialized, escaped JSON string
instead of the unescaped substring.
Co-authored-by: sam <[email protected]>
swolfand
deleted the
cursor/MOBILE-314-add-unsafe-metadata-support-to-clerk-android-4fb6
branch
Summary of changes
Adds support for setting
unsafeMetadatawhen updating a user in the Clerk-Android SDK. This change introduces theunsafeMetadatafield toUser.UpdateParams, aligning the Android SDK with the existing functionality in the Clerk JS SDK.Linear Issue: MOBILE-314
Summary by CodeRabbit
New Features
Tests