avivijay19 commented on code in PR #6069:
URL: https://github.com/apache/fineract/pull/6069#discussion_r3633705772
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/jobs/filter/COBFilterApiMatcher.java:
##########
@@ -20,17 +20,19 @@
import static
org.apache.fineract.batch.command.CommandStrategyUtils.isRelativeUrlVersioned;
-import com.fasterxml.jackson.core.type.TypeReference;
-import com.fasterxml.jackson.databind.ObjectMapper;
import java.io.IOException;
import java.util.List;
import org.apache.commons.lang3.StringUtils;
import org.apache.fineract.batch.domain.BatchRequest;
import
org.apache.fineract.infrastructure.core.http.BodyCachingHttpServletRequestWrapper;
+import tools.jackson.core.json.JsonReadFeature;
+import tools.jackson.core.type.TypeReference;
+import tools.jackson.databind.ObjectMapper;
+import tools.jackson.databind.json.JsonMapper;
public abstract class COBFilterApiMatcher implements COBFilterHelper {
- protected final ObjectMapper objectMapper = new ObjectMapper();
+ protected final ObjectMapper objectMapper =
JsonMapper.builder().enable(JsonReadFeature.ALLOW_UNESCAPED_CONTROL_CHARS).build();
Review Comment:
The two forms are equivalent, and the feature is not newly introduced. On
develop, `LoanCOBFilterHelperImpl.afterPropertiesSet()` already applied
`objectMapper.configure(JsonReadFeature.ALLOW_UNESCAPED_CONTROL_CHARS.mappedFeature(),
true)` to this same field. Jackson 3 object mappers are immutable, so the
feature must now be declared at builder time, which is why the
`InitializingBean` hook was removed.
There is one behavioral difference worth noting. As an instance field, the
feature previously applied only to `LoanCOBFilterHelperImpl`; the two sibling
matchers did not enable it, despite parsing the same raw request bodies. It now
applies to all three. This mapper is used solely to inspect an incoming body
for loan identifiers in order to determine COB routing, and the request
continues to be parsed and validated normally further down the chain, so this
does not relax any validation applied to the request itself.
##########
fineract-provider/src/main/java/org/apache/fineract/portfolio/savings/service/search/SavingsAccountTransactionsSearchServiceImpl.java:
##########
@@ -141,6 +142,7 @@ private static void addFromToFilter(@NonNull String column,
String fromValue, St
}
@Nullable
+ @SuppressFBWarnings("NP_BOOLEAN_RETURN_NULL")
Review Comment:
This is a consequence of the JSpecify migration rather than a new null
return. `org.springframework.lang.Nullable` was a JSR-305
`@TypeQualifierNickname` for `@CheckForNull`, which SpotBugs recognizes and
which suppressed this detector. `org.jspecify.annotations.Nullable` carries no
JSR-305 meta-annotation, so SpotBugs now reports the tri-state `Boolean` return
that has been present in this method throughout.
The method deliberately distinguishes three states: `true` for a filter
applied, `false` for no filter, and `null` for a filter matching nothing, and
the caller depends on that distinction. I chose to suppress rather than alter
behavior within a dependency upgrade. I can raise a separate ticket to replace
the tri-state return with an explicit type if you would like it addressed
properly.
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/core/jersey/JerseyJacksonObjectArgumentHandler.java:
##########
@@ -68,9 +73,27 @@ public T readFrom(Class<T> type, Type genericType,
Annotation[] annotations, jak
return type.cast(json);
} else {
// Create the proper type from the JSON
- HttpHeaders headers = new HttpHeaders();
- headers.putAll(httpHeaders);
- return (T) converter.read(genericType, type, new
MappingJacksonInputMessage(entityStream, headers));
Review Comment:
The same `ObjectMapper` instance is used, so the wire format is unchanged.
`JerseyJacksonConverterConfig`, which this PR does not modify, defines the bean
named `objectMapper`, and the auto-configured
`MappingJackson2HttpMessageConverter` was constructed around that same bean.
All Fineract serializers and deserializers, `NON_NULL` inclusion, the disabled
`FAIL_ON_UNKNOWN_PROPERTIES`, case-insensitive enums and
`JacksonLocalDateArrayModule` therefore continue to apply. What has been
removed is only the converter wrapper, which Spring Boot 4 no longer
auto-configures.
The read path performs the same call the converter made internally, namely
`readValue` against a `JavaType` constructed from the generic type. The write
path calls `writeValue` directly, and `@Produces(APPLICATION_JSON)` retains
UTF-8 encoding as before.
Two aspects were addressed deliberately. The existing
`CommandProcessingResultSerializationTest` was repointed at the mapper and
continues to assert the same output. The `HttpMessageNotReadableException`
translation is performed explicitly in the catch block, so that malformed or
unbindable bodies still reach `HttpMessageNotReadableErrorController` and
produce the platform validation error rather than a raw Jackson message. Beyond
that, every API call in the integration and e2e suites is serialized through
this provider, and those suites pass.
##########
build.gradle:
##########
@@ -97,9 +97,18 @@ buildscript {
maven { url 'https://plugins.gradle.org/m2/' }
}
+ configurations.classpath {
Review Comment:
It is currently required. The Spring Boot 4 Gradle plugin places spring-core
7 on the shared buildscript classpath, while the hierynomus license plugin
resolves `license-maven-plugin:3.0`, which invokes a
`PropertyPlaceholderHelper(String, String, String, boolean)` constructor that
Spring Framework 7 removed.
I have re-verified this against 4.1.0 by removing the block:
```
> Task :fineract-core:licenseMain FAILED
> 'void
org.springframework.util.PropertyPlaceholderHelper.<init>(java.lang.String,
java.lang.String, java.lang.String, boolean)'
```
The constraint applies to the buildscript classpath only and has no effect
on the spring-core version used by the application at runtime. It can be
removed once the license plugin supports Spring Framework 7.
##########
fineract-e2e-tests-core/src/test/resources/fineract-test-application.properties:
##########
@@ -38,14 +38,15 @@
fineract-test.messaging.jms.topic-name=${ACTIVEMQ_TOPIC_NAME:}
fineract-test.event.verification-enabled=${EVENT_VERIFICATION_ENABLED:false}
fineract-test.event.wait-timeout-in-ms=${POLLING_EVENT_WAIT_TIMEOUT_IN_MS:5000}
-fineract-test.event.delay-in-ms=${POLLING_EVENT_WAIT_TIMEOUT_IN_MS:100}
-fineract-test.event.interval-in-ms=${POLLING_EVENT_WAIT_TIMEOUT_IN_MS:100}
Review Comment:
Both `delay-in-ms` and `interval-in-ms` were reading
`POLLING_EVENT_WAIT_TIMEOUT_IN_MS`, which appears to be a copy-paste error: the
two polling parameters could not be configured independently, and overriding
the wait timeout silently altered the polling delay and interval as well. The
default values are unchanged; each property now reads its own variable.
This is unrelated to the upgrade, so I can move it into the same follow-up
PR as the remaining items if you prefer.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
