Hello, can I try to fix this issue?

Hi Alex,

of course, as long as the ticket is not assigned and in progress nobody is actively working on it.
So feel free to send a pull request or ask questions if you need help.

Cheers,
Sebastian

Thanks,
Yes, I have some questions regarding this ticket.

1. Could you please clarify the sentence "if the XPATH matches the document"? What does it mean "match the document"? Is this somehow related to "/" XPath expression?
2. if the XPATH matches no element:
   • Affected package: camunda-spin-dataformat-xml-dom
   • Classes/Tests: DomXPathQuery, XmlDomXPathTest and maybe XmlDomXPathScriptTest
   • Should be the next methods in class DomXPathQuery updated, not only element()?
     • element(), elementList(), attribute(), attributeList() - all these methods return "empty element/objects"
   • In case of invalid node the method DomXPathQuery.string() returns an empty string. Is it okay and this method should not be updated?
   • How do you think, is it a good idea to rewrite test classes XmlDomXPathTest.java and XmlDomXPathScriptTest.java in order to utilize XmlTestConstants.java and load validation strings from the xml files located in src/test/resources, as it is done in other tests in org.camunda.spin.xml.dom package?

Hi Alexander,

1. Yes I meant the XPATH / with "match the document". Currently I think the methods element/List and attribute/List throw exception which aren't really useful. And the methods string, number and bool even return something, which isn't correct.
2. You are right regarding the packages, classes and tests. I think all methods which call query.evaluate in DomXPathQuery should behave the same. Therefore the exceptions thrown for the XPATH / and /dont/exists should be the same for all methods. So yes also string, number and bool should throw an exception. Regarding the rewrite of the XmlDomXPathTest, you don't have to do that but if you think it would be better feel free to do it. But it is not required for this ticket.

Thanks for your help.

Cheers,
Sebastian

Hi Sebastian,

Before creating the pull request can I ask you to check if I'm doing the right things?

Following statements generate the same one exception.
The naming of exception, methods, etc. now maybe is not so important, since during the code reviewing you will propose a lot of improvements and changes.

1. Invocations shown bellow generate the same one exception. Is it correct?
   • element.xPath("/").element();
   • element.xPath("/").attribute();
   • element.xPath("/").string();
   • element.xPath("/").number();
   • element.xPath("/").bool();
   • element.xPath("/root/nonExisting").element();
   • element.xPath("/root/child/nonExisting").elementList();
   • element.xPath("/root/child/@nonExisting").attribute();
   • element.xPath("/root/child/a/@nonExisting").attributeList();
   • element.xPath("string(/root/child/@nonExisting)").string();
   • element.xPath("count(/root/child/nonExisting)").number();
   • element.xPath("boolean(/root/nonExisting)").bool();
2. These statements don't produce exceptions. Is it correct?
   • String value = element.xPath("string(/)").string();
     assertThat(value).isEqualTo("");
   • Double count = element.xPath("count(/)").number();
     assertThat(count).isEqualTo(1);
   • Boolean exists = element.xPath("boolean(/)").bool();
     assertThat(exists).isTrue();
3. This XmlDomXPathScriptTest.java -> canQueryBoolean() test case in previous implementation for not existing node returns false.
   Now this test case will generate exception.

      ...
      @ScriptVariable(name = "expression", value = "boolean(/root/not)")
      ... 
      SpinXPathQuery query = script.getVariable("query");
      Boolean exists = query.bool();
      assertThat(exists).isFalse();
   

Hi Alexander,

thanks for working on this issue. How do you distinguish between element.xPath("string(/root/child/@nonExisting)").string(); and element.xPath("string(/)").string(); ?

I would like to see the code. You can actually create a pull request and continue to work on it. Or you could give me a link to your repository.

I'm currently not sure if these options should throw an exception. Sorry I totally forgot about the builtin XPath functions.

• element.xPath("string(/root/child/@nonExisting)").string();
• element.xPath("count(/root/child/nonExisting)").number();
• element.xPath("boolean(/root/nonExisting)").bool();

Can I ask you what you would expect as an user to be the outcome of these calls? Or general about the topic of this ticket?

Cheers,
Sebastian

Hi Sebastian,

Both element.xPath("string(/root/child/@nonExisting)").string(); and element.xPath("string(/)").string(); return an empty string as a result, but only query.evaluate("/root/child/@nonExisting", domElement.unwrap(), XPathConstants.NODE) returns null.

I will create a pull request tomorrow.

About the topic of this ticket. This is only my thoughts
The first part if the XPATH matches the document throw an usefull exception is okay. It doesn't change much the interface of command-spin. Before we had an exception and now we also have the exception but the new one.

With the second part if the XPATH matches no element don't return a empty element but throw an exception I don't know. We are changing the interface of spin. If somebody was expecting the empty element in her/his code, now spin will throw the exception. Their code will be broken. I'm not aware how widely spin is used. Maybe some people will not be happy with changes and they will have to update their code.
Also, this is only my thoughts, maybe additionally to the new implementation with exception we can add special implementation e.g. element(defaults/EMPTY_ELEMENT) function with default argument that defines the return value in case of not existing element.

Br,
Alexander

Review pull request https://github.com/camunda/camunda-spin/pull/4