You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@geode.apache.org by GitBox <gi...@apache.org> on 2022/05/27 19:18:17 UTC
[GitHub] [geode] jmelchio opened a new pull request, #7737: GEODE-10301: support LocalDate and JodaTime
jmelchio opened a new pull request, #7737:
URL: https://github.com/apache/geode/pull/7737
Co-authored-by: Jinmei Liao <jl...@pivotal.io>
- include libraries so that end-users won't have to add them to the java
path
- ensure proper serialization in gfsh and pulse
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jmelchio merged pull request #7737: GEODE-10301: support LocalDate and JodaTime
Posted by GitBox <gi...@apache.org>.
jmelchio merged PR #7737:
URL: https://github.com/apache/geode/pull/7737
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] onichols-pivotal commented on a diff in pull request #7737: GEODE-10301: support LocalDate and JodaTime
Posted by GitBox <gi...@apache.org>.
onichols-pivotal commented on code in PR #7737:
URL: https://github.com/apache/geode/pull/7737#discussion_r885883301
##########
geode-common/src/main/java/org/apache/geode/util/internal/GeodeJsonMapper.java:
##########
@@ -34,13 +36,21 @@ public class GeodeJsonMapper {
*/
public static ObjectMapper getMapper() {
ObjectMapper mapper = JsonMapper.builder()
+ .addModule(new JavaTimeModule())
Review Comment:
however we need to consider the potential security implications before wildcard-adding things to the serialization stack, as it could open a vector to gadget-chaining or other attacks
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] pivotal-jbarrett commented on a diff in pull request #7737: GEODE-10301: support LocalDate and JodaTime
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on code in PR #7737:
URL: https://github.com/apache/geode/pull/7737#discussion_r887221766
##########
build-tools/geode-dependency-management/src/main/groovy/org/apache/geode/gradle/plugins/DependencyConstraints.groovy:
##########
@@ -46,6 +46,7 @@ class DependencyConstraints {
deps.put("jboss-modules.version", "1.11.0.Final")
deps.put("jackson.version", "2.13.2")
deps.put("jackson.databind.version", "2.13.2.2")
+ deps.put("joda-time.version", "2.10.14")
Review Comment:
The indirection is excessive when there isn't multiple modules for a similar version number or any other need for the version number to be known outside the constraints.
##########
geode-core/src/test/java/org/apache/geode/management/internal/json/QueryResultFormatterTest.java:
##########
@@ -31,6 +31,7 @@
import com.fasterxml.jackson.databind.JsonNode;
import com.fasterxml.jackson.databind.ObjectMapper;
+import org.joda.time.DateTime;
import org.junit.Test;
Review Comment:
Please convert unit test to JUnit 5.
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] onichols-pivotal commented on a diff in pull request #7737: GEODE-10301: support LocalDate and JodaTime
Posted by GitBox <gi...@apache.org>.
onichols-pivotal commented on code in PR #7737:
URL: https://github.com/apache/geode/pull/7737#discussion_r885881588
##########
geode-common/src/main/java/org/apache/geode/util/internal/GeodeJsonMapper.java:
##########
@@ -34,13 +36,21 @@ public class GeodeJsonMapper {
*/
public static ObjectMapper getMapper() {
ObjectMapper mapper = JsonMapper.builder()
+ .addModule(new JavaTimeModule())
Review Comment:
we do already call `findAndRegisterModules` at least one place (geode-core/src/main/java/org/apache/geode/management/internal/json/AbstractJSONFormatter.java) but that stopped working as expected with Jackson 2.10. Stackoverflow suggests the replacement is `findAndAddModules`
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jinmeiliao commented on a diff in pull request #7737: GEODE-10301: support LocalDate and JodaTime
Posted by GitBox <gi...@apache.org>.
jinmeiliao commented on code in PR #7737:
URL: https://github.com/apache/geode/pull/7737#discussion_r885884083
##########
geode-common/src/main/java/org/apache/geode/util/internal/GeodeJsonMapper.java:
##########
@@ -34,13 +36,21 @@ public class GeodeJsonMapper {
*/
public static ObjectMapper getMapper() {
ObjectMapper mapper = JsonMapper.builder()
+ .addModule(new JavaTimeModule())
Review Comment:
it's still available and working on Jackson 2.13. I didn't find the method `findAndAddModules`
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] kirklund commented on a diff in pull request #7737: GEODE-10301: support LocalDate and JodaTime
Posted by GitBox <gi...@apache.org>.
kirklund commented on code in PR #7737:
URL: https://github.com/apache/geode/pull/7737#discussion_r887029999
##########
geode-assembly/src/acceptanceTest/java/org/apache/geode/management/internal/rest/StandaloneClientManagementAPIAcceptanceTest.java:
##########
@@ -115,14 +115,19 @@ locatorPort, httpPort, jmxPort, getSslParameters()),
.withName("startCluster").execute(gfsh);
+ int expectedReturnCode = 0;
assertThat(startCluster.getProcess().exitValue())
- .as("Cluster did not start correctly").isEqualTo(0);
+ .as("Cluster did not start correctly").isEqualTo(expectedReturnCode);
Process process = launchClientProcess(outputJar, httpPort);
- boolean exited = process.waitFor(30, TimeUnit.SECONDS);
- assertThat(exited).as("Process did not exit within 10 seconds").isTrue();
- assertThat(process.exitValue()).as("Process did not exit with 0 return code").isEqualTo(0);
+ int processTimeout = 30;
Review Comment:
All test timeouts should use GeodeAwaitility.getTimeout() which is 5 minutes.
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jinmeiliao commented on a diff in pull request #7737: GEODE-10301: support LocalDate and JodaTime
Posted by GitBox <gi...@apache.org>.
jinmeiliao commented on code in PR #7737:
URL: https://github.com/apache/geode/pull/7737#discussion_r885876741
##########
geode-common/src/main/java/org/apache/geode/util/internal/GeodeJsonMapper.java:
##########
@@ -34,13 +36,21 @@ public class GeodeJsonMapper {
*/
public static ObjectMapper getMapper() {
ObjectMapper mapper = JsonMapper.builder()
+ .addModule(new JavaTimeModule())
Review Comment:
Now I think of it, we should not spell out those modules here. Instead, we should do `findAndRegisterModules` to search for all modules available on the classpath. This would get rid of the need to require these on the management java client. I will submit a commit for iit.
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jinmeiliao commented on a diff in pull request #7737: GEODE-10301: support LocalDate and JodaTime
Posted by GitBox <gi...@apache.org>.
jinmeiliao commented on code in PR #7737:
URL: https://github.com/apache/geode/pull/7737#discussion_r885894426
##########
geode-common/src/main/java/org/apache/geode/util/internal/GeodeJsonMapper.java:
##########
@@ -34,13 +36,21 @@ public class GeodeJsonMapper {
*/
public static ObjectMapper getMapper() {
ObjectMapper mapper = JsonMapper.builder()
+ .addModule(new JavaTimeModule())
Review Comment:
On a 2nd though, since these `getMapper` are called on each query/get/function execution, `findModule` is not cached and could be expensive, so using this might not be a good idea. It's fine that our management java client also supports joda time as well.
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] onichols-pivotal commented on a diff in pull request #7737: GEODE-10301: support LocalDate and JodaTime
Posted by GitBox <gi...@apache.org>.
onichols-pivotal commented on code in PR #7737:
URL: https://github.com/apache/geode/pull/7737#discussion_r885881588
##########
geode-common/src/main/java/org/apache/geode/util/internal/GeodeJsonMapper.java:
##########
@@ -34,13 +36,21 @@ public class GeodeJsonMapper {
*/
public static ObjectMapper getMapper() {
ObjectMapper mapper = JsonMapper.builder()
+ .addModule(new JavaTimeModule())
Review Comment:
we do already call `findAndRegisterModules` but that stopped working as expected with Jackson 2.10. Stackoverflow suggests the replacement is `findAndAddModules`
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] onichols-pivotal commented on a diff in pull request #7737: GEODE-10301: support LocalDate and JodaTime
Posted by GitBox <gi...@apache.org>.
onichols-pivotal commented on code in PR #7737:
URL: https://github.com/apache/geode/pull/7737#discussion_r885887287
##########
geode-common/src/main/java/org/apache/geode/util/internal/GeodeJsonMapper.java:
##########
@@ -34,13 +36,21 @@ public class GeodeJsonMapper {
*/
public static ObjectMapper getMapper() {
ObjectMapper mapper = JsonMapper.builder()
+ .addModule(new JavaTimeModule())
Review Comment:
see https://stackoverflow.com/questions/69831153/jackson-objectmapper-findandregistermodules-not-working-to-serialise-localdate findAndAddModules is on the builder, not the mapper.
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org