SAK-52031 EntityBroker Remove reflectutils library from codebase #14139
Conversation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
entitybroker/utils/src/java/org/sakaiproject/entitybroker/util/core/EntityProviderMethodStoreImpl.java (1)
156-158: Bug: wrong map stored; copies are never used.You build
caswithaction.copy()to avoid cross‑ClassLoader references, but storeactionsintoentityActions. This defeats the copy and can cause classloader leaks/coupling.Apply:
- entityActions.put(prefix, actions); - log.info("Registered "+actions.size()+" custom actions for entity prefix ("+prefix+"): " + sb.toString()); + entityActions.put(prefix, cas); + log.info("Registered {} custom actions for entity prefix ({}): {}", cas.size(), prefix, sb);entitybroker/rest/src/java/org/sakaiproject/entitybroker/rest/EntityHandlerImpl.java (1)
1053-1084: Bug: fallback last-modified ignores lastModifiedTime when entity has none.Method javadoc says “time to use if none is found.” Current code uses currentTime when ed != null but LM not found. Use lastModifiedTime instead.
Apply:
- long lastModified = System.currentTimeMillis(); + long lastModified = lastModifiedTime; @@ - } else { - lastModified = lastModifiedTime; - } + }content-review/impl/turnitin/src/main/java/org/sakaiproject/contentreview/turnitin/util/TurnitinAPIUtil.java (1)
260-285: Harden XML parsing, close stream, and avoid default charset in DOM path
- XXE risk: DocumentBuilderFactory not secured (external entities/DTDs allowed).
- Resource leak: InputStream/BufferedReader not closed.
- Encoding risk: Using InputStreamReader without charset; better to parse InputStream so XML prolog governs encoding.
- Also switch logs to SLF4J parameterization.
Apply this refactor:
- InputStream inputStream = callTurnitinReturnInputStream(apiURL, parameters, secretKey, timeout, proxy, isMultipart); - - BufferedReader in; - in = new BufferedReader(new InputStreamReader(inputStream)); - Document document = null; - try { - DocumentBuilderFactory documentBuilderFactory = DocumentBuilderFactory.newInstance(); - DocumentBuilder parser = documentBuilderFactory.newDocumentBuilder(); - document = parser.parse(new org.xml.sax.InputSource(in)); - } - catch (ParserConfigurationException pce){ - log.error("parser configuration error: " + pce.getMessage()); - throw new TransientSubmissionException ("Parser configuration error", pce); - } catch (Exception t) { - throw new TransientSubmissionException ("Cannot parse Turnitin response. Assuming call was unsuccessful", t); - } - - if (log.isDebugEnabled()) { - log.debug(" Result from call: " + Xml.writeDocumentToString(document)); - } - - return document; + try (InputStream in = callTurnitinReturnInputStream(apiURL, parameters, secretKey, timeout, proxy, isMultipart)) { + DocumentBuilderFactory dbf = DocumentBuilderFactory.newInstance(); + dbf.setFeature(javax.xml.XMLConstants.FEATURE_SECURE_PROCESSING, true); + dbf.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true); + dbf.setFeature("http://xml.org/sax/features/external-general-entities", false); + dbf.setFeature("http://xml.org/sax/features/external-parameter-entities", false); + dbf.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false); + dbf.setXIncludeAware(false); + dbf.setExpandEntityReferences(false); + + DocumentBuilder parser = dbf.newDocumentBuilder(); + Document document = parser.parse(in); + + if (log.isDebugEnabled()) { + log.debug(" Result from call: {}", Xml.writeDocumentToString(document)); + } + return document; + } catch (ParserConfigurationException pce) { + log.error("Parser configuration error", pce); + throw new TransientSubmissionException("Parser configuration error", pce); + } catch (Exception t) { + throw new TransientSubmissionException("Cannot parse Turnitin response. Assuming call was unsuccessful", t); + }As per coding guidelines
entitybroker/rest/src/java/org/sakaiproject/entitybroker/rest/EntityEncodingManager.java (1)
564-569: Harden JSONP callback to prevent XSS; current sanitize allows arbitrary JS identifiersAn attacker can pass a malicious callback (e.g., alert(1)//) leading to executable JS in the response. Prefixing with "/**/" does not stop this. Restrict the callback to a safe identifier pattern and fall back to the default if invalid.
Apply this diff to sanitizeJsonCallback:
- protected String sanitizeJsonCallback(Object param) { - //We might want to sanitize down to something that looks like a valid function call - //This shouldn't be necessary, though, since it will just either work or not - if (param == null || !(param instanceof String)) - return JSON_DEFAULT_CALLBACK; - else - // CVE-2014-4671 -- Mitigate 'Rosetta Flash' exploit by ensuring Flash embedded in callback will break - return "/**/" + param.toString(); - } + protected String sanitizeJsonCallback(Object param) { + if (!(param instanceof String)) { + return JSON_DEFAULT_CALLBACK; + } + String s = ((String) param).trim(); + // Reasonable length cap to avoid abuse + if (s.length() == 0 || s.length() > 100) { + return JSON_DEFAULT_CALLBACK; + } + // Allow dotted identifiers like foo, foo.bar_baz, $.x, _a.b2 + if (!JSONP_CALLBACK_PATTERN.matcher(s).matches()) { + return JSON_DEFAULT_CALLBACK; + } + // Keep Rosetta-Flash mitigation + return "/**/" + s; + }Add this class-level constant (near the JSON_DEFAULT_CALLBACK) and import:
// add near other constants private static final java.util.regex.Pattern JSONP_CALLBACK_PATTERN = java.util.regex.Pattern.compile("^[a-zA-Z_$][0-9A-Za-z_$]*(?:\\.[a-zA-Z_$][0-9A-Za-z_$]*)*$");This blocks callback injection while preserving legitimate JSONP usage.
Also applies to: 1070-1078
🧹 Nitpick comments (26)
entitybroker/rest/src/java/org/sakaiproject/entitybroker/rest/EntityBatchHandler.java (3)
221-222: LGTM!The replacement of
ArrayOrderedMapwithLinkedHashMapis correct and maintains insertion order for batch processing. The use of theMapinterface type is a good practice.Consider using the diamond operator for more concise syntax:
- Map<String, String> dataMap = new LinkedHashMap<String, String>(); - Map<String, ResponseBase> results = new LinkedHashMap<String, ResponseBase>(); + Map<String, String> dataMap = new LinkedHashMap<>(); + Map<String, ResponseBase> results = new LinkedHashMap<>();
410-418: LGTM!The replacement of
ArrayOrderedMapwithLinkedHashMapis correct throughout this method. The use of size hints for the inner maps is good for performance.Consider using the diamond operator for more concise syntax:
- Map<String, Map<String, String[]>> referencedParams = null; if (Method.POST.equals(method) || Method.PUT.equals(method) ) { - referencedParams = new LinkedHashMap<String, Map<String, String[]>>(); + referencedParams = new LinkedHashMap<>(); Map<String, String[]> params = req.getParameterMap(); // create the maps to hold the params - referencedParams.put(UNREFERENCED_PARAMS, new LinkedHashMap<String, String[]>(params.size())); + referencedParams.put(UNREFERENCED_PARAMS, new LinkedHashMap<>(params.size())); for (int i = 0; i < refs.length; i++) { String refKey = "ref" + i + '.'; - referencedParams.put(refKey, new LinkedHashMap<String, String[]>(params.size())); + referencedParams.put(refKey, new LinkedHashMap<>(params.size())); }
615-615: LGTM!The use of
LinkedHashMapwith a size hint is appropriate for maintaining insertion order and optimizing performance.Consider using the diamond operator:
- params = new LinkedHashMap<String, String>(referencedParams.size()); + params = new LinkedHashMap<>(referencedParams.size());entitybroker/utils/src/java/org/sakaiproject/entitybroker/util/core/EntityProviderMethodStoreImpl.java (4)
360-362: Good replacement of ReflectUtils.arrayToString.stringifyClassArray(validTypes) is clear and null-safe. Optional: use Class.getSimpleName() to reduce verbosity in messages.
367-374: Helper is correct and robust.Null/empty handled; output is deterministic. Optional: consider getSimpleName() (shorter) or reuse Arrays.stream(types).map(c -> c.getSimpleName()) if logs are too noisy.
114-117: Use SLF4J parameterized logging (no string concatenation).Per coding guidelines for Java, replace concatenation with placeholders.
- log.warn("A method ("+method.getName()+") in the entity provider for prefix (" - + entityProvider.getEntityPrefix()+") appears to be a custom action method but" - + "does not have a valid set of parameter types, this may be ok but should be checked on: " + e.getMessage()); + log.warn("A method ({}) in the entity provider for prefix ({}) appears to be a custom action method but does not have a valid set of parameter types; please verify: {}", + method.getName(), entityProvider.getEntityPrefix(), e.getMessage());- log.info("Registered "+actions.size()+" custom actions for entity prefix ("+prefix+"): " + sb.toString()); + log.info("Registered {} custom actions for entity prefix ({}): {}", actions.size(), prefix, sb);- log.info("Registered "+redirects.length+" url redirects for entity prefix ("+prefix+"): " + sb.toString()); + log.info("Registered {} url redirects for entity prefix ({}): {}", redirects.length, prefix, sb);As per coding guidelines
Also applies to: 157-158, 271-272
352-357: Minor: break early once a match is found.Avoid scanning remaining validTypes after a match is detected.
- for (int j = 0; j < validTypes.length; j++) { - if (validTypes[j].isAssignableFrom(paramType)) { - validParams[i] = validTypes[j]; - found = true; - } - } + for (int j = 0; j < validTypes.length; j++) { + if (validTypes[j].isAssignableFrom(paramType)) { + validParams[i] = validTypes[j]; + found = true; + break; + } + }entitybroker/utils/src/java/org/sakaiproject/entitybroker/util/EntityDataUtils.java (2)
187-227: Consider caching discovered fields/methods for performance.The implementation correctly handles multiple discovery strategies with appropriate null-safety and fallback logic. However, repeated calls for the same entity class will re-scan the class hierarchy each time.
For high-frequency scenarios, consider caching discovered Field/Method objects by class type in a ConcurrentHashMap to avoid redundant reflection scans:
private static final Map<Class<?>, Field> ID_FIELD_CACHE = new ConcurrentHashMap<>(); private static final Map<Class<?>, Method> ID_METHOD_CACHE = new ConcurrentHashMap<>();Then use
computeIfAbsentto populate the cache on first access.
234-264: Consider caching for performance optimization.The implementation is correct and consistent with
getEntityId, with appropriate null-safety and fallback logic.Similar to
getEntityId, consider caching the discovered field name by class type to avoid redundant hierarchy scans in high-frequency scenarios.entitybroker/impl/src/java/org/sakaiproject/entitybroker/impl/EntityBrokerManagerImpl.java (3)
738-748: Align POJO property lookup with map keys (support common title synonyms).Map path checks displayTitle/title/displayName/name; POJO path only checks title. Expand to match.
Apply this diff:
- return findNamedValue(bean, "title"); + for (String p : new String[] {"displayTitle", "title", "displayName", "name"}) { + String v = findNamedValue(bean, p); + if (v != null) { + return v; + } + } + return null;
717-736: Tighten isBeanCandidate: exclude proxies.Avoid treating JDK/CGLIB proxies as bean candidates; reflection on them is brittle and costly.
Apply this diff:
private boolean isBeanCandidate(Object value) { if (value == null) { return false; } Class<?> type = value.getClass(); + // skip JDK/CGLIB proxies + if (java.lang.reflect.Proxy.isProxyClass(type) || type.getName().contains("$$")) { + return false; + } if (type.isPrimitive() || type.isArray() || type.isEnum()) { return false; }
797-807: Consider Spring utilities and caching to reduce reflection overhead.Per PR goal (removing reflectutils), prefer Spring’s org.springframework.util.ReflectionUtils.getAllDeclaredMethods(...) and getAllDeclaredFields(...), and unwrap proxies with org.springframework.aop.support.AopUtils.getTargetClass or org.springframework.util.ClassUtils.getUserClass. Also cache per-class method/field lists and BeanInfo in ConcurrentHashMaps to avoid repeated scans during list population.
If you’d like, I can provide a small caching wrapper (ConcurrentHashMap<Class<?>, CachedIntrospection>) with ~25 LOC. Based on learnings.
Also applies to: 809-819
entitybroker/rest/src/java/org/sakaiproject/entitybroker/rest/EntityDescriptionManager.java (2)
1200-1246: Cache PropertyIntrospection per class to avoid repeated reflectionanalyzeProperties() is invoked multiple times for the same class in describe flows. Cache results to cut reflection cost and speed up describe pages.
Apply within analyzeProperties:
private PropertyIntrospection analyzeProperties(Class<?> type) { - PropertyIntrospection introspection = new PropertyIntrospection(); + PropertyIntrospection cached = PROPERTY_CACHE.get(type); + if (cached != null) { + return cached; + } + PropertyIntrospection introspection = new PropertyIntrospection(); if (type == null) { return introspection; } try { for (PropertyDescriptor descriptor : Introspector.getBeanInfo(type).getPropertyDescriptors()) { ... } } catch (IntrospectionException e) { throw new IllegalStateException("Failed to introspect " + type, e); } for (Field field : getAllFields(type)) { ... } - return introspection; + PROPERTY_CACHE.put(type, introspection); + return introspection; }Add these outside the method (class field and import):
import java.util.concurrent.ConcurrentHashMap; // ... private static final Map<Class<?>, PropertyIntrospection> PROPERTY_CACHE = new ConcurrentHashMap<>();
1248-1268: Optional: reuse Spring ReflectionUtils for field walkingYou can replace findField/getAllFields loops with Spring’s ReflectionUtils (already in the stack) for less boilerplate and consistent behavior.
entitybroker/utils/src/java/org/sakaiproject/entitybroker/util/devhelper/DeveloperBeanUtils.java (1)
76-98: populate() performs effectively deep/unbounded cloningallowedDepth is set to near-infinite, which is heavy and risky with nested inputs.
Consider limiting depth (e.g., 2) for populate to avoid deep recursion and large allocations:
- int allowedDepth = normalizeDepth(0) - 1; + int allowedDepth = 2; // shallow conversion; avoids deep recursionentitybroker/utils/src/java/org/sakaiproject/entitybroker/util/devhelper/DeveloperHelperServiceMock.java (1)
43-52: Return defaultValue when setting is absent or conversion yields nullCurrent logic can return null even when a default is provided. Prefer defaultValue as fallback.
Apply this diff:
public <T> T getConfigurationSetting(String settingName, T defaultValue) { T value = null; - Object o = settings.get(settingName); - if (defaultValue == null) { - value = (T) o; - } else { - value = DeveloperBeanUtils.convert(settings.get(settingName), (Class<T>) defaultValue.getClass()); - } - return value; + Object raw = settings.get(settingName); + if (defaultValue == null) { + value = (T) raw; + } else { + T converted = DeveloperBeanUtils.convert(raw, (Class<T>) defaultValue.getClass()); + value = (converted != null ? converted : defaultValue); + } + return value; }entitybroker/utils/src/java/org/sakaiproject/entitybroker/util/devhelper/AbstractDeveloperHelperService.java (2)
377-390: Make codec registry immutable and thread‑safeLazy init without synchronization can race. Use a static, unmodifiable map.
Apply this diff:
- private Map<String, EntityCodec> codecs; - private EntityCodec getCodec(String format) { - if (codecs == null) { - codecs = new ConcurrentHashMap<>(); - codecs.put(Formats.JSON, new JsonCodec()); - codecs.put(Formats.XML, new XmlCodec()); - codecs.put(Formats.HTML, new HtmlCodec()); - } - EntityCodec codec = codecs.get(format); + private static final Map<String, EntityCodec> CODECS = java.util.Collections.unmodifiableMap(new LinkedHashMap<String, EntityCodec>() {{ + put(Formats.JSON, new JsonCodec()); + put(Formats.XML, new XmlCodec()); + put(Formats.HTML, new HtmlCodec()); + }}); + private EntityCodec getCodec(String format) { + EntityCodec codec = CODECS.get(format); if (codec == null) { throw new IllegalArgumentException("Failed to find an encoder/decoder for format: " + format); } return codec; }
108-151: Error wrapping OK; consider surfacing original cause/details to clientsUnsupportedOperationException hides details. If this bubbles to REST callers, include format/type context (you already do) and optionally the original message in properties.
No change required; just a note to verify REST error mappers preserve messages.
entitybroker/rest/src/java/org/sakaiproject/entitybroker/rest/EntityHandlerImpl.java (3)
1091-1110: Broaden makeLastModified type support and avoid deprecated Long(String).Support Number, Calendar, Instant; use Long.parseLong for strings.
Apply:
- if (lm != null) { + if (lm != null) { Class<?> c = lm.getClass(); if (Date.class.isAssignableFrom(c)) { lastModified = ((Date)lm).getTime(); - } else if (Long.class.isAssignableFrom(c)) { - lastModified = ((Long)lm); + } else if (Number.class.isAssignableFrom(c)) { + lastModified = ((Number) lm).longValue(); + } else if (java.util.Calendar.class.isAssignableFrom(c)) { + lastModified = ((java.util.Calendar) lm).getTimeInMillis(); + } else if (java.time.Instant.class.isAssignableFrom(c)) { + lastModified = ((java.time.Instant) lm).toEpochMilli(); } else if (String.class.isAssignableFrom(c)) { try { - lastModified = new Long((String)lm); + lastModified = Long.parseLong((String) lm); } catch (NumberFormatException e) { // nothing to do here } } else { log.warn("EntityRequestHandler: Unknown type returned for 'lastModified' (not Date, Long, String): " + lm.getClass() + ", using the default value of current time instead"); } }
1126-1141: Consider Spring’s ReflectionUtils to reduce custom reflection.Spring’s org.springframework.util.ReflectionUtils can discover fields/methods (incl. annotations), handle accessibility, and reduces foot‑guns. It aligns with the PR goal to remove reflectutils without reinventing helpers.
Would you like a patch replacing find*/read*/invoke* with ReflectionUtils equivalents?
Also applies to: 1143-1154, 1156-1168, 1170-1182, 1184-1196, 1198-1208, 1210-1225
1126-1141: Add unit tests for last-modified resolution.Cover: direct field, getter, annotated field, annotated method, and absence fallback to lastModifiedTime.
I can generate parameterized tests for these cases. Want me to open a test PR?
content-review/impl/turnitin/src/main/java/org/sakaiproject/contentreview/turnitin/util/TurnitinAPIUtil.java (1)
286-305: Potential data loss: attributes ignored; confirm Turnitin XML structureflattenResponse/extractValue drop XML attributes entirely; if TII uses attributes (e.g., status codes), they’re lost. Do we need them? If yes, extend extractValue to include attributes.
Optional patch to capture attributes with "@attr" and text as "#text" when mixed:
private static Object extractValue(Element element) { - NodeList childNodes = element.getChildNodes(); + NodeList childNodes = element.getChildNodes(); List<Element> elementChildren = new ArrayList<>(); + // Collect attributes first + Map<String, Object> withAttrs = null; + if (element.hasAttributes()) { + withAttrs = new LinkedHashMap<>(); + for (int i = 0; i < element.getAttributes().getLength(); i++) { + Node a = element.getAttributes().item(i); + withAttrs.put("@" + a.getNodeName(), a.getNodeValue()); + } + } for (int i = 0; i < childNodes.getLength(); i++) { Node node = childNodes.item(i); if (node.getNodeType() == Node.ELEMENT_NODE) { elementChildren.add((Element) node); } } - if (elementChildren.isEmpty()) { - String text = element.getTextContent(); - return text == null ? "" : text.trim(); - } - Map<String, Object> nested = new LinkedHashMap<>(); + if (elementChildren.isEmpty()) { + String text = element.getTextContent(); + String val = text == null ? "" : text.trim(); + if (withAttrs != null && !withAttrs.isEmpty()) { + if (!val.isEmpty()) { + withAttrs.put("#text", val); + } + return withAttrs; + } + return val; + } + Map<String, Object> nested = (withAttrs != null) ? withAttrs : new LinkedHashMap<>(); for (Element child : elementChildren) { mergeValue(nested, child.getNodeName(), extractValue(child)); } return nested; }Also applies to: 307-325, 327-343
entitybroker/rest/src/java/org/sakaiproject/entitybroker/rest/EntityEncodingManager.java (4)
571-578: Use StandardCharsets.UTF_8 to avoid unnecessary checked exceptionUsing getBytes(Formats.UTF_8) forces catching UnsupportedEncodingException. Prefer StandardCharsets.UTF_8.
Apply:
- try { - byte[] b = encoded.getBytes(Formats.UTF_8); - output.write(b); - } catch (UnsupportedEncodingException e) { - throw new EntityEncodingException("Failed to encode UTF-8: " + ref, ref.toString(), e); - } catch (IOException e) { + try { + byte[] b = encoded.getBytes(StandardCharsets.UTF_8); + output.write(b); + } catch (IOException e) { throw new EntityEncodingException("Failed to encode into output stream: " + ref, ref.toString(), e); }
1261-1272: Skip static fields during introspection; avoid setAccessible on public fieldsIncluding static fields can leak constants/loggers into forms/HTML and produce confusing output. Also, calling setAccessible on already public fields is unnecessary.
Apply to analyzeProperties:
- if (Modifier.isPublic(field.getModifiers())) { + if (Modifier.isPublic(field.getModifiers()) && !Modifier.isStatic(field.getModifiers())) { metadata.readableTypes.putIfAbsent(name, field.getType()); metadata.writableTypes.putIfAbsent(name, field.getType()); }Apply to getObjectValues:
- for (Field field : getAllFields(entity.getClass())) { - if (!Modifier.isPublic(field.getModifiers())) { + for (Field field : getAllFields(entity.getClass())) { + if (!Modifier.isPublic(field.getModifiers()) || Modifier.isStatic(field.getModifiers())) { continue; } String name = field.getName(); if (values.containsKey(name)) { continue; } try { - field.setAccessible(true); values.put(name, field.get(entity)); } catch (IllegalAccessException e) { throw new IllegalStateException("Unable to read field " + name + " on " + entity.getClass(), e); } }Also applies to: 1291-1305
1132-1138: Bound input read to avoid large payload OOMsreadAllBytes() loads the entire request body into memory. If upstream limits are not enforced, this can blow heap on large inputs.
- Confirm container/filters enforce a sane max request size for these endpoints.
- Optionally, replace with a bounded read (e.g., cap at configured max) and fail fast on overflow.
Example snippet:
private String readInputStream(InputStream input) { try (java.io.ByteArrayOutputStream baos = new java.io.ByteArrayOutputStream()) { byte[] buf = new byte[8192]; long max = 1_048_576L; // 1MB cap; make configurable long total = 0; int n; while ((n = input.read(buf)) != -1) { total += n; if (total > max) { throw new UncheckedIOException(new IOException("Input too large")); } baos.write(buf, 0, n); } return baos.toString(StandardCharsets.UTF_8); } catch (IOException e) { throw new UncheckedIOException("Failed to read input stream", e); } }
476-476: Use SLF4J parameterized logging instead of string concatenationAs per coding guidelines, prefer placeholders.
As per coding guidelines
- log.debug("EntityEncodingManager: No entities to format ("+format+") and output for ref (" + ref + ")"); + log.debug("EntityEncodingManager: No entities to format ({}) and output for ref ({})", format, ref);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (36)
content-review/impl/turnitin/pom.xml(0 hunks)content-review/impl/turnitin/src/main/java/org/sakaiproject/contentreview/turnitin/util/TurnitinAPIUtil.java(5 hunks)docker/tomcat/conf/catalina.properties(0 hunks)entitybroker/core-providers/pom.xml(0 hunks)entitybroker/core-providers/src/java/org/sakaiproject/entitybroker/providers/SiteEntityProvider.java(5 hunks)entitybroker/core-providers/src/java/org/sakaiproject/entitybroker/providers/UserEntityProvider.java(4 hunks)entitybroker/core-providers/src/java/org/sakaiproject/entitybroker/providers/model/EntityGroup.java(3 hunks)entitybroker/core-providers/src/java/org/sakaiproject/entitybroker/providers/model/EntityMember.java(2 hunks)entitybroker/core-providers/src/java/org/sakaiproject/entitybroker/providers/model/EntitySite.java(3 hunks)entitybroker/core-providers/src/java/org/sakaiproject/entitybroker/providers/model/EntitySitePage.java(2 hunks)entitybroker/core-providers/src/java/org/sakaiproject/entitybroker/providers/model/EntityTool.java(2 hunks)entitybroker/core-providers/src/java/org/sakaiproject/entitybroker/providers/model/EntityUser.java(4 hunks)entitybroker/impl/pom.xml(0 hunks)entitybroker/impl/src/java/org/sakaiproject/entitybroker/impl/EntityBrokerManagerImpl.java(3 hunks)entitybroker/impl/src/java/org/sakaiproject/entitybroker/impl/entityprovider/EntityProviderManagerImpl.java(6 hunks)entitybroker/impl/src/java/org/sakaiproject/entitybroker/impl/event/EventReceiverCoordinator.java(3 hunks)entitybroker/pom.xml(0 hunks)entitybroker/rest/pom.xml(0 hunks)entitybroker/rest/src/java/org/sakaiproject/entitybroker/rest/EntityBatchHandler.java(7 hunks)entitybroker/rest/src/java/org/sakaiproject/entitybroker/rest/EntityDescriptionManager.java(8 hunks)entitybroker/rest/src/java/org/sakaiproject/entitybroker/rest/EntityEncodingManager.java(17 hunks)entitybroker/rest/src/java/org/sakaiproject/entitybroker/rest/EntityHandlerImpl.java(8 hunks)entitybroker/rest/src/test/org/sakaiproject/entitybroker/rest/EntityEncodingManagerTest.java(5 hunks)entitybroker/utils/pom.xml(0 hunks)entitybroker/utils/src/java/org/sakaiproject/entitybroker/util/EntityDataUtils.java(4 hunks)entitybroker/utils/src/java/org/sakaiproject/entitybroker/util/access/AccessProviderManagerImpl.java(2 hunks)entitybroker/utils/src/java/org/sakaiproject/entitybroker/util/core/EntityProviderMethodStoreImpl.java(2 hunks)entitybroker/utils/src/java/org/sakaiproject/entitybroker/util/devhelper/AbstractDeveloperHelperService.java(7 hunks)entitybroker/utils/src/java/org/sakaiproject/entitybroker/util/devhelper/DeveloperBeanUtils.java(1 hunks)entitybroker/utils/src/java/org/sakaiproject/entitybroker/util/devhelper/DeveloperHelperServiceMock.java(2 hunks)entitybroker/utils/src/java/org/sakaiproject/entitybroker/util/http/EntityHttpServletResponse.java(2 hunks)entitybroker/utils/src/java/org/sakaiproject/entitybroker/util/request/RequestStorageImpl.java(2 hunks)entitybroker/utils/src/test/org/sakaiproject/entitybroker/util/reflect/ReflectUtilTest.java(0 hunks)kernel/kernel-impl/pom.xml(0 hunks)kernel/kernel-impl/src/main/java/org/sakaiproject/tool/impl/RebuildBreakdownServiceImpl.java(5 hunks)master/pom.xml(0 hunks)
💤 Files with no reviewable changes (10)
- entitybroker/utils/pom.xml
- entitybroker/impl/pom.xml
- kernel/kernel-impl/pom.xml
- content-review/impl/turnitin/pom.xml
- docker/tomcat/conf/catalina.properties
- entitybroker/core-providers/pom.xml
- master/pom.xml
- entitybroker/pom.xml
- entitybroker/rest/pom.xml
- entitybroker/utils/src/test/org/sakaiproject/entitybroker/util/reflect/ReflectUtilTest.java
🧰 Additional context used
📓 Path-based instructions (1)
**/*.java
📄 CodeRabbit inference engine (.cursor/rules/logging-rule.mdc)
**/*.java: Use SLF4J parameterized logging (e.g., logger.info("Value is: {}", value)) instead of string concatenation in log statements
Write log messages and code comments in English; never translate log messages
Files:
entitybroker/utils/src/java/org/sakaiproject/entitybroker/util/request/RequestStorageImpl.javaentitybroker/core-providers/src/java/org/sakaiproject/entitybroker/providers/model/EntityTool.javaentitybroker/core-providers/src/java/org/sakaiproject/entitybroker/providers/model/EntityUser.javaentitybroker/core-providers/src/java/org/sakaiproject/entitybroker/providers/model/EntitySitePage.javaentitybroker/rest/src/test/org/sakaiproject/entitybroker/rest/EntityEncodingManagerTest.javaentitybroker/core-providers/src/java/org/sakaiproject/entitybroker/providers/model/EntitySite.javakernel/kernel-impl/src/main/java/org/sakaiproject/tool/impl/RebuildBreakdownServiceImpl.javaentitybroker/utils/src/java/org/sakaiproject/entitybroker/util/access/AccessProviderManagerImpl.javaentitybroker/rest/src/java/org/sakaiproject/entitybroker/rest/EntityBatchHandler.javaentitybroker/impl/src/java/org/sakaiproject/entitybroker/impl/event/EventReceiverCoordinator.javaentitybroker/impl/src/java/org/sakaiproject/entitybroker/impl/entityprovider/EntityProviderManagerImpl.javaentitybroker/utils/src/java/org/sakaiproject/entitybroker/util/core/EntityProviderMethodStoreImpl.javaentitybroker/utils/src/java/org/sakaiproject/entitybroker/util/devhelper/DeveloperHelperServiceMock.javaentitybroker/utils/src/java/org/sakaiproject/entitybroker/util/devhelper/DeveloperBeanUtils.javaentitybroker/utils/src/java/org/sakaiproject/entitybroker/util/http/EntityHttpServletResponse.javaentitybroker/rest/src/java/org/sakaiproject/entitybroker/rest/EntityDescriptionManager.javacontent-review/impl/turnitin/src/main/java/org/sakaiproject/contentreview/turnitin/util/TurnitinAPIUtil.javaentitybroker/impl/src/java/org/sakaiproject/entitybroker/impl/EntityBrokerManagerImpl.javaentitybroker/utils/src/java/org/sakaiproject/entitybroker/util/EntityDataUtils.javaentitybroker/core-providers/src/java/org/sakaiproject/entitybroker/providers/model/EntityGroup.javaentitybroker/rest/src/java/org/sakaiproject/entitybroker/rest/EntityHandlerImpl.javaentitybroker/rest/src/java/org/sakaiproject/entitybroker/rest/EntityEncodingManager.javaentitybroker/core-providers/src/java/org/sakaiproject/entitybroker/providers/UserEntityProvider.javaentitybroker/core-providers/src/java/org/sakaiproject/entitybroker/providers/model/EntityMember.javaentitybroker/utils/src/java/org/sakaiproject/entitybroker/util/devhelper/AbstractDeveloperHelperService.javaentitybroker/core-providers/src/java/org/sakaiproject/entitybroker/providers/SiteEntityProvider.java
🧬 Code graph analysis (5)
entitybroker/core-providers/src/java/org/sakaiproject/entitybroker/providers/model/EntityUser.java (1)
entitybroker/core-providers/src/java/org/sakaiproject/entitybroker/providers/model/EntitySitePage.java (1)
JsonIgnoreProperties(42-385)
entitybroker/core-providers/src/java/org/sakaiproject/entitybroker/providers/model/EntitySite.java (1)
entitybroker/core-providers/src/java/org/sakaiproject/entitybroker/providers/model/EntitySitePage.java (1)
JsonIgnoreProperties(42-385)
entitybroker/core-providers/src/java/org/sakaiproject/entitybroker/providers/model/EntityGroup.java (1)
entitybroker/core-providers/src/java/org/sakaiproject/entitybroker/providers/model/EntitySitePage.java (1)
JsonIgnoreProperties(42-385)
entitybroker/rest/src/java/org/sakaiproject/entitybroker/rest/EntityEncodingManager.java (1)
kernel/api/src/main/java/org/sakaiproject/serialization/MapperFactory.java (1)
MapperFactory(36-422)
entitybroker/utils/src/java/org/sakaiproject/entitybroker/util/devhelper/AbstractDeveloperHelperService.java (1)
kernel/api/src/main/java/org/sakaiproject/serialization/MapperFactory.java (1)
MapperFactory(36-422)
🪛 GitHub Actions: Maven Build
entitybroker/utils/src/java/org/sakaiproject/entitybroker/util/devhelper/AbstractDeveloperHelperService.java
[error] 33-39: Compilation failed: missing Jackson and Commons Text dependencies. Packages com.fasterxml.jackson.core, com.fasterxml.jackson.databind, com.fasterxml.jackson.dataformat.xml, and org.apache.commons.text are not found. Ensure Jackson (jackson-core, jackson-databind, jackson-dataformat-xml) and Apache Commons Text are declared as dependencies in the project. The failing command was: 'mvn --show-version --batch-mode -PskipBrokenTests test'.
🪛 GitHub Actions: Windows Build
entitybroker/utils/src/java/org/sakaiproject/entitybroker/util/devhelper/AbstractDeveloperHelperService.java
[error] 33-39: Missing dependencies: com.fasterxml.jackson.core, com.fasterxml.jackson.databind, com.fasterxml.jackson.dataformat.xml, com.fasterxml.jackson.core.type, and org.apache.commons.text. Ensure Jackson (core, databind, dataformat-xml) and Apache Commons Text are included in dependencies.
[error] 408-408: Cannot find symbol: class ObjectMapper
[error] 415-417: Cannot find symbol: class ObjectWriter; Cannot find symbol: class XmlMapper; Cannot find symbol: class TypeReference
⏰ 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). (2)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: sakai-deploy
🔇 Additional comments (68)
entitybroker/core-providers/src/java/org/sakaiproject/entitybroker/providers/model/EntityMember.java (2)
22-22: LGTM! Appropriate Jackson import.The addition of the Jackson
@JsonIgnoreimport is correct and aligns with the PR's objective to migrate from reflectutils to Jackson-based JSON serialization.
263-269: LGTM! Correct use of @JsonIgnore.Adding
@JsonIgnoretogetRole()is appropriate because:
- The method can throw
UnsupportedOperationException, which would break JSON serialization- Role information is already captured in the
memberRoleString field (line 48)- This properly replaces the removed class-level
ReflectIgnoreClassFieldsannotationentitybroker/core-providers/src/java/org/sakaiproject/entitybroker/providers/model/EntitySite.java (3)
31-32: LGTM! Jackson imports correctly added.The import statements properly bring in the Jackson annotations needed to replace the reflectutils serialization metadata. This aligns with the PR objective to remove the reflectutils library dependency.
64-64: LGTM! Class-level serialization control correctly migrated.The
@JsonIgnorePropertiesannotation correctly replaces the reflectutils class-level metadata, maintaining the same set of ignored fields for JSON serialization. This is consistent with the pattern used in related model classes likeEntitySitePage.
754-754: LGTM! Method-level JSON exclusion correctly applied.The
@JsonIgnoreannotation appropriately replaces the reflectutils method-level annotation, ensuring thetoXmlmethod is excluded from JSON serialization. This is the correct Jackson equivalent for the previous@ReflectTransientannotation.entitybroker/core-providers/src/java/org/sakaiproject/entitybroker/providers/model/EntityTool.java (2)
21-21: LGTM! Jackson annotation import is correct.The import of
@JsonIgnoreis appropriate for controlling JSON serialization. This aligns with the PR's goal of replacing reflectutils with industry-standard Jackson annotations.
122-128: LGTM! Correct migration from reflectutils to Jackson.The
@JsonIgnoreannotation ongetAccessSecurity()correctly replicates the original behavior of@ReflectIgnoreClassFields({"accessSecurity"}). The method is excluded from serialization while thegetAccess()method (lines 130-132) still exposes the string representation of the access security setting.This change maintains backward compatibility: API consumers will continue to receive the
accessfield as a string, not the internalAccessSecurityenum.entitybroker/utils/src/java/org/sakaiproject/entitybroker/util/http/EntityHttpServletResponse.java (2)
31-31: LGTM - Standard collection import.The addition of
LinkedHashMapimport is appropriate for replacing the reflectutilsArrayOrderedMapdependency with a standard Java collection.
612-612: Correct replacement - LinkedHashMap preserves order.The change from
ArrayOrderedMaptoLinkedHashMapis appropriate. Both preserve insertion order, and since keys are explicitly sorted on line 616, the behavior remains identical.Note: Since keys are sorted explicitly, a plain
HashMapwould also work here, butLinkedHashMapis a safe choice that maintains predictable iteration order.entitybroker/core-providers/src/java/org/sakaiproject/entitybroker/providers/model/EntityGroup.java (3)
27-28: LGTM! Jackson imports are correct.The import statements correctly reference Jackson annotations for JSON serialization control, aligning with the migration from reflectutils to Jackson-based serialization.
54-54: LGTM! JSON exclusion list is appropriate.The
@JsonIgnorePropertiesannotation correctly excludes complex nested objects and delegated methods from JSON serialization. All specified field names correspond to methods that return complex types (User, Site, ResourceProperties, Set, etc.) that should not be serialized.
359-360: LGTM! XML method correctly excluded from JSON.The
@JsonIgnoreannotation appropriately excludes thetoXmlmethod from JSON serialization, as this method returns an XML DOM Element which is incompatible with JSON output.entitybroker/core-providers/src/java/org/sakaiproject/entitybroker/providers/model/EntitySitePage.java (2)
25-25: LGTM! Jackson import added correctly.The import for
@JsonIgnorePropertiesis appropriate for the migration from reflectutils to Jackson-based serialization control.
42-42: Migration to Jackson annotation looks correct.The replacement of
@ReflectIgnoreClassFieldswith@JsonIgnorePropertiesmaintains the same set of ignored fields and is semantically equivalent for JSON serialization/deserialization purposes. The formatting with consistent spacing is also an improvement.Note: Ensure that no other parts of the codebase rely on reflectutils-based reflection operations on this class that might have respected the old annotation, as Jackson's
@JsonIgnorePropertiesonly controls JSON serialization behavior.Based on learnings: Jackson is the industry-standard library for JSON serialization and the latest stable version is 2.20.0 with improved features and security fixes.
kernel/kernel-impl/src/main/java/org/sakaiproject/tool/impl/RebuildBreakdownServiceImpl.java (4)
26-28: LGTM: Import additions support reflectutils removal.The new imports (URI, URL, TemporalAccessor, ClassUtils) are correctly added to support the expanded simple-type checking and primitive/wrapper detection, replacing the removed ConstructorUtils dependency.
Also applies to: 38-38
80-90: LGTM: Comprehensive simple-type enumeration.The SIMPLE_ASSIGNABLE_TYPES list appropriately covers common serializable types including strings (CharSequence), numbers (Number), dates (Date, Calendar, TemporalAccessor), locales, URIs, URLs, and UUIDs. The use of TemporalAccessor is particularly good as it covers all java.time types (LocalDate, LocalDateTime, Instant, etc.).
886-900: LGTM: Robust type checking implementation.The enhanced
isObjectSimplemethod correctly:
- Returns false for nulls (line 887-888)
- Identifies primitives, wrappers, and enums via ClassUtils (line 891-893)
- Checks assignability against the expanded SIMPLE_ASSIGNABLE_TYPES list (lines 894-898)
This is a clear improvement over the previous ConstructorUtils-based approach, providing explicit and maintainable type checking.
907-914: LGTM: Standard Map detection replaces utility dependency.The refactored
isObjectMapcorrectly uses the standardMap.class.isAssignableFrom(clazz)check, replacing the ConstructorUtils dependency. This is the idiomatic Java approach for type checking and will correctly identify any Map implementation.entitybroker/core-providers/src/java/org/sakaiproject/entitybroker/providers/UserEntityProvider.java (4)
18-18: LGTM! Appropriate replacement of reflectutils with Spring utilities.The migration from the reflectutils library to Spring's ReflectionUtils and standard Java reflection is appropriate and reduces external dependencies.
Also applies to: 55-55
382-405: LGTM! Clean migration from reflectutils to custom helpers.The refactored logic correctly:
- Parses the config string via
parseBooleanwith appropriate exception handling- Falls back to reading the private field via
readBooleanField- Defaults to
Boolean.FALSEif both approaches failThe exception handling is appropriate for the new implementation.
407-425: Consider the security implications of accessing private fields.The implementation correctly uses Spring's
ReflectionUtilsand handles multiple value types appropriately. However, using reflection to access private fields (m_separateIdEid) can have security implications:
- Reflection bypasses normal encapsulation and access controls
- In environments with a security manager, this may fail
- It creates tight coupling to implementation details that may change
This is acceptable for this edge-case workaround (as noted in the comment on line 378), but ensure that:
- This code path is truly necessary and cannot be avoided with proper configuration
- The field name
m_separateIdEidis stable across Sakai versionsConsider adding a comment in the
readBooleanFieldmethod explaining why reflection is necessary and which field it's intended to access.
427-442: LGTM! Robust boolean parsing with clear error handling.The implementation correctly:
- Handles null and empty inputs by returning null
- Performs case-insensitive matching for "true"/"false"
- Throws
IllegalArgumentExceptionfor invalid values, providing clear feedbackThis is more explicit and safer than
Boolean.valueOf(), which silently returnsfalsefor any non-"true" string.entitybroker/core-providers/src/java/org/sakaiproject/entitybroker/providers/model/EntityUser.java (3)
24-25: LGTM!The Jackson imports are correct and necessary for the annotations used in the class.
46-46: LGTM!The Jackson annotation correctly excludes fields that would be problematic for JSON serialization (User objects and ResourceProperties). The migration from reflectutils maintains the same exclusion behavior.
304-310: LGTM!The
@JsonIgnoreannotation is appropriate for thetoXmlmethod, as it returns an XML DOM Element that cannot be JSON-serialized and may throwUnsupportedOperationException.entitybroker/rest/src/test/org/sakaiproject/entitybroker/rest/EntityEncodingManagerTest.java (8)
29-29: LGTM!The addition of the
LinkedHashMapimport is appropriate for replacing the customArrayOrderedMapwith a standard Java collection.
110-110: LGTM!Replacing
ArrayOrderedMapwithLinkedHashMapis appropriate here.LinkedHashMapmaintains insertion order, which is essential for predictable test assertions on encoded output.
151-151: LGTM!The use of
LinkedHashMaphere correctly maintains insertion order for testing encoding behavior with special characters and spaces.
156-162: LGTM!The updated comment clearly explains that the legacy XML encoder (which used reflectutils) couldn't handle spaces in keys, but the new implementation resolves this limitation. The preserved commented-out code provides useful context for the behavior change.
368-368: LGTM!The replacement with
LinkedHashMapis appropriate for JSON encoding tests where consistent key ordering helps ensure predictable output.
376-376: LGTM!Consistent use of
LinkedHashMapfor the nested map structure in the JSON encoding test.
423-423: LGTM!The use of
LinkedHashMapis appropriate for XML encoding tests where maintaining insertion order ensures consistent and testable output.
431-431: LGTM!Consistent use of
LinkedHashMapfor the nested map in the XML encoding test completes the migration fromArrayOrderedMap.entitybroker/rest/src/java/org/sakaiproject/entitybroker/rest/EntityBatchHandler.java (4)
27-29: LGTM!The import changes correctly replace reflectutils dependencies with standard Java collections (
Arrays,LinkedHashMap).
373-373: LGTM!Replacing
ArrayUtils.arrayToString(refs)withArrays.toString(refs)is correct and aligns with using standard Java APIs.
739-739: LGTM!Changing the parameter type from
HashMap<String, String>toMap<String, String>improves the method signature by programming to an interface rather than a concrete implementation.
775-775: LGTM!The parameter type change from
HashMap<String, String>toMap<String, String>is consistent with the other method signature improvements in this file.entitybroker/impl/src/java/org/sakaiproject/entitybroker/impl/entityprovider/EntityProviderManagerImpl.java (5)
24-24: LGTM!The new imports for
ArrayDeque,Deque,LinkedHashSet, andConcurrentHashMapare appropriate and necessary for the refactored storage maps and the newgetAllTypeshelper method.Also applies to: 28-28, 32-32, 38-38
359-359: LGTM!The iterator type correctly aligns with the new
ConcurrentHashMap<String, EntityProviderListener<?>>type forlistenerMap.
614-637: LGTM!The
getAllTypeshelper method correctly implements iterative type hierarchy traversal using a stack-based approach. The use ofLinkedHashSetpreserves insertion order while avoiding duplicates, and the iterative approach prevents potential stack overflow issues on deep hierarchies. The null checks andObject.classtermination logic are appropriate.
603-603: LGTM!The replacement of the reflectutils utility call with the new
getAllTypeshelper method correctly maintains the capability extraction logic while removing the external dependency.
545-545: LGTM!The iterator type correctly aligns with the new
ConcurrentHashMap<String, EntityProviderListener<?>>type forlistenerMap.entitybroker/core-providers/src/java/org/sakaiproject/entitybroker/providers/SiteEntityProvider.java (2)
18-18: LGTM!The import changes appropriately replace the reflectutils library with Spring's ReflectionUtils and standard Java reflection, aligning with the PR objective to remove the reflectutils dependency.
Also applies to: 92-92
871-871: LGTM!The call sites appropriately validate the user ID before invoking the helper method, and the centralized error handling via
setCreatedUserIdimproves code maintainability compared to inline reflection calls.Also applies to: 1109-1109
entitybroker/utils/src/java/org/sakaiproject/entitybroker/util/access/AccessProviderManagerImpl.java (4)
23-35: LGTM! Solid concurrent registry pattern.The migration from the old reference map to
ConcurrentHashMap<String, WeakReference<T>>is appropriate. Making the fieldfinaland usingWeakReferenceallows providers to be garbage-collected when no longer referenced elsewhere, whileConcurrentHashMapensures thread-safe access.
37-42: LGTM! Proper validation and WeakReference wrapping.The null checks and
IllegalArgumentExceptionare appropriate for a registration method. Wrapping the provider inWeakReferencebefore storing is correct.
57-61: LGTM! Safe unconditional removal.The null check and unconditional removal are appropriate for this variant. The
removeoperation is atomic.
63-76: LGTM! Correct WeakReference dereferencing with stale entry cleanup.The implementation properly handles null prefixes, missing entries, and cleared references. Line 73 correctly uses the atomic
remove(prefix, reference)to ensure you only remove the stale entry if no concurrent update occurred. This defensive cleanup prevents memory leaks from accumulating clearedWeakReferenceentries.entitybroker/utils/src/java/org/sakaiproject/entitybroker/util/core/EntityProviderMethodStoreImpl.java (1)
26-26: Imports are appropriate.Arrays and Collectors are correctly introduced for stringifyClassArray.
Also applies to: 34-34
entitybroker/utils/src/java/org/sakaiproject/entitybroker/util/EntityDataUtils.java (8)
24-26: LGTM!The new reflection imports are appropriate for the custom reflection implementation replacing the reflectutils library.
483-495: LGTM!The hierarchical field search correctly walks the class hierarchy and handles null inputs appropriately.
497-509: LGTM!The field lookup by name correctly handles the class hierarchy and exception flow.
511-523: LGTM!The method search correctly filters for zero-parameter methods (appropriate for accessors) and walks the class hierarchy.
525-535: LGTM!The accessor method discovery correctly follows JavaBean naming conventions and uses
Locale.ENGLISHto avoid locale-specific capitalization issues.
537-552: LGTM!The method lookup correctly handles the class hierarchy, verifies zero parameters, and handles exceptions appropriately.
594-616: LGTM!The property name extraction and decapitalization correctly follow the JavaBean specification, including the two-uppercase-character rule for acronyms. The use of
Locale.ENGLISHis appropriate.
554-592: No changes required for reflection utilities
Project baseline is Java 17, soField.canAccess/Method.canAccessare supported. SecurityManager is deprecated in Java 17—wrappingsetAccessible(false)in a catch block isn’t necessary.entitybroker/impl/src/java/org/sakaiproject/entitybroker/impl/EntityBrokerManagerImpl.java (1)
23-27: Verify runtime availability of java.beans (java.desktop module).Using Introspector/BeanInfo requires the java.desktop module on Java 17+. In modular/jlinked runtimes this may be absent.
Please confirm Sakai’s runtime includes java.desktop. If not guaranteed, we can fall back to method/field scanning only or use Spring BeanWrapper (still uses java.beans) explicitly. Based on learnings.
Also applies to: 769-795
entitybroker/impl/src/java/org/sakaiproject/entitybroker/impl/event/EventReceiverCoordinator.java (1)
23-25: Imports look correct and necessary for the new snapshot+weak map approach.Also applies to: 29-29
entitybroker/rest/src/java/org/sakaiproject/entitybroker/rest/EntityDescriptionManager.java (4)
1094-1096: Verify FORM handling source for makeFormUrlHtmlmakeFormUrlHtml checks containsFormat(formats, Formats.FORM), but callers pass outputFormats. Ensure FORM is meant to be discovered from outputs; otherwise pass inputFormats instead.
1164-1175: Type classification helpers look correctSimple/collection/array/map detection and array component extraction are solid and cover java.time types.
Also applies to: 1177-1187, 1189-1198
1152-1162: containsFormat helper is fineNull-safe and avoids allocations. Good.
1037-1051: Remove AccessFormats aggregation from getFormats to avoid duplicate listings
The block merging AccessFormats intogetFormatscauses provider formats to appear both in input/output lists and again under<accessProvider>/<accessFormats>. Either drop this merge or, if you intended a unified list, remove the separate accessFormats section. Please confirm the desired behavior.entitybroker/utils/src/java/org/sakaiproject/entitybroker/util/request/RequestStorageImpl.java (1)
121-128: Fix isAssignableFrom argument order to enable fast-path cast
Replaceif (value.getClass().isAssignableFrom(type)) {with
if (type.isAssignableFrom(value.getClass())) {entitybroker/rest/src/java/org/sakaiproject/entitybroker/rest/EntityHandlerImpl.java (3)
28-29: Imports look good.Field and Method imports are needed for the new helpers.
1069-1074: Good: annotation/property lookup for lastModified.Leveraging property name, then annotated field/method is reasonable and preserves backward compatibility.
Please confirm if annotated method should take precedence when both property and annotation exist. Current order prefers property.
482-482: Confirm contains(...) semantics match legacy ReflectUtils.contains
The newcontainsreturns false for null or empty arrays and only does exact, case-sensitive matches. This may alter behavior at the call sites in EntityHandlerImpl.java (lines 482, 524, 647, 926, 934). Verify no providers depend on null/empty meaning “allow all” or on case-insensitive/wildcard matching.entitybroker/rest/src/java/org/sakaiproject/entitybroker/rest/EntityEncodingManager.java (1)
114-135: Good move: centralized, preconfigured Jackson mappers via MapperFactoryStatic, reusable mappers/writers with sensible defaults (ignore unknowns, exclude nulls, Java time, no timestamps) align with best practices. Keep Jackson modules aligned on the same version across the repo.
...mpl/turnitin/src/main/java/org/sakaiproject/contentreview/turnitin/util/TurnitinAPIUtil.java
Outdated
Show resolved
Hide resolved
...mpl/turnitin/src/main/java/org/sakaiproject/contentreview/turnitin/util/TurnitinAPIUtil.java
Outdated
Show resolved
Hide resolved
...broker/core-providers/src/java/org/sakaiproject/entitybroker/providers/model/EntityUser.java
Outdated
Show resolved
Hide resolved
...oker/core-providers/src/java/org/sakaiproject/entitybroker/providers/SiteEntityProvider.java
Outdated
Show resolved
Hide resolved
entitybroker/impl/src/java/org/sakaiproject/entitybroker/impl/EntityBrokerManagerImpl.java
Show resolved
Hide resolved
entitybroker/rest/src/java/org/sakaiproject/entitybroker/rest/EntityHandlerImpl.java
Show resolved
Hide resolved
...oker/utils/src/java/org/sakaiproject/entitybroker/util/access/AccessProviderManagerImpl.java
Show resolved
Hide resolved
...tybroker/utils/src/java/org/sakaiproject/entitybroker/util/devhelper/DeveloperBeanUtils.java
Outdated
Show resolved
Hide resolved
...tybroker/utils/src/java/org/sakaiproject/entitybroker/util/devhelper/DeveloperBeanUtils.java
Show resolved
Hide resolved
...tybroker/utils/src/java/org/sakaiproject/entitybroker/util/devhelper/DeveloperBeanUtils.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
entitybroker/rest/src/java/org/sakaiproject/entitybroker/rest/EntityHandlerImpl.java (1)
1222-1258: Harden reflection helpers against module access errors.The past review comment about handling module access errors in
readFieldValueandinvokeMethodremains unresolved. On JDK 17+,setAccessible(true)can throwInaccessibleObjectExceptionorSecurityExceptionwhen attempting to access module-restricted members, which would propagate as a 500 error.Apply defensive handling:
private static Object readFieldValue(Field field, Object target) { if (field == null || target == null) { return null; } boolean accessible = field.canAccess(target); + boolean madeAccessible = false; try { if (!accessible) { - field.setAccessible(true); + try { + field.setAccessible(true); + madeAccessible = true; + } catch (SecurityException | RuntimeException ex) { + return null; + } } return field.get(target); } catch (IllegalAccessException e) { return null; } finally { - if (!accessible) { - field.setAccessible(false); + if (madeAccessible) { + try { field.setAccessible(false); } catch (Exception ignore) { } } } } private static Object invokeMethod(Method method, Object target) { if (method == null || target == null) { return null; } boolean accessible = method.canAccess(target); + boolean madeAccessible = false; try { if (!accessible) { - method.setAccessible(true); + try { + method.setAccessible(true); + madeAccessible = true; + } catch (SecurityException | RuntimeException ex) { + return null; + } } return method.invoke(target); } catch (ReflectiveOperationException e) { return null; } finally { - if (!accessible) { - method.setAccessible(false); + if (madeAccessible) { + try { method.setAccessible(false); } catch (Exception ignore) { } } } }entitybroker/rest/src/java/org/sakaiproject/entitybroker/rest/EntityEncodingManager.java (1)
125-136: XXE hardening still needed for XML parsing.As noted in the previous review, the XmlMapper should have DTD and external entity processing disabled for defense-in-depth against XXE attacks. The MapperFactory.xmlBuilder() should configure the underlying XMLInputFactory accordingly.
Please ensure the following properties are set in
MapperFactory.XmlMapperBuilderbefore callingbuild():xmlMapper.getFactory().getXMLInputFactory() .setProperty(XMLInputFactory.SUPPORT_DTD, false); xmlMapper.getFactory().getXMLInputFactory() .setProperty(XMLInputFactory.IS_SUPPORTING_EXTERNAL_ENTITIES, false);Based on past review comments.
entitybroker/utils/src/java/org/sakaiproject/entitybroker/util/devhelper/DeveloperBeanUtils.java (2)
83-90: Handle primitives and null in convert.The
convertmethod usestype.cast(converted)(line 89) without resolving primitives to their wrapper types or validating null, which can causeClassCastExceptionfor primitive target types or whenconvertedis null.Apply this diff to handle primitives and null safely:
public static <T> T convert(Object value, Class<T> type) { if (type == null) { throw new IllegalArgumentException("Target type must be provided"); } + Class<?> boxed = org.springframework.util.ClassUtils.resolvePrimitiveIfNecessary(type); IdentityHashMap<Object, Boolean> visited = new IdentityHashMap<>(); - Object converted = convertValue(value, type, normalizeDepth(0) - 1, visited); - return type.cast(converted); + Object converted = convertValue(value, boxed, normalizeDepth(0) - 1, visited); + if (converted == null) { + return null; + } + if (!boxed.isInstance(converted)) { + throw new IllegalArgumentException("Converted value is not of requested type: " + boxed + " → " + converted.getClass()); + } + @SuppressWarnings("unchecked") + T result = (T) converted; + return result; }
192-223: Use target property type in copyBeanInternal.The
copyBeanInternalmethod uses the source descriptor's property type (line 216) for conversion, which can cause type mismatches when the target property has a different type (e.g., wider or narrower type).Apply this diff to use the target property type:
Object value = sourceWrapper.getPropertyValue(name); if (value == null && ignoreNulls) { continue; } - Object converted = convertValue(value, descriptor.getPropertyType(), allowedDepth - 1, visited); + Class<?> targetType = targetWrapper.getPropertyType(name); + Object converted = convertValue(value, targetType, allowedDepth - 1, visited); try { targetWrapper.setPropertyValue(name, converted); } catch (BeansException e) {
🧹 Nitpick comments (1)
entitybroker/utils/src/java/org/sakaiproject/entitybroker/util/devhelper/AbstractDeveloperHelperService.java (1)
430-458: LGTM – JsonCodec with pretty-print heuristic.The
JsonCodecimplementation is correct. The pretty-print heuristic (line 450) applies formatting only to maps with more than one entry, which is a reasonable optimization.Optionally, you could make the pretty-print threshold configurable or document the heuristic if clarity is desired.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
JDK21-migration-plan.md(1 hunks)entitybroker/core-providers/src/java/org/sakaiproject/entitybroker/providers/model/EntityUser.java(4 hunks)entitybroker/impl/src/java/org/sakaiproject/entitybroker/impl/EntityBrokerManagerImpl.java(3 hunks)entitybroker/rest/pom.xml(1 hunks)entitybroker/rest/src/java/org/sakaiproject/entitybroker/rest/EntityEncodingManager.java(16 hunks)entitybroker/rest/src/java/org/sakaiproject/entitybroker/rest/EntityHandlerImpl.java(10 hunks)entitybroker/utils/pom.xml(1 hunks)entitybroker/utils/src/java/org/sakaiproject/entitybroker/util/core/EntityProviderMethodStoreImpl.java(3 hunks)entitybroker/utils/src/java/org/sakaiproject/entitybroker/util/devhelper/AbstractDeveloperHelperService.java(7 hunks)entitybroker/utils/src/java/org/sakaiproject/entitybroker/util/devhelper/DeveloperBeanUtils.java(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.java
📄 CodeRabbit inference engine (.cursor/rules/logging-rule.mdc)
**/*.java: Use SLF4J parameterized logging (e.g., logger.info("Value is: {}", value)) instead of string concatenation in log statements
Write log messages and code comments in English; never translate log messages
Files:
entitybroker/core-providers/src/java/org/sakaiproject/entitybroker/providers/model/EntityUser.javaentitybroker/rest/src/java/org/sakaiproject/entitybroker/rest/EntityHandlerImpl.javaentitybroker/utils/src/java/org/sakaiproject/entitybroker/util/core/EntityProviderMethodStoreImpl.javaentitybroker/utils/src/java/org/sakaiproject/entitybroker/util/devhelper/DeveloperBeanUtils.javaentitybroker/utils/src/java/org/sakaiproject/entitybroker/util/devhelper/AbstractDeveloperHelperService.javaentitybroker/impl/src/java/org/sakaiproject/entitybroker/impl/EntityBrokerManagerImpl.javaentitybroker/rest/src/java/org/sakaiproject/entitybroker/rest/EntityEncodingManager.java
**/pom.xml
📄 CodeRabbit inference engine (AGENTS.md)
Target Java 17 for trunk builds (update maven-compiler-plugin source/target accordingly)
Files:
entitybroker/utils/pom.xmlentitybroker/rest/pom.xml
🧬 Code graph analysis (4)
entitybroker/core-providers/src/java/org/sakaiproject/entitybroker/providers/model/EntityUser.java (1)
entitybroker/core-providers/src/java/org/sakaiproject/entitybroker/providers/model/EntitySitePage.java (1)
JsonIgnoreProperties(42-385)
entitybroker/utils/src/java/org/sakaiproject/entitybroker/util/devhelper/DeveloperBeanUtils.java (2)
entitybroker/utils/src/java/org/sakaiproject/entitybroker/util/devhelper/AbstractDeveloperHelperService.java (1)
Slf4j(59-549)entitybroker/utils/src/java/org/sakaiproject/entitybroker/util/request/RequestStorageImpl.java (1)
SuppressWarnings(45-291)
entitybroker/utils/src/java/org/sakaiproject/entitybroker/util/devhelper/AbstractDeveloperHelperService.java (1)
kernel/api/src/main/java/org/sakaiproject/serialization/MapperFactory.java (1)
MapperFactory(36-422)
entitybroker/rest/src/java/org/sakaiproject/entitybroker/rest/EntityEncodingManager.java (1)
kernel/api/src/main/java/org/sakaiproject/serialization/MapperFactory.java (1)
MapperFactory(36-422)
🪛 GitHub Actions: Maven Build
entitybroker/rest/src/java/org/sakaiproject/entitybroker/rest/EntityHandlerImpl.java
[error] 59-59: Initialization failed because org.sakaiproject.entitybroker.rest.EntityEncodingManager cannot be loaded.
entitybroker/rest/src/java/org/sakaiproject/entitybroker/rest/EntityEncodingManager.java
[error] 116-116: Caused by missing Jackson JavaTimeModule class; causes initialization failure of EntityEncodingManager.
[error] 116-116: Class com.fasterxml.jackson.datatype.jsr310.JavaTimeModule is missing, breaking static initializer.
[error] 116-116: Initialization failure for EntityEncodingManager due to missing dependency on com.fasterxml.jackson.datatype.jsr310.JavaTimeModule.
[error] 116-116: Test setup failure caused by static initializer failure in EntityEncodingManager.
[error] 116-116: Class org.sakaiproject.entitybroker.rest.EntityEncodingManager could not be initialized because the required Jackson class is missing.
⏰ 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: Codacy Static Code Analysis
- GitHub Check: maven-build
- GitHub Check: sakai-deploy
🔇 Additional comments (47)
entitybroker/core-providers/src/java/org/sakaiproject/entitybroker/providers/model/EntityUser.java (4)
24-25: LGTM!The Jackson imports are correctly added and necessary for the annotations used in this file.
46-46: LGTM!The class-level
@JsonIgnorePropertiesannotation correctly excludescreatedBy,modifiedBy, andpropertiesfrom JSON serialization, consistent with similar model classes in this PR (e.g., EntitySitePage).
209-209: Past review concern addressed.The comment has been updated to correctly reflect that fields are ignored via
@JsonIgnoreProperties, addressing the previous TODO. The informal wording is acceptable for internal documentation.
303-309: LGTM!The
@JsonIgnoreannotation correctly excludes thetoXmlmethod from JSON serialization. This is necessary because the method returns a DOMElementthat is not JSON-serializable and is intended for XML conversion only.entitybroker/rest/src/java/org/sakaiproject/entitybroker/rest/EntityHandlerImpl.java (6)
28-29: LGTM!The new reflection imports are appropriate for the custom reflection helpers that replace ReflectUtils.
1109-1119: LGTM!The
containshelper is a straightforward replacement for the ReflectUtils equivalent, with proper null checks.
481-481: LGTM!All
containshelper usages correctly replace the previous ReflectUtils calls and maintain the same logic.Also applies to: 523-523, 646-646, 924-924, 933-933
830-839: LGTM!Explicit typing of
HttpRESTUtils.Methodimproves code clarity without changing functionality.
1053-1081: LGTM!The integration of
findLastModifiedValuecorrectly replaces the ReflectUtils-based lastModified discovery while maintaining the same fallback logic.
59-59: No ReflectUtil references in EntityEncodingManager; EntityHandlerImpl.java is unaffected by reflectutils removal.Likely an incorrect or invalid review comment.
entitybroker/utils/src/java/org/sakaiproject/entitybroker/util/core/EntityProviderMethodStoreImpl.java (3)
26-34: LGTM!The new imports support the
stringifyClassArrayhelper method and align with the migration away from ReflectUtils.
156-157: LGTM!The change correctly uses parameterized logging and creates a defensive copy of the actions map to avoid holding objects from another ClassLoader.
367-374: LGTM!The
stringifyClassArrayhelper correctly replacesReflectUtils.arrayToStringwith a clean stream-based implementation that produces the same format.entitybroker/utils/pom.xml (1)
53-63: LGTM!The addition of Jackson and Commons Text dependencies aligns with the PR's goal of removing reflectutils. These dependencies are appropriately managed through the parent POM.
entitybroker/rest/pom.xml (1)
57-63: LGTM!The Jackson dependencies are required for the EntityEncodingManager refactor and align with the PR's migration strategy.
entitybroker/rest/src/java/org/sakaiproject/entitybroker/rest/EntityEncodingManager.java (12)
1138-1144: LGTM!The
readInputStreamimplementation correctly usesreadAllBytes()with UTF-8 charset and wraps IOException appropriately.
1131-1136: LGTM!The
containshelper correctly replaces the removed ReflectUtils array checking functionality with a simple Arrays.asList approach.
1234-1280: LGTM!The
analyzePropertiesmethod correctly uses Java Introspector to analyze bean properties and combines it with direct field inspection. The handling of public fields and theEntityFieldRequiredannotation is appropriate.
1282-1313: LGTM!The
getObjectValuesmethod correctly extracts property values using BeanWrapper and supplements with direct field access for public fields.
908-966: LGTM!The serialization depth limiting and cycle detection implementation is solid:
- Uses
IdentityHashMapfor reference-based cycle detection- Correctly handles maps, collections, arrays, and beans recursively
- Respects the configured depth limit
- Summarizes values that exceed depth or are detected as cycles
1146-1160: LGTM!The
populateBeanFromParamsmethod correctly uses Spring'sBeanWrapperImplfor type-safe property binding from HTTP parameters.
1163-1198: LGTM!The
populateBeanFromMapmethod correctly handles nested object population by:
- Detecting when a Map value should become a nested bean
- Attempting to instantiate and populate nested objects recursively
- Falling back to direct field access when BeanWrapper properties aren't writable
1212-1224: LGTM!The
setFieldValuemethod correctly uses reflection to set fields that aren't accessible via BeanWrapper, with appropriate type conversion and error handling.
1315-1336: LGTM!The
isBeanClassmethod correctly identifies beans by excluding primitives, arrays, collections, maps, numbers, strings, dates, and enums, and verifying the presence of a no-arg constructor.
1009-1037: LGTM!The encoding helpers correctly:
- Merge properties into the data envelope
- Support optional root name wrapping for JSON/XML
- Use appropriate Jackson writers
- HTML-encode the output for HTML format with proper escaping
1072-1084: LGTM!The
sanitizeJsonCallbackmethod correctly validates JSONP callback names against a strict regex pattern (alphanumeric + underscore + dollar + dots) and adds the/**/prefix for security.
1099-1129: LGTM!The
decodeDatamethod correctly:
- Handles empty/null input gracefully
- Strips JSONP wrapper when needed
- Uses Jackson mappers for JSON and XML parsing
- Wraps parse failures in appropriate exceptions
- Falls back to wrapping raw HTML data in a map
entitybroker/utils/src/java/org/sakaiproject/entitybroker/util/devhelper/AbstractDeveloperHelperService.java (9)
83-93: LGTM – Clear null-safe resolution.The refactored
getEntityRESTProviderlogic is correct and improves readability with explicit resolution and weak reference handling.
108-127: LGTM – Codec-based decoding.The replacement of the legacy transcoder with the new codec system is correct and improves error messaging.
132-151: LGTM – Codec-based encoding.The encoding logic correctly delegates to the new codec system with improved error messages.
351-375: LGTM – Clean delegation to DeveloperBeanUtils.The bean utility methods correctly delegate to the new
DeveloperBeanUtilsstatic methods, aligning with the refactor goal.
377-390: LGTM – Thread-safe lazy codec initialization.The
getCodecmethod correctly uses aConcurrentHashMapfor thread-safe lazy initialization of codecs, with appropriate error handling for unsupported formats.
402-426: LGTM – Well-configured Jackson mappers.The static
JSON_MAPPERandXML_MAPPERare correctly configured viaMapperFactorywith secure defaults (ignore unknown properties, exclude nulls, no default typing, JavaTime support).
460-485: LGTM – XmlCodec implementation.The
XmlCodeccorrectly uses the configuredXML_MAPPERfor both encoding and decoding with appropriate exception handling.
487-524: LGTM – HtmlCodec with XSS protection.The
HtmlCodecdecode method is minimal (passthrough) and encode usesStringEscapeUtils.escapeHtml4for XSS protection on all user-controlled inputs (name, data, properties).
526-547: LGTM – preparePayload logic.The
preparePayloadhelper correctly wraps data and properties into a structured map, with null-safe key handling when merging map entries.entitybroker/utils/src/java/org/sakaiproject/entitybroker/util/devhelper/DeveloperBeanUtils.java (11)
39-50: LGTM – cloneBean with cycle detection.The
cloneBeanmethod correctly instantiates a copy and delegates tocopyBeanInternalwith anIdentityHashMapfor cycle detection, addressing the recursion concern from previous reviews.
52-56: LGTM – copyBean with cycle detection.The
copyBeanmethod correctly passes avisitedmap tocopyBeanInternalfor cycle detection.
58-81: LGTM – populate with target type conversion.The
populatemethod correctly retrieves the target property type (line 71) and passes it toconvertValue(line 72), addressing the concern from previous reviews about using the correct type for conversion.
92-94: LGTM – Depth normalization prevents unbounded recursion.The
normalizeDepthmethod correctly caps depth atDEFAULT_CLONE_DEPTH(4) whenmaxDepth <= 0, addressing the unbounded recursion concern from previous reviews.
96-103: LGTM – toSkipSet handles nulls.The
toSkipSetmethod correctly filters null entries and returns an immutable set.
105-111: LGTM – instantiate with clear error handling.The
instantiatemethod correctly uses SpringBeanUtils.instantiateClasswith appropriate exception wrapping.
113-143: LGTM – convertValue with array handling and cycle detection.The
convertValuemethod correctly handlesString[]and general array unwrapping, delegates to Spring'sconvertIfNecessary, and clones nested values with cycle detection.
145-178: LGTM – cloneNestedIfNeeded with cycle detection and depth control.The
cloneNestedIfNeededmethod correctly implements cycle detection (lines 149-152) and depth control, with appropriate handling for collections, maps, arrays, and beans.
180-182: LGTM – isSimpleValue includes Enum check.The
isSimpleValuemethod correctly uses Spring'sisSimpleValueTypeand includesEnumtypes.
184-190: LGTM – convertArray with visited map.The
convertArraymethod correctly converts each element withconvertValueand passes thevisitedmap for cycle detection.
225-233: LGTM – instantiateCollection with fallback.The
instantiateCollectionmethod correctly attempts to instantiate the template's collection class and falls back toArrayListon failure.
entitybroker/impl/src/java/org/sakaiproject/entitybroker/impl/EntityBrokerManagerImpl.java
Show resolved
Hide resolved
entitybroker/rest/src/java/org/sakaiproject/entitybroker/rest/EntityEncodingManager.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
entitybroker/impl/src/java/org/sakaiproject/entitybroker/impl/EntityBrokerManagerImpl.java (1)
798-808: Restore interface method scanning for@EntityTitle.Line 799:
getAllMethods()only walks the concrete superclass chain, so interface-level@EntityTitleannotations no longer surface. Entities that expose titles via interface contracts (common with proxies) now lose their display titles. Please reinstate interface traversal (with cycle guards) before merging.-import java.util.ArrayList; -import java.util.Collections; -import java.util.HashMap; -import java.util.List; -import java.util.Map; +import java.util.ArrayList; +import java.util.Collections; +import java.util.HashMap; +import java.util.HashSet; +import java.util.List; +import java.util.Map; +import java.util.Set; @@ - private List<Method> getAllMethods(Class<?> type) { - List<Method> methods = new ArrayList<Method>(); - Class<?> current = type; - while (current != null && !Object.class.equals(current)) { - for (Method method : current.getDeclaredMethods()) { - methods.add(method); - } - current = current.getSuperclass(); - } - return methods; - } + private List<Method> getAllMethods(Class<?> type) { + List<Method> methods = new ArrayList<>(); + collectMethods(type, methods, new HashSet<>()); + return methods; + } + + private void collectMethods(Class<?> type, List<Method> methods, Set<Class<?>> visited) { + if (type == null || Object.class.equals(type) || !visited.add(type)) { + return; + } + try { + for (Method method : type.getDeclaredMethods()) { + methods.add(method); + } + } catch (SecurityException | java.lang.reflect.InaccessibleObjectException ignore) { + // keep scanning siblings + } + collectMethods(type.getSuperclass(), methods, visited); + for (Class<?> iface : type.getInterfaces()) { + collectMethods(iface, methods, visited); + } + }entitybroker/rest/src/java/org/sakaiproject/entitybroker/rest/EntityEncodingManager.java (1)
143-155: Harden the XML mapper against XXEWe still expose the default
XmlMapperconfiguration, which leaves DTDs and external entities enabled and makesdecodeData(..., Formats.XML)vulnerable to XXE. Please disable both on the underlyingXMLInputFactory(or viaMapperFactory) before this ships.+import javax.xml.stream.XMLInputFactory; @@ - private static final ObjectWriter XML_WRITER = XML_MAPPER.writer(); + private static final ObjectWriter XML_WRITER = XML_MAPPER.writer(); + static { + var xmlInputFactory = XML_MAPPER.getFactory().getXMLInputFactory(); + xmlInputFactory.setProperty(XMLInputFactory.SUPPORT_DTD, Boolean.FALSE); + xmlInputFactory.setProperty("javax.xml.stream.isSupportingExternalEntities", Boolean.FALSE); + }
🧹 Nitpick comments (2)
entitybroker/rest/src/java/org/sakaiproject/entitybroker/rest/EntityHandlerImpl.java (1)
831-840: Consider simplifying redundant method assignments.The switch to
HttpRESTUtils.Methodenum is good, but the repeated assignments tomethodare redundant:Apply this diff:
- HttpRESTUtils.Method method = HttpRESTUtils.Method.GET; + HttpRESTUtils.Method method; if (EntityView.VIEW_DELETE.equals(ev.getViewKey())) { method = HttpRESTUtils.Method.DELETE; } else if (EntityView.VIEW_EDIT.equals(ev.getViewKey())) { method = HttpRESTUtils.Method.PUT; } else if (EntityView.VIEW_NEW.equals(ev.getViewKey())) { method = HttpRESTUtils.Method.POST; } else { method = HttpRESTUtils.Method.GET; }entitybroker/rest/src/test/org/sakaiproject/entitybroker/rest/jetty/EntityBrokerServletTest.java (1)
677-706: Consider re-enabling or removing the commented test.The test
testBatchAllInvalidJSONhas been commented out with a TODO note about calling other webapps. The assertion was updated to match the new format even while commented. Consider either:
- Fixing and re-enabling the test if the forward/include limitation has been resolved
- Removing the commented code if it's no longer relevant
- Opening an issue to track re-enabling this test coverage
Do you want me to open a new issue to track re-enabling this test?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
entitybroker/impl/src/java/org/sakaiproject/entitybroker/impl/EntityBrokerManagerImpl.java(4 hunks)entitybroker/rest/pom.xml(1 hunks)entitybroker/rest/src/java/org/sakaiproject/entitybroker/rest/EntityBatchHandler.java(9 hunks)entitybroker/rest/src/java/org/sakaiproject/entitybroker/rest/EntityEncodingManager.java(16 hunks)entitybroker/rest/src/java/org/sakaiproject/entitybroker/rest/EntityHandlerImpl.java(11 hunks)entitybroker/rest/src/test/org/sakaiproject/entitybroker/rest/EntityEncodingManagerTest.java(6 hunks)entitybroker/rest/src/test/org/sakaiproject/entitybroker/rest/jetty/EntityBrokerServletTest.java(7 hunks)entitybroker/utils/pom.xml(1 hunks)entitybroker/utils/src/java/org/sakaiproject/entitybroker/util/EntityDataUtils.java(5 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.java
📄 CodeRabbit inference engine (.cursor/rules/logging-rule.mdc)
**/*.java: Use SLF4J parameterized logging (e.g., logger.info("Value is: {}", value)) instead of string concatenation in log statements
Write log messages and code comments in English; never translate log messages
Files:
entitybroker/utils/src/java/org/sakaiproject/entitybroker/util/EntityDataUtils.javaentitybroker/impl/src/java/org/sakaiproject/entitybroker/impl/EntityBrokerManagerImpl.javaentitybroker/rest/src/java/org/sakaiproject/entitybroker/rest/EntityHandlerImpl.javaentitybroker/rest/src/test/org/sakaiproject/entitybroker/rest/jetty/EntityBrokerServletTest.javaentitybroker/rest/src/java/org/sakaiproject/entitybroker/rest/EntityEncodingManager.javaentitybroker/rest/src/java/org/sakaiproject/entitybroker/rest/EntityBatchHandler.javaentitybroker/rest/src/test/org/sakaiproject/entitybroker/rest/EntityEncodingManagerTest.java
**/pom.xml
📄 CodeRabbit inference engine (AGENTS.md)
Target Java 17 for trunk builds (update maven-compiler-plugin source/target accordingly)
Files:
entitybroker/rest/pom.xmlentitybroker/utils/pom.xml
🧬 Code graph analysis (1)
entitybroker/rest/src/java/org/sakaiproject/entitybroker/rest/EntityEncodingManager.java (1)
kernel/api/src/main/java/org/sakaiproject/serialization/MapperFactory.java (1)
MapperFactory(36-422)
⏰ 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). (4)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: maven-build
- GitHub Check: maven-build
- GitHub Check: sakai-deploy
🔇 Additional comments (19)
entitybroker/rest/src/java/org/sakaiproject/entitybroker/rest/EntityBatchHandler.java (6)
27-29: LGTM: Standard JDK imports replace reflectutils.The addition of
java.util.Arraysandjava.util.LinkedHashMapcorrectly replaces the removed reflectutils dependencies.
221-222: LGTM: LinkedHashMap preserves insertion order.The replacement of
ArrayOrderedMapwithLinkedHashMapis correct.LinkedHashMapmaintains insertion order, which is critical for:
- Processing batch references sequentially
- Applying response headers in order
- Reintegrating data content deterministically
410-418: LGTM: Explicit typing and ordered maps improve predictability.The changes correctly:
- Add explicit generic typing for type safety
- Use
LinkedHashMapto preserve insertion order for reference parameters- Maintain deterministic parameter processing across internal and external requests
373-373: LGTM: Standard JDK method replaces reflectutils utility.
Arrays.toString(refs)provides equivalent functionality toArrayUtils.arrayToString(refs)for array-to-string conversion in error messages.
739-739: LGTM: Generalized signatures improve flexibility.The method signatures now accept
Map<String, String>instead of concrete implementation types. This is good practice and maintains compatibility withLinkedHashMapwhile preserving insertion-order iteration.Also applies to: 775-775
853-855: LGTM: No-arg constructors required for Jackson serialization.The no-arg constructors are necessary for Jackson-based serialization/deserialization of batch response objects. Without these, Jackson cannot instantiate the classes during deserialization.
Also applies to: 885-887
entitybroker/rest/src/java/org/sakaiproject/entitybroker/rest/EntityHandlerImpl.java (8)
28-29: LGTM!The new reflection imports (Field, Method, Spring ReflectionUtils) are appropriate for replacing the removed reflectutils dependency.
Also applies to: 83-83
1110-1120: LGTM!The
containshelper correctly replaces the removed ReflectUtils.contains with proper null safety and straightforward array membership logic.
1122-1150: LGTM!The lastModified detection logic correctly implements a three-tier fallback: direct property access, annotated field, annotated method. The
readPropertyhelper appropriately tries field access before accessor methods.
1152-1178: LGTM!The annotation finders correctly walk the class hierarchy up to
Object.class. The method finder appropriately filters for zero-parameter methods, which is correct for lastModified getters.
1180-1212: LGTM!The field and method finders are implemented correctly:
findFieldproperly walks the hierarchy catching NoSuchFieldExceptionfindAccessorcorrectly capitalizes and tries both "get" and "is" prefixesfindMethodappropriately delegates to Spring ReflectionUtils with parameterCount validation
1214-1236: LGTM!The reflection helpers correctly use Spring's
ReflectionUtils.makeAccessible, which handles JDK 17+ module access constraints internally. CatchingIllegalStateExceptionprovides graceful degradation when reflection fails.Based on past review comments, this implementation addresses the module access hardening concerns.
482-482: LGTM!All
containsusages correctly replace the removed ReflectUtils.contains calls. The null-handling logic is appropriate for each context:
- Lines 524, 647: null array allows all formats (permissive default)
- Lines 925, 934: null array skips the restriction check
- Line 482: direct check after upstream validation
Also applies to: 524-524, 647-647, 925-925, 934-934
1054-1077: LGTM!The updated
setLastModifiedHeaderscorrectly integrates the new reflection-basedfindLastModifiedValuehelper while preserving the existing fallback hierarchy (entity properties first, then annotated fields/methods).entitybroker/rest/src/test/org/sakaiproject/entitybroker/rest/jetty/EntityBrokerServletTest.java (5)
176-176: LGTM!The assertion correctly expects unescaped URL paths in JSON output, consistent with Jackson's default serialization behavior.
218-218: LGTM!Consistent with the JSON serialization format change.
518-518: LGTM!Consistent URL path format expectation.
587-587: LGTM!Consistent URL path format expectation.
620-620: LGTM!Consistent URL path format expectation.
entitybroker/rest/src/test/org/sakaiproject/entitybroker/rest/EntityEncodingManagerTest.java
Outdated
Show resolved
Hide resolved
...tybroker/rest/src/test/org/sakaiproject/entitybroker/rest/jetty/EntityBrokerServletTest.java
Outdated
Show resolved
Hide resolved
entitybroker/utils/src/java/org/sakaiproject/entitybroker/util/EntityDataUtils.java
Show resolved
Hide resolved
|
Can this really be done? Be still my beating heart :) |
ha yeah odds are somewhere around 42% |
ottenhoff
force-pushed
the
codex/remove-reflectutils-library-from-codebase
branch
from
71ec48d to
2cf0102
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
content-review/impl/turnitin/src/main/java/org/sakaiproject/contentreview/turnitin/util/TurnitinAPIUtil.java (1)
260-284: Resource leak: BufferedReader and InputStream are not closed.The
BufferedReader(line 265) and underlyingInputStream(line 262) are never closed, leading to file descriptor leaks that can cause "too many open files" errors under load.Apply this diff to wrap both resources in try-with-resources:
public static Document callTurnitinReturnDocument(String apiURL, Map<String,Object> parameters, String secretKey, int timeout, Proxy proxy, boolean isMultipart) throws TransientSubmissionException, SubmissionException { InputStream inputStream = callTurnitinReturnInputStream(apiURL, parameters, secretKey, timeout, proxy, isMultipart); - BufferedReader in; - in = new BufferedReader(new InputStreamReader(inputStream)); Document document = null; - try { - DocumentBuilderFactory documentBuilderFactory = DocumentBuilderFactory.newInstance(); - DocumentBuilder parser = documentBuilderFactory.newDocumentBuilder(); - document = parser.parse(new org.xml.sax.InputSource(in)); - } - catch (ParserConfigurationException pce){ - log.error("parser configuration error: " + pce.getMessage()); - throw new TransientSubmissionException ("Parser configuration error", pce); - } catch (Exception t) { - throw new TransientSubmissionException ("Cannot parse Turnitin response. Assuming call was unsuccessful", t); - } + try (BufferedReader in = new BufferedReader(new InputStreamReader(inputStream))) { + DocumentBuilderFactory documentBuilderFactory = DocumentBuilderFactory.newInstance(); + DocumentBuilder parser = documentBuilderFactory.newDocumentBuilder(); + document = parser.parse(new org.xml.sax.InputSource(in)); + } catch (ParserConfigurationException pce) { + log.error("parser configuration error: " + pce.getMessage()); + throw new TransientSubmissionException("Parser configuration error", pce); + } catch (Exception t) { + throw new TransientSubmissionException("Cannot parse Turnitin response. Assuming call was unsuccessful", t); + } if (log.isDebugEnabled()) { log.debug(" Result from call: " + Xml.writeDocumentToString(document)); } return document; }kernel/api/src/main/java/org/sakaiproject/serialization/MapperFactory.java (1)
240-249: Harden XmlMapper against XXE (disable DTD & external entities)Before rebuilds, disable DTDs and external entities on the XMLInputFactory for defense-in-depth:
public XmlMapperBuilder() { - xmlMapper = new XmlMapper(WstxInputFactory.newInstance(), WstxOutputFactory.newInstance()); + xmlMapper = new XmlMapper(WstxInputFactory.newInstance(), WstxOutputFactory.newInstance()); + // XXE hardening: disable DTDs and external entities + xmlMapper.getFactory().getXMLInputFactory().setProperty(javax.xml.stream.XMLInputFactory.SUPPORT_DTD, false); + xmlMapper.getFactory().getXMLInputFactory().setProperty(javax.xml.stream.XMLInputFactory.IS_SUPPORTING_EXTERNAL_ENTITIES, false); + xmlMapper.getFactory().rebuild().build(); }Consider doing this once in the constructor so subsequent feature toggles don’t re-enable risky defaults. As per coding guidelines and prior security best practices.
Also applies to: 357-375
♻️ Duplicate comments (14)
entitybroker/impl/src/java/org/sakaiproject/entitybroker/impl/event/EventReceiverCoordinator.java (1)
49-90: Restore weak mapping per receiver to avoid leaks and dropped subscribers.Keying the
WeakHashMapbyClassLoaderlets only one receiver survive per loader and the strong value → class → loader chain prevents reclamation, so entries and beans stick around across redeploys. Please switch to weak keys on the receivers themselves and snapshotkeySet()instead of values.- private final Map<ClassLoader, EventReceiver> receivers = Collections.synchronizedMap(new WeakHashMap<ClassLoader, EventReceiver>()); + private final Map<EventReceiver, Boolean> receivers = Collections.synchronizedMap(new WeakHashMap<EventReceiver, Boolean>()); @@ - if (receiver != null) { - receivers.put(receiver.getClass().getClassLoader(), receiver); - } + if (receiver != null) { + receivers.put(receiver, Boolean.TRUE); + } @@ - List<EventReceiver> currentReceivers; - synchronized (receivers) { - currentReceivers = new ArrayList<EventReceiver>(receivers.values()); - } + List<EventReceiver> currentReceivers; + synchronized (receivers) { + currentReceivers = new ArrayList<EventReceiver>(receivers.keySet()); + }entitybroker/rest/src/test/org/sakaiproject/entitybroker/rest/jetty/EntityBrokerServletTest.java (1)
355-364: JSON formatting inconsistency remains unaddressed.The unescaped path change at line 364 is correct and consistent with the broader PR changes. However, the JSON formatting inconsistency previously flagged still exists: lines 355, 357, 358, 359, and 363 expect spaces after colons (e.g.,
"status":,"headers":), while later batch assertions (lines 377-384) expect compact format without spaces (e.g.,"status":).Apply the previously suggested fix to align JSON formatting expectations:
- assertTrue(content.contains("\"status\": ")); + assertTrue(content.contains("\"status\":")); - assertTrue(content.contains("\"headers\": ")); + assertTrue(content.contains("\"headers\":")); - assertTrue(content.contains("\"reference\": ")); + assertTrue(content.contains("\"reference\":")); - assertTrue(content.contains("\"data\": ")); + assertTrue(content.contains("\"data\":")); - assertTrue(content.contains("\"entityReference\": ")); + assertTrue(content.contains("\"entityReference\":"));entitybroker/utils/src/java/org/sakaiproject/entitybroker/util/EntityDataUtils.java (1)
511-523: Duplicate: Interface methods with @EntityID are not searched.This concern was already raised in a previous review. The current implementation only searches declared methods in the class hierarchy and does not traverse interfaces where
@EntityIdmight be declared on getter methods.entitybroker/utils/src/java/org/sakaiproject/entitybroker/util/access/AccessProviderManagerImpl.java (1)
44-55: Race condition remains unresolved.The get-then-remove sequence still allows a race where another thread can re-register a provider for the same prefix after line 48 but before line 52, causing the newly registered provider to be incorrectly removed.
Apply the previously suggested fix to use atomic compare-and-remove:
public void unregisterProvider(String prefix, T provider) { if (prefix == null) { return; } WeakReference<T> current = prefixMap.get(prefix); if (current != null) { T existing = current.get(); if (existing == null || existing == provider) { - prefixMap.remove(prefix); + prefixMap.remove(prefix, current); } } }This ensures you only remove the exact
WeakReferenceyou checked, preventing accidental removal of a concurrently registered provider.content-review/impl/turnitin/src/main/java/org/sakaiproject/contentreview/turnitin/util/TurnitinAPIUtil.java (2)
180-193: Address the previous review feedback on this method.The raw Map return type and string concatenation in logging flagged in the previous review remain unaddressed.
345-346: Ensure callers close the returned InputStream.The previous review comment already flagged that this method returns an open InputStream that callers must close using try-with-resources.
entitybroker/impl/src/java/org/sakaiproject/entitybroker/impl/entityprovider/EntityProviderManagerImpl.java (1)
144-149: Fix misleading GC comment.
ConcurrentHashMapkeeps strong references; entries only become collectible after an explicitremove. Please update the comment so it doesn’t imply automatic GC of listeners/providers.Apply this diff to clarify:
- // Concurrent maps provide thread safety while allowing listeners and providers to be released for GC when removed + // Concurrent maps provide thread safety; entries remain strongly referenced until explicitly removedentitybroker/impl/src/java/org/sakaiproject/entitybroker/impl/EntityBrokerManagerImpl.java (1)
798-808: Preserve@EntityTitlelookup on interface methods.This implementation only walks the concrete class hierarchy and misses methods declared on interfaces. Any
@EntityTitleon interface methods (common for entity interfaces and proxies) will be skipped, causing titles to disappear silently.This issue was flagged in a previous review but remains unaddressed. Please fold implemented interfaces into the traversal as suggested in the earlier comment.
entitybroker/rest/src/java/org/sakaiproject/entitybroker/rest/EntityDescriptionManager.java (3)
1220-1224: Exclude static fields from properties and "required" detectionStatic fields are not instance properties. They will currently appear as readable/writable and can be marked required, which is incorrect. Filter them out.
Apply this diff:
- if (field != null && Modifier.isPublic(field.getModifiers())) { + if (field != null && !Modifier.isStatic(field.getModifiers()) && Modifier.isPublic(field.getModifiers())) { introspection.readableTypes.putIfAbsent(name, field.getType()); introspection.writableTypes.putIfAbsent(name, field.getType()); }
1237-1241: Exclude static fields from properties detectionStatic fields are not instance properties and should be filtered out.
Apply this diff:
- if (Modifier.isPublic(field.getModifiers())) { + if (!Modifier.isStatic(field.getModifiers()) && Modifier.isPublic(field.getModifiers())) { introspection.readableTypes.putIfAbsent(name, field.getType()); introspection.writableTypes.putIfAbsent(name, field.getType()); }
1270-1281: Exclude static fields from "required" detectionStatic fields should not be marked as required properties.
Apply this diff:
- if (field != null && field.isAnnotationPresent(EntityFieldRequired.class)) { + if (field != null && !Modifier.isStatic(field.getModifiers()) && field.isAnnotationPresent(EntityFieldRequired.class)) { return true; }entitybroker/utils/src/java/org/sakaiproject/entitybroker/util/devhelper/DeveloperBeanUtils.java (2)
212-223: Use target property type for conversion in copyBeanInternalConvert using the target property’s type to avoid mis-conversion when source/target types differ.
- Object converted = convertValue(value, descriptor.getPropertyType(), allowedDepth - 1, visited); + Class<?> targetType = targetWrapper.getPropertyType(name); + Object converted = convertValue(value, targetType, allowedDepth - 1, visited);
83-90: Avoid Class.cast; handle primitives/boxing and nulls safely in convert()Resolve primitive targets to wrappers and validate instance before cast.
- public static <T> T convert(Object value, Class<T> type) { + public static <T> T convert(Object value, Class<T> type) { if (type == null) { throw new IllegalArgumentException("Target type must be provided"); } - IdentityHashMap<Object, Boolean> visited = new IdentityHashMap<>(); - Object converted = convertValue(value, type, normalizeDepth(0) - 1, visited); - return type.cast(converted); + IdentityHashMap<Object, Boolean> visited = new IdentityHashMap<>(); + Class<?> boxed = org.springframework.util.ClassUtils.resolvePrimitiveIfNecessary(type); + Object converted = convertValue(value, boxed, normalizeDepth(0) - 1, visited); + if (converted == null) { + return null; + } + if (!boxed.isInstance(converted)) { + throw new IllegalArgumentException("Converted value is not of requested type: " + boxed + " → " + converted.getClass()); + } + @SuppressWarnings("unchecked") + T result = (T) converted; + return result; }entitybroker/utils/src/java/org/sakaiproject/entitybroker/util/devhelper/AbstractDeveloperHelperService.java (1)
23-49: Build failure: Verify Jackson and Commons Text dependencies are added.The past review comment flagged missing Maven dependencies for Jackson and Commons Text causing build failures. Ensure these dependencies have been added to the
entitybroker/utilsmodule POM or parent POM before proceeding with the review.Based on learnings
🧹 Nitpick comments (5)
entitybroker/rest/src/java/org/sakaiproject/entitybroker/rest/EntityHandlerImpl.java (1)
831-840: Redundant else assignment at line 839.The variable
methodis initialized toHttpRESTUtils.Method.GETat line 831 and reassigned to the same value at line 839 in the else branch. This is redundant.Apply this diff to remove the redundant else branch:
HttpRESTUtils.Method method = HttpRESTUtils.Method.GET; if (EntityView.VIEW_DELETE.equals(ev.getViewKey())) { method = HttpRESTUtils.Method.DELETE; } else if (EntityView.VIEW_EDIT.equals(ev.getViewKey())) { method = HttpRESTUtils.Method.PUT; } else if (EntityView.VIEW_NEW.equals(ev.getViewKey())) { method = HttpRESTUtils.Method.POST; - } else { - method = HttpRESTUtils.Method.GET; }entitybroker/impl/src/java/org/sakaiproject/entitybroker/impl/EntityBrokerManagerImpl.java (2)
718-737: Consider filtering additional non-bean types.The current logic correctly filters most common non-bean types. However, several additional JDK types are typically leaf values not meant for bean introspection:
- I/O types:
java.io.File,java.nio.file.Path- Network types:
java.net.URI,java.net.URL- Identifier types:
java.util.UUID- i18n types:
java.util.Locale,java.util.Currency- Metadata:
Class<?>While these edge cases are less common, filtering them would prevent unnecessary reflection attempts.
Example expansion:
if (Map.class.isAssignableFrom(type) || java.util.Collection.class.isAssignableFrom(type)) { return false; } + // Additional non-bean types + if (Class.class.equals(type) || java.util.UUID.class.equals(type)) { + return false; + } + String packageName = type.getPackage() != null ? type.getPackage().getName() : ""; + if (packageName.startsWith("java.io.") || packageName.startsWith("java.nio.file.") + || packageName.startsWith("java.net.")) { + return false; + } return true;
693-693: Rename variable to match the new method name.The variable
isPOJOis now misleading since it's assigned fromisBeanCandidate(). Consider renaming for clarity.- boolean isPOJO = isBeanCandidate(entityData.getData()); + boolean isBeanCandidate = isBeanCandidate(entityData.getData());Update line 706 accordingly:
- if (isPOJO && titleNotSet) { + if (isBeanCandidate && titleNotSet) {kernel/api/src/main/java/org/sakaiproject/serialization/MapperFactory.java (1)
357-361: Reduce repeated factory rebuilds (minor perf/clarity)You call rebuild().build() after each property change. Batch property sets and rebuild once at the end of the method or in the constructor to avoid repeated factory rebuilds.
Also applies to: 371-375, 386-390, 400-404, 414-418
entitybroker/utils/src/java/org/sakaiproject/entitybroker/util/devhelper/AbstractDeveloperHelperService.java (1)
377-390: Consider static initialization or double-checked locking for codec registry.The lazy initialization pattern has a check-then-act race condition. Multiple threads could simultaneously initialize separate codec registries. While the impact is minimal (identical registries), best practice is to use static initialization or double-checked locking with a volatile field.
Apply this diff to use static initialization:
- private Map<String, EntityCodec> codecs; - private EntityCodec getCodec(String format) { - if (codecs == null) { - codecs = new ConcurrentHashMap<>(); - codecs.put(Formats.JSON, new JsonCodec()); - codecs.put(Formats.XML, new XmlCodec()); - codecs.put(Formats.HTML, new HtmlCodec()); - } + private static final Map<String, EntityCodec> CODECS; + static { + Map<String, EntityCodec> map = new ConcurrentHashMap<>(); + map.put(Formats.JSON, new JsonCodec()); + map.put(Formats.XML, new XmlCodec()); + map.put(Formats.HTML, new HtmlCodec()); + CODECS = map; + } + + private EntityCodec getCodec(String format) { - EntityCodec codec = codecs.get(format); + EntityCodec codec = CODECS.get(format); if (codec == null) { throw new IllegalArgumentException("Failed to find an encoder/decoder for format: " + format); } return codec; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (41)
JDK21-migration-plan.md(1 hunks)content-review/impl/turnitin/pom.xml(0 hunks)content-review/impl/turnitin/src/main/java/org/sakaiproject/contentreview/turnitin/util/TurnitinAPIUtil.java(5 hunks)docker/tomcat/conf/catalina.properties(0 hunks)entitybroker/api/src/java/org/sakaiproject/entitybroker/entityprovider/extension/EntityData.java(1 hunks)entitybroker/core-providers/pom.xml(0 hunks)entitybroker/core-providers/src/java/org/sakaiproject/entitybroker/providers/SiteEntityProvider.java(5 hunks)entitybroker/core-providers/src/java/org/sakaiproject/entitybroker/providers/UserEntityProvider.java(4 hunks)entitybroker/core-providers/src/java/org/sakaiproject/entitybroker/providers/model/EntityGroup.java(3 hunks)entitybroker/core-providers/src/java/org/sakaiproject/entitybroker/providers/model/EntityMember.java(2 hunks)entitybroker/core-providers/src/java/org/sakaiproject/entitybroker/providers/model/EntitySite.java(3 hunks)entitybroker/core-providers/src/java/org/sakaiproject/entitybroker/providers/model/EntitySitePage.java(2 hunks)entitybroker/core-providers/src/java/org/sakaiproject/entitybroker/providers/model/EntityTool.java(2 hunks)entitybroker/core-providers/src/java/org/sakaiproject/entitybroker/providers/model/EntityUser.java(4 hunks)entitybroker/impl/pom.xml(0 hunks)entitybroker/impl/src/java/org/sakaiproject/entitybroker/impl/EntityBrokerManagerImpl.java(4 hunks)entitybroker/impl/src/java/org/sakaiproject/entitybroker/impl/entityprovider/EntityProviderManagerImpl.java(6 hunks)entitybroker/impl/src/java/org/sakaiproject/entitybroker/impl/event/EventReceiverCoordinator.java(3 hunks)entitybroker/pom.xml(0 hunks)entitybroker/rest/pom.xml(1 hunks)entitybroker/rest/src/java/org/sakaiproject/entitybroker/rest/EntityBatchHandler.java(9 hunks)entitybroker/rest/src/java/org/sakaiproject/entitybroker/rest/EntityDescriptionManager.java(8 hunks)entitybroker/rest/src/java/org/sakaiproject/entitybroker/rest/EntityEncodingManager.java(18 hunks)entitybroker/rest/src/java/org/sakaiproject/entitybroker/rest/EntityHandlerImpl.java(11 hunks)entitybroker/rest/src/test/java/org/sakaiproject/entitybroker/rest/EntityEncodingManagerJsonTest.java(1 hunks)entitybroker/rest/src/test/org/sakaiproject/entitybroker/rest/EntityEncodingManagerTest.java(9 hunks)entitybroker/rest/src/test/org/sakaiproject/entitybroker/rest/jetty/EntityBrokerServletTest.java(7 hunks)entitybroker/utils/pom.xml(1 hunks)entitybroker/utils/src/java/org/sakaiproject/entitybroker/util/EntityDataUtils.java(5 hunks)entitybroker/utils/src/java/org/sakaiproject/entitybroker/util/access/AccessProviderManagerImpl.java(2 hunks)entitybroker/utils/src/java/org/sakaiproject/entitybroker/util/core/EntityProviderMethodStoreImpl.java(3 hunks)entitybroker/utils/src/java/org/sakaiproject/entitybroker/util/devhelper/AbstractDeveloperHelperService.java(7 hunks)entitybroker/utils/src/java/org/sakaiproject/entitybroker/util/devhelper/DeveloperBeanUtils.java(1 hunks)entitybroker/utils/src/java/org/sakaiproject/entitybroker/util/devhelper/DeveloperHelperServiceMock.java(2 hunks)entitybroker/utils/src/java/org/sakaiproject/entitybroker/util/http/EntityHttpServletResponse.java(2 hunks)entitybroker/utils/src/java/org/sakaiproject/entitybroker/util/request/RequestStorageImpl.java(2 hunks)entitybroker/utils/src/test/org/sakaiproject/entitybroker/util/reflect/ReflectUtilTest.java(0 hunks)kernel/api/src/main/java/org/sakaiproject/serialization/MapperFactory.java(4 hunks)kernel/kernel-impl/pom.xml(0 hunks)kernel/kernel-impl/src/main/java/org/sakaiproject/tool/impl/RebuildBreakdownServiceImpl.java(5 hunks)master/pom.xml(0 hunks)
💤 Files with no reviewable changes (8)
- entitybroker/impl/pom.xml
- docker/tomcat/conf/catalina.properties
- content-review/impl/turnitin/pom.xml
- master/pom.xml
- entitybroker/core-providers/pom.xml
- entitybroker/pom.xml
- kernel/kernel-impl/pom.xml
- entitybroker/utils/src/test/org/sakaiproject/entitybroker/util/reflect/ReflectUtilTest.java
🚧 Files skipped from review as they are similar to previous changes (8)
- entitybroker/utils/src/java/org/sakaiproject/entitybroker/util/http/EntityHttpServletResponse.java
- entitybroker/core-providers/src/java/org/sakaiproject/entitybroker/providers/model/EntityMember.java
- entitybroker/core-providers/src/java/org/sakaiproject/entitybroker/providers/SiteEntityProvider.java
- entitybroker/utils/src/java/org/sakaiproject/entitybroker/util/devhelper/DeveloperHelperServiceMock.java
- entitybroker/utils/src/java/org/sakaiproject/entitybroker/util/request/RequestStorageImpl.java
- entitybroker/api/src/java/org/sakaiproject/entitybroker/entityprovider/extension/EntityData.java
- entitybroker/rest/pom.xml
- entitybroker/core-providers/src/java/org/sakaiproject/entitybroker/providers/model/EntityGroup.java
🧰 Additional context used
📓 Path-based instructions (2)
**/*.java
📄 CodeRabbit inference engine (.cursor/rules/logging-rule.mdc)
**/*.java: Use SLF4J parameterized logging (e.g., logger.info("Value is: {}", value)) instead of string concatenation in log statements
Write log messages and code comments in English; never translate log messages
Files:
entitybroker/utils/src/java/org/sakaiproject/entitybroker/util/core/EntityProviderMethodStoreImpl.javakernel/kernel-impl/src/main/java/org/sakaiproject/tool/impl/RebuildBreakdownServiceImpl.javaentitybroker/core-providers/src/java/org/sakaiproject/entitybroker/providers/model/EntityUser.javaentitybroker/core-providers/src/java/org/sakaiproject/entitybroker/providers/model/EntitySite.javaentitybroker/utils/src/java/org/sakaiproject/entitybroker/util/EntityDataUtils.javaentitybroker/rest/src/test/java/org/sakaiproject/entitybroker/rest/EntityEncodingManagerJsonTest.javaentitybroker/rest/src/test/org/sakaiproject/entitybroker/rest/EntityEncodingManagerTest.javaentitybroker/impl/src/java/org/sakaiproject/entitybroker/impl/EntityBrokerManagerImpl.javaentitybroker/rest/src/java/org/sakaiproject/entitybroker/rest/EntityBatchHandler.javacontent-review/impl/turnitin/src/main/java/org/sakaiproject/contentreview/turnitin/util/TurnitinAPIUtil.javaentitybroker/rest/src/java/org/sakaiproject/entitybroker/rest/EntityEncodingManager.javaentitybroker/utils/src/java/org/sakaiproject/entitybroker/util/access/AccessProviderManagerImpl.javaentitybroker/utils/src/java/org/sakaiproject/entitybroker/util/devhelper/DeveloperBeanUtils.javaentitybroker/rest/src/java/org/sakaiproject/entitybroker/rest/EntityDescriptionManager.javaentitybroker/utils/src/java/org/sakaiproject/entitybroker/util/devhelper/AbstractDeveloperHelperService.javaentitybroker/core-providers/src/java/org/sakaiproject/entitybroker/providers/model/EntityTool.javakernel/api/src/main/java/org/sakaiproject/serialization/MapperFactory.javaentitybroker/impl/src/java/org/sakaiproject/entitybroker/impl/event/EventReceiverCoordinator.javaentitybroker/core-providers/src/java/org/sakaiproject/entitybroker/providers/model/EntitySitePage.javaentitybroker/rest/src/java/org/sakaiproject/entitybroker/rest/EntityHandlerImpl.javaentitybroker/core-providers/src/java/org/sakaiproject/entitybroker/providers/UserEntityProvider.javaentitybroker/rest/src/test/org/sakaiproject/entitybroker/rest/jetty/EntityBrokerServletTest.javaentitybroker/impl/src/java/org/sakaiproject/entitybroker/impl/entityprovider/EntityProviderManagerImpl.java
**/pom.xml
📄 CodeRabbit inference engine (AGENTS.md)
Target Java 17 for trunk builds (update maven-compiler-plugin source/target accordingly)
Files:
entitybroker/utils/pom.xml
🧬 Code graph analysis (6)
entitybroker/core-providers/src/java/org/sakaiproject/entitybroker/providers/model/EntityUser.java (1)
entitybroker/core-providers/src/java/org/sakaiproject/entitybroker/providers/model/EntitySitePage.java (1)
JsonIgnoreProperties(42-385)
entitybroker/core-providers/src/java/org/sakaiproject/entitybroker/providers/model/EntitySite.java (1)
entitybroker/core-providers/src/java/org/sakaiproject/entitybroker/providers/model/EntitySitePage.java (1)
JsonIgnoreProperties(42-385)
entitybroker/rest/src/test/java/org/sakaiproject/entitybroker/rest/EntityEncodingManagerJsonTest.java (1)
kernel/api/src/main/java/org/sakaiproject/serialization/MapperFactory.java (1)
MapperFactory(37-434)
entitybroker/rest/src/test/org/sakaiproject/entitybroker/rest/EntityEncodingManagerTest.java (1)
kernel/api/src/main/java/org/sakaiproject/serialization/MapperFactory.java (1)
MapperFactory(37-434)
entitybroker/rest/src/java/org/sakaiproject/entitybroker/rest/EntityEncodingManager.java (1)
kernel/api/src/main/java/org/sakaiproject/serialization/MapperFactory.java (1)
MapperFactory(37-434)
entitybroker/utils/src/java/org/sakaiproject/entitybroker/util/devhelper/AbstractDeveloperHelperService.java (1)
kernel/api/src/main/java/org/sakaiproject/serialization/MapperFactory.java (1)
MapperFactory(37-434)
🪛 GitHub Actions: Maven Build
kernel/api/src/main/java/org/sakaiproject/serialization/MapperFactory.java
[error] 92-92: Could not find class 'com.fasterxml.jackson.datatype.jdk8.Jdk8Module' in thread "main"
⏰ 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: Codacy Static Code Analysis
- GitHub Check: sakai-deploy
- GitHub Check: maven-build
🔇 Additional comments (77)
entitybroker/utils/src/java/org/sakaiproject/entitybroker/util/core/EntityProviderMethodStoreImpl.java (4)
26-26: LGTM!The new imports are correctly added to support the
stringifyClassArrayhelper method.Also applies to: 34-34
132-132: LGTM!The changes correctly:
- Create a copy of the actions map to avoid holding references from another ClassLoader
- Use SLF4J parameterized logging as per coding guidelines
- Maintain the same functional behavior while improving safety and logging quality
Also applies to: 156-157
367-374: LGTM!The new
stringifyClassArrayhelper is a clean replacement for the removedReflectUtils.arrayToString. The implementation:
- Correctly handles null and empty arrays
- Uses idiomatic Java 8 streams
- Produces readable output with proper formatting
360-361: LGTM!The error message correctly uses
stringifyClassArrayto format the valid types, providing clear debugging information when validation fails.entitybroker/rest/src/java/org/sakaiproject/entitybroker/rest/EntityBatchHandler.java (11)
27-29: LGTM!The addition of
ArraysandLinkedHashMapimports aligns with the refactoring to remove the reflectutils library. Both are standard Java utilities.
221-222: LGTM!The migration from
ArrayOrderedMaptoLinkedHashMapis appropriate.LinkedHashMappreserves insertion order, which is critical for batch processing where the order of refs must be maintained.
373-373: LGTM!The replacement of
ArrayUtils.arrayToStringwithArrays.toStringis appropriate and removes the dependency on Apache Commons Lang without loss of functionality.
410-412: LGTM!The migration to
LinkedHashMapand the generalization of the type toMapinterface are both appropriate. Insertion order preservation is important for parameter processing in batch operations.
415-415: LGTM!The initialization of the
UNREFERENCED_PARAMSmap withLinkedHashMapand an appropriate initial capacity is efficient and maintains insertion order.
418-418: LGTM!The initialization pattern for each reference's parameter map is consistent with the
UNREFERENCED_PARAMSmap and appropriately usesLinkedHashMapwith initial capacity.
615-615: LGTM!The initialization of the
paramsmap for external requests usesLinkedHashMapwith an appropriate capacity hint, maintaining consistency with the rest of the refactoring.
739-740: LGTM!Generalizing the parameter type from
HashMap<String, String>toMap<String, String>is a best practice that improves flexibility without changing behavior.
775-775: LGTM!The parameter type generalization from
HashMap<String, String>toMap<String, String>is consistent with thereintegrateDataContentmethod and follows best practices.
853-855: LGTM!The addition of a no-arg constructor for
ResponseErroris necessary for Jackson deserialization. The constructor is properly documented and follows standard serialization patterns.
885-887: LGTM!The addition of a no-arg constructor for
ResponseResultis necessary for Jackson deserialization and is consistent with theResponseErrorclass. The constructor is properly documented.entitybroker/utils/src/java/org/sakaiproject/entitybroker/util/EntityDataUtils.java (11)
24-25: LGTM!The new imports for Java reflection and Spring ReflectionUtils appropriately support the custom reflection helpers that replace the removed ReflectUtils library.
Also applies to: 40-40
187-227: LGTM!The refactored
getEntityIdmethod correctly handles Map sources and searches for identifiers across the class hierarchy using fields first, then methods. The fallback strategy (annotation → "entityId" → "id") aligns with common entity patterns.
234-264: LGTM!The refactored
getEntityIdFieldmethod correctly searches for identifier fields across the class hierarchy, then falls back to accessor methods with property name extraction. The logic is sound and null-safe.
483-495: LGTM!The
findFieldWithAnnotationmethod correctly walks the class hierarchy to find a field with the specified annotation. The null checks and hierarchy traversal logic are sound.
497-509: LGTM!The
findFieldmethod correctly walks the class hierarchy to locate a field by name, properly handlingNoSuchFieldExceptionto continue searching superclasses.
525-535: LGTM!The
findAccessormethod correctly implements JavaBean accessor lookup with proper capitalization and prefix handling ("get" and "is").
537-543: LGTM!The
findMethodmethod appropriately delegates to Spring'sReflectionUtils.findMethodand ensures the method has no parameters.
545-556: LGTM!The
readFieldValuemethod correctly uses Spring'sReflectionUtilsto make fields accessible and read their values, with appropriate error handling.
558-570: LGTM!The
invokeMethodmethod correctly uses Spring'sReflectionUtilsto make methods accessible and invoke them, with appropriate error handling.
572-584: LGTM!The
extractPropertyNamemethod correctly extracts property names from JavaBean getter method names, handling both "get" and "is" prefixes appropriately.
586-594: LGTM!The
decapitalizemethod correctly implements JavaBean property name decapitalization rules, including the special handling for acronyms (e.g., "URL" remains "URL", "Url" becomes "url").entitybroker/utils/src/java/org/sakaiproject/entitybroker/util/access/AccessProviderManagerImpl.java (4)
35-35: LGTM! Sound concurrent design.The migration to
ConcurrentHashMap<String, WeakReference<T>>withfinalmodifier provides thread-safe access while allowing garbage collection of unused providers.
37-42: LGTM! Proper validation and storage.The null-safety checks with
IllegalArgumentExceptionand wrapping the provider inWeakReferencebefore storage are correct.
57-61: LGTM! Unconditional removal is correct here.The null-safety check and unconditional removal by prefix are appropriate for this overload.
63-76: LGTM! Proper atomic cleanup of stale entries.The lazy removal of garbage-collected providers using
remove(prefix, reference)at line 73 correctly uses atomic compare-and-remove, preventing accidental removal of concurrently registered providers. This demonstrates the proper pattern that should also be applied inunregisterProvider(String prefix, T provider).content-review/impl/turnitin/src/main/java/org/sakaiproject/contentreview/turnitin/util/TurnitinAPIUtil.java (4)
195-198: LGTM!This convenience overload simplifies the API by defaulting
isMultiparttofalse.
286-305: LGTM!The flattening logic is sound: it defensively handles null inputs, filters for element nodes, and delegates to
extractValueandmergeValueto build the map structure.
307-325: LGTM!The recursive extraction correctly distinguishes leaf nodes (text content) from nested structures (child elements), preserving insertion order with
LinkedHashMap.
327-343: LGTM!The merge logic correctly handles duplicate keys by promoting single values to lists, which aligns with the XML structure where repeated elements should be collected.
entitybroker/core-providers/src/java/org/sakaiproject/entitybroker/providers/model/EntityTool.java (1)
21-123: Jackson ignore annotation matches prior behaviorReplacing the class-level reflectutils ignore with
@JsonIgnoreongetAccessSecurity()preserves the original serialization semantics without pulling in reflectutils. Nicely done.entitybroker/core-providers/src/java/org/sakaiproject/entitybroker/providers/model/EntitySitePage.java (1)
25-43: Class-level JsonIgnoreProperties is an exact migrationThe new
@JsonIgnorePropertieslist mirrors the fields the reflectutils annotation excluded, so JSON exposure stays unchanged while removing the legacy dependency.entitybroker/core-providers/src/java/org/sakaiproject/entitybroker/providers/model/EntitySite.java (1)
31-755: Jackson annotations keep the same suppression setThe class-level
@JsonIgnorePropertiesand the@JsonIgnoreontoXmlline up with the prior reflectutils configuration, preventing the expensive/unsupported getters from being hit during serialization. Looks good.entitybroker/core-providers/src/java/org/sakaiproject/entitybroker/providers/model/EntityUser.java (1)
24-304: Migrated ignores match previous serialization contractShifting to
@JsonIgnorePropertiesplus@JsonIgnoreontoXmlkeeps Jackson from touching the legacy-only bits while freeing the code from reflectutils. All consistent with the existing REST surface.entitybroker/rest/src/java/org/sakaiproject/entitybroker/rest/EntityHandlerImpl.java (8)
28-29: LGTM!The new imports for
java.lang.reflect.Field,java.lang.reflect.Method, andorg.springframework.util.ReflectionUtilscorrectly support the in-house reflection helpers that replace ReflectUtils.Also applies to: 83-83
482-482: LGTM!The replacement of
ReflectUtils.containswith the localcontainshelper is consistent across all call sites and maintains the same semantic behavior.Also applies to: 524-524, 647-647, 925-925, 934-934
1054-1077: LGTM!The enhanced lastModified discovery logic correctly falls back to annotation-based introspection when the property is not found. This aligns with the goal of replacing ReflectUtils with in-house helpers.
1110-1120: LGTM!The
containshelper correctly implements array membership check with proper null/empty handling. Linear search is appropriate for the small arrays used in format and view restrictions.
1122-1137: LGTM!The cascading discovery logic (property → annotated field → annotated method) correctly implements the lastModified value resolution strategy.
1139-1212: LGTM!The introspection helpers correctly:
- Walk the class hierarchy to
Object.class- Handle null inputs gracefully
- Filter methods by parameter count where appropriate
- Use Spring
ReflectionUtilsfor method discovery
1214-1224: LGTM!The use of Spring's
ReflectionUtils.makeAccessibleandReflectionUtils.getFieldis appropriate and handles module access restrictions gracefully on JDK 17+. TheIllegalStateExceptioncatch covers wrapped access exceptions.
1226-1236: LGTM!The
invokeMethodhelper correctly uses Spring'sReflectionUtilsutilities for safe method invocation with proper exception handling.entitybroker/rest/src/java/org/sakaiproject/entitybroker/rest/EntityDescriptionManager.java (10)
23-44: LGTM!The added imports support the new introspection-based property analysis and are appropriate for replacing the reflectutils library.
1164-1198: LGTM!The type-checking helpers (
isSimpleType,isCollectionType,isArrayType,isMapType,getArrayComponentType) are well-implemented and correctly replace the previous reflection-utils-based type detection.
476-485: LGTM!The type classification logic correctly uses the new helper methods to determine entity types for XML generation.
730-738: LGTM!The type classification for HTML generation is consistent with the XML generation approach.
998-1059: LGTM!The refactored
getFormatsmethod is clearer and uses aLinkedHashSetto collect formats while maintaining order and preventing duplicates. The logic correctly handles both output and input formats, and preserves backward compatibility.
1152-1162: LGTM!The
containsFormathelper is a clean utility for checking format presence in arrays.
1248-1268: LGTM!The
findFieldandgetAllFieldshelper methods correctly traverse the class hierarchy to locate and collect fields, stopping appropriately at theObjectclass.
497-500: LGTM!The XML generation correctly uses the
analyzePropertiesresult to extract readable/writable types for property documentation.
762-765: LGTM!The HTML generation correctly uses the
analyzePropertiesresult to extract readable/writable types and required properties for display.
1200-1246: Run the following to verifyfindFieldandgetAllFieldsimplementations:rg -n 'findField' -C5 entitybroker/rest/src/java/org/sakaiproject/entitybroker/rest/EntityDescriptionManager.java rg -n 'getAllFields' -C5 entitybroker/rest/src/java/org/sakaiproject/entitybroker/rest/EntityDescriptionManager.javaentitybroker/rest/src/test/java/org/sakaiproject/entitybroker/rest/EntityEncodingManagerJsonTest.java (1)
43-55: Good coverage for inner-class serializationTest correctly validates non-static inner bean encoding and inspects output with a Jackson mapper.
entitybroker/rest/src/test/org/sakaiproject/entitybroker/rest/EntityEncodingManagerTest.java (4)
147-164: JSON: no root wrapper behavior validatedSolid assertion set confirming absence of prefix wrapper and correctness of payload. Parsing via ObjectMapper reduces brittleness.
166-183: Data-only JSON: wrapper correctly omittedChecks for data-only payload shape and nested content look good.
581-621: EntityData JSON assertions are appropriateVerifies required fields present and transient internals omitted. This aligns with the new encoding path.
441-452: Number handling in decode tests fixedCasting to Number before intValue() avoids brittle type assertions across parsers.
entitybroker/rest/src/java/org/sakaiproject/entitybroker/rest/EntityEncodingManager.java (5)
125-142: Static JSON pretty printing setup looks goodCustom separators to keep “space after colon” format and shared pretty-writer are sensible.
143-153: XML mapper composition consistent with MapperFactoryUsing MapperFactory.xmlBuilder() centralizes config. Ensure MapperFactory is hardened against XXE as noted.
824-882: No JSON root wrapper for beans/dataOnly is correctThe conditional removal of the name wrapper for JSON prevents legacy root wrapping; fallback path is a good safety net with logging.
1292-1309: JSONP callback sanitization is reasonableRegex + length bounds + comment prefixing mitigate injection. Keep the pattern centralized if used elsewhere.
1328-1354: Decode pipeline is sound; map normalization aids consumersensureDecodedMap plus typed map conversion provides a stable shape across formats.
kernel/api/src/main/java/org/sakaiproject/serialization/MapperFactory.java (1)
52-59: Ignore dependency addition suggestion jackson-datatype-jdk8 is already declared in kernel/api/pom.xml.Likely an incorrect or invalid review comment.
entitybroker/utils/src/java/org/sakaiproject/entitybroker/util/devhelper/AbstractDeveloperHelperService.java (10)
84-93: LGTM! Well-structured lazy initialization with WeakReference.The refactored logic correctly handles null cases and lazy resolution of the provider. The WeakReference pattern is appropriate for this optional collaborator.
100-100: LGTM! Modern diamond operator usage.The update to use the diamond operator is a clean, idiomatic improvement.
116-120: LGTM! Codec-based decoding with improved error context.The new codec-based approach is clean, and the enhanced error message provides valuable format context for troubleshooting.
140-144: LGTM! Codec-based encoding with detailed error context.The codec-based approach is consistent with decoding, and the error message provides helpful context including the data type.
402-406: LGTM! Clean interface design.The
EntityCodecinterface provides a clear contract for pluggable encoding/decoding implementations.
408-429: LGTM! Jackson mapper configuration follows best practices.The mapper configurations are well-structured with appropriate feature flags (ignore unknown properties, exclude nulls, Java 8 support, ISO date formatting). The static final mappers are thread-safe for read operations.
Based on learnings
461-486: LGTM! XmlCodec implementation is clean and consistent.The implementation follows the same pattern as
JsonCodecwith appropriate XML-specific mapper usage.
488-525: LGTM! HtmlCodec properly escapes all content.The encode method correctly uses
StringEscapeUtils.escapeHtml4for all user-controlled content (name, data, property keys/values), preventing XSS vulnerabilities. Note that the decode method doesn't parse HTML but simply wraps the input in a map, which is acceptable for this use case.
527-548: LGTM! Payload preparation logic is clean and defensive.The method correctly handles various input scenarios and uses
LinkedHashMapfor consistent ordering. The defensive copy of properties (line 545) prevents external modification. Note the pattern matching at line 534 requires Java 16+ (same asJsonCodec).
431-459: Java 17 supportsinstanceofpattern matching; no changes needed.
| private Boolean readBooleanField(Object target, String fieldName) { | ||
| Field field = ReflectionUtils.findField(target.getClass(), fieldName); | ||
| if (field == null) { | ||
| return null; | ||
| } | ||
| ReflectionUtils.makeAccessible(field); | ||
| try { | ||
| Object value = ReflectionUtils.getField(field, target); | ||
| if (value instanceof Boolean) { | ||
| return (Boolean) value; | ||
| } | ||
| if (value instanceof String) { | ||
| return parseBoolean((String) value); | ||
| } | ||
| return (value != null) ? Boolean.valueOf(value.toString()) : null; | ||
| } catch (IllegalArgumentException e) { | ||
| return null; | ||
| } | ||
| } |
There was a problem hiding this comment.
Unwrap Spring proxies before resolving the field
When userDirectoryService is delivered as a JDK dynamic proxy (a common case for interface-based beans), target.getClass() is com.sun.proxy.$Proxy…, which has no m_separateIdEid field. ReflectionUtils.findField(...) therefore returns null, the fallback never fires, and usesSeparateIdEid is forced to false. Previously FieldUtils handled proxy unwrapping, so this is a regression on any deployment using JDK proxies and relying on the runtime field value.
Please resolve the user class before the lookup, e.g.
-import org.springframework.util.ReflectionUtils;
+import org.springframework.util.ClassUtils;
+import org.springframework.util.ReflectionUtils;
…
- Field field = ReflectionUtils.findField(target.getClass(), fieldName);
+ Class<?> targetClass = ClassUtils.getUserClass(target);
+ Field field = ReflectionUtils.findField(targetClass, fieldName);This preserves the old behavior across both proxied and non-proxied services.
There was a problem hiding this comment.
userDirectoryService should never be deserialized, should be skipped
There was a problem hiding this comment.
@ern, you're absolutely right that userDirectoryService should never be deserialized—it's injected as a Spring-managed service bean, not serialized/deserialized.
However, CodeRabbit's concern about proxies refers to Spring's runtime AOP proxies, not serialization proxies. When Spring wraps userDirectoryService in a JDK dynamic proxy (common for interface-based beans with transactions/AOP), calling target.getClass() returns the proxy class (e.g., $Proxy123) instead of the actual implementation class that contains the m_separateIdEid field. This would cause ReflectionUtils.findField(...) to return null.
CodeRabbit's suggestion to use ClassUtils.getUserClass(target) unwraps the Spring proxy to get the real implementation class, ensuring the field lookup works correctly whether the service is proxied or not. This is a valid concern distinct from serialization.
Would you like to apply the proxy-unwrapping fix, or is the userDirectoryService guaranteed to never be proxied in your deployment?
| return DeveloperBeanUtils.cloneBean(bean, maxDepth, propertiesToSkip); | ||
| } | ||
|
|
||
| /* (non-Javadoc) | ||
| * @see org.sakaiproject.entitybroker.DeveloperHelperService#copyBean(java.lang.Object, java.lang.Object, int, java.lang.String[], boolean) | ||
| */ | ||
| public void copyBean(Object orig, Object dest, int maxDepth, String[] fieldNamesToSkip, | ||
| boolean ignoreNulls) { | ||
| ReflectUtils.getInstance().copy(orig, dest, maxDepth, fieldNamesToSkip, ignoreNulls); | ||
| DeveloperBeanUtils.copyBean(orig, dest, maxDepth, fieldNamesToSkip, ignoreNulls); | ||
| } | ||
|
|
||
| /* (non-Javadoc) | ||
| * @see org.sakaiproject.entitybroker.DeveloperHelperService#populate(java.lang.Object, java.util.Map) | ||
| */ | ||
| public List<String> populate(Object object, Map<String, Object> properties) { | ||
| return ReflectUtils.getInstance().populate(object, properties); | ||
| return DeveloperBeanUtils.populate(object, properties); | ||
| } | ||
|
|
||
| /* (non-Javadoc) | ||
| * @see org.sakaiproject.entitybroker.DeveloperHelperService#convert(java.lang.Object, java.lang.Class) | ||
| */ | ||
| public <T> T convert(Object object, Class<T> type) { | ||
| return ReflectUtils.getInstance().convert(object, type); | ||
| return DeveloperBeanUtils.convert(object, type); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
LGTM! Bean utilities delegated to DeveloperBeanUtils.
The delegation to DeveloperBeanUtils is straightforward and aligns with the PR objective to remove the reflectutils library. Ensure that DeveloperBeanUtils provides equivalent functionality and behavior to prevent regressions.
Verify that DeveloperBeanUtils has equivalent functionality by running existing tests that exercise these bean utility methods:
🏁 Script executed:
#!/bin/bash
# Description: Find and list tests that exercise bean utility methods.
# Search for test files that reference the bean utility methods
rg -n --type=java -C3 '\b(cloneBean|copyBean|populate|convert)\s*\(' \
-g '*Test*.java' -g '*test*.java'Length of output: 21939
Add tests for bean utility delegations
No existing tests cover cloneBean, copyBean, populate, or convert in DeveloperHelperService. Add unit tests to exercise these methods and ensure DeveloperBeanUtils behavior matches the previous implementation.
🤖 Prompt for AI Agents
In
entitybroker/utils/src/java/org/sakaiproject/entitybroker/util/devhelper/AbstractDeveloperHelperService.java
lines 352-374, there are no unit tests covering the delegations to
DeveloperBeanUtils for cloneBean, copyBean, populate, and convert; add a new
JUnit test class that exercises each method to ensure the service delegates
correctly and reproduces expected behavior: 1) test cloneBean by creating a
sample bean with nested objects and properties to skip, call copy and clone
variants and assert deep/shallow behavior and skipped properties; 2) test
copyBean by copying between two beans, exercising the maxDepth and ignoreNulls
flag and asserting destination fields match expectations; 3) test populate by
passing a map of properties to a fresh bean and asserting fields are set; 4)
test convert by converting between compatible types (e.g., Map->bean or
number/string conversions) and asserting returned type and values; instantiate
AbstractDeveloperHelperService via a simple anonymous subclass or the concrete
implementation used in production, avoid complex mocking of DeveloperBeanUtils
(use real behavior) and include assertions for edge cases (nulls, skipped
fields) so behavior matches the previous implementation.
kernel/kernel-impl/src/main/java/org/sakaiproject/tool/impl/RebuildBreakdownServiceImpl.java
Show resolved
Hide resolved
| } catch (TransientSubmissionException | SubmissionException e) { | ||
| throw e; | ||
| } catch (Exception t) { | ||
| throw new TransientSubmissionException ("Cannot parse Turnitin response. Assuming call was unsuccessful", t); | ||
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
entitybroker/impl/src/java/org/sakaiproject/entitybroker/impl/EntityBrokerManagerImpl.java (1)
808-818: Verify that interface method traversal was applied.A past review comment flagged that
getAllMethodsshould traverse interfaces to capture@EntityTitleon interface methods. The comment is marked as "Addressed in commits 800edd6 to 3059ee6", but the current code does not include the suggested fix (no interface traversal is present).Please confirm whether the interface traversal fix was applied in those commits and is present in the current branch, or if it needs to be re-applied here.
content-review/impl/turnitin/src/main/java/org/sakaiproject/contentreview/turnitin/util/TurnitinAPIUtil.java (1)
180-193: Address previous review feedback: use generics and parameterized logging.The concerns raised in the previous review remain unaddressed:
- Return type is still raw
Mapinstead ofMap<String, Object>.- Line 186 still uses string concatenation in logging instead of parameterized logging.
Apply this diff to address both issues:
- public static Map callTurnitinReturnMap(String apiURL, Map<String,Object> parameters, + public static Map<String, Object> callTurnitinReturnMap(String apiURL, Map<String,Object> parameters, String secretKey, int timeout, Proxy proxy) throws TransientSubmissionException, SubmissionException { try { Document document = callTurnitinReturnDocument(apiURL, parameters, secretKey, timeout, proxy, false); Map<String, Object> togo = flattenResponse(document); - log.debug("Turnitin Result Payload: " + togo); + log.debug("Turnitin Result Payload: {}", togo); return togo;As per coding guidelines
🧹 Nitpick comments (1)
entitybroker/impl/src/java/org/sakaiproject/entitybroker/impl/EntityBrokerManagerImpl.java (1)
820-830: Consider interface traversal for consistency.While interface fields are uncommon, for consistency with
getAllMethods(which should traverse interfaces per the past review comment), you may want to also traverse interfaces here.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
content-review/impl/turnitin/src/main/java/org/sakaiproject/contentreview/turnitin/util/TurnitinAPIUtil.java(4 hunks)entitybroker/impl/src/java/org/sakaiproject/entitybroker/impl/EntityBrokerManagerImpl.java(4 hunks)entitybroker/impl/src/java/org/sakaiproject/entitybroker/impl/entityprovider/EntityProviderManagerImpl.java(6 hunks)entitybroker/rest/src/java/org/sakaiproject/entitybroker/rest/EntityHandlerImpl.java(11 hunks)kernel/api/src/main/java/org/sakaiproject/serialization/MapperFactory.java(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- entitybroker/impl/src/java/org/sakaiproject/entitybroker/impl/entityprovider/EntityProviderManagerImpl.java
- kernel/api/src/main/java/org/sakaiproject/serialization/MapperFactory.java
🧰 Additional context used
📓 Path-based instructions (1)
**/*.java
📄 CodeRabbit inference engine (.cursor/rules/logging-rule.mdc)
**/*.java: Use SLF4J parameterized logging (e.g., logger.info("Value is: {}", value)) instead of string concatenation in log statements
Write log messages and code comments in English; never translate log messages
Files:
entitybroker/impl/src/java/org/sakaiproject/entitybroker/impl/EntityBrokerManagerImpl.javaentitybroker/rest/src/java/org/sakaiproject/entitybroker/rest/EntityHandlerImpl.javacontent-review/impl/turnitin/src/main/java/org/sakaiproject/contentreview/turnitin/util/TurnitinAPIUtil.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). (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (13)
entitybroker/impl/src/java/org/sakaiproject/entitybroker/impl/EntityBrokerManagerImpl.java (8)
23-27: LGTM!The new imports align with the bean-introspection refactor, replacing ReflectUtils with standard Java beans and Spring's ReflectionUtils.
Also applies to: 29-29, 31-31, 72-72
693-712: LGTM!The updated title-extraction flow correctly checks if the data is a bean candidate before attempting reflection-based extraction, aligning with the refactor away from ReflectUtils.
718-747: LGTM!The bean-candidate heuristic is well-reasoned and excludes common non-bean types (primitives, collections, JDK types, etc.). This approach is pragmatic for the use case of identifying beans for title extraction.
749-758: LGTM!The title extraction logic correctly prioritizes
@EntityTitleannotations over property name lookups, providing a clear hierarchy for title resolution.
760-778: LGTM!The annotation-based value extraction correctly searches methods and fields, using fail-safe reflection helpers to handle access issues gracefully.
780-806: LGTM!The property-name-based extraction follows standard JavaBeans conventions, with a robust fallback to direct field access.
832-842: LGTM!The reflection helper now uses Spring's
ReflectionUtils.makeAccessibleandReflectionUtils.invokeMethod, which internally handle JDK 17+ strong encapsulation exceptions (SecurityException,InaccessibleObjectException). TheIllegalStateExceptioncatch covers the wrapped exceptions thrown by Spring's utilities, addressing the concerns from the past review comment.
844-854: LGTM!The field-access helper correctly uses Spring's
ReflectionUtilsfor fail-safe field access, consistent with theinvokeMethodapproach.content-review/impl/turnitin/src/main/java/org/sakaiproject/contentreview/turnitin/util/TurnitinAPIUtil.java (5)
260-281: LGTM! Proper resource handling with try-with-resources.The try-with-resources block correctly wraps the
BufferedReader, ensuring the underlyingInputStreamis closed. This addresses the stream-closure concern from the previous review.
283-302: LGTM! Clean DOM traversal with proper null guards.The implementation correctly:
- Uses
LinkedHashMapto preserve element order.- Guards against null document/root.
- Filters for
ELEMENT_NODEtypes only.- Delegates to helper methods for value extraction and merging.
304-322: LGTM! Recursive value extraction handles nested structures correctly.The implementation appropriately:
- Distinguishes leaf nodes (text content) from intermediate nodes (nested elements).
- Recursively builds nested maps for complex structures.
- Trims text and handles null gracefully.
Note: Mixed content (text + child elements) is treated as nested map, which is acceptable for typical API XML responses.
324-340: LGTM! Handles repeated keys by promoting to list.The implementation correctly converts single values to lists when duplicates are encountered. Callers will need to handle both
Object(single occurrence) andList<Object>(multiple occurrences) types, which is standard for XML APIs with repeating elements.
342-344: Add Javadoc for InputStream closure on public API
All internal usages wrap the returned stream in try-with-resources (callTurnitinReturnDocument) and no other internal callers exist. Add Javadoc to callTurnitinWDefaultsReturnInputStream (and callTurnitinReturnInputStream) mandating that callers close the returned InputStream.
entitybroker/rest/src/java/org/sakaiproject/entitybroker/rest/EntityHandlerImpl.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
content-review/impl/turnitin/src/main/java/org/sakaiproject/contentreview/turnitin/util/TurnitinAPIUtil.java (1)
180-186: Complete the generic type fix on the return type.The parameterized logging has been correctly applied (line 184), but the return type remains a raw
Mapinstead ofMap<String, Object>, which is inconsistent with the internal variable type and the previous review feedback.Apply this diff to complete the fix:
- public static Map callTurnitinReturnMap(String apiURL, Map<String,Object> parameters, + public static Map<String, Object> callTurnitinReturnMap(String apiURL, Map<String,Object> parameters, String secretKey, int timeout, Proxy proxy) throws TransientSubmissionException, SubmissionException {As per coding guidelines
🧹 Nitpick comments (1)
content-review/impl/turnitin/src/main/java/org/sakaiproject/contentreview/turnitin/util/TurnitinAPIUtil.java (1)
253-272: Consider including InputStream in try-with-resources for explicit cleanup.The
BufferedReaderin the try-with-resources block will close the underlyingInputStream, but obtaining the stream outside the try block (line 255) makes this implicit. For clarity and to ensure the stream is closed even if an exception occurs between lines 255 and 258, include theInputStreamin the try-with-resources.Apply this diff:
- public static Document callTurnitinReturnDocument(String apiURL, Map<String,Object> parameters, - String secretKey, int timeout, Proxy proxy, boolean isMultipart) throws TransientSubmissionException, SubmissionException { - InputStream inputStream = callTurnitinReturnInputStream(apiURL, parameters, secretKey, timeout, proxy, isMultipart); - Document document = null; - try (BufferedReader in = new BufferedReader(new InputStreamReader(inputStream))) { + try (InputStream inputStream = callTurnitinReturnInputStream(apiURL, parameters, secretKey, timeout, proxy, isMultipart); + BufferedReader in = new BufferedReader(new InputStreamReader(inputStream))) { DocumentBuilderFactory documentBuilderFactory = DocumentBuilderFactory.newInstance();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
content-review/impl/turnitin/src/main/java/org/sakaiproject/contentreview/turnitin/util/TurnitinAPIUtil.java(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.java
📄 CodeRabbit inference engine (.cursor/rules/logging-rule.mdc)
**/*.java: Use SLF4J parameterized logging (e.g., logger.info("Value is: {}", value)) instead of string concatenation in log statements
Write log messages and code comments in English; never translate log messages
Files:
content-review/impl/turnitin/src/main/java/org/sakaiproject/contentreview/turnitin/util/TurnitinAPIUtil.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). (4)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: maven-build
- GitHub Check: maven-build
- GitHub Check: sakai-deploy
🔇 Additional comments (4)
content-review/impl/turnitin/src/main/java/org/sakaiproject/contentreview/turnitin/util/TurnitinAPIUtil.java (4)
37-37: LGTM! DOM and LinkedHashMap imports support the refactoring.The new imports (LinkedHashMap, Map.Entry, Element, Node, NodeList) are appropriate for the DOM-based parsing implementation that replaces XMLTranscoder.
Also applies to: 40-40, 55-57
274-293: LGTM! Well-structured DOM flattening logic.The method correctly handles null documents, filters for element nodes, and delegates to helper methods for value extraction and merging. The use of
LinkedHashMappreserves insertion order.
295-313: LGTM! Recursive value extraction handles both text and nested elements.The method correctly distinguishes between leaf nodes (returning trimmed text) and nested structures (recursively building a map). The null-safe text handling and recursive merging logic are sound.
315-331: LGTM! Duplicate key handling correctly accumulates values into lists.The method appropriately converts duplicate keys into lists, with safe handling of the initial duplicate and subsequent additions. The
@SuppressWarnings("unchecked")cast is justified since the insertion logic is controlled.
|
Warning Rate limit exceeded@ottenhoff has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 7 minutes and 43 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughRemoves the reflectutils dependency across the codebase and replaces its usages with Jackson, DeveloperBeanUtils, Spring reflection utilities, and internal helpers; converts reflection-based serialization annotations to Jackson; modernizes collections to concurrent/standard types; hardens XML mapper XXE protections; adds small API setters and tests. Changes
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (12)
entitybroker/impl/src/java/org/sakaiproject/entitybroker/impl/event/EventReceiverCoordinator.java (1)
49-90: Receivers keyed by ClassLoader still overwrite and leakKeying the
WeakHashMapbyClassLoadermeans every bean from the same loader clobbers the previous one, so only one receiver survives. Worse, the strong reference to theEventReceiverinstance keeps itsClass(and thusClassLoader) alive, so the weak key never clears and the entry sticks around indefinitely—no leak prevention at all. Please store the receivers themselves as the weak keys (e.g.,WeakHashMap<EventReceiver, Boolean>) and snapshot thekeySet()when dispatching, as previously suggested.entitybroker/rest/src/java/org/sakaiproject/entitybroker/rest/EntityDescriptionManager.java (1)
1215-1243: Exclude static members from introspection outputStatic fields are still treated as readable/writable properties and can be marked required, so constants leak into the description metadata. Please skip static members in every path that registers readability/writability or required-ness.
- if (field != null && Modifier.isPublic(field.getModifiers())) { + if (field != null && !Modifier.isStatic(field.getModifiers()) && Modifier.isPublic(field.getModifiers())) { introspection.readableTypes.putIfAbsent(name, field.getType()); introspection.writableTypes.putIfAbsent(name, field.getType()); } @@ - if (Modifier.isPublic(field.getModifiers())) { + if (!Modifier.isStatic(field.getModifiers()) && Modifier.isPublic(field.getModifiers())) { introspection.readableTypes.putIfAbsent(name, field.getType()); introspection.writableTypes.putIfAbsent(name, field.getType()); } - if (field.isAnnotationPresent(EntityFieldRequired.class)) { + if (!Modifier.isStatic(field.getModifiers()) && field.isAnnotationPresent(EntityFieldRequired.class)) { introspection.requiredProperties.add(name); } @@ - if (field != null && field.isAnnotationPresent(EntityFieldRequired.class)) { + if (field != null && !Modifier.isStatic(field.getModifiers()) && field.isAnnotationPresent(EntityFieldRequired.class)) { return true; }entitybroker/utils/src/java/org/sakaiproject/entitybroker/util/EntityDataUtils.java (1)
511-523: Restore interface-annotated IDsEntities that declare their
@EntityIdgetter on an interface now returnnullbecausefindMethodWithAnnotationonly walks the concrete class chain. ReflectUtils used to include interface methods, so this regresses ID discovery. Please extend the search across implemented interfaces (and their parents) to keep parity.for (Class<?> current = type; current != null && current != Object.class; current = current.getSuperclass()) { for (Method method : current.getDeclaredMethods()) { if (method.getParameterCount() == 0 && method.isAnnotationPresent(annotationType)) { return method; } } } + for (Class<?> iface : type.getInterfaces()) { + Method method = findMethodWithAnnotation(iface, annotationType); + if (method != null) { + return method; + } + } return null;entitybroker/rest/src/test/org/sakaiproject/entitybroker/rest/jetty/EntityBrokerServletTest.java (1)
355-364: Inconsistent JSON formatting expectations remain.The assertions at lines 355-359 and 363 still expect spaces after colons (e.g.,
"status":), while the same test method's later assertions (lines 377-381) expect compact format without spaces (e.g.,"status":). This inconsistency should be resolved by aligning all assertions to the actual JSON format produced by Jackson.entitybroker/utils/src/java/org/sakaiproject/entitybroker/util/access/AccessProviderManagerImpl.java (1)
44-55: Race condition between check and remove.The issue previously flagged still applies: between retrieving
current(line 48) and callingremove(line 52), another thread could re-register a provider for the same prefix. UsingprefixMap.remove(prefix, current)ensures atomic compare-and-remove, preventing accidental deletion of a newly registered provider.Apply the previously suggested fix:
public void unregisterProvider(String prefix, T provider) { if (prefix == null) { return; } WeakReference<T> current = prefixMap.get(prefix); if (current != null) { T existing = current.get(); if (existing == null || existing == provider) { - prefixMap.remove(prefix); + prefixMap.remove(prefix, current); } } }entitybroker/core-providers/src/java/org/sakaiproject/entitybroker/providers/SiteEntityProvider.java (1)
124-135: Cache the ownerFieldlookupWe still do a fresh
ReflectionUtils.findField(...)andmakeAccessible(...)on every invocation. Resolve it once and reuse a cachedFieldso site creation/update avoids repeated reflective scans (same concern noted earlier). You can memoise per concrete class to keep it flexible for proxies:+import java.util.concurrent.ConcurrentHashMap; + + private static final Map<Class<?>, Field> CREATED_USER_ID_FIELDS = new ConcurrentHashMap<>(); + private void setCreatedUserId(Site site, String ownerId) { - Field field = ReflectionUtils.findField(site.getClass(), "m_createdUserId"); - if (field == null) { - throw new IllegalStateException("Unable to locate the site owner field"); - } - ReflectionUtils.makeAccessible(field); + Field field = CREATED_USER_ID_FIELDS.computeIfAbsent(site.getClass(), clazz -> { + Field f = ReflectionUtils.findField(clazz, "m_createdUserId"); + if (f == null) { + throw new IllegalStateException("Unable to locate the site owner field on " + clazz.getName()); + } + ReflectionUtils.makeAccessible(f); + return f; + }); try { ReflectionUtils.setField(field, site, ownerId); } catch (IllegalArgumentException e) { throw new IllegalStateException("Unable to set the site owner field", e); } }entitybroker/core-providers/src/java/org/sakaiproject/entitybroker/providers/UserEntityProvider.java (1)
407-425: Unwrap Spring proxies before field lookup.When
userDirectoryServiceis a JDK dynamic proxy (common for interface-based Spring beans),target.getClass()returns the proxy class (e.g.,$Proxy123) instead of the implementation class containingm_separateIdEid. This causesReflectionUtils.findField(...)to returnnull, forcingusesSeparateIdEidtofalseregardless of the actual field value—a regression from the previousFieldUtils-based approach which handled proxy unwrapping.Apply this diff to resolve the target class before field lookup:
+import org.springframework.util.ClassUtils; import org.springframework.util.ReflectionUtils;private Boolean readBooleanField(Object target, String fieldName) { - Field field = ReflectionUtils.findField(target.getClass(), fieldName); + Class<?> targetClass = ClassUtils.getUserClass(target); + Field field = ReflectionUtils.findField(targetClass, fieldName); if (field == null) { return null; }This preserves correct behavior for both proxied and non-proxied services. Based on past review comments.
kernel/kernel-impl/src/main/java/org/sakaiproject/tool/impl/RebuildBreakdownServiceImpl.java (1)
886-900: Critical: Guard against non-serializable types to prevent ClassCastException.The previous review comment on this code segment remains unaddressed.
isObjectSimplereturnstruefor any type assignable from interfaces inSIMPLE_ASSIGNABLE_TYPES(e.g.,CharSequence,TemporalAccessor) without verifying that the concrete class implementsSerializable. This will cause aClassCastExceptionat line 521 inserializeSessionAttributewhen casting to(Serializable) objectfor types likejava.nio.CharBuffer.Apply this diff to add the Serializable check:
private boolean isObjectSimple(Object object) { if (object == null) { return false; } Class<?> clazz = object.getClass(); if (clazz.isPrimitive() || ClassUtils.isPrimitiveOrWrapper(clazz) || clazz.isEnum()) { return true; } for (Class<?> simpleType : SIMPLE_ASSIGNABLE_TYPES) { - if (simpleType.isAssignableFrom(clazz)) { + if (simpleType.isAssignableFrom(clazz) && Serializable.class.isAssignableFrom(clazz)) { return true; } } return false; }entitybroker/impl/src/java/org/sakaiproject/entitybroker/impl/EntityBrokerManagerImpl.java (1)
808-829: Include interface traversal ingetAllMethodsWalking only the concrete class hierarchy drops
@EntityTitleand similar annotations declared on interfaces (common with proxy-based providers), regressing title population. Please fold implemented interfaces into the traversal while avoiding duplicate visits.@@ - private List<Method> getAllMethods(Class<?> type) { - List<Method> methods = new ArrayList<Method>(); - Class<?> current = type; - while (current != null && !Object.class.equals(current)) { - for (Method method : current.getDeclaredMethods()) { - methods.add(method); - } - current = current.getSuperclass(); - } - return methods; - } + private List<Method> getAllMethods(Class<?> type) { + List<Method> methods = new ArrayList<>(); + collectMethods(type, methods, new java.util.HashSet<>()); + return methods; + } + + private void collectMethods(Class<?> type, List<Method> methods, java.util.Set<Class<?>> visited) { + if (type == null || Object.class.equals(type) || !visited.add(type)) { + return; + } + try { + for (Method method : type.getDeclaredMethods()) { + methods.add(method); + } + } catch (SecurityException | java.lang.reflect.InaccessibleObjectException ignore) { + // best-effort: keep traversing + } + collectMethods(type.getSuperclass(), methods, visited); + for (Class<?> iface : type.getInterfaces()) { + collectMethods(iface, methods, visited); + } + }entitybroker/rest/src/java/org/sakaiproject/entitybroker/rest/EntityHandlerImpl.java (1)
28-29: CatchInaccessibleObjectException/SecurityExceptionwhen making members accessibleOn JDK 17+,
ReflectionUtils.makeAccessible(..)delegates tosetAccessible(true)and can raiseInaccessibleObjectExceptionorSecurityExceptionfor strongly encapsulated modules. Because we only catchIllegalStateException, those runtime failures now bubble up and turn the whole REST call into a 500 instead of falling back to the default timestamp. Please broaden the catch (same for both helpers) so we degrade gracefully.-import java.lang.reflect.Field; -import java.lang.reflect.Method; +import java.lang.reflect.Field; +import java.lang.reflect.InaccessibleObjectException; +import java.lang.reflect.Method; @@ - } catch (IllegalStateException e) { + } catch (IllegalStateException | SecurityException | InaccessibleObjectException e) { return null; } @@ - } catch (IllegalStateException e) { + } catch (IllegalStateException | SecurityException | InaccessibleObjectException e) { return null; }Based on learnings
Also applies to: 1214-1235
entitybroker/utils/src/java/org/sakaiproject/entitybroker/util/devhelper/DeveloperBeanUtils.java (2)
21-25: Handle primitive targets safely inconvertReiterating earlier feedback: using
Class#caston a primitive type fails (e.g.,int.class.cast(Integer)throws). Wrap/resolve primitives before casting and verify the converted value’s type.-import org.springframework.beans.BeansException; +import org.springframework.beans.BeansException; +import org.springframework.util.ClassUtils; @@ - public static <T> T convert(Object value, Class<T> type) { + public static <T> T convert(Object value, Class<T> type) { if (type == null) { throw new IllegalArgumentException("Target type must be provided"); } - IdentityHashMap<Object, Boolean> visited = new IdentityHashMap<>(); - Object converted = convertValue(value, type, normalizeDepth(0) - 1, visited); - return type.cast(converted); + Class<?> boxed = ClassUtils.resolvePrimitiveIfNecessary(type); + IdentityHashMap<Object, Boolean> visited = new IdentityHashMap<>(); + Object converted = convertValue(value, boxed, normalizeDepth(0) - 1, visited); + if (converted == null) { + return null; + } + if (!boxed.isInstance(converted)) { + throw new IllegalArgumentException("Converted value is not of requested type: " + boxed + " → " + converted.getClass()); + } + @SuppressWarnings("unchecked") + T result = (T) converted; + return result; }
204-222: Convert using the target bean property typeReiterating the previous review: converting with the source descriptor’s type can mis-convert when the target property expects a different (wider/narrower) type. Ask
targetWrapperfor the property type and fall back only if it is null.- Object converted = convertValue(value, descriptor.getPropertyType(), allowedDepth - 1, visited); + Class<?> targetType = targetWrapper.getPropertyType(name); + Class<?> conversionType = targetType != null ? targetType : descriptor.getPropertyType(); + Object converted = convertValue(value, conversionType, allowedDepth - 1, visited);
🧹 Nitpick comments (2)
entitybroker/rest/src/test/org/sakaiproject/entitybroker/rest/jetty/EntityBrokerServletTest.java (1)
699-699: Assertion updated in commented-out test.This assertion was updated to expect an unescaped URL path, but the entire test method
testBatchAllInvalidJSON()is commented out (lines 677-706). Consider either uncommenting and fixing this test or removing it entirely if it's no longer needed.entitybroker/utils/src/java/org/sakaiproject/entitybroker/util/http/EntityHttpServletResponse.java (1)
612-626: UseHashMapinstead ofLinkedHashMapwhen sorting keys.Since the code explicitly sorts the keys at line 616 (
Collections.sort(keysList)), the insertion-order guarantee ofLinkedHashMapis not utilized. A plainHashMapwould be more appropriate and slightly more efficient here.Apply this diff:
- Map<String, String[]> m = new LinkedHashMap<String, String[]>(); + Map<String, String[]> m = new HashMap<String, String[]>();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (40)
content-review/impl/turnitin/pom.xml(0 hunks)content-review/impl/turnitin/src/main/java/org/sakaiproject/contentreview/turnitin/util/TurnitinAPIUtil.java(4 hunks)docker/tomcat/conf/catalina.properties(0 hunks)entitybroker/api/src/java/org/sakaiproject/entitybroker/entityprovider/extension/EntityData.java(1 hunks)entitybroker/core-providers/pom.xml(0 hunks)entitybroker/core-providers/src/java/org/sakaiproject/entitybroker/providers/SiteEntityProvider.java(5 hunks)entitybroker/core-providers/src/java/org/sakaiproject/entitybroker/providers/UserEntityProvider.java(4 hunks)entitybroker/core-providers/src/java/org/sakaiproject/entitybroker/providers/model/EntityGroup.java(3 hunks)entitybroker/core-providers/src/java/org/sakaiproject/entitybroker/providers/model/EntityMember.java(2 hunks)entitybroker/core-providers/src/java/org/sakaiproject/entitybroker/providers/model/EntitySite.java(3 hunks)entitybroker/core-providers/src/java/org/sakaiproject/entitybroker/providers/model/EntitySitePage.java(2 hunks)entitybroker/core-providers/src/java/org/sakaiproject/entitybroker/providers/model/EntityTool.java(2 hunks)entitybroker/core-providers/src/java/org/sakaiproject/entitybroker/providers/model/EntityUser.java(4 hunks)entitybroker/impl/pom.xml(0 hunks)entitybroker/impl/src/java/org/sakaiproject/entitybroker/impl/EntityBrokerManagerImpl.java(4 hunks)entitybroker/impl/src/java/org/sakaiproject/entitybroker/impl/entityprovider/EntityProviderManagerImpl.java(6 hunks)entitybroker/impl/src/java/org/sakaiproject/entitybroker/impl/event/EventReceiverCoordinator.java(3 hunks)entitybroker/pom.xml(0 hunks)entitybroker/rest/pom.xml(1 hunks)entitybroker/rest/src/java/org/sakaiproject/entitybroker/rest/EntityBatchHandler.java(9 hunks)entitybroker/rest/src/java/org/sakaiproject/entitybroker/rest/EntityDescriptionManager.java(8 hunks)entitybroker/rest/src/java/org/sakaiproject/entitybroker/rest/EntityEncodingManager.java(18 hunks)entitybroker/rest/src/java/org/sakaiproject/entitybroker/rest/EntityHandlerImpl.java(11 hunks)entitybroker/rest/src/test/java/org/sakaiproject/entitybroker/rest/EntityEncodingManagerJsonTest.java(1 hunks)entitybroker/rest/src/test/org/sakaiproject/entitybroker/rest/EntityEncodingManagerTest.java(9 hunks)entitybroker/rest/src/test/org/sakaiproject/entitybroker/rest/jetty/EntityBrokerServletTest.java(7 hunks)entitybroker/utils/pom.xml(1 hunks)entitybroker/utils/src/java/org/sakaiproject/entitybroker/util/EntityDataUtils.java(5 hunks)entitybroker/utils/src/java/org/sakaiproject/entitybroker/util/access/AccessProviderManagerImpl.java(2 hunks)entitybroker/utils/src/java/org/sakaiproject/entitybroker/util/core/EntityProviderMethodStoreImpl.java(3 hunks)entitybroker/utils/src/java/org/sakaiproject/entitybroker/util/devhelper/AbstractDeveloperHelperService.java(7 hunks)entitybroker/utils/src/java/org/sakaiproject/entitybroker/util/devhelper/DeveloperBeanUtils.java(1 hunks)entitybroker/utils/src/java/org/sakaiproject/entitybroker/util/devhelper/DeveloperHelperServiceMock.java(2 hunks)entitybroker/utils/src/java/org/sakaiproject/entitybroker/util/http/EntityHttpServletResponse.java(2 hunks)entitybroker/utils/src/java/org/sakaiproject/entitybroker/util/request/RequestStorageImpl.java(2 hunks)entitybroker/utils/src/test/org/sakaiproject/entitybroker/util/reflect/ReflectUtilTest.java(0 hunks)kernel/api/src/main/java/org/sakaiproject/serialization/MapperFactory.java(7 hunks)kernel/kernel-impl/pom.xml(0 hunks)kernel/kernel-impl/src/main/java/org/sakaiproject/tool/impl/RebuildBreakdownServiceImpl.java(5 hunks)master/pom.xml(0 hunks)
💤 Files with no reviewable changes (8)
- content-review/impl/turnitin/pom.xml
- docker/tomcat/conf/catalina.properties
- kernel/kernel-impl/pom.xml
- master/pom.xml
- entitybroker/pom.xml
- entitybroker/core-providers/pom.xml
- entitybroker/utils/src/test/org/sakaiproject/entitybroker/util/reflect/ReflectUtilTest.java
- entitybroker/impl/pom.xml
🧰 Additional context used
📓 Path-based instructions (2)
**/*.java
📄 CodeRabbit inference engine (.cursor/rules/logging-rule.mdc)
**/*.java: Use SLF4J parameterized logging (e.g., logger.info("Value is: {}", value)) instead of string concatenation in log statements
Write log messages and code comments in English; never translate log messages
Files:
entitybroker/api/src/java/org/sakaiproject/entitybroker/entityprovider/extension/EntityData.javaentitybroker/core-providers/src/java/org/sakaiproject/entitybroker/providers/model/EntityUser.javaentitybroker/core-providers/src/java/org/sakaiproject/entitybroker/providers/model/EntityTool.javaentitybroker/rest/src/test/org/sakaiproject/entitybroker/rest/jetty/EntityBrokerServletTest.javaentitybroker/core-providers/src/java/org/sakaiproject/entitybroker/providers/model/EntitySite.javaentitybroker/impl/src/java/org/sakaiproject/entitybroker/impl/event/EventReceiverCoordinator.javaentitybroker/utils/src/java/org/sakaiproject/entitybroker/util/access/AccessProviderManagerImpl.javaentitybroker/utils/src/java/org/sakaiproject/entitybroker/util/devhelper/DeveloperHelperServiceMock.javaentitybroker/core-providers/src/java/org/sakaiproject/entitybroker/providers/model/EntityGroup.javaentitybroker/impl/src/java/org/sakaiproject/entitybroker/impl/entityprovider/EntityProviderManagerImpl.javaentitybroker/rest/src/test/java/org/sakaiproject/entitybroker/rest/EntityEncodingManagerJsonTest.javaentitybroker/rest/src/java/org/sakaiproject/entitybroker/rest/EntityHandlerImpl.javaentitybroker/rest/src/java/org/sakaiproject/entitybroker/rest/EntityDescriptionManager.javakernel/api/src/main/java/org/sakaiproject/serialization/MapperFactory.javakernel/kernel-impl/src/main/java/org/sakaiproject/tool/impl/RebuildBreakdownServiceImpl.javaentitybroker/rest/src/java/org/sakaiproject/entitybroker/rest/EntityEncodingManager.javaentitybroker/core-providers/src/java/org/sakaiproject/entitybroker/providers/model/EntitySitePage.javaentitybroker/utils/src/java/org/sakaiproject/entitybroker/util/http/EntityHttpServletResponse.javaentitybroker/core-providers/src/java/org/sakaiproject/entitybroker/providers/model/EntityMember.javaentitybroker/impl/src/java/org/sakaiproject/entitybroker/impl/EntityBrokerManagerImpl.javaentitybroker/utils/src/java/org/sakaiproject/entitybroker/util/request/RequestStorageImpl.javacontent-review/impl/turnitin/src/main/java/org/sakaiproject/contentreview/turnitin/util/TurnitinAPIUtil.javaentitybroker/core-providers/src/java/org/sakaiproject/entitybroker/providers/SiteEntityProvider.javaentitybroker/utils/src/java/org/sakaiproject/entitybroker/util/EntityDataUtils.javaentitybroker/utils/src/java/org/sakaiproject/entitybroker/util/core/EntityProviderMethodStoreImpl.javaentitybroker/utils/src/java/org/sakaiproject/entitybroker/util/devhelper/AbstractDeveloperHelperService.javaentitybroker/utils/src/java/org/sakaiproject/entitybroker/util/devhelper/DeveloperBeanUtils.javaentitybroker/rest/src/test/org/sakaiproject/entitybroker/rest/EntityEncodingManagerTest.javaentitybroker/rest/src/java/org/sakaiproject/entitybroker/rest/EntityBatchHandler.javaentitybroker/core-providers/src/java/org/sakaiproject/entitybroker/providers/UserEntityProvider.java
**/pom.xml
📄 CodeRabbit inference engine (AGENTS.md)
Target Java 17 for trunk builds (update maven-compiler-plugin source/target accordingly)
Files:
entitybroker/rest/pom.xmlentitybroker/utils/pom.xml
🧠 Learnings (1)
📚 Learning: 2025-10-03T14:48:27.423Z
Learnt from: ern
PR: sakaiproject/sakai#14139
File: entitybroker/impl/src/java/org/sakaiproject/entitybroker/impl/EntityBrokerManagerImpl.java:832-841
Timestamp: 2025-10-03T14:48:27.423Z
Learning: In newer JDKs (JDK 17+), `java.lang.reflect.InaccessibleObjectException` can be thrown by `setAccessible()` calls due to strong encapsulation. This was a key reason the old reflectutils library needed to be updated. When performing reflection operations, both `SecurityException` and `InaccessibleObjectException` should be caught to ensure fail-safe behavior.
Applied to files:
entitybroker/rest/src/java/org/sakaiproject/entitybroker/rest/EntityHandlerImpl.javaentitybroker/impl/src/java/org/sakaiproject/entitybroker/impl/EntityBrokerManagerImpl.java
🧬 Code graph analysis (7)
entitybroker/core-providers/src/java/org/sakaiproject/entitybroker/providers/model/EntityUser.java (1)
entitybroker/core-providers/src/java/org/sakaiproject/entitybroker/providers/model/EntitySitePage.java (1)
JsonIgnoreProperties(42-385)
entitybroker/core-providers/src/java/org/sakaiproject/entitybroker/providers/model/EntitySite.java (1)
entitybroker/core-providers/src/java/org/sakaiproject/entitybroker/providers/model/EntitySitePage.java (1)
JsonIgnoreProperties(42-385)
entitybroker/core-providers/src/java/org/sakaiproject/entitybroker/providers/model/EntityGroup.java (1)
entitybroker/core-providers/src/java/org/sakaiproject/entitybroker/providers/model/EntitySitePage.java (1)
JsonIgnoreProperties(42-385)
entitybroker/rest/src/test/java/org/sakaiproject/entitybroker/rest/EntityEncodingManagerJsonTest.java (1)
kernel/api/src/main/java/org/sakaiproject/serialization/MapperFactory.java (1)
MapperFactory(37-454)
entitybroker/rest/src/java/org/sakaiproject/entitybroker/rest/EntityEncodingManager.java (1)
kernel/api/src/main/java/org/sakaiproject/serialization/MapperFactory.java (1)
MapperFactory(37-454)
entitybroker/utils/src/java/org/sakaiproject/entitybroker/util/devhelper/AbstractDeveloperHelperService.java (1)
kernel/api/src/main/java/org/sakaiproject/serialization/MapperFactory.java (1)
MapperFactory(37-454)
entitybroker/rest/src/test/org/sakaiproject/entitybroker/rest/EntityEncodingManagerTest.java (1)
kernel/api/src/main/java/org/sakaiproject/serialization/MapperFactory.java (1)
MapperFactory(37-454)
⏰ 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: sakai-deploy
- GitHub Check: maven-build
- GitHub Check: maven-build
🔇 Additional comments (56)
entitybroker/rest/src/test/org/sakaiproject/entitybroker/rest/jetty/EntityBrokerServletTest.java (1)
176-176: LGTM!The assertion correctly expects unescaped URL paths in the JSON response, which aligns with Jackson's default JSON encoding behavior.
entitybroker/utils/src/java/org/sakaiproject/entitybroker/util/access/AccessProviderManagerImpl.java (4)
23-25: LGTM! Thread-safe storage with weak references.The switch to
ConcurrentHashMapwithWeakReference-wrapped values provides thread-safe access while allowing garbage collection of unreferenced providers. This is a solid improvement over the previous implementation.Also applies to: 35-35
37-42: LGTM! Proper input validation added.The null checks for both
prefixandproviderwithIllegalArgumentExceptionfollow best practices for defensive programming. TheWeakReferencewrapping is correctly implemented.
57-61: LGTM! Unconditional removal is appropriate here.This overload correctly removes the provider by prefix alone. The null-check and direct removal are appropriate for this use case where the caller intends to remove whatever is registered for the prefix.
63-76: LGTM! Correct atomic cleanup of stale references.The use of
prefixMap.remove(prefix, reference)at line 73 is the correct approach—it atomically removes only if the map still contains the exact reference you inspected, preventing accidental removal of a concurrently registered provider. The cleanup of expiredWeakReferenceentries is good housekeeping.entitybroker/core-providers/src/java/org/sakaiproject/entitybroker/providers/UserEntityProvider.java (3)
18-18: LGTM! Standard reflection imports.The imports for
java.lang.reflect.Fieldandorg.springframework.util.ReflectionUtilsare appropriate replacements for the removed reflectutils library.Also applies to: 55-55
427-442: LGTM! Robust boolean parsing.The
parseBooleanmethod correctly handles null, empty strings, and case-insensitive true/false values with appropriate error handling.
387-387: Usage of helper methods is correct.The calls to
parseBooleanandreadBooleanFieldare appropriately wrapped in exception handling. Once the proxy unwrapping issue inreadBooleanFieldis resolved, this code will function correctly.Also applies to: 396-396
entitybroker/rest/src/java/org/sakaiproject/entitybroker/rest/EntityBatchHandler.java (9)
27-29: LGTM!The import additions for
ArraysandLinkedHashMapare correct and align with the migration away from reflectutils library to standard Java collections.
221-222: LGTM!Using
LinkedHashMaphere correctly preserves the insertion order of batch references, which is critical for thereintegrateDataContentmethod (line 744 comment confirms "looping order is critical") and for consistent header ordering inapplyOverallHeaders.
373-373: LGTM!The replacement of
ArrayUtils.arrayToString(refs)with the standardArrays.toString(refs)is correct and removes the dependency on Apache Commons Lang, aligning with the PR's goal to remove reflectutils and related utilities.
410-448: LGTM!The migration from
ArrayOrderedMaptoLinkedHashMapfor reference parameter storage is correct. Using the genericMapinterface for the return type is good practice, andLinkedHashMapappropriately preserves parameter order within each reference group.
615-615: LGTM!Using
LinkedHashMapfor HTTP request parameters ensures consistent parameter ordering, which can be important for URL generation and debugging.
739-764: LGTM!Changing the parameter type to the generic
Mapinterface is good practice. The method correctly relies on the insertion order being preserved (as stated in the line 744 comment), which is guaranteed by theLinkedHashMappassed from the caller at line 221.
774-794: LGTM!Changing the parameter type to the generic
Mapinterface improves flexibility and follows best practices for API design.
853-855: LGTM!The no-argument constructor addition is appropriate for Jackson-based serialization, aligning with this PR's migration strategy. This is a standard pattern for DTOs that need to be serialized/deserialized.
885-887: LGTM!The no-argument constructor addition is necessary for Jackson-based serialization and is consistent with the similar change in
ResponseError. This supports the PR's goal of migrating to Jackson for encoding/decoding.kernel/kernel-impl/src/main/java/org/sakaiproject/tool/impl/RebuildBreakdownServiceImpl.java (2)
26-28: LGTM! New imports support the refactored type checking.The new imports (URI, URL, TemporalAccessor, ClassUtils) are necessary for the expanded type handling in
SIMPLE_ASSIGNABLE_TYPESand the migration away from reflectutils to ClassUtils-based inspection.Also applies to: 38-38
907-914: LGTM! Simplified map type check.The refactored
isObjectMapmethod correctly uses standard Java reflection (Map.class.isAssignableFrom) to determine if an object is a Map. This change aligns with the PR objective to remove reflectutils dependencies.entitybroker/api/src/java/org/sakaiproject/entitybroker/entityprovider/extension/EntityData.java (1)
228-233: Constructor addition aligns with Jackson requirementsThis no-arg constructor keeps EntityData Jackson-friendly without impacting existing callers. 👍
entitybroker/rest/src/test/java/org/sakaiproject/entitybroker/rest/EntityEncodingManagerJsonTest.java (1)
43-55: Great regression coverageNice targeted check that non-static inner classes serialize cleanly through the new encoder path.
kernel/api/src/main/java/org/sakaiproject/serialization/MapperFactory.java (1)
166-184: Optional Jdk8Module hookup looks solidAppreciate the reflective registration with graceful fallback—lets us benefit from
Optionalsupport when present without hard-wiring the dependency.entitybroker/rest/src/test/org/sakaiproject/entitybroker/rest/EntityEncodingManagerTest.java (1)
581-621: Stronger JSON assertionsThese checks guard against regressions in wrapper handling and transient-field leakage—great addition.
entitybroker/rest/src/java/org/sakaiproject/entitybroker/rest/EntityEncodingManager.java (1)
122-130: JSONP sanitization upgradeThe stricter callback validation plus
/**/prefix keeps JSONP responses safer without breaking existing clients.entitybroker/core-providers/src/java/org/sakaiproject/entitybroker/providers/model/EntityMember.java (4)
22-22: LGTM: Correct Jackson import added.The import correctly supports the @JsonIgnore annotation used on the getRole() method.
263-269: LGTM: Correct migration from ReflectIgnoreClassFields to Jackson annotations.The @JsonIgnore annotation on the getRole() method correctly preserves the serialization exclusion behavior previously provided by the class-level @ReflectIgnoreClassFields({"role"}) annotation.
22-22: LGTM: Jackson import added for serialization control.The import of
com.fasterxml.jackson.annotation.JsonIgnoreis correct and aligns with the broader PR goal of migrating away from reflectutils to Jackson-based serialization annotations.
263-269: LGTM: Method-level @JsonIgnore correctly replaces class-level exclusion.The
@JsonIgnoreannotation ongetRole()correctly prevents the role field from being serialized in JSON, replacing the previous@ReflectIgnoreClassFields({"role"})class-level annotation. This is the appropriate Jackson-based approach for controlling serialization exposure.entitybroker/core-providers/src/java/org/sakaiproject/entitybroker/providers/model/EntityUser.java (8)
24-25: LGTM: Correct Jackson imports added.Both JsonIgnore and JsonIgnoreProperties imports are correctly added to support the Jackson annotations used for serialization control.
46-46: LGTM: Class-level serialization exclusions correctly migrated to Jackson.The @JsonIgnoreProperties annotation correctly excludes createdBy, modifiedBy, and properties from JSON serialization, preserving the behavior previously provided by ReflectIgnoreClassFields.
209-209: LGTM: Comment updated to reflect Jackson-based exclusion.The comment correctly documents that the fields below are excluded from JSON serialization via the @JsonIgnoreProperties annotation. This addresses the previous review concern about the outdated TODO comment.
303-309: LGTM: @JsonIgnore correctly excludes toXml from serialization.The @JsonIgnore annotation on the toXml method correctly prevents Jackson from treating this method as a property getter during JSON serialization.
24-25: LGTM: Jackson imports added for serialization control.The imports of
com.fasterxml.jackson.annotation.JsonIgnoreandcom.fasterxml.jackson.annotation.JsonIgnorePropertiesare correct and align with the PR goal of migrating from reflectutils to Jackson-based serialization annotations.
46-46: LGTM: Class-level @JsonIgnoreProperties correctly replaces ReflectIgnoreClassFields.The
@JsonIgnoreProperties(value = {"createdBy", "modifiedBy", "properties"})annotation correctly excludes these fields from JSON serialization, replacing the previous@ReflectIgnoreClassFieldsannotation. This is the appropriate Jackson-based approach for class-level serialization control.
209-209: LGTM: Comment updated to reflect current serialization behavior.The comment accurately reflects that the methods below are excluded from JSON serialization via the class-level
@JsonIgnorePropertiesannotation. This addresses the previous review concern about the outdated TODO comment.
303-309: LGTM: @JsonIgnore on toXml method prevents it from being serialized.The
@JsonIgnoreannotation on thetoXml()method correctly prevents it from being treated as a JSON property during serialization. This is consistent with similar changes across other entity model classes in this PR.entitybroker/core-providers/src/java/org/sakaiproject/entitybroker/providers/model/EntityGroup.java (6)
27-28: LGTM: Correct Jackson imports added.Both JsonIgnore and JsonIgnoreProperties imports are correctly added to support the Jackson annotations used in this class.
54-54: LGTM: Class-level serialization exclusions correctly migrated to Jackson.The @JsonIgnoreProperties annotation correctly excludes legacy Group interface properties from JSON serialization, aligning with the pattern used in other entity model classes like EntitySitePage.
359-365: LGTM: @JsonIgnore correctly excludes toXml from serialization.The @JsonIgnore annotation on the toXml method correctly prevents Jackson from treating it as a property getter during JSON serialization.
27-28: LGTM: Jackson imports added for serialization control.The imports of
com.fasterxml.jackson.annotation.JsonIgnoreandcom.fasterxml.jackson.annotation.JsonIgnorePropertiesare correct and align with the PR goal of migrating from reflectutils to Jackson-based serialization annotations.
54-54: LGTM: Class-level @JsonIgnoreProperties correctly replaces ReflectIgnoreClassFields.The
@JsonIgnorePropertiesannotation correctly excludes internal and legacy fields from JSON serialization, replacing the previous@ReflectIgnoreClassFieldsannotation. The excluded fields are appropriate for this entity model.
359-365: LGTM: @JsonIgnore on toXml method prevents it from being serialized.The
@JsonIgnoreannotation on thetoXml()method correctly prevents it from being treated as a JSON property during serialization, replacing the previous@ReflectTransientannotation. This is consistent with similar changes across other entity model classes in this PR.entitybroker/core-providers/src/java/org/sakaiproject/entitybroker/providers/model/EntitySitePage.java (4)
25-25: LGTM: Correct Jackson import added.The JsonIgnoreProperties import is correctly added to support the class-level annotation.
42-42: LGTM: Class-level serialization exclusions correctly migrated to Jackson.The @JsonIgnoreProperties annotation correctly excludes legacy SitePage interface properties from JSON serialization, maintaining consistency with other entity model classes in this PR.
25-25: LGTM: Jackson import added for serialization control.The import of
com.fasterxml.jackson.annotation.JsonIgnorePropertiesis correct and aligns with the PR goal of migrating from reflectutils to Jackson-based serialization annotations.
42-42: LGTM: Class-level @JsonIgnoreProperties correctly replaces ReflectIgnoreClassFields.The
@JsonIgnoreProperties(value = {"createdBy", "modifiedBy", "containingSite", "properties", "propertiesEdit", "tools"})annotation correctly excludes these fields from JSON serialization, replacing the previous@ReflectIgnoreClassFieldsannotation. This is the appropriate Jackson-based approach for class-level serialization control.entitybroker/core-providers/src/java/org/sakaiproject/entitybroker/providers/model/EntityTool.java (4)
21-21: LGTM: Correct Jackson import added.The JsonIgnore import is correctly added to support the annotation on the getAccessSecurity() method.
122-128: LGTM: Method-level @JsonIgnore correctly excludes accessSecurity.The @JsonIgnore annotation on getAccessSecurity() correctly prevents the AccessSecurity enum from being serialized, replacing the previous class-level ReflectIgnoreClassFields annotation. Note that access information remains available through the getAccess() method which returns a String representation.
21-21: LGTM: Jackson import added for serialization control.The import of
com.fasterxml.jackson.annotation.JsonIgnoreis correct and aligns with the broader PR goal of migrating away from reflectutils to Jackson-based serialization annotations.
122-128: LGTM: Method-level @JsonIgnore correctly replaces class-level exclusion.The
@JsonIgnoreannotation ongetAccessSecurity()correctly prevents the AccessSecurity enum from being serialized in JSON, replacing the previous@ReflectIgnoreClassFields({"accessSecurity"})class-level annotation. The separategetAccess()method (line 130) returns a String representation that can be serialized, maintaining the intended API exposure.entitybroker/core-providers/src/java/org/sakaiproject/entitybroker/providers/model/EntitySite.java (6)
31-32: LGTM: Correct Jackson imports added.Both JsonIgnore and JsonIgnoreProperties imports are correctly added to support the Jackson annotations used in this class.
64-64: LGTM: Class-level serialization exclusions correctly migrated to Jackson.The @JsonIgnoreProperties annotation correctly excludes legacy Site interface properties from JSON serialization, with a comprehensive list covering all methods that should not be exposed via REST APIs.
754-760: LGTM: @JsonIgnore correctly excludes toXml from serialization.The @JsonIgnore annotation on the toXml method correctly prevents Jackson from treating it as a property getter during JSON serialization, replacing the previous @ReflectTransient annotation.
31-32: LGTM: Jackson imports added for serialization control.The imports of
com.fasterxml.jackson.annotation.JsonIgnoreandcom.fasterxml.jackson.annotation.JsonIgnorePropertiesare correct and align with the PR goal of migrating from reflectutils to Jackson-based serialization annotations.
64-64: LGTM: Class-level @JsonIgnoreProperties correctly replaces ReflectIgnoreClassFields.The
@JsonIgnorePropertiesannotation correctly excludes internal and legacy fields from JSON serialization, replacing the previous@ReflectIgnoreClassFieldsannotation. The excluded fields are appropriate for this entity model and align with similar changes across other entity classes in this PR.
754-760: LGTM: @JsonIgnore on toXml method prevents it from being serialized.The
@JsonIgnoreannotation on thetoXml()method correctly prevents it from being treated as a JSON property during serialization, replacing the previous@ReflectTransientannotation. This is consistent with similar changes across other entity model classes in this PR.
...mpl/turnitin/src/main/java/org/sakaiproject/contentreview/turnitin/util/TurnitinAPIUtil.java
Show resolved
Hide resolved
entitybroker/impl/src/java/org/sakaiproject/entitybroker/impl/EntityBrokerManagerImpl.java
Show resolved
Hide resolved
| private Map<String, EntityCodec> codecs; | ||
| private EntityCodec getCodec(String format) { | ||
| if (codecs == null) { | ||
| codecs = new ConcurrentHashMap<>(); | ||
| codecs.put(Formats.JSON, new JsonCodec()); | ||
| codecs.put(Formats.XML, new XmlCodec()); | ||
| codecs.put(Formats.HTML, new HtmlCodec()); | ||
| } | ||
| Transcoder transcoder = transcoders.get(format); | ||
| if (transcoder == null) { | ||
| throw new IllegalArgumentException("Failed to find a transcoder for format, none exists, cannot encode or decode data for format: " + format); | ||
| EntityCodec codec = codecs.get(format); | ||
| if (codec == null) { | ||
| throw new IllegalArgumentException("Failed to find an encoder/decoder for format: " + format); | ||
| } | ||
| return transcoder; | ||
| return codec; | ||
| } |
There was a problem hiding this comment.
Fix race when populating codec registry
Because the field is assigned before the put calls complete, a concurrent caller can observe the empty map, skip initialization, and hit the IllegalArgumentException even though the codec should exist. Populate a local instance first (and publish it only after it is ready) to avoid the race; marking the field volatile ensures visibility.
- private Map<String, EntityCodec> codecs;
+ private volatile Map<String, EntityCodec> codecs;
@@
- if (codecs == null) {
- codecs = new ConcurrentHashMap<>();
- codecs.put(Formats.JSON, new JsonCodec());
- codecs.put(Formats.XML, new XmlCodec());
- codecs.put(Formats.HTML, new HtmlCodec());
- }
- EntityCodec codec = codecs.get(format);
+ Map<String, EntityCodec> local = codecs;
+ if (local == null) {
+ Map<String, EntityCodec> initialized = new ConcurrentHashMap<>();
+ initialized.put(Formats.JSON, new JsonCodec());
+ initialized.put(Formats.XML, new XmlCodec());
+ initialized.put(Formats.HTML, new HtmlCodec());
+ local = initialized;
+ codecs = local;
+ }
+ EntityCodec codec = local.get(format);📝 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.
| private Map<String, EntityCodec> codecs; | |
| private EntityCodec getCodec(String format) { | |
| if (codecs == null) { | |
| codecs = new ConcurrentHashMap<>(); | |
| codecs.put(Formats.JSON, new JsonCodec()); | |
| codecs.put(Formats.XML, new XmlCodec()); | |
| codecs.put(Formats.HTML, new HtmlCodec()); | |
| } | |
| Transcoder transcoder = transcoders.get(format); | |
| if (transcoder == null) { | |
| throw new IllegalArgumentException("Failed to find a transcoder for format, none exists, cannot encode or decode data for format: " + format); | |
| EntityCodec codec = codecs.get(format); | |
| if (codec == null) { | |
| throw new IllegalArgumentException("Failed to find an encoder/decoder for format: " + format); | |
| } | |
| return transcoder; | |
| return codec; | |
| } | |
| // Publish‐after‐populate to avoid races; volatile for visibility | |
| private volatile Map<String, EntityCodec> codecs; | |
| private EntityCodec getCodec(String format) { | |
| Map<String, EntityCodec> local = codecs; | |
| if (local == null) { | |
| // Fully populate before assigning to the shared field | |
| Map<String, EntityCodec> initialized = new ConcurrentHashMap<>(); | |
| initialized.put(Formats.JSON, new JsonCodec()); | |
| initialized.put(Formats.XML, new XmlCodec()); | |
| initialized.put(Formats.HTML, new HtmlCodec()); | |
| local = initialized; | |
| codecs = local; | |
| } | |
| EntityCodec codec = local.get(format); | |
| if (codec == null) { | |
| throw new IllegalArgumentException( | |
| "Failed to find an encoder/decoder for format: " + format); | |
| } | |
| return codec; | |
| } |
🤖 Prompt for AI Agents
In
entitybroker/utils/src/java/org/sakaiproject/entitybroker/util/devhelper/AbstractDeveloperHelperService.java
around lines 377-390, the codecs field is published before it's fully populated
causing a race; mark the field "private volatile Map<String, EntityCodec>
codecs;" then when initializing create and populate a local map instance (e.g.,
new ConcurrentHashMap<>() or HashMap), put the JSON/XML/HTML codecs into that
local map, and only after it's fully populated assign it to the codecs field;
leave the subsequent lookup and IllegalArgumentException unchanged.
| private static final List<Class<?>> SIMPLE_ASSIGNABLE_TYPES = Arrays.asList( | ||
| CharSequence.class, | ||
| Number.class, | ||
| Date.class, | ||
| Calendar.class, | ||
| Locale.class, | ||
| URI.class, | ||
| URL.class, | ||
| UUID.class, | ||
| TemporalAccessor.class | ||
| ); |
There was a problem hiding this comment.
Consider the serialization risk with interface-based type checks.
SIMPLE_ASSIGNABLE_TYPES includes interfaces (CharSequence, Number, TemporalAccessor) that not all implementations guarantee to be Serializable. For example, java.nio.CharBuffer implements CharSequence but not Serializable. Without an additional Serializable check in isObjectSimple, this can lead to ClassCastException when the code attempts to cast to Serializable in serializeSessionAttribute (line 521).
See the review comment on lines 886-900 for the recommended fix.
🤖 Prompt for AI Agents
In
kernel/kernel-impl/src/main/java/org/sakaiproject/tool/impl/RebuildBreakdownServiceImpl.java
around lines 80-90, the SIMPLE_ASSIGNABLE_TYPES list contains interfaces that do
not guarantee implementations are Serializable; update the object-type checks so
that isObjectSimple only treats an object as "simple" if it matches one of those
types AND the actual instance is Serializable, and also change
serializeSessionAttribute (around line 521) to guard the cast with an instanceof
Serializable check (or otherwise handle non-serializable instances) as
recommended in the review comments around lines 886-900.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
entitybroker/impl/src/java/org/sakaiproject/entitybroker/impl/EntityBrokerManagerImpl.java (1)
847-864: Handle strong-encapsulation exceptions when accessing members.
ReflectionUtils.makeAccessible(...)can throwSecurityException/InaccessibleObjectExceptionon JDK 17+ modules. Right now those bubble out and breakpopulateEntityData, regressing the fail-safe behaviour we had before. Please swallow them alongsideIllegalStateException. Based on learningstry { ReflectionUtils.makeAccessible(method); return ReflectionUtils.invokeMethod(method, bean); - } catch (IllegalStateException e) { + } catch (IllegalStateException | SecurityException | java.lang.reflect.InaccessibleObjectException e) { return null; } ... try { ReflectionUtils.makeAccessible(field); return ReflectionUtils.getField(field, bean); - } catch (IllegalStateException e) { + } catch (IllegalStateException | SecurityException | java.lang.reflect.InaccessibleObjectException e) { return null; }
🧹 Nitpick comments (1)
entitybroker/rest/src/java/org/sakaiproject/entitybroker/rest/EntityDescriptionManager.java (1)
1200-1246: Static field exclusion properly implemented; consider caching for performance.The implementation correctly addresses the previous review concern by excluding static fields and methods from property detection at all relevant locations (lines 1214, 1217, 1221, 1237, 1241). The introspection logic properly:
- Uses Java's
Introspector.getBeanInfo()for standard bean properties- Falls back to direct field inspection for public fields
- Walks the inheritance hierarchy via
getAllFields()andfindField()- Detects required properties from
@EntityFieldRequiredannotationsThe
IntrospectionExceptionhandling (line 1229) throwsIllegalStateException, which will fail fast if bean introspection fails—reasonable since this indicates a programming error.Performance consideration: The method re-introspects the same class on every call without caching. For systems with many entity types or frequent description requests, consider caching the
PropertyIntrospectionresults keyed byClass<?>.Based on past review feedback: The static field exclusion issue has been resolved.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
entitybroker/impl/src/java/org/sakaiproject/entitybroker/impl/EntityBrokerManagerImpl.java(4 hunks)entitybroker/impl/src/java/org/sakaiproject/entitybroker/impl/event/EventReceiverCoordinator.java(4 hunks)entitybroker/rest/src/java/org/sakaiproject/entitybroker/rest/EntityDescriptionManager.java(8 hunks)entitybroker/rest/src/java/org/sakaiproject/entitybroker/rest/EntityHandlerImpl.java(11 hunks)entitybroker/rest/src/test/org/sakaiproject/entitybroker/rest/jetty/EntityBrokerServletTest.java(7 hunks)entitybroker/utils/src/java/org/sakaiproject/entitybroker/util/EntityDataUtils.java(5 hunks)entitybroker/utils/src/java/org/sakaiproject/entitybroker/util/access/AccessProviderManagerImpl.java(2 hunks)entitybroker/utils/src/java/org/sakaiproject/entitybroker/util/devhelper/DeveloperBeanUtils.java(1 hunks)kernel/kernel-impl/src/main/java/org/sakaiproject/tool/impl/RebuildBreakdownServiceImpl.java(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- kernel/kernel-impl/src/main/java/org/sakaiproject/tool/impl/RebuildBreakdownServiceImpl.java
- entitybroker/utils/src/java/org/sakaiproject/entitybroker/util/devhelper/DeveloperBeanUtils.java
🧰 Additional context used
📓 Path-based instructions (1)
**/*.java
📄 CodeRabbit inference engine (.cursor/rules/logging-rule.mdc)
**/*.java: Use SLF4J parameterized logging (e.g., logger.info("Value is: {}", value)) instead of string concatenation in log statements
Write log messages and code comments in English; never translate log messages
Files:
entitybroker/utils/src/java/org/sakaiproject/entitybroker/util/EntityDataUtils.javaentitybroker/rest/src/java/org/sakaiproject/entitybroker/rest/EntityDescriptionManager.javaentitybroker/rest/src/test/org/sakaiproject/entitybroker/rest/jetty/EntityBrokerServletTest.javaentitybroker/utils/src/java/org/sakaiproject/entitybroker/util/access/AccessProviderManagerImpl.javaentitybroker/impl/src/java/org/sakaiproject/entitybroker/impl/event/EventReceiverCoordinator.javaentitybroker/rest/src/java/org/sakaiproject/entitybroker/rest/EntityHandlerImpl.javaentitybroker/impl/src/java/org/sakaiproject/entitybroker/impl/EntityBrokerManagerImpl.java
🧠 Learnings (1)
📚 Learning: 2025-10-03T14:48:27.423Z
Learnt from: ern
PR: sakaiproject/sakai#14139
File: entitybroker/impl/src/java/org/sakaiproject/entitybroker/impl/EntityBrokerManagerImpl.java:832-841
Timestamp: 2025-10-03T14:48:27.423Z
Learning: In newer JDKs (JDK 17+), `java.lang.reflect.InaccessibleObjectException` can be thrown by `setAccessible()` calls due to strong encapsulation. This was a key reason the old reflectutils library needed to be updated. When performing reflection operations, both `SecurityException` and `InaccessibleObjectException` should be caught to ensure fail-safe behavior.
Applied to files:
entitybroker/rest/src/java/org/sakaiproject/entitybroker/rest/EntityHandlerImpl.javaentitybroker/impl/src/java/org/sakaiproject/entitybroker/impl/EntityBrokerManagerImpl.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 (25)
entitybroker/utils/src/java/org/sakaiproject/entitybroker/util/access/AccessProviderManagerImpl.java (1)
48-54: Nice atomic cleanup of stale registrations.The switch to
remove(prefix, current)tightens the race window and ensures we don’t tear down a freshly re-registered provider. Thanks for closing that gap.entitybroker/rest/src/test/org/sakaiproject/entitybroker/rest/jetty/EntityBrokerServletTest.java (1)
176-176: Approve assertion updates for unescaped slashes
All affected assertions in EntityBrokerServletTest now expect unescaped/, which matches Jackson’s default JSON serialization. Verify that these tests pass locally or in your CI environment.entitybroker/impl/src/java/org/sakaiproject/entitybroker/impl/event/EventReceiverCoordinator.java (3)
23-29: LGTM! Imports correctly updated for WeakHashMap implementation.The imports support the migration from ReferenceMap to WeakHashMap with the snapshot pattern for thread-safe iteration.
49-50: Excellent fix! Correctly addresses the critical classloader leak.This change properly addresses the critical issue flagged in the past review:
- ✅ Receivers are now weak keys (not values), allowing proper garbage collection
- ✅ Each receiver gets its own entry (no more clobbering by ClassLoader)
- ✅ Thread-safe with
synchronizedMapwrapperThe weak-key pattern ensures receivers can be GC'd when the ApplicationContext releases them (e.g., on webapp reload), while preventing premature collection during normal operation since Spring holds references to these beans.
Based on past review comments.
78-78: LGTM! Registration and event handling correctly implement the weak-key pattern.The registration logic now correctly stores receivers as weak keys (line 78), and the event handling safely creates a snapshot of the keySet() inside a synchronized block before iteration (lines 88-92). This pattern:
- ✅ Prevents
ConcurrentModificationExceptionduring iteration- ✅ Allows GC to collect unreferenced receivers between snapshots
- ✅ Is thread-safe for concurrent event handling
- ✅ Doesn't hold locks during receiver notification (good for performance)
The explicit
synchronizedblock is necessary for the compound operation of creating the snapshot, complementing thesynchronizedMapwrapper that protects individual operations.Based on past review comments.
Also applies to: 87-98
entitybroker/utils/src/java/org/sakaiproject/entitybroker/util/EntityDataUtils.java (5)
24-25: LGTM!The new reflection imports are appropriate for replacing the reflectutils dependency with standard Java reflection and Spring utilities.
Also applies to: 40-40
187-227: Well-structured ID extraction logic.The new implementation correctly handles both Map and object entities with a clear fallback hierarchy: annotated fields → conventional field names → annotated methods → conventional accessor methods. Null safety and error handling are appropriate throughout.
234-264: LGTM!The field name resolution follows the same logical hierarchy as
getEntityId, correctly extracting property names from accessor methods and falling back to conventional names when only accessors are present.
522-530: Interface search successfully added.The recursive interface traversal (lines 523-528) now ensures that
@EntityIdannotations declared on interface methods are discovered, addressing the concern raised in the previous review. This restores parity with the historical ReflectUtils behavior.Based on past review comments.
483-601: Helper methods are well-implemented.The private reflection utilities correctly:
- Walk class hierarchies with proper null guards
- Use Spring's ReflectionUtils for secure reflective access
- Wrap exceptions with clear context (field/method names and target classes)
- Follow JavaBeans conventions (accessor prefixes, decapitalization with acronym handling)
The overall approach is sound and type-safe.
entitybroker/rest/src/java/org/sakaiproject/entitybroker/rest/EntityHandlerImpl.java (9)
28-30: LGTM! Appropriate imports for JDK 17+ reflection.The new reflection imports, including
InaccessibleObjectException, are necessary for the module access error handling introduced to support JDK 17+. Using Spring'sReflectionUtilsis a solid choice as a replacement for the removed reflectutils library.Also applies to: 84-84
483-483: LGTM! Clean replacement of ReflectUtils.contains.The new internal
containshelper method successfully replaces the external dependency with a simple, straightforward implementation. All usage sites are consistent and correct.Also applies to: 525-525, 648-648, 926-926, 935-935
832-841: LGTM! Consistent HTTP method handling.The refactoring to use
HttpRESTUtils.Methodenum values provides better type safety and consistency across the codebase.
1055-1078: LGTM! Enhanced Last-Modified detection.The improved implementation now properly searches for
lastModifiedvalues using a sensible fallback strategy: property access, then annotated fields, then annotated methods. This provides more flexible support for entity providers.
1111-1121: LGTM! Simple and correct array search.The
containshelper is straightforward and correctly handles null inputs. WhileArrays.asList().contains()could be an alternative, this implementation avoids additional object allocation and is perfectly suitable for the use case.
1123-1151: LGTM! Well-designed property access strategy.The
findLastModifiedValueandreadPropertymethods implement a sensible fallback strategy that accommodates different entity provider patterns. The separation between direct property access and annotation-based discovery is clean and maintainable.
1153-1213: LGTM! Robust reflection discovery methods.The class hierarchy traversal is correctly implemented, stopping at
Object.classand examining declared members at each level. Key strengths:
findMethodWithAnnotationcorrectly filters for zero-parameter methodsfindAccessorproperly usesLocale.ENGLISHfor capitalization (avoiding locale-specific issues)- Good integration with Spring's
ReflectionUtils.findMethod
1215-1225: LGTM! Proper JDK 17+ module access error handling.The exception handling correctly catches
InaccessibleObjectException,SecurityException, andIllegalStateExceptionto gracefully handle module access failures on JDK 17+. The fail-safe approach of returningnullallows the system to continue functioning even when reflection access is blocked.Based on learnings.
1227-1237: LGTM! Consistent exception handling for method invocation.The method invocation helper mirrors the robust error handling of
readFieldValue, properly catching module access exceptions that can occur on JDK 17+. The graceful degradation pattern is consistent throughout the reflection helpers.Based on learnings.
entitybroker/rest/src/java/org/sakaiproject/entitybroker/rest/EntityDescriptionManager.java (6)
23-44: LGTM! Clean migration to standard Java APIs.The new imports appropriately replace reflectutils with standard Java bean introspection (
java.beans.*) and reflection utilities. The addition ofjava.time.temporal.Temporalfor modern date/time support and updated collections are well-chosen.
1164-1198: LGTM! Well-designed type classification helpers.The type-checking predicates are comprehensive and correctly handle the various type categories. The
getArrayComponentTypemethod properly unwraps multi-dimensional arrays by recursively drilling to the leaf component type.Minor observation:
isSimpleTypereturnstruefornull, which is an edge case but appears intentional for this context where null values might need to be treated as simple for serialization purposes.
1248-1281: LGTM! Robust field discovery and required-property detection.The helper methods correctly:
findField: Searches up the class hierarchy until reachingObject.classgetAllFields: Accumulates all declared fields across the hierarchyisRequired: Checks for@EntityFieldRequiredon the field and both getter/setter methods, with proper static exclusionThe apparent redundancy in
isRequired(checking bothfieldparameter and method parameters) is intentional, as the annotation can be placed on fields, getters, or setters independently.
476-519: LGTM! Consistent integration of new introspection mechanism.The
describeEntitymethod correctly leverages the new property introspection:XML generation (lines 476-519):
- Uses type predicates (
isSimpleType,isCollectionType,isArrayType,isMapType) to categorize entities- Calls
getArrayComponentTypeto retrieve leaf component types for arrays- Uses
analyzePropertiesto obtain readable/writable types for bean field descriptionsHTML generation (lines 730-817):
- Mirrors the XML logic with type predicates and array component type retrieval
- Generates detailed property tables with read/write/required status
- Properly integrates
requiredPropertiesset for marking required fields (line 765)The usage is consistent across both output formats and correctly replaces the previous reflectutils-based implementation.
Also applies to: 730-817
1152-1162: LGTM! Clean helper utilities.
containsFormat(lines 1152-1162): Simple linear-search helper with proper null checks. Linear search is acceptable given that format arrays are typically small (usually < 10 elements).
PropertyIntrospection(lines 1283-1299): Well-designed data holder with:
LinkedHashMap/LinkedHashSetfor deterministic iteration order- Private fields with public getters for encapsulation
- Serves as a clear contract for introspection results
Also applies to: 1283-1299
997-1059: Approve getFormats refactoringFiltering out
Formats.FORMforInputableproviders is intentional—FORM handling is delegated to the form-generation logic in EntityHandlerImpl. The three-state return semantics and NPE catch for backward compatibility are correct.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
entitybroker/impl/src/java/org/sakaiproject/entitybroker/impl/EntityBrokerManagerImpl.java(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.java
📄 CodeRabbit inference engine (.cursor/rules/logging-rule.mdc)
**/*.java: Use SLF4J parameterized logging (e.g., logger.info("Value is: {}", value)) instead of string concatenation in log statements
Write log messages and code comments in English; never translate log messages
Files:
entitybroker/impl/src/java/org/sakaiproject/entitybroker/impl/EntityBrokerManagerImpl.java
🧠 Learnings (1)
📚 Learning: 2025-10-03T14:48:27.423Z
Learnt from: ern
PR: sakaiproject/sakai#14139
File: entitybroker/impl/src/java/org/sakaiproject/entitybroker/impl/EntityBrokerManagerImpl.java:832-841
Timestamp: 2025-10-03T14:48:27.423Z
Learning: In newer JDKs (JDK 17+), `java.lang.reflect.InaccessibleObjectException` can be thrown by `setAccessible()` calls due to strong encapsulation. This was a key reason the old reflectutils library needed to be updated. When performing reflection operations, both `SecurityException` and `InaccessibleObjectException` should be caught to ensure fail-safe behavior.
Applied to files:
entitybroker/impl/src/java/org/sakaiproject/entitybroker/impl/EntityBrokerManagerImpl.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
| private Object invokeMethod(Object bean, Method method) { | ||
| if (bean == null || method == null) { | ||
| return null; | ||
| } | ||
| try { | ||
| ReflectionUtils.makeAccessible(method); | ||
| return ReflectionUtils.invokeMethod(method, bean); | ||
| } catch (IllegalStateException | SecurityException | java.lang.reflect.InaccessibleObjectException e) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Keep populateEntityData fail-safe when @EntityTitle getters misbehave
ReflectionUtils.invokeMethod rethrows any runtime exception thrown by the target method. If an @EntityTitle getter (or the generated bean accessor used for titles) throws, the exception now propagates out of populateEntityData, contradicting the “fail safe” contract and regressing behaviour that previously swallowed such failures. Treat every RuntimeException from this helper as best-effort and return null instead.
- } catch (IllegalStateException | SecurityException | java.lang.reflect.InaccessibleObjectException e) {
+ } catch (RuntimeException e) {
return null;
}🤖 Prompt for AI Agents
In
entitybroker/impl/src/java/org/sakaiproject/entitybroker/impl/EntityBrokerManagerImpl.java
around lines 843 to 852, the helper invokeMethod currently catches
IllegalStateException, SecurityException and InaccessibleObjectException but
lets RuntimeExceptions from the invoked getter propagate; change the catch to
also handle RuntimeException (i.e., catch RuntimeException in addition to the
existing exceptions) and return null so any runtime exception thrown by the
target method is treated as best-effort and does not escape populateEntityData.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
kernel/api/src/main/java/org/sakaiproject/serialization/MapperFactory.java (1)
407-439: Critical: Inconsistent XXE protection application before rebuild() calls.The methods
setMaxAttributeSize()anddisableNamespaceAware()re-apply XXE protections before callingrebuild()(lines 374-376, 391-393), butenableRepairingNamespaces()(line 409),disableOutputCDataAsText()(line 423), andenableOutputCDataAsText()(line 437) callrebuild()without re-applying these protections. Ifrebuild()resets security configurations, these three methods could re-enable XXE vulnerabilities.Apply this pattern consistently to all methods that call
rebuild():public XmlMapperBuilder enableRepairingNamespaces() { xmlMapper.getFactory().getXMLOutputFactory().setProperty(WstxOutputFactory.IS_REPAIRING_NAMESPACES, true); + // Ensure XXE defenses remain in effect prior to rebuild + xmlMapper.getFactory().getXMLInputFactory().setProperty(XMLInputFactory.SUPPORT_DTD, false); + xmlMapper.getFactory().getXMLInputFactory().setProperty(XMLInputFactory.IS_SUPPORTING_EXTERNAL_ENTITIES, false); xmlMapper.getFactory().rebuild().build(); return this; } public XmlMapperBuilder disableOutputCDataAsText() { xmlMapper.getFactory().getXMLOutputFactory().setProperty(WstxOutputProperties.P_OUTPUT_CDATA_AS_TEXT, false); + // Ensure XXE defenses remain in effect prior to rebuild + xmlMapper.getFactory().getXMLInputFactory().setProperty(XMLInputFactory.SUPPORT_DTD, false); + xmlMapper.getFactory().getXMLInputFactory().setProperty(XMLInputFactory.IS_SUPPORTING_EXTERNAL_ENTITIES, false); xmlMapper.getFactory().rebuild().build(); return this; } public XmlMapperBuilder enableOutputCDataAsText() { xmlMapper.getFactory().getXMLOutputFactory().setProperty(WstxOutputProperties.P_OUTPUT_CDATA_AS_TEXT, true); + // Ensure XXE defenses remain in effect prior to rebuild + xmlMapper.getFactory().getXMLInputFactory().setProperty(XMLInputFactory.SUPPORT_DTD, false); + xmlMapper.getFactory().getXMLInputFactory().setProperty(XMLInputFactory.IS_SUPPORTING_EXTERNAL_ENTITIES, false); xmlMapper.getFactory().rebuild().build(); return this; }
♻️ Duplicate comments (1)
entitybroker/core-providers/src/java/org/sakaiproject/entitybroker/providers/UserEntityProvider.java (1)
405-423: Unwrap Spring proxies before resolving the fieldThis is the same issue flagged in the previous review: when
userDirectoryServiceis a JDK dynamic proxy,target.getClass()returns the proxy class rather than the implementation class containingm_separateIdEid. The field lookup fails andusesSeparateIdEidincorrectly defaults tofalse.Apply the previously suggested fix:
+import org.springframework.util.ClassUtils; import org.springframework.util.ReflectionUtils;private Boolean readBooleanField(Object target, String fieldName) { - Field field = ReflectionUtils.findField(target.getClass(), fieldName); + Class<?> targetClass = ClassUtils.getUserClass(target); + Field field = ReflectionUtils.findField(targetClass, fieldName); if (field == null) { return null; }
🧹 Nitpick comments (1)
entitybroker/core-providers/src/java/org/sakaiproject/entitybroker/providers/UserEntityProvider.java (1)
425-440: Consider leveragingBoolean.parseBooleanfor simplicity.The current implementation is correct and provides strict validation by throwing an exception for invalid values. However, if you want a simpler approach that avoids the manual string comparisons, you could use Java's built-in
Boolean.parseBoolean:private Boolean parseBoolean(String value) { if (value == null) { return null; } String normalized = value.trim(); if (normalized.isEmpty()) { return null; } if ("true".equalsIgnoreCase(normalized)) { return Boolean.TRUE; } if ("false".equalsIgnoreCase(normalized)) { return Boolean.FALSE; } throw new IllegalArgumentException("Value is not a boolean: " + value); }Note: The current implementation is more strict than
Boolean.parseBoolean(which silently returnsfalsefor invalid values), so the strictness may be intentional for configuration parsing. The above suggestion maintains that strictness.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
entitybroker/core-providers/pom.xml(0 hunks)entitybroker/core-providers/src/java/org/sakaiproject/entitybroker/providers/UserEntityProvider.java(4 hunks)kernel/api/src/main/java/org/sakaiproject/serialization/MapperFactory.java(5 hunks)kernel/kernel-impl/pom.xml(0 hunks)master/pom.xml(0 hunks)
💤 Files with no reviewable changes (3)
- master/pom.xml
- kernel/kernel-impl/pom.xml
- entitybroker/core-providers/pom.xml
🧰 Additional context used
📓 Path-based instructions (1)
**/*.java
📄 CodeRabbit inference engine (.cursor/rules/logging-rule.mdc)
**/*.java: Use SLF4J parameterized logging (e.g., logger.info("Value is: {}", value)) instead of string concatenation in log statements
Write log messages and code comments in English; never translate log messages
**/*.java: Java: Never use local variable type inference (var). Always declare explicit types.
When proposing or writing Java code, spell out full types in local variable declarations, for-loops, and try-with-resources.
If Java code uses var, replace it with an explicit type.
Review gate: Treat any PR or suggestion containing Java var as non-compliant; require explicit types before merge.Do not use local variable type inference (var) in Java; always declare explicit types
Files:
kernel/api/src/main/java/org/sakaiproject/serialization/MapperFactory.javaentitybroker/core-providers/src/java/org/sakaiproject/entitybroker/providers/UserEntityProvider.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: sakai-deploy
- GitHub Check: maven-build
- GitHub Check: maven-build
🔇 Additional comments (6)
entitybroker/core-providers/src/java/org/sakaiproject/entitybroker/providers/UserEntityProvider.java (2)
18-18: LGTM!The new imports are necessary for the reflection-based field access implementation and are appropriate for this use case.
Also applies to: 53-53
382-403: Logic flow is sound.The method correctly attempts configuration-based detection first, then falls back to reflection, with a sensible default. The exception handling appropriately treats failures as unknowns and eventually defaults to
false.kernel/api/src/main/java/org/sakaiproject/serialization/MapperFactory.java (4)
29-29: LGTM!The Module import is necessary for the
registerJdk8Module()method's instanceof check and is correctly placed with the other Jackson imports.
54-54: LGTM!Adding Jdk8Module registration to the default JSON mapper enables better support for
Optionaland other JDK8 types when the module is available on the classpath.
166-184: LGTM - No issues found.Verification confirms the dependency
jackson-datatype-jdk8is properly declared inkernel/api/pom.xml. The reflection-based implementation is sound with comprehensive exception handling and a defensiveinstanceofcheck. The fail-safe approach appropriately handles the optional nature of the module.
374-377: Approved: XXE protections are correctly scoped to input factory methods only.The re-application of XXE protections at lines 374–377 is correct and represents proper defensive hardening. The apparent inconsistency across rebuild() calls is actually intentional and secure:
- XMLInputFactory methods (lines 375–376, 392–393): Re-apply XXE protections because they modify XML parsing behavior where XXE attacks occur
- XMLOutputFactory methods (lines 409, 423, 437): Do not need XXE re-application because they only affect XML generation, not parsing
XXE vulnerabilities are fundamentally an input/parsing concern. Modifying output factory properties does not affect input factory XXE protections, so re-application there would be redundant. The pattern is correct as implemented.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
entitybroker/core-providers/src/java/org/sakaiproject/entitybroker/providers/SiteEntityProvider.java(2 hunks)entitybroker/core-providers/src/java/org/sakaiproject/entitybroker/providers/model/EntitySite.java(4 hunks)kernel/api/src/main/java/org/sakaiproject/site/api/Site.java(1 hunks)kernel/kernel-impl/src/main/java/org/sakaiproject/site/impl/BaseSite.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- entitybroker/core-providers/src/java/org/sakaiproject/entitybroker/providers/SiteEntityProvider.java
🧰 Additional context used
📓 Path-based instructions (1)
**/*.java
📄 CodeRabbit inference engine (.cursor/rules/logging-rule.mdc)
**/*.java: Use SLF4J parameterized logging (e.g., logger.info("Value is: {}", value)) instead of string concatenation in log statements
Write log messages and code comments in English; never translate log messages
**/*.java: Java: Never use local variable type inference (var). Always declare explicit types.
When proposing or writing Java code, spell out full types in local variable declarations, for-loops, and try-with-resources.
If Java code uses var, replace it with an explicit type.
Review gate: Treat any PR or suggestion containing Java var as non-compliant; require explicit types before merge.Do not use local variable type inference (var) in Java; always declare explicit types
Files:
kernel/kernel-impl/src/main/java/org/sakaiproject/site/impl/BaseSite.javakernel/api/src/main/java/org/sakaiproject/site/api/Site.javaentitybroker/core-providers/src/java/org/sakaiproject/entitybroker/providers/model/EntitySite.java
🧬 Code graph analysis (1)
entitybroker/core-providers/src/java/org/sakaiproject/entitybroker/providers/model/EntitySite.java (1)
entitybroker/core-providers/src/java/org/sakaiproject/entitybroker/providers/model/EntitySitePage.java (1)
JsonIgnoreProperties(42-385)
⏰ 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: sakai-deploy
- GitHub Check: maven-build
- GitHub Check: maven-build
🔇 Additional comments (3)
kernel/api/src/main/java/org/sakaiproject/site/api/Site.java (1)
89-99: LGTM! Well-documented interface addition.The new setter pairs appropriately with the existing getter, and the Javadoc clearly describes the intended behavior.
kernel/kernel-impl/src/main/java/org/sakaiproject/site/impl/BaseSite.java (1)
825-831: LGTM! Clean implementation.The setter correctly sanitizes input with
StringUtils.trimToNull, which is consistent with other setters in this class.entitybroker/core-providers/src/java/org/sakaiproject/entitybroker/providers/model/EntitySite.java (1)
31-32: LGTM! Appropriate Jackson migration.The replacement of reflectutils annotations with Jackson's
@JsonIgnorePropertiesand@JsonIgnoremaintains the same serialization behavior while using a more standard library.Also applies to: 64-64, 761-761
| public void setCreatedBy(String userId) { | ||
| owner = userId; | ||
| if (site != null) { | ||
| site.setCreatedBy(userId); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add input sanitization for consistency with BaseSite.
The BaseSite.setCreatedBy implementation (line 830 in BaseSite.java) uses StringUtils.trimToNull(userId) to sanitize input. This implementation should do the same to maintain consistency and avoid scenarios where owner and the underlying site's creator ID differ due to whitespace.
Apply this diff:
public void setCreatedBy(String userId) {
- owner = userId;
+ owner = org.apache.commons.lang3.StringUtils.trimToNull(userId);
if (site != null) {
site.setCreatedBy(userId);
}
}Note: StringUtils from commons-lang3 is already imported in the file, so you can use the fully qualified name or add a specific import.
🤖 Prompt for AI Agents
In
entitybroker/core-providers/src/java/org/sakaiproject/entitybroker/providers/model/EntitySite.java
around lines 556 to 561, setCreatedBy currently assigns owner directly and
forwards the raw userId to site.setCreatedBy; change this to sanitize the input
using StringUtils.trimToNull(userId) (assign the trimmed result to owner and
pass that trimmed value to site.setCreatedBy) so owner and the underlying site
use the same normalized value; commons-lang3's StringUtils is already available
so just call StringUtils.trimToNull(...) before assignment and forwarding.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
entitybroker/core-providers/src/test/org/sakaiproject/entitybroker/providers/UserEntityProviderFindAndCheckUserId.java (1)
74-81: Consider adding fallback scenario test.The test covers the primary EID-to-ID conversion path, but doesn't test the fallback logic in lines 420-426 of
UserEntityProvider.javawheregetUserIdfails butgetUserEidsucceeds (treating input as ID instead of EID).Consider adding an assertion to verify the fallback:
// Add mock for the fallback case Mockito.doReturn("eid").when(uds).getUserEid("actualId"); // Test that when getUserId fails, it falls back to treating input as ID Assert.assertEquals("actualId", provider.findAndCheckUserId(null, "actualId"));This would provide complete coverage of the non-explicit EID resolution path.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
entitybroker/core-providers/src/java/org/sakaiproject/entitybroker/providers/UserEntityProvider.java(2 hunks)entitybroker/core-providers/src/test/org/sakaiproject/entitybroker/providers/UserEntityProviderFindAndCheckUserId.java(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.java
📄 CodeRabbit inference engine (.cursor/rules/logging-rule.mdc)
**/*.java: Use SLF4J parameterized logging (e.g., logger.info("Value is: {}", value)) instead of string concatenation in log statements
Write log messages and code comments in English; never translate log messages
**/*.java: Java: Never use local variable type inference (var). Always declare explicit types.
When proposing or writing Java code, spell out full types in local variable declarations, for-loops, and try-with-resources.
If Java code uses var, replace it with an explicit type.
Review gate: Treat any PR or suggestion containing Java var as non-compliant; require explicit types before merge.Do not use local variable type inference (var) in Java; always declare explicit types
Files:
entitybroker/core-providers/src/java/org/sakaiproject/entitybroker/providers/UserEntityProvider.javaentitybroker/core-providers/src/test/org/sakaiproject/entitybroker/providers/UserEntityProviderFindAndCheckUserId.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: sakai-deploy
- GitHub Check: maven-build
- GitHub Check: maven-build
🔇 Additional comments (3)
entitybroker/core-providers/src/java/org/sakaiproject/entitybroker/providers/UserEntityProvider.java (1)
636-639: LGTM - Clean configuration-based approach.The new
userIdExplicitOnly()helper method provides a clean, configuration-driven approach that replaces the previous reflection-basedusesSeparateIdEidmechanism. This eliminates the Spring proxy unwrapping issue mentioned in past review comments.entitybroker/core-providers/src/test/org/sakaiproject/entitybroker/providers/UserEntityProviderFindAndCheckUserId.java (2)
60-71: LGTM - Comprehensive failover test coverage.The test properly validates the non-explicit mode behavior where the system attempts ID validation first, then falls back to EID/AID resolution. The mock setup correctly simulates the failover path and covers various input formats including entity references.
84-99: LGTM - Thorough explicit mode coverage.The test comprehensively validates the explicit ID-only mode, covering:
- ID validation without prefix in both parameter paths
id=prefix handling in both paths with different semantics- Missing/invalid user cases
The mock setup correctly simulates the asymmetric behavior where the
id=prefix triggers EID resolution in the EID parameter path but is simply stripped in the ID parameter path.
| if (userIdExplicitOnly()) { | ||
| // only check ID or EID | ||
| if (currentUserEid.length() > ID_PREFIX.length() && currentUserEid.startsWith(ID_PREFIX) ) { | ||
| // strip the id marker out | ||
| currentUserEid = currentUserEid.substring(ID_PREFIX.length()); | ||
| // check EID, do not attempt to check by ID as well | ||
| try { | ||
| User u = userDirectoryService.getUserByAid(currentUserEid); | ||
| userId = u.getId(); | ||
| } catch (UserNotDefinedException e2) { | ||
| userId = null; | ||
| } | ||
| } else { | ||
| // check by ID | ||
| try { | ||
| userDirectoryService.getUserEid(currentUserEid); // simply here to throw an exception or not | ||
| userId = currentUserEid; | ||
| } catch (UserNotDefinedException e2) { | ||
| userId = null; | ||
| } | ||
| } catch (IllegalArgumentException e) { | ||
| userId = null; | ||
| } | ||
| } else { | ||
| if (userIdExplicitOnly()) { | ||
| // only check ID or EID | ||
| if (currentUserEid.length() > ID_PREFIX.length() && currentUserEid.startsWith(ID_PREFIX) ) { | ||
| // strip the id marker out | ||
| currentUserEid = currentUserEid.substring(ID_PREFIX.length()); | ||
| // check EID, do not attempt to check by ID as well | ||
| try { | ||
| User u = userDirectoryService.getUserByAid(currentUserEid); | ||
| userId = u.getId(); | ||
| } catch (UserNotDefinedException e2) { | ||
| userId = null; | ||
| } | ||
| } else { | ||
| // check by ID | ||
| try { | ||
| userDirectoryService.getUserEid(currentUserEid); // simply here to throw an exception or not | ||
| userId = currentUserEid; | ||
| } catch (UserNotDefinedException e2) { | ||
| userId = null; | ||
| } | ||
| } | ||
| } else { | ||
| // check for EID and then ID | ||
| // check for EID and then ID | ||
| try { | ||
| userId = userDirectoryService.getUserId(currentUserEid); | ||
| } catch (UserNotDefinedException e) { | ||
| try { | ||
| userId = userDirectoryService.getUserId(currentUserEid); | ||
| } catch (UserNotDefinedException e) { | ||
| try { | ||
| userDirectoryService.getUserEid(currentUserEid); // simply here to throw an exception or not | ||
| userId = currentUserEid; | ||
| } catch (UserNotDefinedException e2) { | ||
| userId = null; | ||
| } | ||
| userDirectoryService.getUserEid(currentUserEid); // simply here to throw an exception or not | ||
| userId = currentUserEid; | ||
| } catch (UserNotDefinedException e2) { | ||
| userId = null; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Critical documentation mismatch with implementation.
The implementation of explicit ID/EID resolution in this path contradicts the documentation at line 632.
Current implementation behavior (in EID path when currentUserId is null):
- Input without
id=prefix (line 407-413): callsgetUserEid(currentUserEid)to validate as ID - Input with
id=prefix (line 396-405): callsgetUserByAid(currentUserEid)to resolve as EID/AID to ID
Documentation at line 632 states:
"the user id must be prefixed with 'id=', otherwise it will be treated like an eid"
This is inverted. The implementation treats:
- No prefix → ID
id=prefix → EID
Either the documentation or the implementation is wrong. Based on the test expectations in UserEntityProviderFindAndCheckUserId.testExplicitIdOnly(), the implementation appears correct, meaning the documentation at line 632-633 needs correction.
Suggested documentation fix at line 632:
- * In other words, the user id must be prefixed with "id=", otherwise it will be treated like an eid
+ * In other words, when in the EID parameter path, values without "id=" prefix are treated as IDs (validated directly),
+ * while values with "id=" prefix are treated as EIDs (resolved to internal IDs via getUserByAid)Additionally, consider clarifying the asymmetric semantics: the id= prefix has different meanings depending on whether it appears in the currentUserId or currentUserEid parameter.
---
🏁 Script executed:
```shell
#!/bin/bash
# Find the UserEntityProvider.java file and extract line 632 and surrounding context
fd -e java "UserEntityProvider" | head -5
Length of output: 457
🏁 Script executed:
#!/bin/bash
# Find test file and extract testExplicitIdOnly method
fd -e java "UserEntityProviderFindAndCheckUserId"Length of output: 118
🏁 Script executed:
#!/bin/bash
# Search for getUserByAid and getUserEid calls to understand the logic flow
rg -n "getUserByAid|getUserEid" --type=java -B2 -A2Length of output: 147182
🏁 Script executed:
#!/bin/bash
# Extract lines around 632 in UserEntityProvider.java to verify documentation
fd -e java "UserEntityProvider\.java" -x cat {} | head -650 | tail -30Length of output: 1248
🏁 Script executed:
#!/bin/bash
# Extract testExplicitIdOnly test method to see what it expects
sed -n '84,105p' entitybroker/core-providers/src/test/org/sakaiproject/entitybroker/providers/UserEntityProviderFindAndCheckUserId.javaLength of output: 1042
🏁 Script executed:
#!/bin/bash
# Also get testResolveByIdWithFailover to see the difference
sed -n '60,75p' entitybroker/core-providers/src/test/org/sakaiproject/entitybroker/providers/UserEntityProviderFindAndCheckUserId.javaLength of output: 944
🏁 Script executed:
#!/bin/bash
# Get the full context around the ID_PREFIX constant to understand what "id=" means
rg -n "ID_PREFIX" entitybroker/core-providers/src/java/org/sakaiproject/entitybroker/providers/UserEntityProvider.java -B2 -A2Length of output: 1564
The documentation mismatch is confirmed and the review comment is accurate.
The test testExplicitIdOnly() demonstrates the actual behavior:
- Input
"id"(no prefix) → callsgetUserEid("id")→ treats as ID - Input
"id=id"(with prefix) → callsgetUserByAid("id")→ treats as EID
This contradicts the documentation at line 632: "the user id must be prefixed with "id=", otherwise it will be treated like an eid"
The implementation correctly treats values without the prefix as IDs and values with the prefix as EIDs—the opposite of what the documentation states.
Update the documentation at line 632-633 to reflect actual behavior:
- * In other words, the user id must be prefixed with "id=", otherwise it will be treated like an eid
+ * In other words, values without "id=" prefix are treated as IDs (validated directly),
+ * while values with "id=" prefix are treated as EIDs (resolved via getUserByAid)Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
entitybroker/core-providers/src/java/org/sakaiproject/entitybroker/providers/UserEntityProvider.java
lines 394-427: the implementation treats values without the "id=" prefix as IDs
and values with the "id=" prefix as EIDs (opposite the current docs), so update
the documentation at lines 632-633 to state that an unprefixed value is treated
as an ID and a value prefixed with "id=" is treated as an EID (adjust wording to
match code behavior exactly).
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
entitybroker/rest/src/java/org/sakaiproject/entitybroker/rest/EntityDescriptionManager.java (1)
1042-1044: Replace NPE catch with explicit null checks.Catching NullPointerException for control flow is a code smell. While the comment indicates this maintains previous behavior, explicit null checks would be clearer and more maintainable.
Consider refactoring to:
- } catch (NullPointerException e) { - // ignore, maintain previous behaviour of returning null - } + } catch (RuntimeException e) { + log.debug("Error retrieving formats for prefix {}: {}", prefix, e.getMessage()); + }And add explicit null checks for the provider objects before calling their methods.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
entitybroker/rest/src/java/org/sakaiproject/entitybroker/rest/EntityDescriptionManager.java(16 hunks)entitybroker/rest/src/java/org/sakaiproject/entitybroker/rest/EntityEncodingManager.java(18 hunks)entitybroker/rest/src/test/org/sakaiproject/entitybroker/rest/EntityEncodingManagerTest.java(9 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.java
📄 CodeRabbit inference engine (.cursor/rules/logging-rule.mdc)
**/*.java: Use SLF4J parameterized logging (e.g., logger.info("Value is: {}", value)) instead of string concatenation in log statements
Write log messages and code comments in English; never translate log messages
**/*.java: Java: Never use local variable type inference (var). Always declare explicit types.
When proposing or writing Java code, spell out full types in local variable declarations, for-loops, and try-with-resources.
If Java code uses var, replace it with an explicit type.
Review gate: Treat any PR or suggestion containing Java var as non-compliant; require explicit types before merge.Do not use local variable type inference (var) in Java; always declare explicit types
Files:
entitybroker/rest/src/java/org/sakaiproject/entitybroker/rest/EntityDescriptionManager.javaentitybroker/rest/src/java/org/sakaiproject/entitybroker/rest/EntityEncodingManager.javaentitybroker/rest/src/test/org/sakaiproject/entitybroker/rest/EntityEncodingManagerTest.java
🧬 Code graph analysis (2)
entitybroker/rest/src/java/org/sakaiproject/entitybroker/rest/EntityEncodingManager.java (1)
kernel/api/src/main/java/org/sakaiproject/serialization/MapperFactory.java (1)
MapperFactory(37-455)
entitybroker/rest/src/test/org/sakaiproject/entitybroker/rest/EntityEncodingManagerTest.java (1)
kernel/api/src/main/java/org/sakaiproject/serialization/MapperFactory.java (1)
MapperFactory(37-455)
⏰ 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: sakai-deploy
- GitHub Check: maven-build
- GitHub Check: maven-build
🔇 Additional comments (25)
entitybroker/rest/src/test/org/sakaiproject/entitybroker/rest/EntityEncodingManagerTest.java (6)
26-26: LGTM: Import additions support Jackson migration.The new imports (StandardCharsets, Collections, LinkedHashMap, Jackson types, MapperFactory) are necessary for the reflectutils removal and properly support the migration to Jackson-based encoding/decoding.
Also applies to: 28-28, 31-31, 39-41, 53-53
116-116: LGTM: LinkedHashMap appropriately replaces ArrayOrderedMap.The migration from ArrayOrderedMap to LinkedHashMap preserves insertion-order semantics while removing the reflectutils dependency. This is the correct standard Java replacement.
Also applies to: 174-174, 204-204, 231-231, 448-448, 456-456, 501-501, 509-509
148-182: LGTM: New XML tests provide excellent coverage.The three new XML tests validate critical encoding behaviors:
- XML fragments exclude declarations (testEncodeEntityXmlDoesNotContainDeclaration)
- Complete output has exactly one declaration (testInternalOutputFormatterProducesSingleDeclaration)
- Ampersands are properly escaped (testXmlEscapesAmpersands)
These tests appropriately verify the Jackson-based XML encoding implementation.
184-220: LGTM: New JSON tests validate unwrapped encoding.Both tests properly verify that JSON encoding produces unwrapped output (no prefix root wrapper):
- testEncodeEntityJsonWithoutRootWrapper validates entity-backed EntityData encoding and uses Jackson parsing for verification
- testEncodeDataOnlyJsonWithoutRootWrapper validates data-only payload encoding
The use of MapperFactory for parsing validation is appropriate and provides strong verification of JSON structure.
480-482: LGTM: Correct handling of Jackson's Number types.The updated assertions properly handle Jackson returning Number types (Integer, Long, etc.) by casting to Number and calling intValue(). The explanatory comment on line 480 helps future maintainers understand the type handling.
Also applies to: 488-489
618-658: LGTM: Comprehensive EntityData JSON serialization test.This new test provides excellent coverage of EntityData JSON encoding:
- Validates no extra wrapper (aligning with client expectations)
- Verifies included fields (displayTitle, entityReference, entityURL, data)
- Confirms transient fields are excluded (dataOnly, displayTitleSet, entityRef, populated)
The test is well-documented and validates real-world usage (Commons userPerms endpoint). This ensures the Jackson migration maintains correct serialization behavior.
entitybroker/rest/src/java/org/sakaiproject/entitybroker/rest/EntityDescriptionManager.java (7)
23-46: LGTM! Appropriate imports for bean introspection refactor.The new imports support the migration from reflectutils to Java bean introspection, including proper XML escaping via Apache Commons Text.
893-898: LGTM! Proper XML escaping prevents injection vulnerabilities.The escapeXml helper correctly uses StringEscapeUtils.escapeXml11 and safely handles null values.
1173-1207: LGTM! Type detection helpers are correct and comprehensive.The type classification methods properly handle primitives, collections, arrays, maps, and simple types. The getArrayComponentType correctly unwraps nested arrays.
1209-1255: LGTM! Bean introspection logic is thorough and correct.The analyzeProperties method properly:
- Uses Java bean introspection via Introspector
- Filters static members at all detection points
- Handles both bean properties and public fields
- Walks the inheritance hierarchy
- Detects required properties via annotations
The static field filtering that was flagged in previous reviews has been correctly implemented.
1257-1290: LGTM! Helper methods correctly handle inheritance and static filtering.The findField, getAllFields, and isRequired methods properly:
- Walk the class hierarchy up to Object
- Filter static members consistently
- Check annotations on fields, getters, and setters
1292-1308: LGTM! PropertyIntrospection is a well-designed data holder.The use of LinkedHashMap and LinkedHashSet preserves insertion order, which is important for consistent UI display in the describe functionality.
335-558: Excellent security improvement: consistent XML escaping throughout.The escapeXml helper is properly used throughout the XML generation in describeEntity, preventing potential XML injection vulnerabilities. This is a significant security improvement over the previous implementation.
entitybroker/rest/src/java/org/sakaiproject/entitybroker/rest/EntityEncodingManager.java (12)
123-159: Jackson mapper configuration looks solid.The static initialization properly configures JSON and XML mappers with appropriate settings including Java time module support, unknown property handling, and pretty printing. The XXE protections for XML parsing were confirmed addressed in previous reviews.
483-487: Good defensive practice with format trimming.The format trimming with debug logging helps catch potential issues with whitespace in format parameters while maintaining clean logging output.
Also applies to: 622-626
912-939: Encoding and decoding logic is well-structured.The methods properly handle depth limiting, format-specific encoding/decoding, and edge cases like null data and JSONP wrapping. Exception handling provides clear error messages for troubleshooting.
Also applies to: 1352-1382
1399-1451: Bean population logic handles nested structures properly.The methods correctly handle nested bean population, including automatic instantiation of nested beans and fallback to direct field access when properties aren't writable. The type checking at line 1432 appropriately distinguishes between Map properties and nested beans.
1531-1574: Correct handling of transient fields in serialization.The method properly excludes transient fields from serialization by building an exclusion set and checking both bean properties and direct fields against it. This respects Java's transient modifier semantics.
970-1036: Robust cycle detection prevents infinite recursion.The method uses an IdentityHashMap to track visited objects by identity (not equality), properly handles cycles by returning null, and cleans up the visited set in a finally block to avoid memory leaks during recursive traversal.
1307-1337: JSONP callback sanitization is properly implemented.The validation pattern restricts callbacks to valid JavaScript identifier paths, enforces a reasonable length limit, and includes the
/**/prefix as a security measure. The stripJsonp method correctly unwraps JSONP responses for processing.Also applies to: 123-124
863-884: Fallback encoding strategy improves resilience.The fallback mechanism for JSON/JSONP formats provides a safety net when the primary encoding fails, with comprehensive logging to help diagnose issues. The conditional wrapping logic correctly handles cases where a root name is needed.
1576-1617: Comprehensive bean class detection.The method uses a multi-stage approach: first excluding known non-bean types, then checking for a no-arg constructor, and finally falling back to introspection-based detection of readable properties or instance fields. This handles various bean patterns effectively.
1483-1529: Thorough property analysis combining introspection and reflection.The method correctly identifies readable/writable properties through both JavaBeans introspection and direct field access, properly handles the @EntityFieldRequired annotation across fields and methods, and appropriately filters out static fields and the "class" property.
1182-1305: Legacy format conversion handles reflectutils migration.The type conversion logic appropriately handles the metadata format from the old reflectutils library, including type attributes and associated metadata keys. The fallback chains in number and date conversion provide robustness for various input formats.
1135-1135: Pattern matching syntax is compatible with this project's Java version target.The project is configured to compile against Java 17 (defined as
<sakai.jdk.version>17</sakai.jdk.version>in master/pom.xml), which fully supports pattern matching forinstanceofexpressions as a standardized feature (finalized in Java 16). The code at lines 1135 and 1272 in EntityEncodingManager.java, along with similar patterns elsewhere in the codebase, is valid and will compile without errors.Likely an incorrect or invalid review comment.
|
This PR has been automatically marked as stale due to 10 days of inactivity. It will be closed in 4 days unless there is new activity. |
3 participants
Summary by CodeRabbit
New Features
Security
Bug Fixes
Refactor
Tests