SAK-51434 LTI Remove the XMLMap Library from LTI #14309
Conversation
WalkthroughThis pull request introduces Jackson-based XML serialization for LTI request/response handling, replacing legacy Map-based and DOM-based XML utilities. Adds Maven dependencies for Jackson XML dataformat, refactors ServiceServlet to use builder-pattern responses, introduces comprehensive POX/LTI data model classes, and removes obsolete utility classes. Changes
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 6
🧹 Nitpick comments (25)
lti/tsugi-util/src/java/org/tsugi/lti/POXJacksonParser.java (3)
61-113: Consider caching theXmlMapperinstance.
XmlMapperis thread-safe and reusable. Creating a new instance on everyparseRequest/parseResponsecall adds unnecessary overhead, especially if called frequently.🔎 Proposed refactor
public class POXJacksonParser { private static final Logger log = LoggerFactory.getLogger(POXJacksonParser.class); + + private static final XmlMapper XML_MAPPER; + + static { + XML_MAPPER = new XmlMapper(); + XML_MAPPER.configure(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES, false); + XML_MAPPER.setDefaultUseWrapper(false); + } // ... constants ... public static POXEnvelopeRequest parseRequest(String xmlString) { if (xmlString == null || xmlString.trim().isEmpty()) { log.warn("XML string is null or empty"); return null; } try { - XmlMapper mapper = new XmlMapper(); - mapper.configure(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES, false); - mapper.setDefaultUseWrapper(false); - - POXEnvelopeRequest request = mapper.readValue(xmlString.trim(), POXEnvelopeRequest.class); + POXEnvelopeRequest request = XML_MAPPER.readValue(xmlString.trim(), POXEnvelopeRequest.class); // ... rest of method
127-135: UsePOXConstantsoperation constants for consistency.Hardcoded string literals duplicate values already defined in
POXConstants. Using the constants improves maintainability and reduces typo risk.🔎 Proposed fix
if (body.getReplaceResultRequest() != null) { - return "replaceResultRequest"; + return POXConstants.OPERATION_REPLACE_RESULT; } else if (body.getReadResultRequest() != null) { - return "readResultRequest"; + return POXConstants.OPERATION_READ_RESULT; } else if (body.getDeleteResultRequest() != null) { - return "deleteResultRequest"; + return POXConstants.OPERATION_DELETE_RESULT; } else if (body.getReadMembershipRequest() != null) { - return "readMembershipRequest"; + return POXConstants.OPERATION_READ_MEMBERSHIP; }
168-255: Consider extracting a helper method to reduce duplication.The
if-elsechain for extractingResultRecordfrom the request body is duplicated acrossgetBodySourcedId,getBodyLanguage, andgetBodyTextString.🔎 Proposed helper extraction
private static ResultRecord getResultRecord(POXRequestBody body) { if (body.getReplaceResultRequest() != null) { return body.getReplaceResultRequest().getResultRecord(); } else if (body.getReadResultRequest() != null) { return body.getReadResultRequest().getResultRecord(); } else if (body.getDeleteResultRequest() != null) { return body.getDeleteResultRequest().getResultRecord(); } return null; }Then simplify the methods:
public static String getBodySourcedId(POXEnvelopeRequest request) { if (request == null || request.getPoxBody() == null) { return null; } POXRequestBody body = request.getPoxBody(); - ResultRecord resultRecord = null; - - if (body.getReplaceResultRequest() != null) { - resultRecord = body.getReplaceResultRequest().getResultRecord(); - } else if (body.getReadResultRequest() != null) { - resultRecord = body.getReadResultRequest().getResultRecord(); - } else if (body.getDeleteResultRequest() != null) { - resultRecord = body.getDeleteResultRequest().getResultRecord(); - } + ResultRecord resultRecord = getResultRecord(body); if (resultRecord != null && resultRecord.getSourcedGUID() != null) { return resultRecord.getSourcedGUID().getSourcedId(); } // ... rest of methodlti/tsugi-util/src/java/org/tsugi/lti/objects/ResultScore.java (1)
7-16: Consider consolidating duplicate structure with AdminPeriod.
ResultScorehas an identical structure toAdminPeriod(both havelanguageandtextStringfields with the same XML mappings). If these represent the same semantic concept, consider:
- Using a shared base class or common type
- Or documenting why separate types are needed for domain clarity
If they represent distinct domain concepts despite structural similarity, this duplication is acceptable.
lti/tsugi-util/src/java/org/tsugi/lti/objects/Member.java (1)
3-3: Unused import: JacksonXmlElementWrapper.Line 3 imports
JacksonXmlElementWrapperbut this annotation is not used in the class. Consider removing it.lti/tsugi-util/src/java/org/tsugi/lti/objects/Group.java (1)
1-20: LGTM!The Group class correctly models group data with proper Jackson XML annotations. The absence of a namespace here is intentional since this is part of the membership response structure rather than the POX envelope.
Minor: Consider removing the trailing blank line at line 20 for consistency with other files in the package.
lti/tsugi-util/src/java/org/tsugi/lti/objects/MessageResponse.java (1)
1-25: LGTM!The MessageResponse class provides a clean root element for LTI message responses with appropriate Jackson XML annotations.
Consider adding
@JsonInclude(JsonInclude.Include.NON_NULL)to optional fields (likemembers,result) if null fields should be omitted from the serialized XML output, similar to the pattern used inPOXRequestBodyandPOXResponseBody. This would produce cleaner XML when only some response types are populated.🔎 Proposed enhancement
package org.tsugi.lti.objects; import com.fasterxml.jackson.dataformat.xml.annotation.JacksonXmlProperty; import com.fasterxml.jackson.dataformat.xml.annotation.JacksonXmlRootElement; +import com.fasterxml.jackson.annotation.JsonInclude; import lombok.Data; import lombok.ToString; @Data @ToString @JacksonXmlRootElement(localName = "message_response") public class MessageResponse { @JacksonXmlProperty(localName = "lti_message_type") private String ltiMessageType; @JacksonXmlProperty(localName = "members") + @JsonInclude(JsonInclude.Include.NON_NULL) private Members members; @JacksonXmlProperty(localName = "result") + @JsonInclude(JsonInclude.Include.NON_NULL) private Result result; @JacksonXmlProperty(localName = "statusinfo") private StatusInfo statusInfo; }lti/tsugi-util/src/java/org/tsugi/lti/objects/POXStatusInfo.java (1)
4-7: Remove unused imports.
JacksonXmlElementWrapperandjava.util.Listare imported but not used in this class.🔎 Proposed fix
package org.tsugi.lti.objects; import com.fasterxml.jackson.dataformat.xml.annotation.JacksonXmlProperty; -import com.fasterxml.jackson.dataformat.xml.annotation.JacksonXmlElementWrapper; import lombok.Data; import lombok.ToString; -import java.util.List;lti/tsugi-util/src/test/java/org/tsugi/pox/IMSPOXRequestJacksonTest.java (1)
191-194: Test method lacks assertions.
testRunTest()only calls the method without verifying any behavior or outcome. Consider adding assertions or documenting that this is purely a smoke test to verify the method doesn't throw exceptions.🔎 Proposed improvement
@Test public void testRunTest() { + // Smoke test to verify runTest() executes without throwing exceptions IMSPOXRequestJackson.runTest(); + // If runTest() completes without exception, the test passes }lti/tsugi-util/src/java/org/tsugi/lti/objects/POXRequestBody.java (1)
5-5: Remove unused import.
JacksonXmlTextis imported but not used in this class.🔎 Proposed fix
import com.fasterxml.jackson.annotation.JsonInclude; import com.fasterxml.jackson.dataformat.xml.annotation.JacksonXmlProperty; -import com.fasterxml.jackson.dataformat.xml.annotation.JacksonXmlText; import lombok.Data;lti/tsugi-util/src/java/org/tsugi/lti/objects/POXResponseBody.java (1)
5-5: Remove unused import.
JacksonXmlTextis imported but not used in this class.🔎 Proposed fix
import com.fasterxml.jackson.annotation.JsonInclude; import com.fasterxml.jackson.dataformat.xml.annotation.JacksonXmlProperty; -import com.fasterxml.jackson.dataformat.xml.annotation.JacksonXmlText; import lombok.Data;lti/tsugi-util/src/test/java/org/tsugi/lti/POXJacksonTest.java (2)
73-76: Consider removing empty setUp method.The
@Beforeannotated method is empty and serves no purpose. It can be safely removed.🔎 Proposed fix
- @Before - public void setUp() { - - } - @Test public void testParseRequest_Success() {
326-332: Unused variable in performance test loop.The
versionvariable is retrieved but never used. Either use it in an assertion or remove the assignment.🔎 Proposed fix
for (int i = 0; i < iterations; i++) { POXEnvelopeRequest jacksonRequest = POXJacksonParser.parseRequest(SAMPLE_REQUEST_XML); String operation = POXJacksonParser.getOperation(jacksonRequest); Map<String, String> headerInfo = POXJacksonParser.getHeaderInfo(jacksonRequest); - String version = headerInfo.get("version"); + assertNotNull("Header info should contain version", headerInfo.get("version")); }lti/tsugi-util/src/java/org/tsugi/pox/POXResponseBuilder.java (2)
26-32: Unused static XmlMapper configuration.The
xmlMapperis configured in the static block but never used for actual serialization. ThebuildAsXml()method uses manual string building instead. Consider removing this dead code or implementing Jackson-based serialization.🔎 Proposed fix (remove unused code)
-import com.fasterxml.jackson.dataformat.xml.XmlMapper; -import com.fasterxml.jackson.dataformat.xml.ser.ToXmlGenerator; - import lombok.extern.slf4j.Slf4j; import java.util.ArrayList; @Slf4j public class POXResponseBuilder { - - private static final XmlMapper xmlMapper = new XmlMapper(); - - static { - xmlMapper.configure(ToXmlGenerator.Feature.WRITE_XML_DECLARATION, true); - xmlMapper.configure(com.fasterxml.jackson.databind.SerializationFeature.FAIL_ON_EMPTY_BEANS, false); - xmlMapper.setDefaultUseWrapper(false); - } private String description;
160-166: Duplicate XML header stripping logic.The logic to strip
<?xmldeclarations from body content is duplicated in bothbuild()(lines 160-166) andbuildManualXml()(lines 196-203). Consider extracting to a private helper method.🔎 Proposed refactor
+ private String cleanBodyContent(String content) { + if (content == null || content.trim().isEmpty()) { + return ""; + } + String cleanBody = content.trim(); + if (cleanBody.startsWith("<?xml")) { + int pos = cleanBody.indexOf("<", 1); + if (pos > 0) cleanBody = cleanBody.substring(pos); + } + return cleanBody; + } + public POXEnvelopeResponse build() { // ... earlier code ... POXResponseBody body = new POXResponseBody(); - if (bodyContent != null && !bodyContent.trim().isEmpty()) { - String cleanBody = bodyContent.trim(); - if (cleanBody.startsWith("<?xml")) { - int pos = cleanBody.indexOf("<", 1); - if (pos > 0) cleanBody = cleanBody.substring(pos); - } + String cleanBody = cleanBodyContent(bodyContent); + if (!cleanBody.isEmpty()) { body.setRawContent(cleanBody); }Also applies to: 196-203
lti/tsugi-util/src/java/org/tsugi/lti/MessageResponseBuilder.java (1)
80-93: Silent failure on XML serialization error.Returning an empty string on serialization failure may cause downstream issues as callers cannot distinguish between a successful empty response and a failure. Consider throwing a runtime exception or returning an
Optional<String>to make failures explicit.The related
POXResponseFactory.createResponseFromObject(Lines 195-202) throws aRuntimeExceptionon serialization failure, which provides more explicit error handling.🔎 Proposed fix to propagate errors explicitly
public String buildAsXml(boolean pretty) { try { MessageResponse msgResponse = build(); if (pretty) { return xmlMapper.writerWithDefaultPrettyPrinter() .writeValueAsString(msgResponse); } else { return xmlMapper.writeValueAsString(msgResponse); } } catch (Exception e) { log.error("Error serializing MessageResponse to XML", e); - return ""; + throw new RuntimeException("Failed to serialize MessageResponse to XML", e); } }lti/tsugi-util/src/java/org/tsugi/pox/POXResponseFactory.java (1)
261-286: Duplicate validation logic - consider reusing POXConstants.The validation methods
isValidMajorCode,isValidSeverity, andisValidMinorCodeduplicate logic already present inPOXConstants(lines 103-141). This creates maintenance overhead and potential inconsistencies.🔎 Proposed fix to delegate to POXConstants
private static boolean isValidMajorCode(String major) { - for (String validMajor : IMSPOXRequestJackson.validMajor) { - if (validMajor.equals(major)) { - return true; - } - } - return false; + return POXConstants.isValidMajorCode(major); } private static boolean isValidSeverity(String severity) { - for (String validSeverity : IMSPOXRequestJackson.validSeverity) { - if (validSeverity.equals(severity)) { - return true; - } - } - return false; + return POXConstants.isValidSeverity(severity); } private static boolean isValidMinorCode(String minor) { - for (String validMinor : IMSPOXRequestJackson.validMinor) { - if (validMinor.equals(minor)) { - return true; - } - } - return false; + return POXConstants.isValidMinorCode(minor); }lti/tsugi-util/src/java/org/tsugi/pox/IMSPOXRequestJackson.java (4)
195-211: SHA-1 usage is spec-mandated for OAuth 1.0a body hash.The static analysis flagged SHA-1 as insecure, but this is required by the OAuth 1.0a specification for
oauth_body_hashcomputation. The code correctly supports SHA-256 whenHMAC-SHA256signature method is used (lines 197-199).However,
postBody.getBytes()on line 200 uses the platform default charset, which may cause hash mismatches if the client and server use different encodings. The OAuth spec recommends UTF-8.🔎 Proposed fix to specify UTF-8 charset
- md.update(postBody.getBytes()); + md.update(postBody.getBytes(java.nio.charset.StandardCharsets.UTF_8));
47-91: Duplicate constants with POXConstants.The major/severity/minor constants and their arrays are duplicated between
IMSPOXRequestJacksonandPOXConstants. This creates maintenance overhead and risks inconsistencies if one is updated without the other.Consider consolidating by having
IMSPOXRequestJacksonreferencePOXConstantsdirectly, or deprecating these in favor of the centralized constants class.
359-363: String concatenation in loop for error message building.Line 361 uses string concatenation (
"Invalid imsx_codeMinorFieldValue=" + value) inside a loop. While not a significant performance issue here, using the existingStringBuilderpattern (as done forinternalError) would be more consistent.
458-464: Consider using POXConstants.isValidMinorCode instead of inArray.The
inArrayhelper method duplicates validation logic available inPOXConstants. UsingPOXConstants.isValidMinorCode(value)and similar methods would reduce code duplication and ensure consistency.lti/tsugi-util/src/java/org/tsugi/pox/POXConstants.java (2)
3-12: Consider adding private constructor to prevent instantiation.As a utility class containing only static constants and methods,
POXConstantsshould have a private constructor to prevent accidental instantiation.🔎 Proposed fix
public class POXConstants { + + private POXConstants() { + // Utility class - prevent instantiation + } public static final String MAJOR_SUCCESS = "success";
10-12: Mutable array constants expose internal state.The
VALID_MAJOR,VALID_SEVERITY,VALID_MINOR, and other array constants are mutable. Callers could modify these arrays, affecting all subsequent validations. Consider usingCollections.unmodifiableListor defensive copies in the validation methods.Also applies to: 18-20, 43-51
lti/lti-blis/src/java/org/sakaiproject/lti/ServiceServlet.java (2)
669-678: XmlMapper instantiation inside debug block.A new
XmlMapperis instantiated on each debug log call (line 671). While this only executes when debug logging is enabled, it's inefficient. Consider reusing the staticPOX_XML_MAPPERor creating a dedicated mapper if different configuration is needed.🔎 Proposed fix to reuse existing mapper
if ( log.isDebugEnabled() ) { try { - XmlMapper mapper = new XmlMapper(); - Object obj = mapper.readValue(pox.getPostBody(), Object.class); - String pretty = mapper.writerWithDefaultPrettyPrinter().writeValueAsString(obj); + Object obj = POX_XML_MAPPER.readValue(pox.getPostBody(), Object.class); + String pretty = POX_XML_MAPPER.writerWithDefaultPrettyPrinter().writeValueAsString(obj); log.debug("POST\n{}", pretty); } catch (Exception e) { log.debug("POST\n{}", pox.getPostBody()); } }
571-581: Use enhanced for-loop with explicit type instead of Iterator.The Iterator pattern on line 572-573 can be simplified using an enhanced for-loop with explicit casting, improving readability while maintaining compliance with the coding guidelines (no
var).🔎 Proposed fix
if (groups.size() > 0) { List<org.tsugi.lti.objects.Group> lgm = new ArrayList<>(); - for (Iterator i = groups.iterator();i.hasNext();) { - org.sakaiproject.site.api.Group group = (org.sakaiproject.site.api.Group) i.next(); + for (Object groupObj : groups) { + org.sakaiproject.site.api.Group group = (org.sakaiproject.site.api.Group) groupObj; org.tsugi.lti.objects.Group ltiGroup = MessageResponseBuilder.createGroup( group.getId(), group.getTitle()); lgm.add(ltiGroup); }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (51)
lti/lti-blis/pom.xmllti/lti-blis/src/java/org/sakaiproject/lti/ServiceServlet.javalti/tsugi-util/pom.xmllti/tsugi-util/src/java/org/tsugi/json/IMSJSONRequest.javalti/tsugi-util/src/java/org/tsugi/lti/MessageResponseBuilder.javalti/tsugi-util/src/java/org/tsugi/lti/POXJacksonParser.javalti/tsugi-util/src/java/org/tsugi/lti/XMLMap.javalti/tsugi-util/src/java/org/tsugi/lti/objects/AdminPeriod.javalti/tsugi-util/src/java/org/tsugi/lti/objects/DeleteResultRequest.javalti/tsugi-util/src/java/org/tsugi/lti/objects/DeleteResultResponse.javalti/tsugi-util/src/java/org/tsugi/lti/objects/Group.javalti/tsugi-util/src/java/org/tsugi/lti/objects/GroupSet.javalti/tsugi-util/src/java/org/tsugi/lti/objects/Groups.javalti/tsugi-util/src/java/org/tsugi/lti/objects/Member.javalti/tsugi-util/src/java/org/tsugi/lti/objects/Members.javalti/tsugi-util/src/java/org/tsugi/lti/objects/Membership.javalti/tsugi-util/src/java/org/tsugi/lti/objects/MembershipRecord.javalti/tsugi-util/src/java/org/tsugi/lti/objects/MessageResponse.javalti/tsugi-util/src/java/org/tsugi/lti/objects/POXCodeMinor.javalti/tsugi-util/src/java/org/tsugi/lti/objects/POXCodeMinorField.javalti/tsugi-util/src/java/org/tsugi/lti/objects/POXEnvelopeRequest.javalti/tsugi-util/src/java/org/tsugi/lti/objects/POXEnvelopeResponse.javalti/tsugi-util/src/java/org/tsugi/lti/objects/POXRequestBody.javalti/tsugi-util/src/java/org/tsugi/lti/objects/POXRequestHeader.javalti/tsugi-util/src/java/org/tsugi/lti/objects/POXRequestHeaderInfo.javalti/tsugi-util/src/java/org/tsugi/lti/objects/POXResponseBody.javalti/tsugi-util/src/java/org/tsugi/lti/objects/POXResponseHeader.javalti/tsugi-util/src/java/org/tsugi/lti/objects/POXResponseHeaderInfo.javalti/tsugi-util/src/java/org/tsugi/lti/objects/POXStatusInfo.javalti/tsugi-util/src/java/org/tsugi/lti/objects/ReadMembershipRequest.javalti/tsugi-util/src/java/org/tsugi/lti/objects/ReadMembershipResponse.javalti/tsugi-util/src/java/org/tsugi/lti/objects/ReadResultRequest.javalti/tsugi-util/src/java/org/tsugi/lti/objects/ReadResultResponse.javalti/tsugi-util/src/java/org/tsugi/lti/objects/ReplaceResultRequest.javalti/tsugi-util/src/java/org/tsugi/lti/objects/ReplaceResultResponse.javalti/tsugi-util/src/java/org/tsugi/lti/objects/Result.javalti/tsugi-util/src/java/org/tsugi/lti/objects/ResultData.javalti/tsugi-util/src/java/org/tsugi/lti/objects/ResultRecord.javalti/tsugi-util/src/java/org/tsugi/lti/objects/ResultScore.javalti/tsugi-util/src/java/org/tsugi/lti/objects/Role.javalti/tsugi-util/src/java/org/tsugi/lti/objects/SourcedGUID.javalti/tsugi-util/src/java/org/tsugi/lti/objects/StatusInfo.javalti/tsugi-util/src/java/org/tsugi/lti/objects/TimeFrame.javalti/tsugi-util/src/java/org/tsugi/pox/IMSPOXRequest.javalti/tsugi-util/src/java/org/tsugi/pox/IMSPOXRequestJackson.javalti/tsugi-util/src/java/org/tsugi/pox/POXConstants.javalti/tsugi-util/src/java/org/tsugi/pox/POXResponseBuilder.javalti/tsugi-util/src/java/org/tsugi/pox/POXResponseFactory.javalti/tsugi-util/src/test/java/org/tsugi/lti/POXJacksonTest.javalti/tsugi-util/src/test/java/org/tsugi/pox/IMSPOXRequestJacksonTest.javalti/tsugi-util/src/test/java/org/tsugi/pox/POXResponseBuilderTest.java
💤 Files with no reviewable changes (3)
- lti/tsugi-util/src/java/org/tsugi/json/IMSJSONRequest.java
- lti/tsugi-util/src/java/org/tsugi/lti/XMLMap.java
- lti/tsugi-util/src/java/org/tsugi/pox/IMSPOXRequest.java
🧰 Additional context used
📓 Path-based instructions (5)
**/*.java
📄 CodeRabbit inference engine (.cursor/rules/logging-rule.mdc)
**/*.java: Use SLF4J parameterized logging (logger.info("Value is: {}", value)) instead of string concatenation (logger.info("Value is: " + value))
Log messages and code comments should be in English. Log messages should never be translated.
**/*.java: Java: Never use local variable type inference (var). Always declare explicit types. Yes:Map<String, Integer> counts = new HashMap<>();No:var counts = new HashMap<String, Integer>();
When proposing Java code, spell out full types in local variable declarations,forloops, and try-with-resources
When editing Java, prefer clarity over brevity; avoid introducing language features that aren't widely used in the repo
Treat any PR or suggestion containing Javavaras non-compliant. Recommend replacing with explicit types before merge
**/*.java: Use Java 17 for trunk development (Java 11 was used for Sakai 22 and Sakai 23)
Do not use local variable type inference (var) in Java code. Always declare explicit types (e.g.,List<String> names = new ArrayList<>();notvar names = new ArrayList<String>();). Enforced by Checkstyle rule duringmvn validate
Files:
lti/tsugi-util/src/java/org/tsugi/lti/objects/ReadMembershipResponse.javalti/tsugi-util/src/java/org/tsugi/lti/objects/Role.javalti/tsugi-util/src/java/org/tsugi/lti/objects/ReplaceResultRequest.javalti/tsugi-util/src/java/org/tsugi/lti/objects/Groups.javalti/tsugi-util/src/java/org/tsugi/lti/objects/POXEnvelopeResponse.javalti/tsugi-util/src/java/org/tsugi/lti/objects/StatusInfo.javalti/tsugi-util/src/java/org/tsugi/lti/objects/POXRequestHeaderInfo.javalti/tsugi-util/src/java/org/tsugi/lti/objects/ResultRecord.javalti/tsugi-util/src/java/org/tsugi/lti/objects/ReadResultResponse.javalti/tsugi-util/src/java/org/tsugi/lti/objects/AdminPeriod.javalti/tsugi-util/src/java/org/tsugi/lti/objects/SourcedGUID.javalti/tsugi-util/src/test/java/org/tsugi/pox/IMSPOXRequestJacksonTest.javalti/tsugi-util/src/test/java/org/tsugi/pox/POXResponseBuilderTest.javalti/tsugi-util/src/java/org/tsugi/lti/objects/POXCodeMinorField.javalti/tsugi-util/src/java/org/tsugi/lti/objects/ReadResultRequest.javalti/tsugi-util/src/java/org/tsugi/lti/objects/MessageResponse.javalti/tsugi-util/src/java/org/tsugi/lti/objects/ReadMembershipRequest.javalti/tsugi-util/src/java/org/tsugi/lti/objects/POXResponseHeaderInfo.javalti/tsugi-util/src/java/org/tsugi/lti/objects/Membership.javalti/tsugi-util/src/java/org/tsugi/lti/objects/DeleteResultResponse.javalti/tsugi-util/src/java/org/tsugi/lti/objects/ReplaceResultResponse.javalti/tsugi-util/src/java/org/tsugi/lti/objects/MembershipRecord.javalti/tsugi-util/src/java/org/tsugi/lti/objects/Result.javalti/tsugi-util/src/java/org/tsugi/lti/objects/ResultScore.javalti/tsugi-util/src/java/org/tsugi/lti/objects/ResultData.javalti/tsugi-util/src/java/org/tsugi/lti/objects/POXEnvelopeRequest.javalti/tsugi-util/src/java/org/tsugi/lti/objects/GroupSet.javalti/tsugi-util/src/java/org/tsugi/lti/objects/DeleteResultRequest.javalti/tsugi-util/src/java/org/tsugi/lti/objects/POXRequestBody.javalti/tsugi-util/src/java/org/tsugi/lti/objects/TimeFrame.javalti/tsugi-util/src/java/org/tsugi/lti/objects/Members.javalti/tsugi-util/src/test/java/org/tsugi/lti/POXJacksonTest.javalti/tsugi-util/src/java/org/tsugi/lti/objects/POXRequestHeader.javalti/tsugi-util/src/java/org/tsugi/lti/objects/Member.javalti/tsugi-util/src/java/org/tsugi/lti/objects/POXCodeMinor.javalti/tsugi-util/src/java/org/tsugi/pox/POXResponseFactory.javalti/tsugi-util/src/java/org/tsugi/lti/objects/POXResponseBody.javalti/tsugi-util/src/java/org/tsugi/pox/IMSPOXRequestJackson.javalti/tsugi-util/src/java/org/tsugi/lti/POXJacksonParser.javalti/tsugi-util/src/java/org/tsugi/lti/objects/POXResponseHeader.javalti/tsugi-util/src/java/org/tsugi/lti/objects/POXStatusInfo.javalti/tsugi-util/src/java/org/tsugi/pox/POXResponseBuilder.javalti/tsugi-util/src/java/org/tsugi/lti/objects/Group.javalti/tsugi-util/src/java/org/tsugi/lti/MessageResponseBuilder.javalti/tsugi-util/src/java/org/tsugi/pox/POXConstants.javalti/lti-blis/src/java/org/sakaiproject/lti/ServiceServlet.java
**/*.{html,jsp,vm,xhtml,xml}
📄 CodeRabbit inference engine (.cursor/rules/frontend.mdc)
**/*.{html,jsp,vm,xhtml,xml}: Use Bootstrap 5.2 as the preferred UI framework for styling in Sakai frontends
Leverage Bootstrap 5 components for consistent UI/UX
Files:
lti/tsugi-util/pom.xmllti/lti-blis/pom.xml
**/*.{html,jsp,vm,xhtml,xml,css,scss}
📄 CodeRabbit inference engine (.cursor/rules/frontend.mdc)
Ensure all UI components work across different screen sizes (Responsive Design)
Files:
lti/tsugi-util/pom.xmllti/lti-blis/pom.xml
**/*.{js,html,jsp,vm,xhtml,xml}
📄 CodeRabbit inference engine (.cursor/rules/frontend.mdc)
Use the web components in the webcomponents/ directory when possible in Sakai frontends
Files:
lti/tsugi-util/pom.xmllti/lti-blis/pom.xml
**/*.{html,jsp,jspx,xml,ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Prefer kebab-case for values of HTML class and id attributes
Files:
lti/tsugi-util/pom.xmllti/lti-blis/pom.xml
🧠 Learnings (2)
📓 Common learnings
Learnt from: csev
Repo: sakaiproject/sakai PR: 14094
File: lti/lti-common/src/java/org/sakaiproject/lti/util/SakaiLTIUtil.java:1439-1447
Timestamp: 2025-09-28T04:09:37.259Z
Learning: csev prefers to keep POJO adapter methods thin/minimal during the transition phase in SAK-51970. The long-term plan is to eventually flip roles where POJOs become primary implementations and Map versions become thin adapters, then get deprecated and removed. This transition will happen over multiple commits over time.
📚 Learning: 2025-09-28T04:09:37.259Z
Learnt from: csev
Repo: sakaiproject/sakai PR: 14094
File: lti/lti-common/src/java/org/sakaiproject/lti/util/SakaiLTIUtil.java:1439-1447
Timestamp: 2025-09-28T04:09:37.259Z
Learning: csev prefers to keep POJO adapter methods thin/minimal during the transition phase in SAK-51970. The long-term plan is to eventually flip roles where POJOs become primary implementations and Map versions become thin adapters, then get deprecated and removed. This transition will happen over multiple commits over time.
Applied to files:
lti/lti-blis/src/java/org/sakaiproject/lti/ServiceServlet.java
🧬 Code graph analysis (34)
lti/tsugi-util/src/java/org/tsugi/lti/objects/ReadMembershipResponse.java (3)
lti/tsugi-util/src/java/org/tsugi/lti/objects/DeleteResultRequest.java (1)
Data(7-13)lti/tsugi-util/src/java/org/tsugi/lti/objects/DeleteResultResponse.java (1)
Data(12-17)lti/tsugi-util/src/java/org/tsugi/lti/objects/MembershipRecord.java (1)
Data(7-16)
lti/tsugi-util/src/java/org/tsugi/lti/objects/ReplaceResultRequest.java (2)
lti/tsugi-util/src/java/org/tsugi/lti/objects/DeleteResultRequest.java (1)
Data(7-13)lti/tsugi-util/src/java/org/tsugi/lti/objects/POXRequestBody.java (1)
Data(9-31)
lti/tsugi-util/src/java/org/tsugi/lti/objects/Groups.java (2)
lti/tsugi-util/src/java/org/tsugi/lti/objects/Group.java (1)
Data(7-19)lti/tsugi-util/src/java/org/tsugi/lti/objects/GroupSet.java (1)
Data(7-16)
lti/tsugi-util/src/java/org/tsugi/lti/objects/POXEnvelopeResponse.java (3)
lti/tsugi-util/src/java/org/tsugi/lti/objects/POXEnvelopeRequest.java (1)
JacksonXmlRootElement(8-18)lti/tsugi-util/src/java/org/tsugi/lti/objects/DeleteResultResponse.java (1)
Data(12-17)lti/tsugi-util/src/java/org/tsugi/lti/objects/POXRequestHeader.java (1)
Data(7-13)
lti/tsugi-util/src/java/org/tsugi/lti/objects/StatusInfo.java (1)
lti/tsugi-util/src/java/org/tsugi/lti/objects/MessageResponse.java (1)
Data(8-24)
lti/tsugi-util/src/java/org/tsugi/lti/objects/ResultRecord.java (2)
lti/tsugi-util/src/java/org/tsugi/lti/objects/DeleteResultRequest.java (1)
Data(7-13)lti/tsugi-util/src/java/org/tsugi/lti/objects/DeleteResultResponse.java (1)
Data(12-17)
lti/tsugi-util/src/java/org/tsugi/lti/objects/ReadResultResponse.java (1)
lti/tsugi-util/src/java/org/tsugi/lti/objects/DeleteResultResponse.java (1)
Data(12-17)
lti/tsugi-util/src/java/org/tsugi/lti/objects/AdminPeriod.java (16)
lti/tsugi-util/src/java/org/tsugi/lti/objects/DeleteResultRequest.java (1)
Data(7-13)lti/tsugi-util/src/java/org/tsugi/lti/objects/DeleteResultResponse.java (1)
Data(12-17)lti/tsugi-util/src/java/org/tsugi/lti/objects/Group.java (1)
Data(7-19)lti/tsugi-util/src/java/org/tsugi/lti/objects/GroupSet.java (1)
Data(7-16)lti/tsugi-util/src/java/org/tsugi/lti/objects/Groups.java (1)
Data(9-16)lti/tsugi-util/src/java/org/tsugi/lti/objects/Member.java (1)
Data(9-42)lti/tsugi-util/src/java/org/tsugi/lti/objects/Members.java (1)
Data(9-16)lti/tsugi-util/src/java/org/tsugi/lti/objects/Membership.java (1)
Data(9-28)lti/tsugi-util/src/java/org/tsugi/lti/objects/MembershipRecord.java (1)
Data(7-16)lti/tsugi-util/src/java/org/tsugi/lti/objects/MessageResponse.java (1)
Data(8-24)lti/tsugi-util/src/java/org/tsugi/lti/objects/POXCodeMinor.java (1)
Data(9-16)lti/tsugi-util/src/java/org/tsugi/lti/objects/POXCodeMinorField.java (1)
Data(7-16)lti/tsugi-util/src/java/org/tsugi/lti/objects/POXRequestBody.java (1)
Data(9-31)lti/tsugi-util/src/java/org/tsugi/lti/objects/POXRequestHeader.java (1)
Data(7-13)lti/tsugi-util/src/java/org/tsugi/lti/objects/POXRequestHeaderInfo.java (1)
Data(7-16)lti/tsugi-util/src/java/org/tsugi/lti/objects/POXResponseBody.java (1)
Data(9-31)
lti/tsugi-util/src/java/org/tsugi/lti/objects/SourcedGUID.java (5)
lti/tsugi-util/src/java/org/tsugi/lti/objects/DeleteResultRequest.java (1)
Data(7-13)lti/tsugi-util/src/java/org/tsugi/lti/objects/GroupSet.java (1)
Data(7-16)lti/tsugi-util/src/java/org/tsugi/lti/objects/Member.java (1)
Data(9-42)lti/tsugi-util/src/java/org/tsugi/lti/objects/POXCodeMinor.java (1)
Data(9-16)lti/tsugi-util/src/java/org/tsugi/lti/objects/POXRequestHeaderInfo.java (1)
Data(7-16)
lti/tsugi-util/src/test/java/org/tsugi/pox/IMSPOXRequestJacksonTest.java (1)
lti/tsugi-util/src/java/org/tsugi/lti/POXJacksonParser.java (1)
POXJacksonParser(21-635)
lti/tsugi-util/src/java/org/tsugi/lti/objects/POXCodeMinorField.java (1)
lti/tsugi-util/src/java/org/tsugi/lti/objects/POXCodeMinor.java (1)
Data(9-16)
lti/tsugi-util/src/java/org/tsugi/lti/objects/ReadResultRequest.java (2)
lti/tsugi-util/src/java/org/tsugi/lti/objects/DeleteResultRequest.java (1)
Data(7-13)lti/tsugi-util/src/java/org/tsugi/lti/objects/POXRequestBody.java (1)
Data(9-31)
lti/tsugi-util/src/java/org/tsugi/lti/objects/MessageResponse.java (2)
lti/tsugi-util/src/java/org/tsugi/lti/objects/Member.java (1)
Data(9-42)lti/tsugi-util/src/java/org/tsugi/lti/objects/Members.java (1)
Data(9-16)
lti/tsugi-util/src/java/org/tsugi/lti/objects/POXResponseHeaderInfo.java (1)
lti/tsugi-util/src/java/org/tsugi/lti/objects/POXRequestHeaderInfo.java (1)
Data(7-16)
lti/tsugi-util/src/java/org/tsugi/lti/objects/Membership.java (5)
lti/tsugi-util/src/java/org/tsugi/lti/objects/Group.java (1)
Data(7-19)lti/tsugi-util/src/java/org/tsugi/lti/objects/GroupSet.java (1)
Data(7-16)lti/tsugi-util/src/java/org/tsugi/lti/objects/Groups.java (1)
Data(9-16)lti/tsugi-util/src/java/org/tsugi/lti/objects/Member.java (1)
Data(9-42)lti/tsugi-util/src/java/org/tsugi/lti/objects/Members.java (1)
Data(9-16)
lti/tsugi-util/src/java/org/tsugi/lti/objects/DeleteResultResponse.java (4)
lti/tsugi-util/src/java/org/tsugi/lti/objects/POXEnvelopeResponse.java (1)
JacksonXmlRootElement(8-18)lti/tsugi-util/src/java/org/tsugi/lti/objects/DeleteResultRequest.java (1)
Data(7-13)lti/tsugi-util/src/java/org/tsugi/lti/objects/POXRequestBody.java (1)
Data(9-31)lti/tsugi-util/src/java/org/tsugi/lti/objects/POXResponseBody.java (1)
Data(9-31)
lti/tsugi-util/src/java/org/tsugi/lti/objects/ReplaceResultResponse.java (2)
lti/tsugi-util/src/java/org/tsugi/lti/objects/DeleteResultRequest.java (1)
Data(7-13)lti/tsugi-util/src/java/org/tsugi/lti/objects/DeleteResultResponse.java (1)
Data(12-17)
lti/tsugi-util/src/java/org/tsugi/lti/objects/MembershipRecord.java (2)
lti/tsugi-util/src/java/org/tsugi/lti/objects/Member.java (1)
Data(9-42)lti/tsugi-util/src/java/org/tsugi/lti/objects/Membership.java (1)
Data(9-28)
lti/tsugi-util/src/java/org/tsugi/lti/objects/Result.java (1)
lti/tsugi-util/src/java/org/tsugi/lti/objects/MessageResponse.java (1)
Data(8-24)
lti/tsugi-util/src/java/org/tsugi/lti/objects/ResultScore.java (1)
lti/tsugi-util/src/java/org/tsugi/lti/objects/AdminPeriod.java (1)
Data(7-16)
lti/tsugi-util/src/java/org/tsugi/lti/objects/ResultData.java (2)
lti/tsugi-util/src/java/org/tsugi/lti/objects/DeleteResultRequest.java (1)
Data(7-13)lti/tsugi-util/src/java/org/tsugi/lti/objects/POXRequestHeaderInfo.java (1)
Data(7-16)
lti/tsugi-util/src/java/org/tsugi/lti/objects/DeleteResultRequest.java (3)
lti/tsugi-util/src/java/org/tsugi/lti/objects/DeleteResultResponse.java (1)
Data(12-17)lti/tsugi-util/src/java/org/tsugi/lti/objects/POXRequestBody.java (1)
Data(9-31)lti/tsugi-util/src/java/org/tsugi/lti/objects/POXResponseBody.java (1)
Data(9-31)
lti/tsugi-util/src/java/org/tsugi/lti/objects/POXRequestBody.java (2)
lti/tsugi-util/src/java/org/tsugi/lti/objects/DeleteResultRequest.java (1)
Data(7-13)lti/tsugi-util/src/java/org/tsugi/lti/objects/POXRequestHeader.java (1)
Data(7-13)
lti/tsugi-util/src/java/org/tsugi/lti/objects/TimeFrame.java (1)
lti/tsugi-util/src/java/org/tsugi/lti/objects/AdminPeriod.java (1)
Data(7-16)
lti/tsugi-util/src/java/org/tsugi/lti/objects/Members.java (3)
lti/tsugi-util/src/java/org/tsugi/lti/objects/Groups.java (1)
Data(9-16)lti/tsugi-util/src/java/org/tsugi/lti/objects/Member.java (1)
Data(9-42)lti/tsugi-util/src/java/org/tsugi/lti/objects/MessageResponse.java (1)
Data(8-24)
lti/tsugi-util/src/test/java/org/tsugi/lti/POXJacksonTest.java (1)
lti/tsugi-util/src/java/org/tsugi/lti/POXJacksonParser.java (1)
POXJacksonParser(21-635)
lti/tsugi-util/src/java/org/tsugi/lti/objects/POXRequestHeader.java (3)
lti/tsugi-util/src/java/org/tsugi/lti/objects/POXRequestBody.java (1)
Data(9-31)lti/tsugi-util/src/java/org/tsugi/lti/objects/POXRequestHeaderInfo.java (1)
Data(7-16)lti/tsugi-util/src/java/org/tsugi/lti/objects/POXResponseBody.java (1)
Data(9-31)
lti/tsugi-util/src/java/org/tsugi/lti/objects/Member.java (2)
lti/tsugi-util/src/java/org/tsugi/lti/objects/Groups.java (1)
Data(9-16)lti/tsugi-util/src/java/org/tsugi/lti/objects/Membership.java (1)
Data(9-28)
lti/tsugi-util/src/java/org/tsugi/lti/objects/POXCodeMinor.java (1)
lti/tsugi-util/src/java/org/tsugi/lti/objects/POXCodeMinorField.java (1)
Data(7-16)
lti/tsugi-util/src/java/org/tsugi/lti/objects/POXResponseBody.java (1)
lti/tsugi-util/src/java/org/tsugi/lti/objects/DeleteResultResponse.java (1)
Data(12-17)
lti/tsugi-util/src/java/org/tsugi/lti/POXJacksonParser.java (1)
lti/tsugi-util/src/java/org/tsugi/pox/POXConstants.java (1)
POXConstants(3-142)
lti/tsugi-util/src/java/org/tsugi/lti/objects/POXResponseHeader.java (2)
lti/tsugi-util/src/java/org/tsugi/lti/objects/POXRequestHeader.java (1)
Data(7-13)lti/tsugi-util/src/java/org/tsugi/lti/objects/POXRequestHeaderInfo.java (1)
Data(7-16)
lti/tsugi-util/src/java/org/tsugi/lti/objects/POXStatusInfo.java (4)
lti/tsugi-util/src/java/org/tsugi/lti/objects/POXCodeMinor.java (1)
Data(9-16)lti/tsugi-util/src/java/org/tsugi/lti/objects/POXCodeMinorField.java (1)
Data(7-16)lti/tsugi-util/src/java/org/tsugi/lti/objects/POXRequestHeader.java (1)
Data(7-13)lti/tsugi-util/src/java/org/tsugi/lti/objects/POXRequestHeaderInfo.java (1)
Data(7-16)
lti/lti-blis/src/java/org/sakaiproject/lti/ServiceServlet.java (1)
lti/tsugi-util/src/java/org/tsugi/lti/POXJacksonParser.java (1)
POXJacksonParser(21-635)
🪛 ast-grep (0.40.3)
lti/tsugi-util/src/java/org/tsugi/pox/IMSPOXRequestJackson.java
[warning] 195-195: Detected SHA1 hash algorithm which is considered insecure. SHA1 is not collision resistant and is therefore not suitable as a cryptographic signature. Instead, use PBKDF2 for password hashing or SHA256 or SHA512 for other hash function applications.
Context: MessageDigest.getInstance("SHA-1")
Note: [CWE-328] Use of Weak Hash. [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(use-of-sha1-java)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: maven-build
- GitHub Check: maven-build
- GitHub Check: sakai-deploy
🔇 Additional comments (50)
lti/tsugi-util/src/java/org/tsugi/lti/objects/AdminPeriod.java (1)
1-16: LGTM!The
AdminPeriodclass follows the established pattern used by other POX data model classes in this package (e.g.,GroupSet,POXCodeMinorField). The Lombok annotations and Jackson XML property mappings are correctly applied.lti/tsugi-util/src/java/org/tsugi/lti/objects/Role.java (1)
1-28: LGTM!The
Roleclass is well-structured and consistent with other POX data model classes. The use ofStringfordateTimeis acceptable for XML binding flexibility, as different XML sources may use varying date formats.lti/tsugi-util/src/java/org/tsugi/lti/objects/TimeFrame.java (1)
1-22: LGTM!The
TimeFrameclass follows the established POX data model patterns. Good choice usingBooleanwrapper type forrestrictto handle optional/null values in XML deserialization. TheStringtypes forbeginandendprovide flexibility for various date format handling during XML binding.lti/lti-blis/pom.xml (1)
65-68: LGTM!The
jackson-dataformat-xmldependency addition is consistent with the existing Jackson dependencies pattern in this module and properly relies on parent POM for version management.lti/tsugi-util/src/java/org/tsugi/lti/objects/GroupSet.java (1)
7-16: LGTM!Clean implementation following the established pattern for POX value objects in this module.
Nit:
@ToStringis redundant since@Dataalready includes it. However, this pattern appears consistently across all similar POJOs in this PR, so keeping it for uniformity is acceptable.lti/tsugi-util/src/java/org/tsugi/lti/objects/SourcedGUID.java (1)
7-13: LGTM!Simple value object with proper Jackson XML mapping for the
sourcedIdelement.lti/tsugi-util/src/java/org/tsugi/lti/objects/ReadMembershipRequest.java (1)
7-13: LGTM!Request object follows the established pattern and integrates correctly with
POXRequestBody.lti/tsugi-util/src/java/org/tsugi/lti/objects/ResultData.java (1)
7-14: LGTM!Value object for result data with proper XML mapping.
lti/tsugi-util/src/java/org/tsugi/lti/objects/DeleteResultResponse.java (1)
7-17: LGTM!Empty response body is appropriate for delete operations. Good documentation clarifying the intentional empty design.
lti/tsugi-util/src/java/org/tsugi/lti/objects/ReadResultRequest.java (1)
7-13: LGTM!Request object follows the same pattern as
DeleteResultRequestand integrates correctly withPOXRequestBody.lti/tsugi-util/src/java/org/tsugi/lti/POXJacksonParser.java (4)
25-53: LGTM!Re-exporting constants from
POXConstantsprovides a convenient single-class API for consumers. Based on learnings, this aligns with the transition approach where POJO adapters remain thin during migration.
146-159: LGTM!Proper null-safety with early return for invalid input.
575-595: LGTM!The
ValidationResultclass provides a clean API for accumulating validation errors. Uses explicit types as per coding guidelines.
597-634: LGTM!The
OperationInfoinner class provides structured access to operation details.Nit: Other POJOs in this PR use Lombok
@Data. Consider using it here for consistency, though manual getters/setters are also fine for inner classes.lti/tsugi-util/pom.xml (1)
64-67: LGTM: Jackson XML dependency properly added.The
jackson-dataformat-xmldependency addition is correctly formatted and complements the existing Jackson dependencies for XML serialization support.lti/tsugi-util/src/java/org/tsugi/lti/objects/MembershipRecord.java (1)
1-16: LGTM: Clean data holder for membership records.The class follows a consistent DTO pattern with appropriate Jackson XML mappings for the POX envelope structure.
lti/tsugi-util/src/java/org/tsugi/lti/objects/Member.java (1)
9-42: LGTM: Comprehensive member data model.The class provides a well-structured representation of member data with appropriate Jackson XML mappings for LTI/POX standards.
lti/tsugi-util/src/java/org/tsugi/lti/objects/ReplaceResultResponse.java (1)
1-12: LGTM: Appropriate empty response DTO.The empty response body with
@JacksonXmlRootElementis suitable for operations that don't return data beyond status codes.lti/tsugi-util/src/java/org/tsugi/lti/objects/ReplaceResultRequest.java (1)
1-13: LGTM: Clean request DTO.The class follows the established pattern for POX request types with appropriate Jackson XML mapping.
lti/tsugi-util/src/java/org/tsugi/lti/objects/Result.java (1)
1-20: LGTM: Well-structured result data model.The class provides appropriate fields for LTI result representation with correct Jackson XML mappings.
lti/tsugi-util/src/java/org/tsugi/lti/objects/ReadMembershipResponse.java (1)
1-13: LGTM: Clean response DTO for membership data.The class follows the established pattern for POX response types with appropriate Jackson XML mapping.
lti/tsugi-util/src/java/org/tsugi/lti/objects/Members.java (1)
9-16: LGTM!The class correctly uses
@JacksonXmlElementWrapper(useWrapping = false)with@JacksonXmlPropertyto serialize the list as repeated<member>elements without a container wrapper. This pattern is consistent with theGroupsclass implementation.lti/tsugi-util/src/java/org/tsugi/lti/objects/ResultRecord.java (1)
7-16: LGTM!The class is a well-structured Jackson-annotated POJO with appropriate XML mappings for the
sourcedGUIDandresultfields. Lombok annotations provide the necessary boilerplate.lti/tsugi-util/src/java/org/tsugi/lti/objects/POXCodeMinor.java (1)
9-16: LGTM!The class correctly implements the unwrapped list pattern with the IMS namespace for POX code minor fields. The
@JacksonXmlElementWrapper(useWrapping = false)ensures eachPOXCodeMinorFieldserializes as a separate<imsx_codeMinorField>element.lti/tsugi-util/src/java/org/tsugi/lti/objects/Groups.java (1)
9-16: LGTM!The class follows the established pattern for unwrapped XML list serialization, consistent with
Members.java. TheGroupelements from the related snippet show proper XML mappings for nested properties.lti/tsugi-util/src/java/org/tsugi/lti/objects/ReadResultResponse.java (1)
8-16: LGTM!The class correctly defines the XML root element and result property mapping. It follows the same pattern as
DeleteResultResponseand integrates well with thePOXResponseBodystructure.lti/tsugi-util/src/java/org/tsugi/lti/objects/StatusInfo.java (1)
7-22: LGTM!The class provides proper XML bindings for status information fields used by
MessageResponse. The lowercase XML element names align with typical XML conventions while maintaining Java camelCase for field names.lti/tsugi-util/src/java/org/tsugi/lti/objects/DeleteResultRequest.java (1)
7-13: LGTM!The class is correctly structured as a Jackson-annotated request POJO, consistent with the other request types (
ReplaceResultRequest,ReadResultRequest) referenced inPOXRequestBody.lti/tsugi-util/src/java/org/tsugi/lti/objects/POXEnvelopeResponse.java (1)
1-18: LGTM!The POXEnvelopeResponse class is well-structured with appropriate Jackson XML annotations. The namespace and localName values are consistent with the POXEnvelopeRequest counterpart, and the Lombok annotations reduce boilerplate effectively.
lti/tsugi-util/src/java/org/tsugi/lti/objects/POXRequestHeader.java (1)
1-13: LGTM!Clean wrapper class for the POX request header with proper Jackson XML binding. The namespace is consistent with the IMS POX specification.
lti/tsugi-util/src/java/org/tsugi/lti/objects/POXResponseHeaderInfo.java (1)
1-19: LGTM!The POXResponseHeaderInfo correctly mirrors POXRequestHeaderInfo while adding the
statusInfofield that's specific to responses. The XML mapping with the IMS namespace is consistent throughout.lti/tsugi-util/src/java/org/tsugi/lti/objects/POXCodeMinorField.java (1)
1-16: LGTM!Well-defined element class for code minor fields. The XML property mappings align with the IMS POX schema, and the class integrates correctly with
POXCodeMinorwhich uses it as a list element type.lti/tsugi-util/src/java/org/tsugi/lti/objects/POXRequestHeaderInfo.java (1)
1-16: LGTM!Clean data class for POX request header info with appropriate XML binding annotations. The IMS namespace is correctly applied to both fields.
lti/tsugi-util/src/java/org/tsugi/lti/objects/Membership.java (1)
1-28: LGTM!The Membership class is well-structured with appropriate Jackson XML annotations. The use of
@JacksonXmlElementWrapper(useWrapping = false)on thememberslist ensures direct serialization of<member>elements without an enclosing wrapper. UsingIntegerforcreditHoursis appropriate as it allows null values.lti/tsugi-util/src/test/java/org/tsugi/pox/POXResponseBuilderTest.java (1)
1-224: LGTM! Comprehensive test coverage for POXResponseBuilder.The test class provides thorough coverage of the builder pattern including:
- Success, failure, unsupported, and processing response types
- Both fluent builder API and static factory methods
- Edge cases like null messageId, empty body content, and XML header stripping
lti/tsugi-util/src/java/org/tsugi/lti/objects/POXStatusInfo.java (1)
9-30: LGTM! Well-structured POX status info model.The class correctly maps all IMS LTI v1.1 status info fields with proper namespace annotations.
lti/tsugi-util/src/test/java/org/tsugi/pox/IMSPOXRequestJacksonTest.java (1)
1-251: Good test coverage for IMSPOXRequestJackson.The test suite comprehensively covers:
- Parsing valid/invalid/null/empty XML
- Header and body field extraction
- Response generation (success, failure, unsupported)
- Minor code handling and validation
lti/tsugi-util/src/java/org/tsugi/lti/objects/POXRequestBody.java (1)
9-31: LGTM! Well-structured POX request body model.The class correctly uses
@JsonInclude(NON_NULL)to handle optional request types, allowing only one operation type to be present in the body at a time.lti/tsugi-util/src/java/org/tsugi/lti/objects/POXResponseBody.java (1)
9-31: LGTM! Consistent structure with POXRequestBody.The response body model correctly mirrors the request body structure for proper POX envelope handling.
lti/tsugi-util/src/java/org/tsugi/lti/objects/POXEnvelopeRequest.java (1)
1-18: LGTM! Clean POX envelope request model.The class correctly defines the root element with proper IMS namespace and maps header/body components appropriately.
lti/tsugi-util/src/test/java/org/tsugi/lti/POXJacksonTest.java (1)
1-339: Good test coverage for POXJacksonParser.The test suite comprehensively covers parsing, operation extraction, header/body info retrieval, and response creation scenarios including edge cases.
lti/tsugi-util/src/java/org/tsugi/pox/POXResponseBuilder.java (1)
1-275: Good fluent builder implementation for POX responses.The builder provides a clean API for constructing POX responses with proper defaults and convenience methods for common response types.
lti/tsugi-util/src/java/org/tsugi/lti/MessageResponseBuilder.java (1)
109-128: LGTM - Helper methods for Member and Group creation.The static factory methods
createMemberandcreateGroupprovide convenient ways to create objects with common field patterns. The redundantsetRoleandsetRoleson Member aligns with supporting both singular and plural role representations in LTI.lti/tsugi-util/src/java/org/tsugi/pox/POXResponseFactory.java (1)
42-54: LGTM - Factory methods with proper delegation.The factory methods correctly delegate to
POXResponseBuilderusing a fluent builder pattern. The overloaded methods provide convenient shortcuts while maintaining a consistent API.lti/tsugi-util/src/java/org/tsugi/pox/IMSPOXRequestJackson.java (1)
112-126: LGTM - Constructor overloads provide flexible initialization.The constructors appropriately handle different input sources (HttpServletRequest with/without credentials, raw body string) and follow a consistent pattern of delegating to helper methods.
lti/tsugi-util/src/java/org/tsugi/pox/POXConstants.java (1)
103-141: LGTM - Validation methods are well-implemented.The validation methods correctly delegate to a private helper
isValidValuethat handles null checks and performs efficient array iteration. This provides a clean, consistent API for validating POX values.lti/lti-blis/src/java/org/sakaiproject/lti/ServiceServlet.java (4)
941-951: LGTM - Helper method with appropriate error handling.The
buildPoxBodyXmlmethod provides clean null handling and catches serialization exceptions with appropriate logging. Returning an empty string on failure is acceptable here since the caller (processOutcomeXml) uses this as body content where empty is a valid fallback.
142-159: LGTM - Clean migration to builder-based error responses.The
doErrormethod now usesMessageResponseBuilder.error()to construct XML responses, replacing the previous Map-based approach. The HTML escaping of the message before passing to the builder is appropriate for security.
820-848: LGTM - Robust null-safe extraction of POX body fields.The extraction of
result_resultscore_textstring,result_resultdata_text, andsourced_idfrom the nested POX request body structure includes appropriate null checks at each level, preventing NPEs.
123-140: LGTM - Static XmlMapper configuration is thread-safe.The static
POX_XML_MAPPERis properly configured in a static initializer block.XmlMapperis thread-safe after configuration, making this pattern safe for concurrent servlet access.
| public static MessageResponseBuilder success() { | ||
| return new MessageResponseBuilder() | ||
| .withStatusInfo("Success", "Status", "fullsuccess", null); | ||
| } | ||
|
|
||
| public static MessageResponseBuilder error(String description) { | ||
| return new MessageResponseBuilder() | ||
| .withStatusInfo("Fail", "Error", null, description); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "POXConstants.java" -o -name "IMSPOXRequestJackson.java"Repository: sakaiproject/sakai
Length of output: 185
🏁 Script executed:
find . -name "MessageResponseBuilder.java" | head -1 | xargs -I {} cat -n {}Repository: sakaiproject/sakai
Length of output: 5068
🏁 Script executed:
rg -t java "fullsuccess|codeMajor" --max-count 20Repository: sakaiproject/sakai
Length of output: 2459
🏁 Script executed:
cat -n ./lti/tsugi-util/src/java/org/tsugi/pox/POXConstants.javaRepository: sakaiproject/sakai
Length of output: 7533
🏁 Script executed:
cat -n ./lti/tsugi-util/src/java/org/tsugi/pox/IMSPOXRequestJackson.java | head -150Repository: sakaiproject/sakai
Length of output: 7105
🏁 Script executed:
rg -t java "MessageResponseBuilder\.(success|error)" -A 3 -B 1Repository: sakaiproject/sakai
Length of output: 1481
🏁 Script executed:
rg -t java "isValidMajorCode|isValidSeverity" -A 2Repository: sakaiproject/sakai
Length of output: 2049
🏁 Script executed:
rg -t java -B 3 -A 3 "setCodeMajor|setSeverity" lti/tsugi-util/src/java/org/tsugi/lti/objects/StatusInfo.javaRepository: sakaiproject/sakai
Length of output: 44
🏁 Script executed:
rg -t java "MessageResponseBuilder\.(success|error)" -A 5 lti/tsugi-util/src/test/java/Repository: sakaiproject/sakai
Length of output: 44
🏁 Script executed:
find ./lti -name "*Test.java" -type f -exec rg -l "MessageResponseBuilder" {} \;Repository: sakaiproject/sakai
Length of output: 44
🏁 Script executed:
rg -t java "codeMajor.*=.*Success|codeMajor.*=.*Fail" -B 2 -A 2Repository: sakaiproject/sakai
Length of output: 44
🏁 Script executed:
cat -n ./lti/tsugi-util/src/java/org/tsugi/pox/POXResponseFactory.java | head -100Repository: sakaiproject/sakai
Length of output: 4529
Use POXConstants instead of hardcoded status values in factory methods.
The success() and error() methods hardcode capitalized status values that contradict the lowercase constants defined in POXConstants (MAJOR_SUCCESS = "success", MAJOR_FAILURE = "failure", SEVERITY_STATUS = "status", SEVERITY_ERROR = "error"). Since validation methods in POXConstants and POXResponseFactory use exact string matching, the capitalized values will fail validation checks and generate warnings in production.
Replace the hardcoded strings:
- Line 101:
success()should usePOXConstants.MAJOR_SUCCESSinstead of"Success"andPOXConstants.SEVERITY_STATUSinstead of"Status" - Line 106:
error()should usePOXConstants.MAJOR_FAILUREinstead of"Fail"andPOXConstants.SEVERITY_ERRORinstead of"Error"
🤖 Prompt for AI Agents
In lti/tsugi-util/src/java/org/tsugi/lti/MessageResponseBuilder.java around
lines 99 to 107, the factory methods use hardcoded, capitalized status and
severity strings which don't match the lowercase constants used by POXConstants
and will fail validation; update success() to call withStatusInfo using
POXConstants.MAJOR_SUCCESS and POXConstants.SEVERITY_STATUS, and update
error(String) to use POXConstants.MAJOR_FAILURE and POXConstants.SEVERITY_ERROR
(leave the description/null arguments as-is), so the builder uses the canonical
constants instead of literal strings.
| @JacksonXmlProperty(localName = "imsx_POXResponseHeaderInfo", namespace = "http://www.imsglobal.org/services/ltiv1p1/xsd/imsoms_v1p0") | ||
| private POXResponseHeaderInfo responseHeaderInfo; |
There was a problem hiding this comment.
Inconsistent prefix casing: imsX_ vs imsx_
The localName uses "imsX_POXResponseHeaderInfo" with a capital X, while the corresponding POXRequestHeader uses "imsx_POXRequestHeaderInfo" with lowercase x. The IMS specification typically uses the imsx_ prefix consistently. This mismatch could cause XML parsing issues.
🔎 Proposed fix
- @JacksonXmlProperty(localName = "imsX_POXResponseHeaderInfo", namespace = "http://www.imsglobal.org/services/ltiv1p1/xsd/imsoms_v1p0")
+ @JacksonXmlProperty(localName = "imsx_POXResponseHeaderInfo", namespace = "http://www.imsglobal.org/services/ltiv1p1/xsd/imsoms_v1p0")📝 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.
| @JacksonXmlProperty(localName = "imsx_POXResponseHeaderInfo", namespace = "http://www.imsglobal.org/services/ltiv1p1/xsd/imsoms_v1p0") | |
| private POXResponseHeaderInfo responseHeaderInfo; | |
| @JacksonXmlProperty(localName = "imsx_POXResponseHeaderInfo", namespace = "http://www.imsglobal.org/services/ltiv1p1/xsd/imsoms_v1p0") | |
| private POXResponseHeaderInfo responseHeaderInfo; |
🤖 Prompt for AI Agents
In lti/tsugi-util/src/java/org/tsugi/lti/objects/POXResponseHeader.java around
lines 16-17, the @JacksonXmlProperty localName uses "imsX_POXResponseHeaderInfo"
(capital X) which is inconsistent with the IMS spec and the POXRequestHeader
class that uses "imsx_POXRequestHeaderInfo"; change the localName to
"imsx_POXResponseHeaderInfo" (lowercase x) to match the spec and other classes,
and run any XML-binding/unit tests or regenerate sample XML to verify parsing
interoperability.
| return String.format( | ||
| "<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n" + | ||
| "<imsx_POXEnvelopeResponse xmlns=\"http://www.imsglobal.org/services/ltiv1p1/xsd/imsoms_v1p0\">\n" + | ||
| " <imsx_POXHeader>\n" + | ||
| " <imsx_POXResponseHeaderInfo>\n" + | ||
| " <imsx_version>V1.0</imsx_version>\n" + | ||
| " <imsx_messageIdentifier>%s</imsx_messageIdentifier>\n" + | ||
| " <imsx_statusInfo>\n" + | ||
| " <imsx_codeMajor>failure</imsx_codeMajor>\n" + | ||
| " <imsx_severity>error</imsx_severity>\n" + | ||
| " <imsx_description>%s</imsx_description>\n" + | ||
| " <imsx_operationRefIdentifier>%s</imsx_operationRefIdentifier>" + | ||
| " </imsx_statusInfo>\n" + | ||
| " </imsx_POXResponseHeaderInfo>\n" + | ||
| " </imsx_POXHeader>\n" + | ||
| " <imsx_POXBody/>\n" + | ||
| "</imsx_POXEnvelopeResponse>", | ||
| StringEscapeUtils.escapeXml11(messageIdValue), | ||
| StringEscapeUtils.escapeXml11(description), | ||
| StringEscapeUtils.escapeXml11(messageId) | ||
| ); | ||
| } |
There was a problem hiding this comment.
Minor XML formatting inconsistency.
Line 289 is missing a newline before </imsx_statusInfo>, unlike the formatting in createResponse. This may cause the imsx_operationRefIdentifier closing tag to run into the </imsx_statusInfo> tag.
🔎 Proposed fix
- " <imsx_operationRefIdentifier>%s</imsx_operationRefIdentifier>" +
+ " <imsx_operationRefIdentifier>%s</imsx_operationRefIdentifier>\n" +
" </imsx_statusInfo>\n" +🤖 Prompt for AI Agents
In lti/tsugi-util/src/java/org/tsugi/lti/POXJacksonParser.java around lines
278-299, the string built for the failure response concatenates the
imsx_operationRefIdentifier closing tag directly into the following
</imsx_statusInfo> without a newline; update the format string to insert a
newline (or include "\n" at the end of the imsx_operationRefIdentifier line) so
the closing tags are on separate lines, mirroring the formatting used in
createResponse and preserving the existing StringEscapeUtils.escapeXml11 usage
for the inserted values.
| StringEscapeUtils.escapeXml11(messageId), | ||
| StringEscapeUtils.escapeXml11(major), | ||
| StringEscapeUtils.escapeXml11(severity), | ||
| StringEscapeUtils.escapeXml11(description), | ||
| StringEscapeUtils.escapeXml11(messageId), | ||
| StringEscapeUtils.escapeXml11(operation), | ||
| StringEscapeUtils.escapeXml11(minorString.toString()), | ||
| bodyString, newLine | ||
| ); |
There was a problem hiding this comment.
Double XML escaping corrupts the minor codes section.
minorString already contains properly constructed XML markup (built at lines 370-386). Passing it through StringEscapeUtils.escapeXml11() at line 425 will escape the angle brackets, producing malformed XML like <imsx_codeMinor> instead of <imsx_codeMinor>.
🔎 Proposed fix
return String.format(
"<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n" +
// ... template ...
StringEscapeUtils.escapeXml11(messageId),
StringEscapeUtils.escapeXml11(major),
StringEscapeUtils.escapeXml11(severity),
StringEscapeUtils.escapeXml11(description),
StringEscapeUtils.escapeXml11(messageId),
StringEscapeUtils.escapeXml11(operation),
- StringEscapeUtils.escapeXml11(minorString.toString()),
+ minorString.toString(),
bodyString, newLine
);📝 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.
| StringEscapeUtils.escapeXml11(messageId), | |
| StringEscapeUtils.escapeXml11(major), | |
| StringEscapeUtils.escapeXml11(severity), | |
| StringEscapeUtils.escapeXml11(description), | |
| StringEscapeUtils.escapeXml11(messageId), | |
| StringEscapeUtils.escapeXml11(operation), | |
| StringEscapeUtils.escapeXml11(minorString.toString()), | |
| bodyString, newLine | |
| ); | |
| StringEscapeUtils.escapeXml11(messageId), | |
| StringEscapeUtils.escapeXml11(major), | |
| StringEscapeUtils.escapeXml11(severity), | |
| StringEscapeUtils.escapeXml11(description), | |
| StringEscapeUtils.escapeXml11(messageId), | |
| StringEscapeUtils.escapeXml11(operation), | |
| minorString.toString(), | |
| bodyString, newLine | |
| ); |
🤖 Prompt for AI Agents
In lti/tsugi-util/src/java/org/tsugi/lti/POXJacksonParser.java around lines
419-427, the code double-escapes the already-constructed XML in minorString by
passing it to StringEscapeUtils.escapeXml11(), which converts tags like
<imsx_codeMinor> to <imsx_codeMinor> and corrupts the XML; fix this by
removing the escape call for minorString (use the constructed minorString as-is
when formatting the output) while leaving the escaping for the other plain text
fields intact so only pre-escaped XML is preserved.
| StringEscapeUtils.escapeXml11(description), | ||
| StringEscapeUtils.escapeXml11(messageId), | ||
| StringEscapeUtils.escapeXml11(operation), | ||
| StringEscapeUtils.escapeXml11(minorString.toString()), |
There was a problem hiding this comment.
Double-escaping bug: minorString content will be corrupted.
The minorString StringBuilder already contains XML-escaped content (from lines 185-188 where StringEscapeUtils.escapeXml11() is applied to field names and values). Passing it through StringEscapeUtils.escapeXml11() again at line 233 will double-escape the content, corrupting the XML output (e.g., < becomes &lt;).
🔎 Proposed fix
StringEscapeUtils.escapeXml11(messageId),
StringEscapeUtils.escapeXml11(operation),
- StringEscapeUtils.escapeXml11(minorString.toString()),
+ minorString.toString(),
bodyStr, newLine
);📝 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.
| StringEscapeUtils.escapeXml11(minorString.toString()), | |
| minorString.toString(), |
🤖 Prompt for AI Agents
In lti/tsugi-util/src/java/org/tsugi/pox/POXResponseBuilder.java around line
233, the code calls StringEscapeUtils.escapeXml11(minorString.toString()) which
double-escapes content already escaped when building minorString; remove the
second escape and pass minorString.toString() directly (or otherwise ensure you
only escape individual field names/values once when appended to the builder) so
the XML entities are not double-encoded.
| public static String createFatalResponse(String description, String messageId) { | ||
| if (messageId == null) { | ||
| messageId = String.valueOf(new Date().getTime()); | ||
| } | ||
|
|
||
| return String.format( | ||
| "<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n" + | ||
| "<imsx_POXEnvelopeResponse xmlns=\"http://www.imsglobal.org/services/ltiv1p1/xsd/imsoms_v1p0\">\n" + | ||
| " <imsx_POXHeader>\n" + | ||
| " <imsx_POXResponseHeaderInfo>\n" + | ||
| " <imsx_version>V1.0</imsx_version>\n" + | ||
| " <imsx_messageIdentifier>%s</imsx_messageIdentifier>\n" + | ||
| " <imsx_statusInfo>\n" + | ||
| " <imsx_codeMajor>failure</imsx_codeMajor>\n" + | ||
| " <imsx_severity>error</imsx_severity>\n" + | ||
| " <imsx_description>%s</imsx_description>\n" + | ||
| " <imsx_operationRefIdentifier>%s</imsx_operationRefIdentifier>" + | ||
| " </imsx_statusInfo>\n" + | ||
| " </imsx_POXResponseHeaderInfo>\n" + | ||
| " </imsx_POXHeader>\n" + | ||
| " <imsx_POXBody/>\n" + | ||
| "</imsx_POXEnvelopeResponse>", | ||
| messageId, | ||
| description, | ||
| messageId | ||
| ); | ||
| } |
There was a problem hiding this comment.
Potential XML injection vulnerability in createFatalResponse.
The description and messageId parameters are directly interpolated into the XML string via String.format without XML escaping. If description contains characters like <, >, &, or ", the resulting XML will be malformed or could allow injection.
Other response methods in IMSPOXRequestJackson (e.g., createFallbackResponse at lines 399-456) use StringEscapeUtils.escapeXml11() for all interpolated values.
🔎 Proposed fix to escape XML special characters
+import org.apache.commons.text.StringEscapeUtils;
+
public static String createFatalResponse(String description, String messageId) {
if (messageId == null) {
messageId = String.valueOf(new Date().getTime());
}
return String.format(
"<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n" +
"<imsx_POXEnvelopeResponse xmlns=\"http://www.imsglobal.org/services/ltiv1p1/xsd/imsoms_v1p0\">\n" +
" <imsx_POXHeader>\n" +
" <imsx_POXResponseHeaderInfo>\n" +
" <imsx_version>V1.0</imsx_version>\n" +
" <imsx_messageIdentifier>%s</imsx_messageIdentifier>\n" +
" <imsx_statusInfo>\n" +
" <imsx_codeMajor>failure</imsx_codeMajor>\n" +
" <imsx_severity>error</imsx_severity>\n" +
" <imsx_description>%s</imsx_description>\n" +
" <imsx_operationRefIdentifier>%s</imsx_operationRefIdentifier>" +
" </imsx_statusInfo>\n" +
" </imsx_POXResponseHeaderInfo>\n" +
" </imsx_POXHeader>\n" +
" <imsx_POXBody/>\n" +
"</imsx_POXEnvelopeResponse>",
- messageId,
- description,
- messageId
+ StringEscapeUtils.escapeXml11(messageId),
+ StringEscapeUtils.escapeXml11(description),
+ StringEscapeUtils.escapeXml11(messageId)
);
}🤖 Prompt for AI Agents
In lti/tsugi-util/src/java/org/tsugi/pox/POXResponseFactory.java around lines
132 to 158, the createFatalResponse method interpolates description and
messageId directly into XML causing possible XML injection/malformed XML; fix by
XML-escaping both values (e.g., call StringEscapeUtils.escapeXml11(...) on
description and messageId or on a safe default messageId) before passing them to
String.format, and ensure nulls are handled (generate/escape a timestamp-based
id when messageId is null and treat a null description as an empty escaped
string).
1 participant
Created convenience methods in POXJacksonaParser.java, added hierarchical dot notation navigation.
Summary by CodeRabbit
Release Notes
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.