[GitHub] activemq-artemis pull request #1688: ARTEMIS-1537 broker was less strict whi...

[asfgit]

Github user asfgit closed the pull request at:

https://github.com/apache/activemq-artemis/pull/1688


---

[GitHub] activemq-artemis pull request #1688: ARTEMIS-1537 broker was less strict whi...

[stanlyDoge]

Github user stanlyDoge commented on a diff in the pull request:

https://github.com/apache/activemq-artemis/pull/1688#discussion_r156064778
  
--- Diff: 
artemis-server/src/main/java/org/apache/activemq/artemis/core/deployers/impl/FileConfigurationParser.java
 ---
@@ -253,9 +253,12 @@ public Configuration parseMainConfig(final InputStream 
input) throws Exception {
   String xml = XMLUtil.readerToString(reader);
   xml = XMLUtil.replaceSystemProps(xml);
   Element e = XMLUtil.stringToElement(xml);
-
+  NodeList children = e.getElementsByTagName("core");
--- End diff --

Ah, didn't know about this, sorry. Should be ok now.


---

[GitHub] activemq-artemis pull request #1688: ARTEMIS-1537 broker was less strict whi...

[michaelandrepearce]

Github user michaelandrepearce commented on a diff in the pull request:

https://github.com/apache/activemq-artemis/pull/1688#discussion_r156030409
  
--- Diff: 
artemis-server/src/main/java/org/apache/activemq/artemis/core/deployers/impl/FileConfigurationParser.java
 ---
@@ -253,9 +253,12 @@ public Configuration parseMainConfig(final InputStream 
input) throws Exception {
   String xml = XMLUtil.readerToString(reader);
   xml = XMLUtil.replaceSystemProps(xml);
   Element e = XMLUtil.stringToElement(xml);
-
+  NodeList children = e.getElementsByTagName("core");
--- End diff --


https://github.com/apache/activemq-artemis/blob/master/artemis-server/src/main/resources/schema/artemis-server.xsd
 it does.


---

[GitHub] activemq-artemis pull request #1688: ARTEMIS-1537 broker was less strict whi...

[stanlyDoge]

Github user stanlyDoge commented on a diff in the pull request:

https://github.com/apache/activemq-artemis/pull/1688#discussion_r156024601
  
--- Diff: 
artemis-server/src/main/java/org/apache/activemq/artemis/core/deployers/impl/FileConfigurationParser.java
 ---
@@ -253,9 +253,12 @@ public Configuration parseMainConfig(final InputStream 
input) throws Exception {
   String xml = XMLUtil.readerToString(reader);
   xml = XMLUtil.replaceSystemProps(xml);
   Element e = XMLUtil.stringToElement(xml);
-
+  NodeList children = e.getElementsByTagName("core");
--- End diff --

Hmm, after another investigation I am not sure if this is possible with 
current XSD schema. It does not contain 'configuration' element, so it cannot 
be checked this way. There are two solutions - modify XSD schema to contain 
missing element or use my solution, where I am getting first child of 
'configuration', which is 'core'.


---

[GitHub] activemq-artemis pull request #1688: ARTEMIS-1537 broker was less strict whi...

[stanlyDoge]

Github user stanlyDoge commented on a diff in the pull request:

https://github.com/apache/activemq-artemis/pull/1688#discussion_r155719121
  
--- Diff: 
artemis-server/src/main/java/org/apache/activemq/artemis/core/deployers/impl/FileConfigurationParser.java
 ---
@@ -253,9 +253,12 @@ public Configuration parseMainConfig(final InputStream 
input) throws Exception {
   String xml = XMLUtil.readerToString(reader);
   xml = XMLUtil.replaceSystemProps(xml);
   Element e = XMLUtil.stringToElement(xml);
-
+  NodeList children = e.getElementsByTagName("core");
--- End diff --

Yes, that seems better than my solution. I wanted to use the same process 
while reloading conf. as parsing conf. when starting broker 
(https://github.com/apache/activemq-artemis/blob/master/artemis-server/src/main/java/org/apache/activemq/artemis/core/config/FileDeploymentManager.java#L85).
 


---

[GitHub] activemq-artemis pull request #1688: ARTEMIS-1537 broker was less strict whi...

[michaelandrepearce]

Github user michaelandrepearce commented on a diff in the pull request:

https://github.com/apache/activemq-artemis/pull/1688#discussion_r155675210
  
--- Diff: 
artemis-server/src/main/java/org/apache/activemq/artemis/core/deployers/impl/FileConfigurationParser.java
 ---
@@ -253,9 +253,12 @@ public Configuration parseMainConfig(final InputStream 
input) throws Exception {
   String xml = XMLUtil.readerToString(reader);
   xml = XMLUtil.replaceSystemProps(xml);
   Element e = XMLUtil.stringToElement(xml);
-
+  NodeList children = e.getElementsByTagName("core");
--- End diff --

e.g.
  SchemaFactory schemaFactory = 
SchemaFactory.newInstance(XMLConstants.W3C_XML_SCHEMA_NS_URI);
  Schema schema = schemaFactory.newSchema(new 
File("schema/artemis-configuration.xsd"));
  Validator validator = schema.newValidator();
  validator.validate(new DOMSource(e));


---

[GitHub] activemq-artemis pull request #1688: ARTEMIS-1537 broker was less strict whi...

[michaelandrepearce]

Github user michaelandrepearce commented on a diff in the pull request:

https://github.com/apache/activemq-artemis/pull/1688#discussion_r155674146
  
--- Diff: 
artemis-server/src/main/java/org/apache/activemq/artemis/core/deployers/impl/FileConfigurationParser.java
 ---
@@ -253,9 +253,12 @@ public Configuration parseMainConfig(final InputStream 
input) throws Exception {
   String xml = XMLUtil.readerToString(reader);
   xml = XMLUtil.replaceSystemProps(xml);
   Element e = XMLUtil.stringToElement(xml);
-
+  NodeList children = e.getElementsByTagName("core");
--- End diff --

Why not just get the broker.xml fully validated by the xsd using 
javax.xml.validation.Validator instead of manually getting elements and 
recursing.


---

[GitHub] activemq-artemis pull request #1688: ARTEMIS-1537 broker was less strict whi...

[jdanekrh]

Github user jdanekrh commented on a diff in the pull request:

https://github.com/apache/activemq-artemis/pull/1688#discussion_r155199713
  
--- Diff: 
tests/unit-tests/src/test/java/org/apache/activemq/artemis/tests/unit/core/config/impl/ConfigurationValidationTest.java
 ---
@@ -57,4 +57,23 @@ public void testFullConfiguration() throws Exception {
 
   Assert.assertEquals(true, fc.isPersistDeliveryCountBeforeDelivery());
}
+
+   @Test
+   public void testChangeConfiguration() throws Exception {
+  FileConfiguration fc = new FileConfiguration();
+  FileDeploymentManager deploymentManager = new 
FileDeploymentManager("ConfigurationTest-full-config.xml");
+  deploymentManager.addDeployable(fc);
+  deploymentManager.readConfiguration();
+
+  boolean success = false; // test should fail because config contains 
wrong element
+
+  deploymentManager = new 
FileDeploymentManager("ConfigurationTest-full-config-wrong-address.xml");
+  deploymentManager.addDeployable(fc);
+  try {
+ deploymentManager.readConfiguration();
+  } catch (Exception e) {
+ success = true;
+  }
+  Assert.assertTrue(success);
--- End diff --

Tests like that seem to be usually written a bit differently, like this 
https://github.com/apache/activemq-artemis/blob/f698a7f8189af7b70160ba18596be371642776bb/tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/jms/jms2client/BodyTest.java#L66

It can be worth it to name the exception variable `ignored`, not `e`, 
because that is a hint to unused variable inspection in IntelliJ. 
https://www.reddit.com/r/ProgrammerHumor/comments/2so5tu/mildly_amusing_intellij_suggests_to_rename_a/
 Not sure what it would suggest if you have multiple ignored exceptions in 
scope... It is less verbose than 
https://docs.oracle.com/javase/7/docs/api/java/lang/SuppressWarnings.html 
("unused")


---

[GitHub] activemq-artemis pull request #1688: ARTEMIS-1537 broker was less strict whi...

[stanlyDoge]

GitHub user stanlyDoge opened a pull request:

https://github.com/apache/activemq-artemis/pull/1688

ARTEMIS-1537 broker was less strict while reloading configuration



You can merge this pull request into a Git repository by running:

$ git pull https://github.com/stanlyDoge/activemq-artemis-1 E689

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/activemq-artemis/pull/1688.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #1688


commit 653b995e3dcda0b026dce4dcf4155d317950faf1
Author: Stanislav Knot 
Date:   2017-12-05T16:01:20Z

ARTEMIS-1537 broker was less strict while reloading configuration




---