[NIFI-6498] Set error event listener on both the TransformerFactory a… by dan-s1 · Pull Request #6798 · apache/nifi

dan-s1 · 2022-12-19T21:17:53Z

…nd Transformer to enable logging errors when processing transformation instructions and when running the transformation itself. Also added ability to log errors from messages that are emitted with xsl:message/.

Summary

NIFI-6498

Tracking

Please complete the following tracking steps prior to pull request creation.

Issue Tracking

Apache NiFi Jira issue created

Pull Request Tracking

Pull Request title starts with Apache NiFi Jira issue number, such as NIFI-00000
Pull Request commit message starts with Apache NiFi Jira issue number, as such NIFI-00000

Pull Request Formatting

Pull Request based on current revision of the main branch
Pull Request refers to a feature branch with one commit containing changes

Verification

Please indicate the verification steps performed prior to pull request creation.

Build

Build completed using mvn clean install -P contrib-check
- JDK 8
- JDK 11
- JDK 17

Licensing

New dependencies are compatible with the Apache License 2.0 according to the License Policy
New dependencies are documented in applicable LICENSE and NOTICE files

Documentation

Documentation formatting appears as expected in rendered files

…nd Transformer to enable logging errors when processing transformation instructions and when running the transformation itself. Also added ability to log errors from messages that are emitted with <xsl:message/>.

exceptionfactory

Thanks for putting together this improvement @dan-s1. The ErrorListener implementation looks straightforward as an implementation of the standard JAXP interface.

I'm not sure the Saxon-specific XMLEmitter implementation should be included. The code introduces tighter coupling to the Saxon library Although the xsl:message element is a standard feature, it is of questionable value in the context of a NiFi Processor. I would prefer to scope these changes to the ErrorListener implementation, which would satisfy the Jira issue description.

dan-s1 · 2022-12-20T04:01:00Z

@exceptionfactory Thank you for taking the time and reviewing this PR. I would just like to clarify what you would like me to remove. I understand you would like me to remove method setMessageReceiver which has the Saxon specific code. Would you like me also to remove unit tests testMessageTerminate and testMessageNonTerminate also or should I keep them as they still demonstrate behavior that TransformXml exhibits when xsl:message is used with terminate and without terminate?

exceptionfactory · 2022-12-20T04:34:02Z

Thanks for the reply @dan-s1.

Although the testMessage methods include the xsl:message examples, they do not appear to assert any particular behavior, which makes sense given that it just ends up writing messages. For that reason, it seems best to remove those test methods and related files.

Reviewing the Saxon Configuration Features there is a reference to a message emitter class named MessageWarner that notifies the standard JAXP ErrorListener. Introducing this feature would still place a direct relationship to the Saxon implementation, so it still seems like it would be best to avoid for now, since xsl:message is a defined element used for testing.

dan-s1 · 2022-12-20T16:23:47Z

@exceptionfactory They actually do assert behavior when terminate is used whether the transform succeeds (terminate=no) or fails (terminate=yes). In addition I noticed with setting the ErrorListener on the Transformer it seems to redirect the xsl:message messages to the log when there is an error (terminate is yes ) and not the nifi-bootstrap.log but when there is only a warning (i.e. terminate is no) then the message is written out to the bootstrap.log. I had not noticed that until I removed the custom Saxon code.

exceptionfactory · 2022-12-20T17:05:47Z

Thanks @dan-s1, I was expecting something related to the message output, but on further review, I see the success and failure output scenarios, which seem worth maintaining.

…xsl:message/>.

dan-s1 · 2022-12-20T18:03:00Z

@exceptionfactory So i ended up only removing the method setMessageReceiver. With that, the only issue which remains is when there is a warning from xsl:message for some reason its not redirected to use the ErrorListener configured. I did find the following Saxon issue which details a similar issue but not the exact one where it gives a lot of history behind Saxon's implementation on handling xsl:message. It does seem to indicate though the only path for correctly redirecting messages from xsl:message is to use custom Saxon code with the use of MessageWarner as you had mentioned previously.
BTW I believe the code I had in setMessageReceiver essentially mimicked MessageWarner as MessageWarner extends net.sf.saxon.serialize.XMLEmitter.

exceptionfactory

Thanks for the updates @dan-s1, the approach looks good. I noted several minor adjustments to include the exception in the log arguments, and removing some unnecessary lines. Otherwise this looks close to completion.

exceptionfactory · 2022-12-20T19:22:20Z

...nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/TransformXml.java

    }
+}
+
+class ErrorListenerLogger implements ErrorListener {


This can be private and static:

Suggested change

class ErrorListenerLogger implements ErrorListener {

private static class ErrorListenerLogger implements ErrorListener {

exceptionfactory · 2022-12-20T19:23:22Z

...nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/TransformXml.java

+
+    @Override
+    public void warning(TransformerException exception) throws TransformerException {
+        logger.warn(exception.getMessageAndLocation());


Suggested change

logger.warn(exception.getMessageAndLocation());

logger.warn(exception.getMessageAndLocation(), exception);

exceptionfactory · 2022-12-20T19:23:47Z

...nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/TransformXml.java

+
+    @Override
+    public void error(TransformerException exception) throws TransformerException {
+        logger.error(exception.getMessageAndLocation());


Suggested change

logger.error(exception.getMessageAndLocation());

logger.error(exception.getMessageAndLocation(), exception);

exceptionfactory · 2022-12-20T19:24:40Z

...nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/TransformXml.java

+
+    @Override
+    public void fatalError(TransformerException exception) throws TransformerException {
+        logger.log(LogLevel.FATAL, exception.getMessageAndLocation());


Suggested change

logger.log(LogLevel.FATAL, exception.getMessageAndLocation());

logger.log(LogLevel.FATAL, exception.getMessageAndLocation(), exception);

exceptionfactory · 2022-12-20T19:24:50Z

...nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/TransformXml.java

+    }
+
+    @Override
+    public void fatalError(TransformerException exception) throws TransformerException {


Suggested change

public void fatalError(TransformerException exception) throws TransformerException {

public void fatalError(TransformerException exception) {

Isn't throws TransformerException part of the signature of the method defined in the interface?

Yes, but if it isn't thrown, it doesn't need to be declared on the implementing method.

In this particular case, on further review, fatalError should throw the exception to prevent further processing.

So I just rethrow the exception argument like below ?

throw exception;

Yes, thanks!

exceptionfactory · 2022-12-20T19:24:59Z

...nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/TransformXml.java

+    }
+
+    @Override
+    public void error(TransformerException exception) throws TransformerException {


Suggested change

public void error(TransformerException exception) throws TransformerException {

public void error(TransformerException exception) {

exceptionfactory · 2022-12-20T19:25:11Z

...nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/TransformXml.java

+    }
+
+    @Override
+    public void warning(TransformerException exception) throws TransformerException {


Suggested change

public void warning(TransformerException exception) throws TransformerException {

public void warning(TransformerException exception) {

exceptionfactory · 2022-12-20T19:26:31Z

...-standard-processors/src/test/java/org/apache/nifi/processors/standard/TestTransformXml.java

+        final Map<String, String> attributes = new HashMap<>();
+        runner.enqueue(Paths.get("src/test/resources/TestTransformXml/math.xml"), attributes);


Suggested change

final Map<String, String> attributes = new HashMap<>();

runner.enqueue(Paths.get("src/test/resources/TestTransformXml/math.xml"), attributes);

runner.enqueue(Paths.get("src/test/resources/TestTransformXml/math.xml"));

exceptionfactory · 2022-12-20T19:26:43Z

...-standard-processors/src/test/java/org/apache/nifi/processors/standard/TestTransformXml.java

+        final Map<String, String> attributes = new HashMap<>();
+        runner.enqueue(Paths.get("src/test/resources/TestTransformXml/employee.xml"), attributes);


Suggested change

final Map<String, String> attributes = new HashMap<>();

runner.enqueue(Paths.get("src/test/resources/TestTransformXml/employee.xml"), attributes);

runner.enqueue(Paths.get("src/test/resources/TestTransformXml/employee.xml"));

exceptionfactory · 2022-12-20T19:26:58Z

...-standard-processors/src/test/java/org/apache/nifi/processors/standard/TestTransformXml.java

+        final Map<String, String> attributes = new HashMap<>();
+        runner.enqueue(Paths.get("src/test/resources/TestTransformXml/employee.xml"), attributes);


Suggested change

final Map<String, String> attributes = new HashMap<>();

runner.enqueue(Paths.get("src/test/resources/TestTransformXml/employee.xml"), attributes);

runner.enqueue(Paths.get("src/test/resources/TestTransformXml/employee.xml"));

dan-s1 · 2022-12-21T15:07:52Z

@exceptionfactory I made the changes you requested. I am not sure though what to do as two of the ci builds timed out. Please advise. Thanks!

exceptionfactory

Thanks for making the adjustments @dan-s1, the latest version looks good. I restarted the failed automated builds for good measure, and will plan on merging soon.

dan-s1 · 2022-12-21T16:09:13Z

@exceptionfactory Thanks for the review and restarting those builds. I just learnt something about the TestRunner that we could access logging that occurred during running a processor. Do you think I should add assertions to the unit tests I added that confirms there was logging when ever a compile time or runtime error occurs when transforming XML?

exceptionfactory · 2022-12-21T16:13:24Z

Do you think I should add assertions to the unit tests I added that confirms there was logging when ever a compile time or runtime error occurs when transforming XML?

If you are inclined to add a check for a log at a given level, that would be a helpful addition. It is not necessary to check the content of the message, but check for a warning or error log as appropriate could be useful.

dan-s1 · 2022-12-21T16:14:41Z

@exceptionfactory Ok great! I will see if I can add that.

dan-s1 · 2022-12-21T16:36:17Z

@exceptionfactory Do you mean just asserting for example getErrorMessages on the MockComponentLog is not empty? I thought it also may be helpful to have the contents of the message similar to in the ticket

Error on line 17 column 5 SXXP0003: Error reported by XML parser: The element type "xsl:tomplate" must be terminated by the matching end-tag "</xsl:tomplate>".

in order to demonstrate the issue was fixed.

exceptionfactory · 2022-12-21T16:39:01Z

@exceptionfactory Do you mean just asserting for example getErrorMessages on the MockComponentLog is not empty? I thought it also may be helpful to have the contents of the message similar to in the ticket

Error on line 17 column 5 SXXP0003: Error reported by XML parser: The element type "xsl:tomplate" must be terminated by the matching end-tag "</xsl:tomplate>".

in order to demonstrate the issue was fixed.

Asserting an exact message makes the unit test more brittle when it comes to library changes. In this particular case, asserting that the message contains the word tomplate seems like a good approach.

dan-s1 · 2022-12-21T17:02:05Z

@exceptionfactory I added the assertions. Please let me know if that is enough or if there should be more. Thanks!

exceptionfactory

Thanks for the additional logging assertions, recommend streamlining several of them to focus checking based on the presence of messages, and the existence of the element name.

exceptionfactory · 2022-12-21T17:05:35Z

...-standard-processors/src/test/java/org/apache/nifi/processors/standard/TestTransformXml.java

+        assertTrue(logger.getErrorMessages().size() >= 1);
+        String firstMessage = logger.getErrorMessages().get(0).getMsg();
+        assertTrue(firstMessage.contains("xsl:template"));
+        assertTrue(firstMessage.contains("Line#"));
+        assertTrue(firstMessage.contains("Column#"));


Recommend simplifying the assertions as follows:

Suggested change

assertTrue(logger.getErrorMessages().size() >= 1);

String firstMessage = logger.getErrorMessages().get(0).getMsg();

assertTrue(firstMessage.contains("xsl:template"));

assertTrue(firstMessage.contains("Line#"));

assertTrue(firstMessage.contains("Column#"));

String firstMessage = logger.getErrorMessages().get(0).getMsg();

assertTrue(firstMessage.contains("xsl:template"));

No problem. I thought asserting the existence of the Line# and Column# would be helpful as it addresses the request in the ticket to have them included and that they end up as part of a bulletin in the UI.

exceptionfactory · 2022-12-21T17:05:51Z

...-standard-processors/src/test/java/org/apache/nifi/processors/standard/TestTransformXml.java

+        assertTrue(logger.getErrorMessages().size() >= 1);
+        String firstMessage = logger.getErrorMessages().get(0).getMsg();
+        assertTrue(firstMessage.contains("xsl:message"));
+        assertTrue(firstMessage.contains("Line#"));
+        assertTrue(firstMessage.contains("Column#"));


Suggested change

assertTrue(logger.getErrorMessages().size() >= 1);

String firstMessage = logger.getErrorMessages().get(0).getMsg();

assertTrue(firstMessage.contains("xsl:message"));

assertTrue(firstMessage.contains("Line#"));

assertTrue(firstMessage.contains("Column#"));

String firstMessage = logger.getErrorMessages().get(0).getMsg();

assertTrue(firstMessage.contains("xsl:message"));

…eview.

dan-s1 · 2022-12-21T20:08:38Z

@exceptionfactory I believe I took care of all of your requests.

exceptionfactory

Thanks for working through the feedback @dan-s1, the latest version looks good! +1 merging

This closes apache#6798 Signed-off-by: David Handermann <exceptionfactory@apache.org>

exceptionfactory reviewed Dec 19, 2022

View reviewed changes

[NIFI-6498] Removed custom Saxon code meant to handle messages from <…

be69001

…xsl:message/>.

exceptionfactory requested changes Dec 20, 2022

View reviewed changes

[NIFI-6498] Made changes per code review

4a2cc72

exceptionfactory approved these changes Dec 21, 2022

View reviewed changes

[NIFI-6498] Added logging assertions.

ae992fd

exceptionfactory requested changes Dec 21, 2022

View reviewed changes

dan-s1 added 2 commits December 21, 2022 17:18

[NIFI-6498] Removed some assertions per code review.

22960aa

[NIFI-6498] Removed the rest of the assertions detailed in the code r…

04a5af3

…eview.

exceptionfactory approved these changes Dec 22, 2022

View reviewed changes

exceptionfactory closed this in dde821c Dec 22, 2022

lizhizhou pushed a commit to lizhizhou/nifi that referenced this pull request Jan 2, 2023

NIFI-6498 Update TransformXml to send errors to Component Log

3a464bd

This closes apache#6798 Signed-off-by: David Handermann <exceptionfactory@apache.org>

	class ErrorListenerLogger implements ErrorListener {
	private static class ErrorListenerLogger implements ErrorListener {

	logger.warn(exception.getMessageAndLocation());
	logger.warn(exception.getMessageAndLocation(), exception);

	logger.error(exception.getMessageAndLocation());
	logger.error(exception.getMessageAndLocation(), exception);

	logger.log(LogLevel.FATAL, exception.getMessageAndLocation());
	logger.log(LogLevel.FATAL, exception.getMessageAndLocation(), exception);

	public void fatalError(TransformerException exception) throws TransformerException {
	public void fatalError(TransformerException exception) {

	public void error(TransformerException exception) throws TransformerException {
	public void error(TransformerException exception) {

	public void warning(TransformerException exception) throws TransformerException {
	public void warning(TransformerException exception) {

		final Map<String, String> attributes = new HashMap<>();
		runner.enqueue(Paths.get("src/test/resources/TestTransformXml/math.xml"), attributes);

Conversation

dan-s1 commented Dec 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Tracking

Issue Tracking

Pull Request Tracking

Pull Request Formatting

Verification

Build

Licensing

Documentation

Uh oh!

exceptionfactory left a comment

Choose a reason for hiding this comment

Uh oh!

dan-s1 commented Dec 20, 2022

Uh oh!

exceptionfactory commented Dec 20, 2022

Uh oh!

dan-s1 commented Dec 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

exceptionfactory commented Dec 20, 2022

Uh oh!

dan-s1 commented Dec 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

exceptionfactory left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dan-s1 Dec 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dan-s1 commented Dec 21, 2022

Uh oh!

exceptionfactory left a comment

Choose a reason for hiding this comment

Uh oh!

dan-s1 commented Dec 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

exceptionfactory commented Dec 21, 2022

Uh oh!

dan-s1 commented Dec 21, 2022

Uh oh!

dan-s1 commented Dec 21, 2022

Uh oh!

exceptionfactory commented Dec 21, 2022

Uh oh!

dan-s1 commented Dec 21, 2022

Uh oh!

exceptionfactory left a comment

dan-s1 commented Dec 19, 2022 •

edited

Loading

dan-s1 commented Dec 20, 2022 •

edited

Loading

dan-s1 commented Dec 20, 2022 •

edited

Loading

dan-s1 Dec 20, 2022 •

edited

Loading

dan-s1 commented Dec 21, 2022 •

edited

Loading