[NAE-2451] Optimize and Reduce Outcome Size #462
Open
renczesstefan wants to merge 7 commits into
Open
Conversation
…nt outcomes Added `FieldSelector` and related classes (`FieldSelectorHolder`, `FieldSelectorInterceptor`, `ChangeRecordSerializer`, and `ObjectMapperConfiguration`) to enable selective field serialization based on request parameters. Integrated the interceptor into `WebConfig` for seamless request handling.
Introduced `DynamicFieldSerializer` to enable nested field selection during serialization. Updated `ChangeRecordSerializer` to leverage `DynamicFieldSerializer` for handling complex object structures with field selectors. Enhanced `FieldSelector` functionality to support nested field inclusion checks.
Renamed `ChangeRecordSerializer` to `LocalizedEventOutcomeSerializer` for better clarity and alignment with its purpose and usage in serializing `LocalisedEventOutcome`. Updated references in `ObjectMapperConfiguration`.
Enhanced JavaDocs for `FieldSelector`, `FieldSelectorHolder`, `FieldSelectorInterceptor`, `LocalizedEventOutcomeSerializer`, and `DynamicFieldSerializer`. Improved clarity and provided detailed explanations of class functionalities, methods, and usage scenarios to support selective field serialization.
Enhanced handling of blank or null specifications, added trimming for field names, and ensured empty names are excluded during parsing.
- Refactor serialization components to replace `JsonSerializer` with `ValueSerializer` and update `ObjectMapper` configuration for improved compatibility.
WalkthroughThis PR adds a field-selection mechanism for JSON serialization: a ChangesDynamic field selection for JSON serialization
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant FieldSelectorInterceptor
participant FieldSelectorHolder
participant LocalizedEventOutcomeSerializer
participant DynamicFieldSerializer
Client->>FieldSelectorInterceptor: HTTP request with "fields" query param
FieldSelectorInterceptor->>FieldSelectorInterceptor: FieldSelector.parse(fields)
FieldSelectorInterceptor->>FieldSelectorHolder: setSelector(parsedSelector)
Client->>LocalizedEventOutcomeSerializer: serialize(outcome)
LocalizedEventOutcomeSerializer->>FieldSelectorHolder: getSelector()
LocalizedEventOutcomeSerializer->>LocalizedEventOutcomeSerializer: write message/frontActions/outcomes if included
LocalizedEventOutcomeSerializer->>DynamicFieldSerializer: serializeWithSelector(nestedValue, nestedSelector)
DynamicFieldSerializer->>DynamicFieldSerializer: filter bean properties by selector.includes()
DynamicFieldSerializer-->>LocalizedEventOutcomeSerializer: JSON written
LocalizedEventOutcomeSerializer-->>Client: JSON response
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
coderabbitai
Bot
added
improvement
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@application-engine/src/main/java/com/netgrif/application/engine/serialization/DynamicFieldSerializer.java`:
- Around line 108-124: The DynamicFieldSerializer#serializeWithSelector loop can
throw a NullPointerException when an included property value is null because the
nested.includeAll() branch calls propValue.getClass() without a null check.
Update the handling around prop.getAccessor().getValue(value) so the includeAll
path serializes null values safely, reusing the method’s existing null handling
logic before calling context.findValueSerializer, and keep the fix localized to
serializeWithSelector and its nested.includeAll() branch.
- Around line 3-4: The Javadocs in DynamicFieldSerializer are still using
Jackson 2 terminology, and the Jackson 2 imports are no longer needed since
those types are only mentioned in comments. Update the doc references in
serializeWithSelector(...) and related Javadocs to use ValueSerializer and
SerializationContext instead of JsonSerializer and SerializerProvider, and
remove the unused Jackson 2 imports from the class.
In
`@application-engine/src/main/java/com/netgrif/application/engine/serialization/FieldSelector.java`:
- Around line 85-113: `FieldSelector.parse()` is recursively parsing nested
field specs without any depth limit, so a crafted `fields` value can trigger
`StackOverflowError`. Add a recursion-depth guard to `parse()` (and its nested
`parse(inner)` call path), using a bounded counter or maximum nesting constant,
and reject overly deep specs before recursing further. Use the existing `parse`,
`findMatchingParen`, and `nextCommaOrEnd` flow to keep the change localized.
In
`@application-engine/src/main/java/com/netgrif/application/engine/serialization/FieldSelectorHolder.java`:
- Around line 1-36: The field-selection serializer setup is incomplete because
the SimpleModule containing LocalizedEventOutcomeSerializer is created but never
actually registered with the Jackson builder. Update the customizer that builds
the ObjectMapper so the module is explicitly attached before returning it,
rather than relying on findAndAddModules(); use the existing module-building
logic around LocalizedEventOutcomeSerializer and ensure it is added to the
builder so FieldSelectorHolder-driven selection takes effect.
In
`@application-engine/src/main/java/com/netgrif/application/engine/serialization/FieldSelectorInterceptor.java`:
- Around line 18-22: The FieldSelectorInterceptor.preHandle path lets
FieldSelector.parse() throw IllegalArgumentException for malformed fields input,
which bubbles up as a 500. Update preHandle() to catch parse failures around
holder.setSelector(FieldSelector.parse(fields)) and reject the request with a
400 Bad Request instead of allowing the exception to escape; use
FieldSelector.parse and FieldSelectorInterceptor as the key places to change.
In
`@application-engine/src/main/java/com/netgrif/application/engine/serialization/LocalizedEventOutcomeSerializer.java`:
- Around line 99-101: The `LocalizedEventOutcomeSerializer` case branch is using
the selector key `case` while emitting the JSON field name `aCase`, which is
inconsistent with the other fields. Update the `outcome instanceof
LocalisedCaseEventOutcome` handling so the selector check, nested selector, and
written property name all use the same identifier (matching the actual output
name used elsewhere in the serializer), and keep
`dynamicSerializer.serializeWithSelector` aligned with that same symbol.
In
`@application-engine/src/main/java/com/netgrif/application/engine/serialization/ObjectMapperConfiguration.java`:
- Around line 17-19: The ObjectMapperConfiguration change is too broad because
`changeDefaultPropertyInclusion` makes `NON_NULL` apply globally instead of only
to `LocalisedEventOutcome`. Update `ObjectMapperConfiguration` so the
null-handling is scoped to `LocalisedEventOutcome` (or register the
`SimpleModule` separately for `LocalizedEventOutcomeSerializer`) and keep the
mapper’s default inclusion behavior unchanged for all other responses.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e4a8b2c2-0d9b-4dee-902a-dfa57f6a37d7
📒 Files selected for processing (7)
application-engine/src/main/java/com/netgrif/application/engine/configuration/web/config/WebConfig.javaapplication-engine/src/main/java/com/netgrif/application/engine/serialization/DynamicFieldSerializer.javaapplication-engine/src/main/java/com/netgrif/application/engine/serialization/FieldSelector.javaapplication-engine/src/main/java/com/netgrif/application/engine/serialization/FieldSelectorHolder.javaapplication-engine/src/main/java/com/netgrif/application/engine/serialization/FieldSelectorInterceptor.javaapplication-engine/src/main/java/com/netgrif/application/engine/serialization/LocalizedEventOutcomeSerializer.javaapplication-engine/src/main/java/com/netgrif/application/engine/serialization/ObjectMapperConfiguration.java
Comment on lines
+3
to
+4
| import com.fasterxml.jackson.databind.JsonSerializer; | ||
| import com.fasterxml.jackson.databind.SerializerProvider; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
🧩 Analysis chain
🌐 Web query:
tools.jackson.databind SerializationContext introspectBeanDescription Jackson 3 API
💡 Result:
In the Jackson 3 API, the introspectBeanDescription method is defined within the SerializationContext class [1]. However, it is explicitly intended for internal framework use rather than as a public API for general application code [2]. Jackson maintainers have clarified that the introspection mechanism used by internal serializers is not designed to be exposed for external use [2]. While the method may appear in the SerializationContext documentation, it is typically considered restricted to testing or internal processing [2]. For tasks requiring external introspection of types or bean properties, the recommended public API approach is to use ObjectMapper.acceptJsonFormatVisitor() [2]. This method is the officially supported way to inspect or interact with the serialization handling logic of a type [2]. For more advanced requirements, SerializationContextExt is available as an extension of SerializationContext that exposes additional methods needed specifically by ObjectMapper and ObjectWriter, which are further decoupled from the standard SerializationContext API to prevent accidental misuse [3].
Citations:
- 1: https://javadoc.io/static/tools.jackson.core/jackson-databind/3.0.0-rc5/tools.jackson.databind/tools/jackson/databind/SerializationContext.html
- 2: Access the introspector FasterXML/jackson#330
- 3: https://javadoc.io/static/tools.jackson.core/jackson-databind/3.0.0/tools.jackson.databind/tools/jackson/databind/ser/SerializationContextExt.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="application-engine/src/main/java/com/netgrif/application/engine/serialization/DynamicFieldSerializer.java"
echo "== file exists =="
test -f "$file" && echo yes
echo
echo "== line-numbered excerpt =="
cat -n "$file" | sed -n '1,220p'
echo
echo "== dependency/version hints =="
rg -n --hidden --glob '!**/target/**' --glob '!**/build/**' \
'tools\.jackson|com\.fasterxml\.jackson|jackson-databind|jackson-core' \
pom.xml **/pom.xml build.gradle* settings.gradle* gradle.properties 2>/dev/null || trueRepository: netgrif/application-engine
Length of output: 7136
Update the Javadocs to Jackson 3. JsonSerializer/SerializerProvider are only referenced in doc comments, and serializeWithSelector(...) still documents the old SerializerProvider signature. Switch those references to ValueSerializer/SerializationContext and drop the Jackson 2 imports.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@application-engine/src/main/java/com/netgrif/application/engine/serialization/DynamicFieldSerializer.java`
around lines 3 - 4, The Javadocs in DynamicFieldSerializer are still using
Jackson 2 terminology, and the Jackson 2 imports are no longer needed since
those types are only mentioned in comments. Update the doc references in
serializeWithSelector(...) and related Javadocs to use ValueSerializer and
SerializationContext instead of JsonSerializer and SerializerProvider, and
remove the unused Jackson 2 imports from the class.
Comment on lines
+108
to
+124
| for (BeanPropertyDefinition prop : desc.findProperties()) { | ||
|
|
||
| String name = prop.getName(); | ||
| if (!selector.includes(name)) { | ||
| continue; | ||
| } | ||
|
|
||
| Object propValue = prop.getAccessor().getValue(value); | ||
| gen.writeName(name); | ||
|
|
||
| FieldSelector nested = selector.nested(name); | ||
| if (nested.includeAll()) { | ||
| context.findValueSerializer(propValue.getClass()).serialize(propValue, gen, context); | ||
| } else { | ||
| serializeWithSelector(propValue, gen, context, nested); | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
NPE when an included property value is null.
propValue can be null. In the nested.includeAll() branch (Line 120) you call propValue.getClass(), which throws a NullPointerException. This is reachable for any included leaf field whose value is null (the common case, since leaf fields have no nested selector and thus take the includeAll() path). The recursive else branch handles null via the top-of-method guard, but the includeAll branch does not.
🐛 Proposed fix to handle null property values
Object propValue = prop.getAccessor().getValue(value);
gen.writeName(name);
+ if (propValue == null) {
+ gen.writeNull();
+ continue;
+ }
+
FieldSelector nested = selector.nested(name);
if (nested.includeAll()) {
context.findValueSerializer(propValue.getClass()).serialize(propValue, gen, context);
} else {
serializeWithSelector(propValue, gen, context, nested);
}📝 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.
Suggested change
| for (BeanPropertyDefinition prop : desc.findProperties()) { | |
| String name = prop.getName(); | |
| if (!selector.includes(name)) { | |
| continue; | |
| } | |
| Object propValue = prop.getAccessor().getValue(value); | |
| gen.writeName(name); | |
| FieldSelector nested = selector.nested(name); | |
| if (nested.includeAll()) { | |
| context.findValueSerializer(propValue.getClass()).serialize(propValue, gen, context); | |
| } else { | |
| serializeWithSelector(propValue, gen, context, nested); | |
| } | |
| } | |
| for (BeanPropertyDefinition prop : desc.findProperties()) { | |
| String name = prop.getName(); | |
| if (!selector.includes(name)) { | |
| continue; | |
| } | |
| Object propValue = prop.getAccessor().getValue(value); | |
| gen.writeName(name); | |
| if (propValue == null) { | |
| gen.writeNull(); | |
| continue; | |
| } | |
| FieldSelector nested = selector.nested(name); | |
| if (nested.includeAll()) { | |
| context.findValueSerializer(propValue.getClass()).serialize(propValue, gen, context); | |
| } else { | |
| serializeWithSelector(propValue, gen, context, nested); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@application-engine/src/main/java/com/netgrif/application/engine/serialization/DynamicFieldSerializer.java`
around lines 108 - 124, The DynamicFieldSerializer#serializeWithSelector loop
can throw a NullPointerException when an included property value is null because
the nested.includeAll() branch calls propValue.getClass() without a null check.
Update the handling around prop.getAccessor().getValue(value) so the includeAll
path serializes null values safely, reusing the method’s existing null handling
logic before calling context.findValueSerializer, and keep the fix localized to
serializeWithSelector and its nested.includeAll() branch.
Comment on lines
+85
to
+113
| public static FieldSelector parse(String spec) { | ||
| if (spec == null || spec.isBlank()) { | ||
| return new FieldSelector(null, null); | ||
| } | ||
|
|
||
| Set<String> fields = new HashSet<>(); | ||
| Map<String, FieldSelector> nested = new HashMap<>(); | ||
|
|
||
| int i = 0; | ||
| while (i < spec.length()) { | ||
| int commaIdx = nextCommaOrEnd(spec, i); | ||
| int parenthesisIdx = spec.indexOf('(', i); | ||
|
|
||
| if (parenthesisIdx != -1 && parenthesisIdx < commaIdx) { | ||
| String name = spec.substring(i, parenthesisIdx).trim(); | ||
| int closeParenthesisIdx = findMatchingParen(spec, parenthesisIdx); | ||
| String inner = spec.substring(parenthesisIdx + 1, closeParenthesisIdx); | ||
| fields.add(name); | ||
| nested.put(name, parse(inner)); | ||
| i = closeParenthesisIdx + 1; | ||
| if (i < spec.length() && spec.charAt(i) == ',') i++; | ||
| } else { | ||
| String name = spec.substring(i, commaIdx).trim(); | ||
| if (!name.isEmpty()) fields.add(name); | ||
| i = commaIdx + 1; | ||
| } | ||
| } | ||
| return new FieldSelector(fields, nested); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Unbounded recursion in parse() allows StackOverflowError from user-controlled input.
The fields query parameter is user-controlled and parse() recurses without a depth limit. A crafted specification with thousands of nesting levels (e.g., a(a(a(a(...))))) can exhaust the call stack and crash the request-handling thread. With typical Tomcat URL limits (~8KB) and Java's default stack size (~512KB), this is achievable in a single GET request.
🔒 Proposed fix: add a depth guard
public static FieldSelector parse(String spec) {
+ return parse(spec, 0);
+}
+
+private static final int MAX_DEPTH = 32;
+
+private static FieldSelector parse(String spec, int depth) {
+ if (depth >= MAX_DEPTH) {
+ throw new IllegalArgumentException("Field selector nesting exceeds maximum depth of " + MAX_DEPTH);
+ }
if (spec == null || spec.isBlank()) {
return new FieldSelector(null, null);
}
Set<String> fields = new HashSet<>();
Map<String, FieldSelector> nested = new HashMap<>();
int i = 0;
while (i < spec.length()) {
int commaIdx = nextCommaOrEnd(spec, i);
int parenthesisIdx = spec.indexOf('(', i);
if (parenthesisIdx != -1 && parenthesisIdx < commaIdx) {
String name = spec.substring(i, parenthesisIdx).trim();
int closeParenthesisIdx = findMatchingParen(spec, parenthesisIdx);
String inner = spec.substring(parenthesisIdx + 1, closeParenthesisIdx);
fields.add(name);
- nested.put(name, parse(inner));
+ nested.put(name, parse(inner, depth + 1));
i = closeParenthesisIdx + 1;
if (i < spec.length() && spec.charAt(i) == ',') i++;
} else {
String name = spec.substring(i, commaIdx).trim();
if (!name.isEmpty()) fields.add(name);
i = commaIdx + 1;
}
}
return new FieldSelector(fields, nested);
}📝 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.
Suggested change
| public static FieldSelector parse(String spec) { | |
| if (spec == null || spec.isBlank()) { | |
| return new FieldSelector(null, null); | |
| } | |
| Set<String> fields = new HashSet<>(); | |
| Map<String, FieldSelector> nested = new HashMap<>(); | |
| int i = 0; | |
| while (i < spec.length()) { | |
| int commaIdx = nextCommaOrEnd(spec, i); | |
| int parenthesisIdx = spec.indexOf('(', i); | |
| if (parenthesisIdx != -1 && parenthesisIdx < commaIdx) { | |
| String name = spec.substring(i, parenthesisIdx).trim(); | |
| int closeParenthesisIdx = findMatchingParen(spec, parenthesisIdx); | |
| String inner = spec.substring(parenthesisIdx + 1, closeParenthesisIdx); | |
| fields.add(name); | |
| nested.put(name, parse(inner)); | |
| i = closeParenthesisIdx + 1; | |
| if (i < spec.length() && spec.charAt(i) == ',') i++; | |
| } else { | |
| String name = spec.substring(i, commaIdx).trim(); | |
| if (!name.isEmpty()) fields.add(name); | |
| i = commaIdx + 1; | |
| } | |
| } | |
| return new FieldSelector(fields, nested); | |
| } | |
| public static FieldSelector parse(String spec) { | |
| return parse(spec, 0); | |
| } | |
| private static final int MAX_DEPTH = 32; | |
| private static FieldSelector parse(String spec, int depth) { | |
| if (depth >= MAX_DEPTH) { | |
| throw new IllegalArgumentException("Field selector nesting exceeds maximum depth of " + MAX_DEPTH); | |
| } | |
| if (spec == null || spec.isBlank()) { | |
| return new FieldSelector(null, null); | |
| } | |
| Set<String> fields = new HashSet<>(); | |
| Map<String, FieldSelector> nested = new HashMap<>(); | |
| int i = 0; | |
| while (i < spec.length()) { | |
| int commaIdx = nextCommaOrEnd(spec, i); | |
| int parenthesisIdx = spec.indexOf('(', i); | |
| if (parenthesisIdx != -1 && parenthesisIdx < commaIdx) { | |
| String name = spec.substring(i, parenthesisIdx).trim(); | |
| int closeParenthesisIdx = findMatchingParen(spec, parenthesisIdx); | |
| String inner = spec.substring(parenthesisIdx + 1, closeParenthesisIdx); | |
| fields.add(name); | |
| nested.put(name, parse(inner, depth + 1)); | |
| i = closeParenthesisIdx + 1; | |
| if (i < spec.length() && spec.charAt(i) == ',') i++; | |
| } else { | |
| String name = spec.substring(i, commaIdx).trim(); | |
| if (!name.isEmpty()) fields.add(name); | |
| i = commaIdx + 1; | |
| } | |
| } | |
| return new FieldSelector(fields, nested); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@application-engine/src/main/java/com/netgrif/application/engine/serialization/FieldSelector.java`
around lines 85 - 113, `FieldSelector.parse()` is recursively parsing nested
field specs without any depth limit, so a crafted `fields` value can trigger
`StackOverflowError`. Add a recursion-depth guard to `parse()` (and its nested
`parse(inner)` call path), using a bounded counter or maximum nesting constant,
and reject overly deep specs before recursing further. Use the existing `parse`,
`findMatchingParen`, and `nextCommaOrEnd` flow to keep the change localized.
Comment on lines
+1
to
+36
| package com.netgrif.application.engine.serialization; | ||
|
|
||
| import lombok.Getter; | ||
| import lombok.Setter; | ||
| import org.springframework.stereotype.Component; | ||
| import org.springframework.web.context.annotation.RequestScope; | ||
|
|
||
| /** | ||
| * Request-scoped holder component for managing {@link FieldSelector} instances during HTTP request processing. | ||
| * <p> | ||
| * This component is used in conjunction with {@link FieldSelectorInterceptor} to parse and store field selection | ||
| * criteria from HTTP request parameters (typically the "fields" query parameter). The held {@link FieldSelector} | ||
| * is then used by custom serializers like {@link LocalizedEventOutcomeSerializer} to control which fields | ||
| * are included in JSON response bodies. | ||
| * </p> | ||
| * <p> | ||
| * Being request-scoped ensures that each HTTP request has its own isolated instance of this holder, | ||
| * preventing thread-safety issues in concurrent request processing. | ||
| * </p> | ||
| * | ||
| * @see FieldSelector | ||
| * @see FieldSelectorInterceptor | ||
| * @see LocalizedEventOutcomeSerializer | ||
| */ | ||
| @Setter | ||
| @Getter | ||
| @Component | ||
| @RequestScope | ||
| public class FieldSelectorHolder { | ||
|
|
||
| /** | ||
| * The field selector that determines which fields should be included during serialization. | ||
| * Initialized with a default selector that includes all fields (parsed from null). | ||
| */ | ||
| private FieldSelector selector = FieldSelector.parse(null); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect ObjectMapperConfiguration for module registration.
fd "ObjectMapperConfiguration.java" --exec cat -nRepository: netgrif/application-engine
Length of output: 1277
Register the SimpleModule before returning the customizer
module is populated with LocalizedEventOutcomeSerializer but never attached to the builder. findAndAddModules() only picks up discoverable modules, so this serializer is never registered and the field-selection path stays inactive.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@application-engine/src/main/java/com/netgrif/application/engine/serialization/FieldSelectorHolder.java`
around lines 1 - 36, The field-selection serializer setup is incomplete because
the SimpleModule containing LocalizedEventOutcomeSerializer is created but never
actually registered with the Jackson builder. Update the customizer that builds
the ObjectMapper so the module is explicitly attached before returning it,
rather than relying on findAndAddModules(); use the existing module-building
logic around LocalizedEventOutcomeSerializer and ensure it is added to the
builder so FieldSelectorHolder-driven selection takes effect.
Comment on lines
+18
to
+22
| @Override | ||
| public boolean preHandle(HttpServletRequest request, @NonNull HttpServletResponse response, @NonNull Object handler) { | ||
| String fields = request.getParameter("fields"); | ||
| holder.setSelector(FieldSelector.parse(fields)); | ||
| return true; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Malformed fields parameter causes HTTP 500 instead of 400.
FieldSelector.parse() throws IllegalArgumentException for unmatched parentheses (via findMatchingParen). This propagates unhandled from preHandle(), resulting in a 500 Internal Server Error for what is a client-side input mistake. A 400 Bad Request is the correct response.
🛡️ Proposed fix: catch parse errors and reject the request
`@Override`
public boolean preHandle(HttpServletRequest request, `@NonNull` HttpServletResponse response, `@NonNull` Object handler) {
String fields = request.getParameter("fields");
- holder.setSelector(FieldSelector.parse(fields));
+ try {
+ holder.setSelector(FieldSelector.parse(fields));
+ } catch (IllegalArgumentException e) {
+ response.setStatus(HttpServletResponse.SC_BAD_REQUEST);
+ return false;
+ }
return true;
}📝 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.
Suggested change
| @Override | |
| public boolean preHandle(HttpServletRequest request, @NonNull HttpServletResponse response, @NonNull Object handler) { | |
| String fields = request.getParameter("fields"); | |
| holder.setSelector(FieldSelector.parse(fields)); | |
| return true; | |
| `@Override` | |
| public boolean preHandle(HttpServletRequest request, `@NonNull` HttpServletResponse response, `@NonNull` Object handler) { | |
| String fields = request.getParameter("fields"); | |
| try { | |
| holder.setSelector(FieldSelector.parse(fields)); | |
| } catch (IllegalArgumentException e) { | |
| response.setStatus(HttpServletResponse.SC_BAD_REQUEST); | |
| return false; | |
| } | |
| return true; |
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 19-19: Prevent HTTP parameter pollution
Context: request.getParameter("fields")
Note: [CWE-235] Improper Handling of Extra Parameters. Security best practice.
(http-parameter-pollution)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@application-engine/src/main/java/com/netgrif/application/engine/serialization/FieldSelectorInterceptor.java`
around lines 18 - 22, The FieldSelectorInterceptor.preHandle path lets
FieldSelector.parse() throw IllegalArgumentException for malformed fields input,
which bubbles up as a 500. Update preHandle() to catch parse failures around
holder.setSelector(FieldSelector.parse(fields)) and reject the request with a
400 Bad Request instead of allowing the exception to escape; use
FieldSelector.parse and FieldSelectorInterceptor as the key places to change.
Comment on lines
+99
to
+101
| if (outcome instanceof LocalisedCaseEventOutcome specificOutcome && selector.includes("case") && specificOutcome.getaCase() != null) { | ||
| gen.writeName("aCase"); | ||
| dynamicSerializer.serializeWithSelector(specificOutcome.getaCase(), gen, context, selector.nested("case")); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Selector key case does not match emitted field name aCase.
Inclusion is checked via selector.includes("case") and selector.nested("case"), but the JSON property is written as aCase. Clients must request fields=case yet receive aCase in the response — inconsistent with every other field here (net, task, data), where the query key matches the output name. Align the selector key and output name to avoid confusing API consumers.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@application-engine/src/main/java/com/netgrif/application/engine/serialization/LocalizedEventOutcomeSerializer.java`
around lines 99 - 101, The `LocalizedEventOutcomeSerializer` case branch is
using the selector key `case` while emitting the JSON field name `aCase`, which
is inconsistent with the other fields. Update the `outcome instanceof
LocalisedCaseEventOutcome` handling so the selector check, nested selector, and
written property name all use the same identifier (matching the actual output
name used elsewhere in the serializer), and keep
`dynamicSerializer.serializeWithSelector` aligned with that same symbol.
Comment on lines
+17
to
+19
| return builder -> builder | ||
| .changeDefaultPropertyInclusion(include -> include.withValueInclusion(JsonInclude.Include.NON_NULL)) | ||
| .findAndAddModules(); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the configuration file and surrounding package context.
FILE="application-engine/src/main/java/com/netgrif/application/engine/serialization/ObjectMapperConfiguration.java"
wc -l "$FILE"
cat -n "$FILE"
# Find where this configuration class is used and whether it applies to a single mapper/module.
rg -n "ObjectMapperConfiguration|changeDefaultPropertyInclusion|JsonInclude.Include.NON_NULL|LocalisedEventOutcome|findAndAddModules|addModule" application-engine/src/main/java
# Map nearby serialization-related classes.
fd -a ".*Serialization.*|.*ObjectMapper.*|.*Outcome.*|.*Jackson.*" application-engine/src/main/javaRepository: netgrif/application-engine
Length of output: 17895
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect related Jackson configuration and any other customizers to determine scope.
FILES=(
"application-engine/src/main/java/com/netgrif/application/engine/configuration/properties/JacksonConfigurationProperties.java"
"application-engine/src/main/java/com/netgrif/application/engine/serialization/ObjectMapperConfiguration.java"
"application-engine/src/main/java/com/netgrif/application/engine/workflow/service/MenuImportExportService.java"
"application-engine/src/main/java/com/netgrif/application/engine/workflow/service/FilterImportExportService.java"
"application-engine/src/main/java/com/netgrif/application/engine/workflow/service/DataService.java"
)
for f in "${FILES[@]}"; do
if [ -f "$f" ]; then
echo "===== $f ====="
wc -l "$f"
cat -n "$f" | sed -n '1,220p'
fi
done
# Search for Jackson builder customizers and object mapper customization patterns.
rg -n "JsonMapperBuilderCustomizer|changeDefaultPropertyInclusion|findAndAddModules|addModule\\(|ObjectMapper|Jackson2ObjectMapperBuilder|JsonInclude.Include" application-engine/src/main/javaRepository: netgrif/application-engine
Length of output: 44435
Scope this inclusion change Global NON_NULL here applies to every response, while the SimpleModule is never registered, so LocalizedEventOutcomeSerializer is not used. Keep the null-handling on LocalisedEventOutcome only, or add the module separately and leave the default mapper behavior unchanged.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@application-engine/src/main/java/com/netgrif/application/engine/serialization/ObjectMapperConfiguration.java`
around lines 17 - 19, The ObjectMapperConfiguration change is too broad because
`changeDefaultPropertyInclusion` makes `NON_NULL` apply globally instead of only
to `LocalisedEventOutcome`. Update `ObjectMapperConfiguration` so the
null-handling is scoped to `LocalisedEventOutcome` (or register the
`SimpleModule` separately for `LocalizedEventOutcomeSerializer`) and keep the
mapper’s default inclusion behavior unchanged for all other responses.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Optimized outcome size that is returned to frontend.
Implements NAE-2451
Dependencies
No new dependencies were introduced
Third party dependencies
No new dependencies were introduced
Blocking Pull requests
There are no dependencies on other PR
How Has Been This Tested?
This was tested manually and with unit tests
Test Configuration
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes