SAK-51434 LTI Remove the XMLMap Library from LTI #14262
Conversation
WalkthroughThis pull request migrates the LTI module from POX XML handling using legacy utilities to a Jackson-based XML serialization framework. It replaces custom map-based XML construction with structured builder patterns, removes obsolete XML/JSON parsing utilities, and introduces comprehensive Jackson-annotated data model classes for POX envelope structures, LTI responses, members, results, and error handling. Changes
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lti/lti-blis/src/java/org/sakaiproject/lti/ServiceServlet.java (1)
588-613: POX XML outcome flow: sourcedId path inconsistency and serialization behaviorTwo points worth tightening up in the POX XML outcome flow:
sourcedIdpath inconsistency (likely typo/pre‑existing bug)
IndoPostXmlyou extract the mainsourcedidfrombodyMap.get("/resultRecord/sourcedGUID/sourcedId"), but inprocessOutcomeXmlyou build the response’sResult.sourcedIdfrombodyMap.get("/resultRecord/result/sourcedId"). This path mismatch means the response’ssourcedIdwill typically benulleven when the request carried a valid sourced GUID.Consider aligning
sourced_idwith the same key you use earlier:
String sourced_id = bodyMap.get("/resultRecord/result/sourcedId");
String sourced_id = bodyMap.get("/resultRecord/sourcedGUID/sourcedId");so the response echoes the same identifier.
- Serialization fallback on error
buildPoxBodyXmlreturns an empty string when serialization fails, andpox.getResponseSuccess(message, bodyXml)will still build a success response. If you want tools to distinguish serialization failures from normal “empty body” outcomes, you might want to log aterrorand/or calldoErrorXMLinstead whenbodyXmlends up empty unexpectedly.Overall, the move to POJO payloads plus Jackson serialization looks solid; these tweaks would just tighten correctness and observability.
Also applies to: 644-665, 799-901
🧹 Nitpick comments (18)
lti/tsugi-util/src/java/org/tsugi/lti/objects/TimeFrame.java (1)
7-21: TimeFrame POJO looks consistent; Lombok@ToStringis redundantThe field set and XML names look coherent for a simple envelope object; nothing blocking here. Minor nit:
@Dataalready includes@ToString, so keeping both is redundant. You can drop the explicit@ToStringhere (and in similar classes) if you prefer to keep Lombok usage minimal and consistent.lti/tsugi-util/src/java/org/tsugi/lti/objects/AdminPeriod.java (1)
7-15: AdminPeriod is fine; consider dropping redundant@ToStringThis is a straightforward value object and fits cleanly with the surrounding types. As with
TimeFrame,@Dataalready generatestoString(), so the separate@ToStringannotation is redundant and can be removed for simplicity if desired.lti/tsugi-util/src/java/org/tsugi/lti/objects/Role.java (1)
7-27: Role structure and XML mapping look good;@ToStringis optionalThe Role model ties together the expected fields (including
TimeFrame) and the XML element names are consistent. Only minor nit:@Dataalready includes@ToString, so you can safely remove the explicit@ToStringannotation if you want to reduce Lombok noise.lti/tsugi-util/src/java/org/tsugi/lti/objects/StatusInfo.java (1)
7-21: StatusInfo XML mapping is correct; consider enums as an optional refinementThe four fields are correctly mapped to the expected XML element names and integrate cleanly with
MessageResponse. As an optional future improvement,codeMajorandseveritycould be modeled as enums to restrict values to the spec-defined set, but the current String-based approach is perfectly acceptable and keeps the model simple.lti/tsugi-util/src/java/org/tsugi/lti/objects/Membership.java (1)
1-27: Membership mapping is coherent; considercreditHourstypeThe field and XML element names are consistent with the surrounding POX DTOs, and the
memberscollection mapping (no wrapper,memberelements) matches the pattern used elsewhere. One design consideration: if upstream systems ever send fractional credit hours, you may wantBigDecimalorDoubleinstead ofIntegerforcreditHours; otherwiseIntegeris fine.lti/tsugi-util/src/java/org/tsugi/lti/objects/POXResponseBody.java (1)
13-30: ConfirmrawContentserialization matches legacy POX body expectationsThe typed response mappings (replace/read/delete result and membership) look good and mirror
POXRequestBody. One potential subtle point israwContent: with@JacksonXmlProperty(localName = "rawContent")it will serialize as a<rawContent>...</rawContent>child element, and Jackson will escape any XML markup in theString. If callers (for example,IMSPOXRequestJacksonTest.testGetResponseSuccess) expect a fragment like<test>body</test>to appear directly under<imsx_POXBody>rather than wrapped/escaped, please double‑check howbodyString/rawContentare being used inPOXJacksonParser/ the response factory and adjust the mapping or builder logic if necessary.lti/tsugi-util/src/java/org/tsugi/lti/objects/POXRequestBody.java (1)
9-30: Unify NON_NULL handling at class level and includerawContentYou can simplify and make behavior more consistent by moving
@JsonInclude(Include.NON_NULL)to the class and dropping the per‑field annotations, which will also coverrawContentso you do not emit empty<rawContent/>elements.-@Data -@ToString -public class POXRequestBody { +@Data +@ToString +@JsonInclude(JsonInclude.Include.NON_NULL) +public class POXRequestBody { @@ - @JacksonXmlProperty(localName = "replaceResultRequest") - @JsonInclude(JsonInclude.Include.NON_NULL) - private ReplaceResultRequest replaceResultRequest; + @JacksonXmlProperty(localName = "replaceResultRequest") + private ReplaceResultRequest replaceResultRequest; @@ - @JacksonXmlProperty(localName = "readResultRequest") - @JsonInclude(JsonInclude.Include.NON_NULL) - private ReadResultRequest readResultRequest; + @JacksonXmlProperty(localName = "readResultRequest") + private ReadResultRequest readResultRequest; @@ - @JacksonXmlProperty(localName = "deleteResultRequest") - @JsonInclude(JsonInclude.Include.NON_NULL) - private DeleteResultRequest deleteResultRequest; + @JacksonXmlProperty(localName = "deleteResultRequest") + private DeleteResultRequest deleteResultRequest; @@ - @JacksonXmlProperty(localName = "readMembershipRequest") - @JsonInclude(JsonInclude.Include.NON_NULL) - private ReadMembershipRequest readMembershipRequest; + @JacksonXmlProperty(localName = "readMembershipRequest") + private ReadMembershipRequest readMembershipRequest; @@ - @JacksonXmlProperty(localName = "rawContent") - private String rawContent; + @JacksonXmlProperty(localName = "rawContent") + private String rawContent;lti/tsugi-util/src/java/org/tsugi/lti/MessageResponseBuilder.java (2)
34-46: Avoid magic strings for StatusInfo; prefer shared constants or enums
withStatusInfoplus thesuccess()anderror()helpers currently rely on hard‑coded strings like"Success","Fail","Status","Error","fullsuccess". If these values are constrained by a spec or reused elsewhere, consider centralizing them as constants (or an enum) to prevent drift and typos across the codebase.public class MessageResponseBuilder { - + + // Example: centralize commonly used status values + public static final String CODE_MAJOR_SUCCESS = "Success"; + public static final String CODE_MAJOR_FAIL = "Fail"; + public static final String SEVERITY_STATUS = "Status"; + public static final String SEVERITY_ERROR = "Error"; + public static final String CODE_MINOR_FULLSUCCESS = "fullsuccess"; @@ - public static MessageResponseBuilder success() { - return new MessageResponseBuilder() - .withStatusInfo("Success", "Status", "fullsuccess", null); - } + public static MessageResponseBuilder success() { + return new MessageResponseBuilder() + .withStatusInfo(CODE_MAJOR_SUCCESS, SEVERITY_STATUS, CODE_MINOR_FULLSUCCESS, null); + } @@ - public static MessageResponseBuilder error(String description) { - return new MessageResponseBuilder() - .withStatusInfo("Fail", "Error", null, description); - } + public static MessageResponseBuilder error(String description) { + return new MessageResponseBuilder() + .withStatusInfo(CODE_MAJOR_FAIL, SEVERITY_ERROR, null, description); + }Also applies to: 99-107
57-73: Skip creating an empty<result>when both score and data are null
withResultcurrently creates aResultand attaches it even when bothresultScoreandresultDataare null, which can yield an empty<result/>element in the XML. If that is not required by the consumers, consider early‑returning in that case.- public MessageResponseBuilder withResult(String resultScore, String resultData) { - Result result = new Result(); - - if (resultScore != null) { - ResultScore score = new ResultScore(); - score.setTextString(resultScore); - result.setResultScore(score); - } - - if (resultData != null) { - ResultData data = new ResultData(); - data.setText(resultData); - result.setResultData(data); - } - - response.setResult(result); - return this; - } + public MessageResponseBuilder withResult(String resultScore, String resultData) { + if (resultScore == null && resultData == null) { + return this; + } + + Result result = new Result(); + + if (resultScore != null) { + ResultScore score = new ResultScore(); + score.setTextString(resultScore); + result.setResultScore(score); + } + + if (resultData != null) { + ResultData data = new ResultData(); + data.setText(resultData); + result.setResultData(data); + } + + response.setResult(result); + return this; + }lti/tsugi-util/src/test/java/org/tsugi/pox/POXResponseBuilderTest.java (1)
16-223: Tests are comprehensive; consider future structure-aware assertionsThe test suite covers the main builder paths (majors, severities, minor codes, null/empty body, XML header trimming) and should guard against most regressions. If you ever need stricter guarantees, you might consider parsing the XML and asserting on specific elements/attributes instead of
contains(...), but that’s optional given the current scope.lti/tsugi-util/src/java/org/tsugi/pox/POXResponseBuilder.java (1)
26-32: Static XmlMapper is currently unused
xmlMapperis configured but never used in this class; all XML output is generated bybuildManualXml. You may want to either:
- Remove the mapper for now, or
- Wire
buildAsXml()to actually serialize thePOXEnvelopeResponsevia Jackson instead of manual string building.This is non-blocking but would reduce confusion for future maintainers.
lti/tsugi-util/src/java/org/tsugi/pox/POXConstants.java (1)
3-141: Centralize usage of POX constants to avoid driftThis class is a good single source of truth for POX codes and validation. However,
IMSPOXRequestJackson(andPOXResponseFactoryvia its private validators) still define and use their ownvalidMajor/validSeverity/validMinorarrays and related constants.To prevent future divergence, consider refactoring those classes to delegate to
POXConstants(e.g.,POXConstants.VALID_MAJOR,POXConstants.isValidMajorCode(...)) instead of maintaining parallel definitions.lti/tsugi-util/src/java/org/tsugi/pox/POXResponseFactory.java (1)
237-286: Leverage POXConstants for validation instead of IMSPOXRequestJackson.validXXX arrays
validateResponseParametersand the privateisValidMajorCode/isValidSeverity/isValidMinorCodehelpers currently iterate overIMSPOXRequestJackson.validMajor,validSeverity, andvalidMinor. SincePOXConstantsprovidesVALID_MAJOR,VALID_SEVERITY,VALID_MINOR, and correspondingisValid*helpers, you can simplify and avoid duplicating allowed values:- private static boolean isValidMajorCode(String major) { - for (String validMajor : IMSPOXRequestJackson.validMajor) { - if (validMajor.equals(major)) { - return true; - } - } - return false; - } + private static boolean isValidMajorCode(String major) { + return POXConstants.isValidMajorCode(major); + } @@ - private static boolean isValidSeverity(String severity) { - for (String validSeverity : IMSPOXRequestJackson.validSeverity) { - if (validSeverity.equals(severity)) { - return true; - } - } - return false; - } + private static boolean isValidSeverity(String severity) { + return POXConstants.isValidSeverity(severity); + } @@ - private static boolean isValidMinorCode(String minor) { - for (String validMinor : IMSPOXRequestJackson.validMinor) { - if (validMinor.equals(minor)) { - return true; - } - } - return false; - } + private static boolean isValidMinorCode(String minor) { + return POXConstants.isValidMinorCode(minor); + }This keeps all code lists and validation rules in one place.
lti/tsugi-util/src/java/org/tsugi/pox/IMSPOXRequestJackson.java (1)
195-205: SHA‑1 usage is per OAuth 1.0a but worth documentingStatic analysis flags
MessageDigest.getInstance("SHA-1")as a weak hash. In this method, the digest is used only to computeoauth_body_hashforHMAC-SHA1requests and is paired with an HMAC-based OAuth 1.0a signature:MessageDigest md = MessageDigest.getInstance("SHA-1"); if ("HMAC-SHA256".equals(oauth_signature_method)) { md = MessageDigest.getInstance("SHA-256"); }Given protocol compatibility, you likely must continue supporting SHA‑1 here for
oauth_signature_method = HMAC-SHA1. To make this clearer and help future reviewers/tools, consider:
- Adding a short comment noting that SHA‑1 is required by the OAuth 1.0a body-hash specification in this context.
- Encouraging new integrations to prefer
HMAC-SHA256where possible.No immediate code change is strictly necessary, but documenting the rationale will reduce recurring false positives.
lti/tsugi-util/src/java/org/tsugi/lti/POXJacksonParser.java (2)
121-138: PreferPOXConstants.OPERATION_*instead of hard‑coded operation strings
getOperationreturns string literals like"replaceResultRequest"whilegetOperationInfousesPOXConstants.OPERATION_*. For consistency and typo‑safety, both helpers should usePOXConstants.OPERATION_*everywhere:- if (body.getReplaceResultRequest() != null) { - return "replaceResultRequest"; - } else if (body.getReadResultRequest() != null) { - return "readResultRequest"; - } else if (body.getDeleteResultRequest() != null) { - return "deleteResultRequest"; - } else if (body.getReadMembershipRequest() != null) { - return "readMembershipRequest"; - } + if (body.getReplaceResultRequest() != null) { + return POXConstants.OPERATION_REPLACE_RESULT; + } else if (body.getReadResultRequest() != null) { + return POXConstants.OPERATION_READ_RESULT; + } else if (body.getDeleteResultRequest() != null) { + return POXConstants.OPERATION_DELETE_RESULT; + } else if (body.getReadMembershipRequest() != null) { + return POXConstants.OPERATION_READ_MEMBERSHIP; + }This keeps all operation identifiers centralized in
POXConstants.Also applies to: 473-514
61-86: Consider reusing a sharedXmlMapperinstead of constructing per‑callBoth
parseRequestandparseResponsecreate and configure a newXmlMapperon every invocation. For a servlet‑style environment this adds unnecessary object churn; using a single, statically configuredXmlMapper(similar toServiceServlet.POX_XML_MAPPER) would be cheaper and still thread‑safe once configuration is complete.If you align the configuration with the rest of the POX stack (e.g.,
FAIL_ON_UNKNOWN_PROPERTIES=false,setDefaultUseWrapper(false)), you can centralize it in a static field and reuse it across all parse methods.Also applies to: 94-113
lti/lti-blis/src/java/org/sakaiproject/lti/ServiceServlet.java (2)
119-126: SharedPOX_XML_MAPPER+buildPoxBodyXmlis a good, thread‑safe centralizationThe static, pre‑configured
XmlMappercombined withbuildPoxBodyXmlgives you a single place to control POX body serialization (no XML declaration, no wrappers, empty‑bean tolerant). This is a solid pattern for a servlet.You might later consider reusing this same mapper in debugging (the pretty‑print block in
doPostXml) to keep configuration fully consistent, but that is not required for correctness.Also applies to: 903-913
128-145: The verification confirms the original review comment's observation. I can now generate the rewritten response:
Set HTTP status code and content type in
doErrorto ensure error responses are properly signaled to LTI tool consumersThe
doErrormethod now centralizes error XML generation viaMessageResponseBuilder, but does not set HTTP status codes or content type. Currently, error responses are returned with the default HTTP 200 status, which may confuse LTI tool consumers expecting error status codes.The codebase shows inconsistency: early guard-clause errors (lines 185, 640) set
SC_FORBIDDENdirectly and return, while business logic errors calldoErrorwithout setting status. To maintain consistency and ensure clients properly recognize errors at the HTTP layer, add status code and content type todoError:response.setStatus(HttpServletResponse.SC_BAD_REQUEST); response.setContentType("application/xml"); response.setCharacterEncoding("UTF-8");Alternatively, if different error types warrant different status codes (e.g.,
SC_INTERNAL_SERVER_ERRORfor exception-based errors vs.SC_BAD_REQUESTfor validation), move status setting to call sites where the error context is known.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (51)
lti/lti-blis/pom.xml(1 hunks)lti/lti-blis/src/java/org/sakaiproject/lti/ServiceServlet.java(23 hunks)lti/tsugi-util/pom.xml(1 hunks)lti/tsugi-util/src/java/org/tsugi/json/IMSJSONRequest.java(0 hunks)lti/tsugi-util/src/java/org/tsugi/lti/MessageResponseBuilder.java(1 hunks)lti/tsugi-util/src/java/org/tsugi/lti/POXJacksonParser.java(1 hunks)lti/tsugi-util/src/java/org/tsugi/lti/XMLMap.java(0 hunks)lti/tsugi-util/src/java/org/tsugi/lti/objects/AdminPeriod.java(1 hunks)lti/tsugi-util/src/java/org/tsugi/lti/objects/DeleteResultRequest.java(1 hunks)lti/tsugi-util/src/java/org/tsugi/lti/objects/DeleteResultResponse.java(1 hunks)lti/tsugi-util/src/java/org/tsugi/lti/objects/Group.java(1 hunks)lti/tsugi-util/src/java/org/tsugi/lti/objects/GroupSet.java(1 hunks)lti/tsugi-util/src/java/org/tsugi/lti/objects/Groups.java(1 hunks)lti/tsugi-util/src/java/org/tsugi/lti/objects/Member.java(1 hunks)lti/tsugi-util/src/java/org/tsugi/lti/objects/Members.java(1 hunks)lti/tsugi-util/src/java/org/tsugi/lti/objects/Membership.java(1 hunks)lti/tsugi-util/src/java/org/tsugi/lti/objects/MembershipRecord.java(1 hunks)lti/tsugi-util/src/java/org/tsugi/lti/objects/MessageResponse.java(1 hunks)lti/tsugi-util/src/java/org/tsugi/lti/objects/POXCodeMinor.java(1 hunks)lti/tsugi-util/src/java/org/tsugi/lti/objects/POXCodeMinorField.java(1 hunks)lti/tsugi-util/src/java/org/tsugi/lti/objects/POXEnvelopeRequest.java(1 hunks)lti/tsugi-util/src/java/org/tsugi/lti/objects/POXEnvelopeResponse.java(1 hunks)lti/tsugi-util/src/java/org/tsugi/lti/objects/POXRequestBody.java(1 hunks)lti/tsugi-util/src/java/org/tsugi/lti/objects/POXRequestHeader.java(1 hunks)lti/tsugi-util/src/java/org/tsugi/lti/objects/POXRequestHeaderInfo.java(1 hunks)lti/tsugi-util/src/java/org/tsugi/lti/objects/POXResponseBody.java(1 hunks)lti/tsugi-util/src/java/org/tsugi/lti/objects/POXResponseHeader.java(1 hunks)lti/tsugi-util/src/java/org/tsugi/lti/objects/POXResponseHeaderInfo.java(1 hunks)lti/tsugi-util/src/java/org/tsugi/lti/objects/POXStatusInfo.java(1 hunks)lti/tsugi-util/src/java/org/tsugi/lti/objects/ReadMembershipRequest.java(1 hunks)lti/tsugi-util/src/java/org/tsugi/lti/objects/ReadMembershipResponse.java(1 hunks)lti/tsugi-util/src/java/org/tsugi/lti/objects/ReadResultRequest.java(1 hunks)lti/tsugi-util/src/java/org/tsugi/lti/objects/ReadResultResponse.java(1 hunks)lti/tsugi-util/src/java/org/tsugi/lti/objects/ReplaceResultRequest.java(1 hunks)lti/tsugi-util/src/java/org/tsugi/lti/objects/ReplaceResultResponse.java(1 hunks)lti/tsugi-util/src/java/org/tsugi/lti/objects/Result.java(1 hunks)lti/tsugi-util/src/java/org/tsugi/lti/objects/ResultData.java(1 hunks)lti/tsugi-util/src/java/org/tsugi/lti/objects/ResultRecord.java(1 hunks)lti/tsugi-util/src/java/org/tsugi/lti/objects/ResultScore.java(1 hunks)lti/tsugi-util/src/java/org/tsugi/lti/objects/Role.java(1 hunks)lti/tsugi-util/src/java/org/tsugi/lti/objects/SourcedGUID.java(1 hunks)lti/tsugi-util/src/java/org/tsugi/lti/objects/StatusInfo.java(1 hunks)lti/tsugi-util/src/java/org/tsugi/lti/objects/TimeFrame.java(1 hunks)lti/tsugi-util/src/java/org/tsugi/pox/IMSPOXRequest.java(0 hunks)lti/tsugi-util/src/java/org/tsugi/pox/IMSPOXRequestJackson.java(1 hunks)lti/tsugi-util/src/java/org/tsugi/pox/POXConstants.java(1 hunks)lti/tsugi-util/src/java/org/tsugi/pox/POXResponseBuilder.java(1 hunks)lti/tsugi-util/src/java/org/tsugi/pox/POXResponseFactory.java(1 hunks)lti/tsugi-util/src/test/java/org/tsugi/lti/POXJacksonTest.java(1 hunks)lti/tsugi-util/src/test/java/org/tsugi/pox/IMSPOXRequestJacksonTest.java(1 hunks)lti/tsugi-util/src/test/java/org/tsugi/pox/POXResponseBuilderTest.java(1 hunks)
💤 Files with no reviewable changes (3)
- lti/tsugi-util/src/java/org/tsugi/pox/IMSPOXRequest.java
- lti/tsugi-util/src/java/org/tsugi/json/IMSJSONRequest.java
- lti/tsugi-util/src/java/org/tsugi/lti/XMLMap.java
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{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
**/*.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/POXResponseBody.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/ReplaceResultResponse.javalti/tsugi-util/src/java/org/tsugi/lti/objects/ReplaceResultRequest.javalti/tsugi-util/src/java/org/tsugi/lti/objects/Group.javalti/tsugi-util/src/java/org/tsugi/lti/objects/Role.javalti/tsugi-util/src/java/org/tsugi/lti/objects/ReadMembershipResponse.javalti/tsugi-util/src/java/org/tsugi/lti/objects/Groups.javalti/tsugi-util/src/java/org/tsugi/lti/objects/AdminPeriod.javalti/tsugi-util/src/test/java/org/tsugi/lti/POXJacksonTest.javalti/tsugi-util/src/java/org/tsugi/lti/objects/ResultScore.javalti/tsugi-util/src/java/org/tsugi/lti/objects/DeleteResultRequest.javalti/tsugi-util/src/java/org/tsugi/lti/objects/POXCodeMinorField.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/ReadMembershipRequest.javalti/tsugi-util/src/java/org/tsugi/lti/objects/POXEnvelopeRequest.javalti/tsugi-util/src/java/org/tsugi/lti/objects/Members.javalti/tsugi-util/src/java/org/tsugi/lti/objects/TimeFrame.javalti/tsugi-util/src/java/org/tsugi/lti/objects/POXRequestBody.javalti/tsugi-util/src/java/org/tsugi/lti/MessageResponseBuilder.javalti/tsugi-util/src/java/org/tsugi/lti/objects/SourcedGUID.javalti/tsugi-util/src/java/org/tsugi/lti/objects/DeleteResultResponse.javalti/tsugi-util/src/java/org/tsugi/lti/objects/POXCodeMinor.javalti/tsugi-util/src/java/org/tsugi/lti/objects/ResultData.javalti/tsugi-util/src/java/org/tsugi/pox/POXResponseBuilder.javalti/tsugi-util/src/java/org/tsugi/lti/objects/Member.javalti/tsugi-util/src/java/org/tsugi/lti/objects/Result.javalti/tsugi-util/src/java/org/tsugi/lti/objects/MembershipRecord.javalti/tsugi-util/src/java/org/tsugi/lti/objects/Membership.javalti/tsugi-util/src/java/org/tsugi/lti/objects/StatusInfo.javalti/tsugi-util/src/java/org/tsugi/lti/objects/ReadResultResponse.javalti/tsugi-util/src/java/org/tsugi/lti/objects/POXRequestHeaderInfo.javalti/tsugi-util/src/java/org/tsugi/lti/objects/POXEnvelopeResponse.javalti/tsugi-util/src/test/java/org/tsugi/pox/POXResponseBuilderTest.javalti/tsugi-util/src/java/org/tsugi/lti/objects/POXRequestHeader.javalti/tsugi-util/src/java/org/tsugi/lti/objects/POXStatusInfo.javalti/tsugi-util/src/java/org/tsugi/pox/POXConstants.javalti/tsugi-util/src/java/org/tsugi/pox/POXResponseFactory.javalti/tsugi-util/src/java/org/tsugi/lti/objects/ResultRecord.javalti/tsugi-util/src/test/java/org/tsugi/pox/IMSPOXRequestJacksonTest.javalti/tsugi-util/src/java/org/tsugi/lti/objects/GroupSet.javalti/tsugi-util/src/java/org/tsugi/lti/POXJacksonParser.javalti/tsugi-util/src/java/org/tsugi/pox/IMSPOXRequestJackson.javalti/lti-blis/src/java/org/sakaiproject/lti/ServiceServlet.java
🧠 Learnings (1)
📚 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 (26)
lti/tsugi-util/src/java/org/tsugi/lti/objects/POXResponseBody.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/POXRequestBody.java (1)
Data(9-31)
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 (1)
lti/tsugi-util/src/java/org/tsugi/lti/objects/Members.java (1)
Data(9-16)
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/ReadMembershipResponse.java (3)
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/MembershipRecord.java (1)
Data(7-16)
lti/tsugi-util/src/java/org/tsugi/lti/objects/Groups.java (4)
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/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/POXResponseHeader.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/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/POXResponseHeaderInfo.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/POXEnvelopeRequest.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/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/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/POXRequestBody.java (1)
lti/tsugi-util/src/java/org/tsugi/lti/objects/DeleteResultRequest.java (1)
Data(7-13)
lti/tsugi-util/src/java/org/tsugi/lti/objects/POXCodeMinor.java (2)
lti/tsugi-util/src/java/org/tsugi/lti/objects/POXCodeMinorField.java (1)
Data(7-16)lti/tsugi-util/src/java/org/tsugi/lti/objects/POXRequestHeaderInfo.java (1)
Data(7-16)
lti/tsugi-util/src/java/org/tsugi/lti/objects/ResultData.java (1)
lti/tsugi-util/src/java/org/tsugi/lti/objects/MessageResponse.java (1)
Data(8-24)
lti/tsugi-util/src/java/org/tsugi/pox/POXResponseBuilder.java (1)
lti/tsugi-util/src/java/org/tsugi/pox/IMSPOXRequestJackson.java (1)
Slf4j(44-552)
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/Membership.java (4)
lti/tsugi-util/src/java/org/tsugi/lti/objects/Group.java (1)
Data(7-19)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/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/ReadResultResponse.java (2)
lti/tsugi-util/src/java/org/tsugi/lti/objects/DeleteResultResponse.java (1)
Data(12-17)lti/tsugi-util/src/java/org/tsugi/lti/objects/MessageResponse.java (1)
Data(8-24)
lti/tsugi-util/src/java/org/tsugi/lti/objects/POXRequestHeaderInfo.java (1)
lti/tsugi-util/src/java/org/tsugi/lti/objects/POXRequestHeader.java (1)
Data(7-13)
lti/tsugi-util/src/java/org/tsugi/lti/objects/POXEnvelopeResponse.java (2)
lti/tsugi-util/src/java/org/tsugi/lti/objects/POXEnvelopeRequest.java (1)
JacksonXmlRootElement(8-18)lti/tsugi-util/src/java/org/tsugi/lti/objects/POXRequestHeader.java (1)
Data(7-13)
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/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/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/test/java/org/tsugi/pox/IMSPOXRequestJacksonTest.java (1)
lti/tsugi-util/src/java/org/tsugi/lti/POXJacksonParser.java (1)
POXJacksonParser(21-576)
lti/tsugi-util/src/java/org/tsugi/lti/objects/GroupSet.java (1)
lti/tsugi-util/src/java/org/tsugi/lti/objects/Group.java (1)
Data(7-19)
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)
🪛 ast-grep (0.40.0)
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: sakai-deploy
- GitHub Check: maven-build
🔇 Additional comments (36)
lti/tsugi-util/src/java/org/tsugi/lti/objects/GroupSet.java (1)
7-15: GroupSet DTO mapping looks correct and consistent with GroupThe
GroupSetclass is a clean Lombok DTO withidandtitlecorrectly exposed via@JacksonXmlProperty, matching the wayGroupreferences it; no issues spotted.lti/tsugi-util/src/java/org/tsugi/lti/objects/ReadResultResponse.java (1)
9-15: ReadResultResponse XML model is well-structuredThe root element name and single
resultpayload field match the expected read-result response shape and are consistent with the surrounding Result/MessageResponse models.lti/tsugi-util/src/java/org/tsugi/lti/objects/ResultScore.java (1)
7-15: ResultScore DTO matches expected XML element structureThe
languageandtextStringfields are correctly exposed as XML elements and the class is a straightforward Lombok DTO; this aligns well with typical LIS/LTI resultScore structures.lti/tsugi-util/pom.xml (1)
64-67: Version is centrally managed and properly aligned—no issues foundVerification confirms the dependency follows the correct pattern: all Jackson modules in
lti/tsugi-util/pom.xml(including the newjackson-dataformat-xml) are versionless declarations that inherit from the parent BOM's central version definition (${sakai.jackson.version}inmaster/pom.xml). This approach is consistent across the codebase's 17 pom.xml files using this dependency. No version conflicts or misalignments exist.lti/lti-blis/pom.xml (1)
65-68: Verified: Jackson version alignment is consistent across modulesThe addition of
jackson-dataformat-xmlto lti/lti-blis follows the established codebase pattern. All 17 modules declaring this dependency (including the newly modified lti/tsugi-util) omit explicit versions and inherit from master/pom.xml's centralized version management (sakai.jackson.version=2.19.2). Version consistency is maintained through the parent pom chain: lti-blis → lti → master. No alignment issues detected.lti/tsugi-util/src/java/org/tsugi/lti/objects/ReadMembershipRequest.java (1)
1-12: ReadMembershipRequest DTO looks consistent and minimalSingle
sourcedIdfield with XML mapping is clear and aligns with the other request holder classes in this PR. No changes needed.lti/tsugi-util/src/java/org/tsugi/lti/objects/ResultData.java (1)
1-12: ResultData is a straightforward text holderSimple
textfield with matching XML element name; this is clear and adequate for embedding result payload text. No changes needed.lti/tsugi-util/src/java/org/tsugi/lti/objects/DeleteResultResponse.java (1)
1-17: DeleteResultResponse root wrapper looks appropriateUsing
deleteResultResponseas the XML root with an empty body matches the typical pattern for delete POX operations; documentation is clear. No further changes needed.lti/tsugi-util/src/java/org/tsugi/lti/objects/SourcedGUID.java (1)
1-12: SourcedGUID DTO matches expected structureThe class cleanly models a
sourcedIdcontainer for use inside result records; naming and XML mapping look consistent. No issues.lti/tsugi-util/src/java/org/tsugi/lti/objects/Group.java (1)
1-18: Group structure is clear and aligns with related classes
id,title, andset(viaGroupSet) give a straightforward representation of a group; XML element names and types are consistent with the surrounding grouping model. No changes suggested.lti/tsugi-util/src/java/org/tsugi/lti/objects/ReadResultRequest.java (1)
1-12: ReadResultRequest matches the other result request DTOsUsing a single
resultRecordfield here keeps the request shape consistent withDeleteResultRequestand the POX body mappings shown in the snippets. Looks good as-is.lti/tsugi-util/src/java/org/tsugi/lti/objects/ReadMembershipResponse.java (1)
1-12: ReadMembershipResponse composition looks correctWrapping a single
membershipRecordhere cleanly exposes theMembershipRecordtree for POX responses. Naming and mapping are consistent with the other response DTOs.lti/tsugi-util/src/java/org/tsugi/lti/objects/Groups.java (1)
1-16: Groups POJO and XML mapping look consistent with surrounding modelThe Jackson + Lombok setup is straightforward and matches the existing
Members/Memberpattern; theuseWrapping = falseconfiguration is appropriate for an unwrapped<group>list inside<groups>. No changes needed.lti/tsugi-util/src/java/org/tsugi/lti/objects/Members.java (1)
1-16: Members container matches expected unwrapped list patternThe
Membersclass mirrors theGroupspattern with an unwrapped list of<member>elements, which is appropriate for the POX-style XML structure. Looks good as-is.lti/tsugi-util/src/java/org/tsugi/lti/objects/DeleteResultRequest.java (1)
1-13: DeleteResultRequest is a clean, minimal wrapper forresultRecordThe class is a simple Lombok/Jackson POJO with
@JacksonXmlProperty(localName = "resultRecord"), which fits the POX request body pattern. No issues from this snippet.lti/tsugi-util/src/java/org/tsugi/lti/objects/Result.java (1)
1-20: Double‑check XML element names against the existing LTI/POX schemaThe
ResultPOJO wiring (sourcedId, resultScore, resultData) is structurally sound and integrates cleanly withMessageResponse. Since theselocalNamevalues are part of the external XML contract, please confirm they exactly match the prior XMLMap-based payloads / IMS schema (especiallysourcedIdcasing) before relying on this in production.lti/tsugi-util/src/java/org/tsugi/lti/objects/ReplaceResultRequest.java (1)
1-13: ReplaceResultRequest mirrors DeleteResultRequest and looks correctThe class is a minimal Jackson/Lombok wrapper around
ResultRecordwithlocalName = "resultRecord", consistent with the delete request pattern. No changes needed from this snippet.lti/tsugi-util/src/java/org/tsugi/lti/objects/ReplaceResultResponse.java (1)
1-12: LGTM! Clean implementation of empty response payload.The class correctly represents an empty response body for LTI replace result operations. The Jackson XML annotations are properly configured, and the structure aligns with similar response classes like
DeleteResultResponsein the codebase.lti/tsugi-util/src/java/org/tsugi/lti/objects/POXEnvelopeRequest.java (1)
1-18: LGTM! Proper POX envelope structure.The class correctly defines the IMS POX envelope request structure with appropriate Jackson XML annotations and namespace declarations. The structure mirrors the response envelope pattern and properly encapsulates the header and body components.
lti/tsugi-util/src/java/org/tsugi/lti/objects/MembershipRecord.java (1)
1-16: LGTM! Clean data holder implementation.The class appropriately encapsulates membership record data with proper Jackson XML property mappings. The structure is straightforward and well-suited for XML serialization/deserialization of LTI membership information.
lti/tsugi-util/src/java/org/tsugi/lti/objects/Member.java (1)
1-42: LGTM! Comprehensive member data model.The class provides a well-structured representation of LTI member information with appropriate Jackson XML annotations for all fields. The field names follow LTI conventions (e.g.,
user_id,person_name_given,lis_result_sourcedid), and the nestedGroupsrelationship is properly defined.lti/tsugi-util/src/java/org/tsugi/lti/objects/MessageResponse.java (1)
1-24: LGTM! Well-structured message response model.The class properly defines the LTI message response structure with all necessary fields for conveying response data, including members, results, and status information. The Jackson XML annotations are correctly configured for root element serialization.
lti/tsugi-util/src/java/org/tsugi/lti/objects/POXRequestHeaderInfo.java (1)
1-16: LGTM! Proper header information structure.The class correctly defines the POX request header information with appropriate IMS namespace declarations. The version and message identifier fields are essential for POX envelope handling and are properly annotated.
lti/tsugi-util/src/java/org/tsugi/lti/objects/POXCodeMinor.java (1)
1-16: LGTM! Correct list wrapper configuration.The class properly configures Jackson XML annotations to serialize multiple
imsx_codeMinorFieldentries without a wrapper element usinguseWrapping = false. This ensures the XML structure matches IMS specifications for POX code minor details.lti/tsugi-util/src/test/java/org/tsugi/lti/POXJacksonTest.java (1)
1-333: LGTM! Comprehensive test coverage.The test class provides excellent coverage of the POXJacksonParser functionality:
- Request/response parsing with valid, null, empty, and invalid inputs
- Operation type detection and extraction
- Header and body information retrieval
- Response creation for various status types (success, failure, fatal, unsupported)
- Edge cases including empty bodies, unknown operations, and special character escaping
- Performance validation
All variable declarations use explicit types as required by the coding guidelines. The test structure is well-organized with clear assertions and descriptive test names.
lti/tsugi-util/src/java/org/tsugi/lti/objects/POXRequestHeader.java (1)
7-12: POXRequestHeader POJO and XML mapping look correctThe Lombok +
@JacksonXmlPropertysetup cleanly models the request header wrapper and matches the IMS POX element name/namespace; no issues spotted.lti/tsugi-util/src/java/org/tsugi/lti/objects/POXResponseHeader.java (1)
7-17: Response header mapping is consistent and well-documentedThe class-level Javadoc and the
responseHeaderInfoXML mapping align with the POX envelope design and other header/info types; implementation looks solid.lti/tsugi-util/src/java/org/tsugi/lti/objects/POXCodeMinorField.java (1)
7-15: POXCodeMinorField correctly models minor field entriesField names and Jackson XML mappings match the IMS codeMinor element structure; this is a clean, minimal POJO for minor fields.
lti/tsugi-util/src/test/java/org/tsugi/pox/IMSPOXRequestJacksonTest.java (1)
13-245: Comprehensive coverage of Jackson-based POX request/response behaviorThese tests exercise constructor validity, header/body extraction, response generation (success/failure/unsupported/fatal), minor code handling, and edge-case XML inputs, giving good confidence in the new Jackson-backed POX flow. The substring assertions on response XML also help guard against regressions in the serialized format.
lti/tsugi-util/src/java/org/tsugi/lti/objects/POXResponseHeaderInfo.java (1)
7-18: POXResponseHeaderInfo structure aligns with request header and status modelVersion, messageIdentifier, and typed
statusInfoare mapped cleanly to their IMS elements, matching the pattern used byPOXRequestHeaderInfoand the broader POX status objects.lti/tsugi-util/src/java/org/tsugi/lti/objects/ResultRecord.java (1)
7-15: ResultRecord XML mapping looks consistent and focusedThe DTO cleanly models
sourcedGUIDandresultwith appropriate Jackson XML property names, matching howDeleteResultRequestembeds it. No issues from a mapping or structure standpoint.lti/tsugi-util/src/java/org/tsugi/lti/objects/POXStatusInfo.java (1)
9-29: POXStatusInfo matches IMS POX structure and namespace usageThe fields and Jackson XML annotations (including the shared OMS namespace and
POXCodeMinornesting) are consistent with the other POX* objects and look correct.lti/tsugi-util/src/java/org/tsugi/lti/objects/POXEnvelopeResponse.java (1)
8-17: POXEnvelopeResponse is correctly wired to header/body in IMS namespaceThe root element, namespace, and header/body mappings mirror
POXEnvelopeRequestand are suitable for use by the POX response builder/factory code.lti/tsugi-util/src/java/org/tsugi/pox/POXResponseBuilder.java (1)
135-171: The review comment is based on incorrect assumptions about StringEscapeUtils behavior and does not identify a real NPE vulnerability.After examining the code:
- StringEscapeUtils.escapeXml11(null) returns null rather than throwing a NullPointerException
messageIdis already guarded in thebuild()method (line 136-137)majorandseverityhave initialized default values (lines 36-37)- While
descriptionandoperationcan be null, passing them toescapeXml11()will not cause an NPE—the method will simply return nullNo NullPointerException will occur at runtime, regardless of whether these fields are null. The suggested fix is unnecessary for preventing NPE.
Likely an incorrect or invalid review comment.
lti/lti-blis/src/java/org/sakaiproject/lti/ServiceServlet.java (2)
449-586: Roster response migration toMessageResponseBuilder+ POJOs looks correct and keeps adapter thinThe new
processRosterbuildsMemberandGroupPOJOs and feeds them toMessageResponseBuilder, instead of mutating a generic map. Role mapping, outcomes handling, and group assignment logic remain unchanged and the adapter stays fairly thin, which matches the stated transition strategy.No functional issues spotted here; this is a good incremental refactor.
375-447: Response shape is spec-compliant; no changes neededThe current implementation correctly follows the LTI Outcomes Management specification:
- Read operations (
basic-lis-readresult): Include<result>with grade/comment data ✓- Write operations (
basic-lis-deleteresult,basic-lis-replaceresult,basic-lis-updateresult,basic-lis-createresult): Omit<result>element; status/success conveyed in header only ✓Per the LTI spec, write operation responses contain an empty body element (e.g.,
<deleteResultResponse/>) with success/failure status in the header (imsx_statusInfo). The result element is reserved for read operations to return the grade value.
| public static String createResponse(String description, String major, String severity, | ||
| String messageId, String operation, Properties minorCodes, | ||
| String bodyString) { | ||
| if (major == null) major = MAJOR_FAILURE; | ||
| if (severity == null && MAJOR_PROCESSING.equals(major)) severity = SEVERITY_STATUS; | ||
| if (severity == null && MAJOR_SUCCESS.equals(major)) severity = SEVERITY_STATUS; | ||
| if (severity == null) severity = SEVERITY_ERROR; | ||
| if (messageId == null) { | ||
| Date dt = new Date(); | ||
| messageId = "" + dt.getTime(); | ||
| } | ||
|
|
||
| StringBuilder minorString = new StringBuilder(); | ||
| if (minorCodes != null && minorCodes.size() > 0) { | ||
| minorString.append("\n <imsx_codeMinor>\n"); | ||
| for (Object okey : minorCodes.keySet()) { | ||
| String key = (String) okey; | ||
| String value = minorCodes.getProperty(key); | ||
| if (key == null || value == null) continue; | ||
|
|
||
| minorString.append(" <imsx_codeMinorField>\n"); | ||
| minorString.append(" <imsx_codeMinorFieldName>"); | ||
| minorString.append(StringEscapeUtils.escapeXml11(key)); | ||
| minorString.append("</imsx_codeMinorFieldName>\n"); | ||
| minorString.append(" <imsx_codeMinorFieldValue>"); | ||
| minorString.append(StringEscapeUtils.escapeXml11(value)); | ||
| minorString.append("</imsx_codeMinorFieldValue>\n"); | ||
| minorString.append(" </imsx_codeMinorField>\n"); | ||
| } | ||
| minorString.append(" </imsx_codeMinor>"); | ||
| } | ||
|
|
||
| if (bodyString == null) bodyString = ""; | ||
|
|
||
| if (bodyString.startsWith("<?xml")) { | ||
| int pos = bodyString.indexOf("<", 1); | ||
| if (pos > 0) bodyString = bodyString.substring(pos); | ||
| } | ||
| bodyString = bodyString.trim(); | ||
| String newLine = ""; | ||
| if (bodyString.length() > 0) newLine = "\n"; | ||
|
|
||
| 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>%s</imsx_codeMajor>\n" + | ||
| " <imsx_severity>%s</imsx_severity>\n" + | ||
| " <imsx_description>%s</imsx_description>\n" + | ||
| " <imsx_messageRefIdentifier>%s</imsx_messageRefIdentifier>\n" + | ||
| " <imsx_operationRefIdentifier>%s</imsx_operationRefIdentifier>" + | ||
| "%s\n"+ | ||
| " </imsx_statusInfo>\n" + | ||
| " </imsx_POXResponseHeaderInfo>\n" + | ||
| " </imsx_POXHeader>\n" + | ||
| " <imsx_POXBody>\n" + | ||
| "%s%s"+ | ||
| " </imsx_POXBody>\n" + | ||
| "</imsx_POXEnvelopeResponse>", | ||
| 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.
Minor-code XML block is being double‑escaped, breaking <imsx_codeMinor> markup
minorString is built as XML (with its contents already XML‑escaped), but then the whole block is passed through StringEscapeUtils.escapeXml11 before formatting into the response. This will escape the tags themselves (e.g. <imsx_codeMinor> → <imsx_codeMinor>), so consumers will never see proper <imsx_codeMinor> elements.
You should inject the block as raw XML while continuing to escape only the inner key/value content:
- "%s\n"+
+ "%s\n"+
" </imsx_statusInfo>\n" +
@@
- StringEscapeUtils.escapeXml11(operation),
- StringEscapeUtils.escapeXml11(minorString.toString()),
+ StringEscapeUtils.escapeXml11(operation),
+ minorString.toString(),
bodyString, newLineThe key and value are already safely escaped when building minorString.
[scratchpad_start]
[done]
Critical XML escaping bug identified and fix suggested.
[scratchpad_end]
🤖 Prompt for AI Agents
In lti/tsugi-util/src/java/org/tsugi/lti/POXJacksonParser.java around lines
297-369, the XML block built in minorString is being double-escaped by
StringEscapeUtils.escapeXml11 when injected into the final String.format call,
which turns tags like <imsx_codeMinor> into <imsx_codeMinor>; to fix, stop
escaping the whole minorString (it already contains escaped key/value content)
and pass minorString.toString() directly to the formatter (or an empty string
when no minor codes) so the imsx_codeMinor markup is emitted as raw XML while
keeping inner key/value escape intact.
| @SuppressWarnings("deprecation") | ||
| public void loadFromRequest(HttpServletRequest request) { | ||
| String contentType = request.getContentType(); | ||
| String baseContentType; | ||
|
|
||
| try { | ||
| MimeType mimeType = new MimeType(contentType); | ||
| baseContentType = mimeType.getBaseType(); | ||
| } catch (MimeTypeParseException e) { | ||
| errorMessage = "Unable to parse mime type"; | ||
| log.info("{}\n{}", errorMessage, contentType); | ||
| return; | ||
| } | ||
|
|
||
| if (!"application/xml".equals(baseContentType)) { | ||
| errorMessage = "Content Type must be application/xml"; | ||
| log.info("{}\n{}", errorMessage, contentType); | ||
| return; | ||
| } | ||
|
|
||
| header = request.getHeader("Authorization"); | ||
| oauth_body_hash = null; | ||
| if (header != null) { | ||
| if (header.startsWith("OAuth ")) header = header.substring(5); | ||
| String[] parms = header.split(","); | ||
| for (String parm : parms) { | ||
| parm = parm.trim(); | ||
| if (parm.startsWith("oauth_body_hash=")) { | ||
| String[] pieces = parm.split("\""); | ||
| oauth_body_hash = URLDecoder.decode(pieces[1]); | ||
| } | ||
| if (parm.startsWith("oauth_consumer_key=")) { | ||
| String[] pieces = parm.split("\""); | ||
| oauth_consumer_key = URLDecoder.decode(pieces[1]); | ||
| } | ||
| if (parm.startsWith("oauth_signature_method=")) { | ||
| String[] pieces = parm.split("\""); | ||
| oauth_signature_method = URLDecoder.decode(pieces[1]); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if (oauth_body_hash == null) { | ||
| errorMessage = "Did not find oauth_body_hash"; | ||
| log.info("{}\n{}", errorMessage, header); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Harden content-type handling and consider reducing Authorization header logging
In loadFromRequest:
String contentType = request.getContentType();can legally returnnull. Passing that intonew MimeType(contentType)risks aNullPointerExceptionrather than a clean error path.- On certain failures you log the full
Authorizationheader (log.info("{}\n{}", errorMessage, header);). That header typically containsoauth_consumer_key,oauth_signature, and other OAuth parameters; while not raw secrets, logging them in full can still be sensitive.
You can tighten both behaviors with a small change:
public void loadFromRequest(HttpServletRequest request) {
String contentType = request.getContentType();
- String baseContentType;
+ String baseContentType;
- try {
+ if (contentType == null) {
+ errorMessage = "Content Type must be application/xml";
+ log.info("{}", errorMessage);
+ return;
+ }
+
+ try {
MimeType mimeType = new MimeType(contentType);
baseContentType = mimeType.getBaseType();
} catch (MimeTypeParseException e) {
errorMessage = "Unable to parse mime type";
- log.info("{}\n{}", errorMessage, contentType);
+ log.info("{} ({})", errorMessage, contentType);
return;
}
@@
- if (oauth_body_hash == null) {
- errorMessage = "Did not find oauth_body_hash";
- log.info("{}\n{}", errorMessage, header);
- return;
- }
+ if (oauth_body_hash == null) {
+ errorMessage = "Did not find oauth_body_hash";
+ // Consider logging only that the header was present/missing, not its full value
+ log.info("{}", errorMessage);
+ return;
+ }This avoids a potential NPE on missing content type and reduces sensitive header exposure in logs.
I'll verify this review comment by examining the actual file and checking the validity of the concerns raised.
🏁 Script executed:
#!/bin/bash
# Find and examine the file in question
fd -type f -name "IMSPOXRequestJackson.java" -exec cat {} \;Length of output: 170
🏁 Script executed:
#!/bin/bash
# Check for other instances of getContentType handling in the codebase
rg -n "getContentType" --type java -A 5 -B 2Length of output: 266216
🏁 Script executed:
#!/bin/bash
# Look for Authorization header handling patterns
rg -n "Authorization" --type java -A 3 -B 2Length of output: 269810
🏁 Script executed:
#!/bin/bash
# Check for similar OAuth/sensitive data logging
rg -n "oauth_" --type java -C 3Length of output: 109947
🌐 Web query:
HttpServletRequest.getContentType() null behavior Java
💡 Result:
Short answer: getContentType() returns null when the request has no known MIME type (i.e., there is no Content-Type header or the container can't determine it). Use a null-safe check (and string contains/prefix check for multipart) instead of assuming a non-null value. [1][2]
Example pattern:
if (request.getContentType() != null && request.getContentType().toLowerCase().contains("multipart/form-data")) { ... }
Sources:
[1] ServletRequest.getContentType() Javadoc — "Returns the MIME type ... or null if the type is not known."
[2] Common guidance (StackOverflow) — request.getContentType() may be null; check for null and use contains/startsWith for multipart/form-data.
🌐 Web query:
OAuth 1.0a RFC content type requirements
💡 Result:
Short answer — OAuth 1.0a (RFC 5849) requires form-url-encoded bodies for OAuth protocol parameters:
-
Request body parameters are only included in the signature/source if all of these hold: the entity is single-part, it follows the encoding rules of application/x-www-form-urlencoded, and the Content-Type header is set to application/x-www-form-urlencoded. (RFC 5849 §3.4.1.3). [1]
-
Token responses (request_token / access_token) are returned in the HTTP response body using the application/x-www-form-urlencoded content type. (RFC 5849 §2 and §2.1 examples / §3.4.1.3). [1][2]
Implication: do not treat JSON or multipart/form-data bodies as OAuth parameter sources unless they are encoded and labelled as application/x-www-form-urlencoded. [1]
Sources:
[1] RFC 5849 (The OAuth 1.0 Protocol), see §3.4.1.3 and examples.
[2] RFC 5849 (token response examples showing Content-Type: application/x-www-form-urlencoded).
Harden content-type handling and avoid logging full Authorization header
- In loadFromRequest() (lti/tsugi-util/src/java/org/tsugi/pox/IMSPOXRequestJackson.java): request.getContentType() can be null — check for null before creating new MimeType(contentType) and return a clear error path to avoid an NPE.
- Do not log the full Authorization header on errors (it contains oauth_* parameters); log only that the header was present/missing or log a sanitized summary (avoid printing the raw header value).
| 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.
🧩 Analysis chain
Escape fatal-response fields and avoid reusing messageId as operationRefIdentifier
In createFatalResponse, the XML is constructed with raw messageId and description:
" <imsx_messageIdentifier>%s</imsx_messageIdentifier>\n" +
...
" <imsx_description>%s</imsx_description>\n" +
" <imsx_operationRefIdentifier>%s</imsx_operationRefIdentifier>" +
...
messageId,
description,
messageIdTwo issues:
- No XML escaping – if
description(ormessageId) ever contains<,&, or quotes, the response becomes malformed XML and could potentially surface unsanitized text back to a tool. - Reusing
messageIdas operationRefIdentifier – semantically,imsx_operationRefIdentifieris supposed to reference the operation, not the message ID. InIMSPOXRequestJackson.getFatalResponse(...)the default is"unknown", so this is at least inconsistent.
At minimum, the fields should be escaped and messageId reused consistently:
+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(
+ String safeMessageId = StringEscapeUtils.escapeXml11(messageId);
+ String safeDescription = StringEscapeUtils.escapeXml11(description == null ? "" : description);
+
+ return String.format(
@@
- "</imsx_POXEnvelopeResponse>",
- messageId,
- description,
- messageId
+ "</imsx_POXEnvelopeResponse>",
+ safeMessageId,
+ safeDescription,
+ safeMessageId
);
}Separately, you may want to decide whether imsx_operationRefIdentifier should actually be a fixed value like "unknown" (to mirror IMSPOXRequestJackson.getFatalResponse) instead of the message ID, but that’s a behavioral choice.
I'll verify the concerns raised in this review comment by examining the codebase. Let me start by investigating the actual implementation, dependencies, and related code patterns.
---
🏁 Script executed:
```shell
#!/bin/bash
# 1. Find and examine POXResponseFactory.java
fd -t f -i "POXResponseFactory.java" --exec cat {} \;
Length of output: 10839
🏁 Script executed:
#!/bin/bash
# 2. Search for pom.xml and check for commons-text dependency
fd -t f "pom.xml" --exec grep -l "commons-text\|commons.text" {} \;Length of output: 823
🏁 Script executed:
#!/bin/bash
# 3. Find IMSPOXRequestJackson to verify getFatalResponse behavior
fd -t f -i "IMSPOXRequestJackson.java" --exec cat {} \;Length of output: 22316
🏁 Script executed:
#!/bin/bash
# 4. Search for XML escaping patterns in the codebase
rg -n "escapeXml|StringEscapeUtils|XMLUtils" --type java -C 2Length of output: 152066
🏁 Script executed:
#!/bin/bash
# 5. Find other places where XML is constructed manually (potential patterns to compare)
rg -n "String\.format.*xml|<.*%s" --type java -C 2 | head -100Length of output: 12628
Add XML escaping to POXResponseFactory.createFatalResponse
The createFatalResponse method constructs XML with raw, unescaped messageId and description values:
" <imsx_messageIdentifier>%s</imsx_messageIdentifier>\n" +
...
" <imsx_description>%s</imsx_description>\n" +
" <imsx_operationRefIdentifier>%s</imsx_operationRefIdentifier>"This is unsafe. If either parameter contains XML special characters (<, &, quotes), the response becomes malformed XML. Related methods in the codebase (POXResponseBuilder and IMSPOXRequestJackson) already escape such values using StringEscapeUtils.escapeXml11(). Apply the same pattern here.
Additionally, the method reuses messageId for both imsx_messageIdentifier and imsx_operationRefIdentifier. While semantically imsx_operationRefIdentifier should reference an operation (not a message), this is a design decision—the diff in the review addresses the critical escaping issue.
1 participant
Removed XMLMap. Added Jackson based objects for implementation.
Summary by CodeRabbit
Release Notes
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.