You are viewing a plain text version of this content. The canonical link for it is here.
Posted to gitbox@activemq.apache.org by GitBox <gi...@apache.org> on 2021/05/25 11:47:58 UTC
[GitHub] [activemq-artemis] brusdev opened a new pull request #3594: ARTEMIS-3315 QueueControl fails on serializing AMQP messages with inv…
brusdev opened a new pull request #3594:
URL: https://github.com/apache/activemq-artemis/pull/3594
…alid application properties
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on a change in pull request #3594: ARTEMIS-3315 Fix JSON serialization of AMQP messages
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on a change in pull request #3594:
URL: https://github.com/apache/activemq-artemis/pull/3594#discussion_r640740125
##########
File path: tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/amqp/JMXManagementTest.java
##########
@@ -137,6 +142,45 @@ public void testGetFirstMessage() throws Exception {
}
}
+ @Test
+ public void testGetFirstMessageWithAMQPTypes() throws Exception {
+ AmqpClient client = createAmqpClient();
+ AmqpConnection connection = addConnection(client.connect());
+
+ try {
+ UUID uuid = UUID.randomUUID();
+ Character character = new Character('C');
+ AmqpSession session = connection.createSession();
+ AmqpSender sender = session.createSender(getQueueName());
+
+ session.begin();
+ AmqpMessage message = new AmqpMessage();
+ message.setApplicationProperty("TEST_UUID", uuid);
+ message.setApplicationProperty("TEST_CHAR", character);
+ message.setApplicationProperty("TEST_DECIMAL_32", new Decimal32(BigDecimal.ONE));
+ message.setApplicationProperty("TEST_DECIMAL_64", new Decimal64(BigDecimal.ONE));
+ message.setApplicationProperty("TEST_DECIMAL_128", new Decimal128(BigDecimal.ONE));
+
+ sender.send(message);
+ session.commit();
+
+ SimpleString queue = new SimpleString(getQueueName());
+ QueueControl queueControl = createManagementControl(queue, queue);
+ String firstMessageAsJSON = queueControl.getFirstMessageAsJSON();
+ Assert.assertNotNull(firstMessageAsJSON);
+
+ JsonObject firstMessageObject = JsonUtil.readJsonArray(firstMessageAsJSON).getJsonObject(0);
+
+ Assert.assertEquals(uuid.toString(), firstMessageObject.getString("TEST_UUID"));
+ Assert.assertEquals(character.toString(), firstMessageObject.getString("TEST_CHAR"));
+ Assert.assertNotNull(firstMessageObject.getJsonNumber("TEST_DECIMAL_32").doubleValue());
Review comment:
I am merging with a slight change here.. no need to check for .doubleValue()..I'm amending the change as yours..
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on a change in pull request #3594: ARTEMIS-3315 Fix JSON serialization of AMQP messages
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on a change in pull request #3594:
URL: https://github.com/apache/activemq-artemis/pull/3594#discussion_r640235326
##########
File path: artemis-core-client/src/main/java/org/apache/activemq/artemis/api/core/JsonUtil.java
##########
@@ -173,7 +172,7 @@ public static void addToObject(final String key, final Object param, final JsonO
}
jsonObjectBuilder.add(key, objectArrayBuilder);
} else {
- throw ActiveMQClientMessageBundle.BUNDLE.invalidManagementParam(param.getClass().getName());
Review comment:
You missed one thing here...
add an instance Number.. and add a double number...
look at my original proposal.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on a change in pull request #3594: ARTEMIS-3315 Fix serializing AMQP messages with invalid application properties
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on a change in pull request #3594:
URL: https://github.com/apache/activemq-artemis/pull/3594#discussion_r638956841
##########
File path: artemis-core-client/src/main/java/org/apache/activemq/artemis/api/core/JsonUtil.java
##########
@@ -212,10 +224,10 @@ public static void addToArray(final Object param, final JsonArrayBuilder jsonArr
} else if (param instanceof Object[]) {
JsonArrayBuilder objectArrayBuilder = JsonLoader.createArrayBuilder();
for (Object parameter : (Object[])param) {
- addToArray(parameter, objectArrayBuilder);
+ addToArray(parameter, objectArrayBuilder, ignoreInvalidParams);
}
jsonArrayBuilder.add(objectArrayBuilder);
- } else {
+ } else if (!ignoreInvalidParams) {
Review comment:
Instead of throwing an error,
what about just make the method be:
```java
public static void addToObject(final String key, final Object param, final JsonObjectBuilder jsonObjectBuilder) {
if (param instanceof Integer) {
jsonObjectBuilder.add(key, (Integer) param);
} else if (param instanceof Long) {
jsonObjectBuilder.add(key, (Long) param);
} else if (param instanceof Double) {
jsonObjectBuilder.add(key, (Double) param);
} else if (param instanceof String) {
jsonObjectBuilder.add(key, (String) param);
} else if (param instanceof Boolean) {
jsonObjectBuilder.add(key, (Boolean) param);
} else if (param instanceof Map) {
JsonObject mapObject = toJsonObject((Map<String, Object>) param);
jsonObjectBuilder.add(key, mapObject);
} else if (param instanceof Short) {
jsonObjectBuilder.add(key, (Short) param);
} else if (param instanceof Byte) {
jsonObjectBuilder.add(key, ((Byte) param).shortValue());
} else if (param instanceof SimpleString) {
jsonObjectBuilder.add(key, param.toString());
} else if (param == null) {
jsonObjectBuilder.addNull(key);
} else if (param instanceof byte[]) {
JsonArrayBuilder byteArrayObject = toJsonArrayBuilder((byte[]) param);
jsonObjectBuilder.add(key, byteArrayObject);
} else if (param instanceof Object[]) {
final JsonArrayBuilder objectArrayBuilder = JsonLoader.createArrayBuilder();
for (Object parameter : (Object[])param) {
addToArray(parameter, objectArrayBuilder);
}
jsonObjectBuilder.add(key, objectArrayBuilder);
} else if (param instanceof Number) {
jsonObjectBuilder.add(key, ((Number)param).doubleValue());
} else {
jsonObjectBuilder.add(key, param.toString());
}
}
```
remove the argument ignoreInvalidParams
and do a similar change on the addToArray
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] asfgit closed pull request #3594: ARTEMIS-3315 Fix JSON serialization of AMQP messages
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #3594:
URL: https://github.com/apache/activemq-artemis/pull/3594
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on a change in pull request #3594: ARTEMIS-3315 Fix JSON serialization of AMQP messages
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on a change in pull request #3594:
URL: https://github.com/apache/activemq-artemis/pull/3594#discussion_r640235621
##########
File path: artemis-core-client/src/main/java/org/apache/activemq/artemis/api/core/JsonUtil.java
##########
@@ -173,7 +172,7 @@ public static void addToObject(final String key, final Object param, final JsonO
}
jsonObjectBuilder.add(key, objectArrayBuilder);
} else {
- throw ActiveMQClientMessageBundle.BUNDLE.invalidManagementParam(param.getClass().getName());
Review comment:
```
public static void addToObject(final String key, final Object param, final JsonObjectBuilder jsonObjectBuilder) {
if (param instanceof Integer) {
jsonObjectBuilder.add(key, (Integer) param);
} else if (param instanceof Long) {
jsonObjectBuilder.add(key, (Long) param);
} else if (param instanceof Double) {
jsonObjectBuilder.add(key, (Double) param);
} else if (param instanceof String) {
jsonObjectBuilder.add(key, (String) param);
} else if (param instanceof Boolean) {
jsonObjectBuilder.add(key, (Boolean) param);
} else if (param instanceof Map) {
JsonObject mapObject = toJsonObject((Map<String, Object>) param);
jsonObjectBuilder.add(key, mapObject);
} else if (param instanceof Short) {
jsonObjectBuilder.add(key, (Short) param);
} else if (param instanceof Byte) {
jsonObjectBuilder.add(key, ((Byte) param).shortValue());
} else if (param instanceof SimpleString) {
jsonObjectBuilder.add(key, param.toString());
} else if (param == null) {
jsonObjectBuilder.addNull(key);
} else if (param instanceof byte[]) {
JsonArrayBuilder byteArrayObject = toJsonArrayBuilder((byte[]) param);
jsonObjectBuilder.add(key, byteArrayObject);
} else if (param instanceof Object[]) {
final JsonArrayBuilder objectArrayBuilder = JsonLoader.createArrayBuilder();
for (Object parameter : (Object[])param) {
addToArray(parameter, objectArrayBuilder);
}
jsonObjectBuilder.add(key, objectArrayBuilder);
} else if (param instanceof Number) {
jsonObjectBuilder.add(key, ((Number)param).doubleValue());
} else {
jsonObjectBuilder.add(key, param.toString());
}
}
```
the instance Number is important
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] brusdev commented on pull request #3594: ARTEMIS-3315 Fix JSON serialization of AMQP messages
Posted by GitBox <gi...@apache.org>.
brusdev commented on pull request #3594:
URL: https://github.com/apache/activemq-artemis/pull/3594#issuecomment-848847380
@clebertsuconic @gemmellr thanks for your feedback, I have just update the PR
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on pull request #3594: ARTEMIS-3315 Fix JSON serialization of AMQP messages
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on pull request #3594:
URL: https://github.com/apache/activemq-artemis/pull/3594#issuecomment-849641489
change looks great, however the test fails:
java.lang.ClassCastException: Cannot cast org.apache.johnzon.core.JsonNumberImpl to javax.json.JsonString
at java.lang.Class.cast(Class.java:3369)
at org.apache.johnzon.core.JsonObjectImpl.value(JsonObjectImpl.java:41)
at org.apache.johnzon.core.JsonObjectImpl.getJsonString(JsonObjectImpl.java:68)
at org.apache.johnzon.core.JsonObjectImpl.getString(JsonObjectImpl.java:73)
at org.apache.activemq.artemis.tests.integration.amqp.JMXManagementTest.testGetFirstMessageWithAMQPTypes(JMXManagementTest.java:178)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.lang.reflect.Method.invoke(Method.java:498)
at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:59)
at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:56)
at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
at org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:26)
at org.junit.internal.runners.statements.RunAfters.evaluate(RunAfters.java:27)
at org.junit.rules.ExternalResource$1.evaluate(ExternalResource.java:54)
at org.junit.rules.TestWatcher$1.evaluate(TestWatcher.java:61)
at org.junit.rules.TestWatcher$1.evaluate(TestWatcher.java:61)
at org.junit.rules.ExternalResource$1.evaluate(ExternalResource.java:54)
at org.junit.rules.ExternalResource$1.evaluate(ExternalResource.java:54)
at org.junit.rules.TestWatcher$1.evaluate(TestWatcher.java:61)
at org.junit.runners.ParentRunner$3.evaluate(ParentRunner.java:306)
at org.junit.runners.BlockJUnit4ClassRunner$1.evaluate(BlockJUnit4ClassRunner.java:100)
at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:366)
at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:103)
at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:63)
at org.junit.runners.ParentRunner$4.run(ParentRunner.java:331)
at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:79)
at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:329)
at org.junit.runners.ParentRunner.access$100(ParentRunner.java:66)
at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:293)
at org.junit.internal.runners.statements.RunAfters.evaluate(RunAfters.java:27)
at org.junit.rules.TestWatcher$1.evaluate(TestWatcher.java:61)
at org.junit.rules.TestWatcher$1.evaluate(TestWatcher.java:61)
at org.junit.rules.TestWatcher$1.evaluate(TestWatcher.java:61)
at org.junit.rules.RunRules.evaluate(RunRules.java:20)
at org.junit.runners.ParentRunner$3.evaluate(ParentRunner.java:306)
at org.junit.runners.ParentRunner.run(ParentRunner.java:413)
at org.junit.runner.JUnitCore.run(JUnitCore.java:137)
at com.intellij.junit4.JUnit4IdeaTestRunner.startRunnerWithArgs(JUnit4IdeaTestRunner.java:69)
at com.intellij.rt.junit.IdeaTestRunner$Repeater.startRunnerWithArgs(IdeaTestRunner.java:33)
at com.intellij.rt.junit.JUnitStarter.prepareStreamsAndStart(JUnitStarter.java:221)
at com.intellij.rt.junit.JUnitStarter.main(JUnitStarter.java:54)
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] gemmellr commented on pull request #3594: ARTEMIS-3315 Fix serializing AMQP messages with invalid application properties
Posted by GitBox <gi...@apache.org>.
gemmellr commented on pull request #3594:
URL: https://github.com/apache/activemq-artemis/pull/3594#issuecomment-847987635
The JIRA/PR/commit title/description is perhaps a bit misleading. These appear to be valid application properties for an AMQP message to have. The broker just fails trying to encode them to another format.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on pull request #3594: ARTEMIS-3315 Fix serializing AMQP messages with invalid application properties
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on pull request #3594:
URL: https://github.com/apache/activemq-artemis/pull/3594#issuecomment-847999472
The changes here are perfect..
however the description is misleading as Robbie (@gemmellr) said..
the issue is that you can't possibly convert these as JSON all the time.. so...
if you changed the commit and JIRA title to something like "Fix JSON Serialization to certain property types in AMQP... it would be perfect to be merged"
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on a change in pull request #3594: ARTEMIS-3315 Fix JSON serialization of AMQP messages
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on a change in pull request #3594:
URL: https://github.com/apache/activemq-artemis/pull/3594#discussion_r640235803
##########
File path: artemis-core-client/src/main/java/org/apache/activemq/artemis/api/core/JsonUtil.java
##########
@@ -173,7 +172,7 @@ public static void addToObject(final String key, final Object param, final JsonO
}
jsonObjectBuilder.add(key, objectArrayBuilder);
} else {
- throw ActiveMQClientMessageBundle.BUNDLE.invalidManagementParam(param.getClass().getName());
Review comment:
don't necessarily copy my version though. what I meant was the param instanceof Number..
you probably should do the same for the Object[] ? just a suggestion though.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org