[TT-12193] Update error handling on webhook events when the event template has errors by titpetric · Pull Request #6312 · TykTechnologies/tyk

titpetric · 2024-05-27T12:47:16Z

User description

Error log raised from Warning to Error, stops handling the event further
CreateBody adjusted to not return rendered template contents if error occurs

PR Type

Bug fix, Enhancement

Description

Updated CreateBody method to return an empty string and error when template rendering fails, instead of a partially rendered template.
Changed log level from warning to error in HandleEvent method when a template rendering error occurs.
Added early return in HandleEvent to stop further processing if a template error is encountered.

Changes walkthrough 📝

Relevant files

Bug fix

event_handler_webhooks.go `Improve error handling in webhook event processing` gateway/event_handler_webhooks.go Updated `CreateBody` to return an empty string on error instead of a partially rendered template. Changed log level from warning to error in `HandleEvent` when a template rendering error occurs. Added early return in `HandleEvent` to stop processing on template error.	+7/-4

💡 PR-Agent usage:
Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

…on template error

github-actions · 2024-05-27T12:47:58Z

PR Description updated to latest commit (c5ab60e)

github-actions · 2024-05-27T12:47:58Z

API Changes

--- prev.txt	2024-05-27 12:47:58.061924897 +0000
+++ current.txt	2024-05-27 12:47:55.173907251 +0000
@@ -10255,9 +10255,9 @@
 func (w *WebHookHandler) Checksum(reqBody string) (string, error)
 
 func (w *WebHookHandler) CreateBody(em config.EventMessage) (string, error)
-    CreateBody will render the webhook event message template. If an error
-    occurs, a partially rendered template will be returned alongside the error
-    that occured.
+    CreateBody will render the webhook event message template and return it as
+    a string. If an error occurs, an empty string will be returned alongside an
+    error.
 
 func (w *WebHookHandler) HandleEvent(em config.EventMessage)
     HandleEvent will be fired when the event handler instance is found in an

github-actions · 2024-05-27T12:48:15Z

PR Review 🔍

⏱️ Estimated effort to review [1-5]	2, because the changes are localized to specific functions in a single file, and the modifications are straightforward with clear intent.
🧪 Relevant tests	No
⚡ Possible issues	No
🔒 Security concerns	No

Code feedback:

relevant file	gateway/event_handler_webhooks.go
suggestion	Consider adding a log entry before returning an empty string in the `CreateBody` function to ensure that all error occurrences are properly logged for debugging purposes. [important]
relevant line	return "", err

relevant file	gateway/event_handler_webhooks.go
suggestion	It's a good practice to check if `reqBody` is nil or empty before proceeding to build the request in `HandleEvent`, to avoid unnecessary processing or potential errors in downstream methods. [important]
relevant line	return

github-actions · 2024-05-27T12:48:53Z

PR Code Suggestions ✨

Category	Suggestion	Score
Enhancement	Enhance error logs with more context for better troubleshooting Add more specific error details to the logging context to aid in debugging. Include relevant fields such as the event type or other identifiers from the `EventMessage`. gateway/event_handler_webhooks.go [220-222] log.WithError(err).WithFields(logrus.Fields{ "prefix": "webhooks", + "eventType": em.Type, + "eventID": em.ID }).Error("Webhook template rendering error") Suggestion importance[1-10]: 9 Why: Adding more specific error details to the logging context can significantly aid in debugging and troubleshooting. This suggestion is highly beneficial for maintaining and understanding the system.	9
Enhancement	Return the partially rendered template on error for better error insight Instead of returning an empty string and an error when the template execution fails, consider returning the partially rendered template. This can be useful for debugging and understanding what part of the template caused the error. gateway/event_handler_webhooks.go [206-207] if err != nil { - return "", err + return reqBody.String(), err } Suggestion importance[1-10]: 8 Why: Returning the partially rendered template can provide valuable debugging information, which can help in identifying the cause of the error. This suggestion is contextually accurate and improves error handling.	8
Possible issue	Consider using a less severe log level for non-critical errors Changing the log level from 'Warn' to 'Error' might be too severe if the system is designed to handle these errors gracefully. Consider keeping the log level as 'Warn' unless the error is critical. gateway/event_handler_webhooks.go [222] -}).Error("Webhook template rendering error") +}).Warn("Webhook template rendering error") Suggestion importance[1-10]: 7 Why: The suggestion to use 'Warn' instead of 'Error' is valid if the system is designed to handle these errors gracefully. However, without more context on the severity of the error, it's not possible to definitively say which log level is more appropriate.	7
Best practice	Handle template execution failures more gracefully with fallback mechanisms Consider handling the error scenario where the template execution fails by logging the error and continuing with a default or fallback mechanism, rather than just returning. gateway/event_handler_webhooks.go [206-207] if err != nil { - return "", err + log.WithError(err).Error("Failed to execute template") + // Consider a fallback mechanism or default value + return defaultBody, nil } Suggestion importance[1-10]: 6 Why: While handling errors with a fallback mechanism can improve robustness, the suggestion is somewhat speculative without knowing the specific requirements or context of the application. It is a good practice but may not be necessary in all cases.	6

github-actions · 2024-05-27T12:54:54Z

💥 CI tests failed 🙈

git-state

all ok

Please look at the run or in the Checks tab.

sonarqubecloud · 2024-05-27T13:47:54Z

Quality Gate failed

Failed conditions
53.3% Coverage on New Code (required ≥ 80%)

See analysis details on SonarCloud

titpetric · 2024-05-27T14:19:36Z

/release to release-5.3

tykbot · 2024-05-27T14:19:56Z

Working on it! Note that it can take a few minutes.

…plate has errors (#6312) ### **User description** - Error log raised from Warning to Error, stops handling the event further - CreateBody adjusted to not return rendered template contents if error occurs ___ ### **PR Type** Bug fix, Enhancement ___ ### **Description** - Updated `CreateBody` method to return an empty string and error when template rendering fails, instead of a partially rendered template. - Changed log level from warning to error in `HandleEvent` method when a template rendering error occurs. - Added early return in `HandleEvent` to stop further processing if a template error is encountered. ___ ### **Changes walkthrough** 📝 <table><thead><tr><th></th><th align="left">Relevant files</th></tr></thead><tbody><tr><td><strong>Bug fix </strong></td><td><table> <tr> <td> <details> <summary><strong>event_handler_webhooks.go</strong><dd><code>Improve error handling in webhook event processing</code>              </dd></summary> <hr> gateway/event_handler_webhooks.go <li>Updated <code>CreateBody</code> to return an empty string on error instead of a <br>partially rendered template.<br> <li> Changed log level from warning to error in <code>HandleEvent</code> when a template <br>rendering error occurs.<br> <li> Added early return in <code>HandleEvent</code> to stop processing on template <br>error.<br> </details> </td> <td><a href="https://github.com/TykTechnologies/tyk/pull/6312/files#diff-6587ad3f2629cfa6c84a71144127acd6cc7824e5141f0b4961848945a87e0198">+7/-4</a>      </td> </tr> </table></td></tr></tr></tbody></table> ___ > 💡 **PR-Agent usage**: >Comment `/help` on the PR to get a list of all available PR-Agent tools and their descriptions Co-authored-by: Tit Petric <[email protected]> (cherry picked from commit f7cda28)

tykbot · 2024-05-27T14:20:13Z

@titpetric Succesfully merged PR

…vents when the event template has errors (#6312) [TT-12193] Update error handling on webhook events when the event template has errors (#6312) ### **User description** - Error log raised from Warning to Error, stops handling the event further - CreateBody adjusted to not return rendered template contents if error occurs ___ ### **PR Type** Bug fix, Enhancement ___ ### **Description** - Updated `CreateBody` method to return an empty string and error when template rendering fails, instead of a partially rendered template. - Changed log level from warning to error in `HandleEvent` method when a template rendering error occurs. - Added early return in `HandleEvent` to stop further processing if a template error is encountered. ___ ### **Changes walkthrough** 📝 <table><thead><tr><th></th><th align="left">Relevant files</th></tr></thead><tbody><tr><td><strong>Bug fix </strong></td><td><table> <tr> <td> <details> <summary><strong>event_handler_webhooks.go</strong><dd><code>Improve error handling in webhook event processing</code>              </dd></summary> <hr> gateway/event_handler_webhooks.go <li>Updated <code>CreateBody</code> to return an empty string on error instead of a <br>partially rendered template.<br> <li> Changed log level from warning to error in <code>HandleEvent</code> when a template <br>rendering error occurs.<br> <li> Added early return in <code>HandleEvent</code> to stop processing on template <br>error.<br> </details> </td> <td><a href="https://github.com/TykTechnologies/tyk/pull/6312/files#diff-6587ad3f2629cfa6c84a71144127acd6cc7824e5141f0b4961848945a87e0198">+7/-4</a>      </td> </tr> </table></td></tr></tr></tbody></table> ___ > 💡 **PR-Agent usage**: >Comment `/help` on the PR to get a list of all available PR-Agent tools and their descriptions Co-authored-by: Tit Petric <[email protected]>

…plate has errors (#6312) ### **User description** - Error log raised from Warning to Error, stops handling the event further - CreateBody adjusted to not return rendered template contents if error occurs ___ ### **PR Type** Bug fix, Enhancement ___ ### **Description** - Updated `CreateBody` method to return an empty string and error when template rendering fails, instead of a partially rendered template. - Changed log level from warning to error in `HandleEvent` method when a template rendering error occurs. - Added early return in `HandleEvent` to stop further processing if a template error is encountered. ___ ### **Changes walkthrough** 📝 <table><thead><tr><th></th><th align="left">Relevant files</th></tr></thead><tbody><tr><td><strong>Bug fix </strong></td><td><table> <tr> <td> <details> <summary><strong>event_handler_webhooks.go</strong><dd><code>Improve error handling in webhook event processing</code>              </dd></summary> <hr> gateway/event_handler_webhooks.go <li>Updated <code>CreateBody</code> to return an empty string on error instead of a <br>partially rendered template.<br> <li> Changed log level from warning to error in <code>HandleEvent</code> when a template <br>rendering error occurs.<br> <li> Added early return in <code>HandleEvent</code> to stop processing on template <br>error.<br> </details> </td> <td><a href="https://github.com/TykTechnologies/tyk/pull/6312/files#diff-6587ad3f2629cfa6c84a71144127acd6cc7824e5141f0b4961848945a87e0198">+7/-4</a>      </td> </tr> </table></td></tr></tr></tbody></table> ___ > 💡 **PR-Agent usage**: >Comment `/help` on the PR to get a list of all available PR-Agent tools and their descriptions Co-authored-by: Tit Petric <[email protected]>

Throw away a partially rendered template, don't handle webhook event …

c5ab60e

…on template error

titpetric changed the title ~~Update error handling on webhook events when the event template has errors~~ [TT-12192] Update error handling on webhook events when the event template has errors May 27, 2024

titpetric changed the title ~~[TT-12192] Update error handling on webhook events when the event template has errors~~ [TT-12193] Update error handling on webhook events when the event template has errors May 27, 2024

github-actions Bot added enhancement Bug fix labels May 27, 2024

github-actions Bot added the Review effort [1-5]: 2 label May 27, 2024

titpetric requested a review from lghiur May 27, 2024 12:48

titpetric requested a review from andrei-tyk May 27, 2024 12:49

lghiur approved these changes May 27, 2024

View reviewed changes

titpetric enabled auto-merge (squash) May 27, 2024 13:36

titpetric merged commit f7cda28 into master May 27, 2024

titpetric deleted the fix/tt-12193/fix-create-body-error-handling-2 branch May 27, 2024 13:47

buger mentioned this pull request May 27, 2024

Merging to release-5.3: [TT-12193] Update error handling on webhook events when the event template has errors (#6312) #6313

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[TT-12193] Update error handling on webhook events when the event template has errors#6312

[TT-12193] Update error handling on webhook events when the event template has errors#6312
titpetric merged 1 commit into
masterfrom
fix/tt-12193/fix-create-body-error-handling-2

titpetric commented May 27, 2024 •

edited by github-actions Bot

Loading

Uh oh!

github-actions Bot commented May 27, 2024

Uh oh!

github-actions Bot commented May 27, 2024

Uh oh!

github-actions Bot commented May 27, 2024

Uh oh!

github-actions Bot commented May 27, 2024

Uh oh!

github-actions Bot commented May 27, 2024

Uh oh!

sonarqubecloud Bot commented May 27, 2024

Uh oh!

titpetric commented May 27, 2024

Uh oh!

tykbot Bot commented May 27, 2024

Uh oh!

tykbot Bot commented May 27, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

titpetric commented May 27, 2024 • edited by github-actions Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

User description

PR Type

Description

Changes walkthrough 📝

Uh oh!

github-actions Bot commented May 27, 2024

Uh oh!

github-actions Bot commented May 27, 2024

Uh oh!

github-actions Bot commented May 27, 2024

PR Review 🔍

Uh oh!

github-actions Bot commented May 27, 2024

PR Code Suggestions ✨

Uh oh!

github-actions Bot commented May 27, 2024

💥 CI tests failed 🙈

git-state

Uh oh!

sonarqubecloud Bot commented May 27, 2024

Quality Gate failed

Uh oh!

titpetric commented May 27, 2024

Uh oh!

tykbot Bot commented May 27, 2024

Uh oh!

tykbot Bot commented May 27, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

titpetric commented May 27, 2024 •

edited by github-actions Bot

Loading