Adds a model for apps to define credentials#1497
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughAdds a new Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Test as Test
participant Parser as ProfileParser
participant XML as basic_profile.ccpr
participant Profile as Profile
participant Cred as Credential
Test->>Parser: parse(profile XML)
Parser->>XML: read <features>
XML-->>Parser: <credentials>...<credential level,type/>...</credentials>
rect rgba(200,235,255,0.3)
note right of Parser: New: credentials parsing
loop for each <credential/>
Parser->>Cred: new Credential(level, type)
Parser-->>Parser: collect in list
end
end
Parser->>Profile: setCredentials(list)
Parser-->>Test: Profile
Test->>Profile: getCredentials()
Profile-->>Test: Vector<Credential>
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
✨ Finishing Touches🧪 Generate unit tests
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/test/java/org/commcare/backend/suite/model/test/ProfileTests.java (1)
69-75: Extend serialization comparison to include credentials.Current compareProfiles omits credentials; add a check to ensure round-trip integrity.
Example (outside changed lines):
// inside compareProfiles(Profile a, Profile b) assertEquals("Mismatched credentials", Arrays.toString(a.getCredentials().toArray()), Arrays.toString(b.getCredentials().toArray()));If you adopt equals/hashCode, prefer:
assertArrayEquals("Mismatched credentials", a.getCredentials().toArray(new Credential[0]), b.getCredentials().toArray(new Credential[0]));
🧹 Nitpick comments (4)
src/main/java/org/commcare/suite/model/Credential.java (1)
1-11: Give Credential value-type semantics (equals/hashCode) to enable robust comparisons and collection use.This will simplify testing (no toString hacks) and future map/set usage.
Apply:
@@ package org.commcare.suite.model; import org.javarosa.core.util.externalizable.DeserializationException; import org.javarosa.core.util.externalizable.ExtUtil; import org.javarosa.core.util.externalizable.Externalizable; import org.javarosa.core.util.externalizable.PrototypeFactory; import java.io.DataInputStream; import java.io.DataOutputStream; import java.io.IOException; +import java.util.Objects; @@ @Override public String toString() { return "Credential{" + "level='" + level + '\'' + ", type='" + type + '\'' + '}'; } + + @Override + public boolean equals(Object o) { + if (this == o) return true; + if (!(o instanceof Credential)) return false; + Credential that = (Credential) o; + return Objects.equals(level, that.level) && + Objects.equals(type, that.type); + } + + @Override + public int hashCode() { + return Objects.hash(level, type); + } }Also applies to: 52-59
src/test/java/org/commcare/backend/suite/model/test/ProfileTests.java (1)
96-104: Use array equality instead of stringifying for comparisons.With equals/hashCode on Credential, assert arrays directly for clearer failures.
- assertEquals(Arrays.toString(expectedCredentials),Arrays.toString(p.getCredentials().toArray())); + assertArrayEquals(expectedCredentials, p.getCredentials().toArray(new Credential[0]));src/main/java/org/commcare/xml/ProfileParser.java (1)
261-279: Include parser context in exceptions for better error diagnostics.This matches other exceptions in this class and improves crash logs.
- if (level == null) { - throw new InvalidStructureException("No level defined for credential"); - } - if (type == null) { - throw new InvalidStructureException("No type defined for credential"); - } + if (level == null) { + throw new InvalidStructureException("No level defined for credential", parser); + } + if (type == null) { + throw new InvalidStructureException("No type defined for credential", parser); + }src/main/java/org/commcare/suite/model/Profile.java (1)
196-203: Defensive null-handling in setter.Prevents later NPEs if called with null.
- public void setCredentials(Vector<Credential> credentials) { - this.credentials = credentials; - } + public void setCredentials(Vector<Credential> credentials) { + this.credentials = (credentials != null) ? credentials : new Vector<>(); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
src/main/java/org/commcare/suite/model/Credential.java(1 hunks)src/main/java/org/commcare/suite/model/Profile.java(5 hunks)src/main/java/org/commcare/xml/ProfileParser.java(3 hunks)src/test/java/org/commcare/backend/suite/model/test/ProfileTests.java(2 hunks)src/test/resources/basic_profile.ccpr(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
src/main/java/org/commcare/suite/model/Credential.java (1)
src/main/java/org/javarosa/core/util/externalizable/ExtUtil.java (1)
ExtUtil(18-395)
src/test/java/org/commcare/backend/suite/model/test/ProfileTests.java (1)
src/main/java/org/commcare/suite/model/Credential.java (1)
Credential(15-59)
src/main/java/org/commcare/suite/model/Profile.java (1)
src/main/java/org/javarosa/core/util/externalizable/ExtUtil.java (1)
ExtUtil(18-395)
src/main/java/org/commcare/xml/ProfileParser.java (1)
src/main/java/org/commcare/suite/model/Credential.java (1)
Credential(15-59)
⏰ 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). (2)
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
🔇 Additional comments (6)
src/test/resources/basic_profile.ccpr (1)
94-98: Credentials block placement and schema look correct.Tags, attribute names, and activation flag align with the parser’s expectations.
src/main/java/org/commcare/xml/ProfileParser.java (3)
9-9: Import addition is correct.
31-36: Constant names are consistent with XML schema and usage.
252-254: Feature wiring LGTM.Parsing credentials and registering feature activation mirrors existing dependencies handling.
src/main/java/org/commcare/suite/model/Profile.java (2)
82-82: Initialize credentials vector — good default.
259-259: Writing credentials at the end keeps stream append-only; OK if read guard is added.Confirm no legacy deserialization paths rely on exact field count without EOF tolerance.
| credentials = (Vector<Credential>)ExtUtil.read(in, | ||
| new ExtWrapList(Credential.class), pf); | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Potential backward-compat break in externalization.
Older serialized Profiles (pre-credentials) would lack trailing bytes; reading will EOF. Guard read to default to empty when the stream ends.
- credentials = (Vector<Credential>)ExtUtil.read(in,
- new ExtWrapList(Credential.class), pf);
+ try {
+ credentials = (Vector<Credential>) ExtUtil.read(in, new ExtWrapList(Credential.class), pf);
+ } catch (java.io.EOFException eof) {
+ // Backward-compat: profiles serialized before credentials existed
+ credentials = new Vector<>();
+ }Run to confirm Profiles aren’t persisted across versions (or identify call sites to test upgrade paths):
🏁 Script executed:
#!/bin/bash
# Locate any storage usage of Profile objects/keys
rg -nP -C2 '\bProfile\.STORAGE_KEY\b|"PROFILE"'
rg -nP -C3 'IStorageUtility\s*<\s*Profile|StorageManager.*Profile|save\(.+Profile|write\(.+Profile'Length of output: 2940
🏁 Script executed:
#!/bin/bash
sed -n '200,260p' src/main/java/org/commcare/suite/model/Profile.javaLength of output: 2819
Guard credentials deserialization for backward‐compatibility
In Profile.readExternal (src/main/java/org/commcare/suite/model/Profile.java), wrap the credentials read in a try/catch for EOFException so legacy profiles without credentials deserialize to an empty Vector<Credential>:
- credentials = (Vector<Credential>)ExtUtil.read(in,
- new ExtWrapList(Credential.class), pf);
+ try {
+ credentials = (Vector<Credential>) ExtUtil.read(in,
+ new ExtWrapList(Credential.class), pf);
+ } catch (java.io.EOFException eof) {
+ // Backward‐compat: profiles serialized before credentials existed
+ credentials = new Vector<>();
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| credentials = (Vector<Credential>)ExtUtil.read(in, | |
| new ExtWrapList(Credential.class), pf); | |
| } | |
| try { | |
| credentials = (Vector<Credential>) ExtUtil.read(in, | |
| new ExtWrapList(Credential.class), pf); | |
| } catch (java.io.EOFException eof) { | |
| // Backward‐compat: profiles serialized before credentials existed | |
| credentials = new Vector<>(); | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/main/java/org/commcare/suite/model/Profile.java around lines 241 to 243,
the credentials deserialization should be guarded for backward compatibility:
wrap the ExtUtil.read(...) call in a try/catch that catches EOFException and,
when caught, assigns credentials = new Vector<Credential>() so legacy profiles
without credentials deserialize to an empty list; rethrow or let other
exceptions propagate unchanged so only missing-data EOF is handled.
avazirna
left a comment
avazirna
left a comment
There was a problem hiding this comment.
Is there a table with all potential levels and types? Or is that something yet to be determined.
We do have some fixed types and levels on server but think it doesn't matter for mobile as mobile will just show those values as it is for now. |
| if (level == null) { | ||
| throw new InvalidStructureException("No level defined for credential"); | ||
| } | ||
| if (type == null) { | ||
| throw new InvalidStructureException("No type defined for credential"); | ||
| } |
There was a problem hiding this comment.
@shubham1g5 Blank check is already along with null here?
| if (level == null) { | ||
| throw new InvalidStructureException("No level defined for credential"); |
There was a problem hiding this comment.
It is required to check the format of level e.g. for monthly it should be checking for MON_ACTIVE?
There was a problem hiding this comment.
I am keepint it free form at the moment on mobile as there is quite a bit of uncertainity regarding what all values these fields can take and we don't want to do a mobile update everytime we change the values here (given mobile is not doing any actions based on these values)
3 participants
Product Description
https://dimagi.atlassian.net/browse/CCCT-1339
Adds model for credentials, spec
Special deploy instructions
Rollback instructions
Review
Duplicate PR
Automatically duplicate this PR as defined in contributing.md.
Summary by CodeRabbit
New Features
Tests
cross-request: dimagi/commcare-android#3327