improve error handling of streams in non-ee version (TT-13269)

[pvormste]

User description

TT-13269

Summary	[GW EE] Implement conditional compilation for streams in GW EE
Type	Story
Status	In Dev
Points	N/A
Labels	-

---

This PR improves the error handling of streams API in Tyk Non-EE version.

Types of changes

• [x] Bug fix (non-breaking change which fixes an issue)
• [ ] New feature (non-breaking change which adds functionality)
• [ ] Breaking change (fix or feature that would cause existing functionality to change)
• [ ] Refactoring or add test (improvements in base code or adds test coverage to functionality)

---

PR Type

Bug fix, Enhancement, Tests

---

Description

• Introduced a new error variable ErrActionNotAllowed for handling restricted actions.
• Added a method isStreamingAPI to determine if an API spec supports streaming.
• Enhanced the dummyStreamingMiddleware to log errors and return appropriate HTTP status codes when streaming is not supported.
• Added tests for the isStreamingAPI method to ensure correct functionality.

---

Changes walkthrough 📝

	Relevant files
Enhancement	errors.go Introduce new error variable for action restrictions          ee/errors.go - Added a new error variable `ErrActionNotAllowed`. +9/-0      api_definition.go Add method to check streaming API support                                gateway/api_definition.go Added a new method isStreamingAPI to check for streaming API extensions. Imported streams package for streaming API support. +11/-1
Tests	api_definition_test.go Add tests for streaming API detection method                          gateway/api_definition_test.go Added tests for the new isStreamingAPI method. Included streams package in imports for testing. +51/-0
Bug fix	mw_streaming.go Improve error handling and logging for streaming middleware gateway/mw_streaming.go Enhanced error handling in dummyStreamingMiddleware. Added logging for unsupported streaming actions. Introduced constant message for unsupported streaming. +12/-3

---

💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

[buger]

Let's make that PR title a 💯 shall we? 💪

<p>
Your <em>PR title</em> and <em>story title</em> look <strong>slightly different</strong>. Just checking in to know if it was intentional!
</p>
<table>
  <tr>
    <th>Story Title</th>
    <td>[GW EE] Implement conditional compilation for streams in GW EE</td>
  </tr>
  <tr>
      <th>PR Title</th>
      <td>improve error handling of streams in non-ee version (TT-13269)</td>
    </tr>
</table>
<p>
  Check out this <a href="https://www.atlassian.com/blog/git/written-unwritten-guide-pull-requests">guide</a> to learn more about PR best-practices.
</p>

[github-actions[bot]]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Error Handling The error message and HTTP status code for unsupported streaming in non-EE versions are hardcoded in the dummyStreamingMiddleware. Consider making these configurable or using constants to improve maintainability. Dependency Injection The method isStreamingAPI uses a direct dependency on the streams package, which might complicate testing and future changes. Consider abstracting this dependency.

[github-actions[bot]]

API Changes

--- prev.txt    2024-11-05 15:35:04.859403357 +0000
+++ current.txt 2024-11-05 15:34:58.099400506 +0000
@@ -8149,6 +8149,16 @@

 func (ms *MockStorage) Set(key string, addrs []string)

+# Package: ./ee
+
+package ee // import "github.com/TykTechnologies/tyk/ee"
+
+
+VARIABLES
+
+var (
+   ErrActionNotAllowed = errors.New("action not allowed")
+)
 # Package: ./ee/middleware/streams

 package streams // import "github.com/TykTechnologies/tyk/ee/middleware/streams"
@@ -8697,6 +8707,9 @@

 const ListDetailed = "detailed"
 const LoopScheme = "tyk"
+const (
+   MessageStreamingOnlySupportedInEE = "streaming is supported only in Tyk Enterprise Edition"
+)
 const OIDPREFIX = "openid"
 const RPCKeyPrefix = "rpc:"
 const RestrictedFieldValidationFailedLogMsg = "Error during GraphQL request restricted fields validation: '%s'"

[github-actions[bot]]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Score
Enhancement	Enhance test coverage by adding a scenario where the streaming key is missing in the extensions ___ **Add a test case in TestAPISpec_isStreamingAPI to handle the scenario where Extensions is not nil but does not contain the streams.ExtensionTykStreaming key.** [gateway/api_definition_test.go [1554-1563]](https://github.com/TykTechnologies/tyk/pull/6691/files#diff-2394daab6fdc5f8dc234699c80c0548947ee3d68d2e33858258d73a8b5eb6f44R1554-R1563) ```diff { - name: "should return false if streaming section is missing", + name: "should return false if streaming key is missing", inputOAS: oas.OAS{ T: openapi3.T{ Extensions: map[string]any{ "x-tyk-api-gateway": nil, + "some-other-extension": nil, }, }, }, expectedResult: false, } ``` Suggestion importance[1-10]: 8 Why: This suggestion effectively increases the test coverage by considering a realistic scenario where the streaming key is missing, which is crucial for ensuring the robustness of the streaming API feature detection.	8
	Add logging for cases where streaming is enabled but the API is not a streaming API ___ **In the EnabledForSpec method, add a check to log an error message if streamingConfig.Enabled is true but the API is not a streaming API.** [gateway/mw_streaming.go [33-36]](https://github.com/TykTechnologies/tyk/pull/6691/files#diff-6f565750150d990575c808f1ca8f38483160dc6edf05f1534cd0bedb27c2e6c8R33-R36) ```diff -if streamingConfig.Enabled && d.Spec.isStreamingAPI() { - d.Logger().Warnf("Warning: %s", MessageStreamingOnlySupportedInEE) - return true +if streamingConfig.Enabled { + if d.Spec.isStreamingAPI() { + d.Logger().Warnf("Warning: %s", MessageStreamingOnlySupportedInEE) + return true + } else { + d.Logger().Errorf("Error: Streaming configuration enabled but API is not a streaming API") + } } ``` Suggestion importance[1-10]: 7 Why: This suggestion improves error handling by adding a specific log message when streaming is enabled but the API is not a streaming API, which enhances the debuggability and correctness of the middleware behavior.	7
Maintainability	Refactor logging into a separate method to clean up the EnabledForSpec method ___ **Refactor the EnabledForSpec method to separate concerns by extracting the logging functionality into a separate method.** [gateway/mw_streaming.go [33-36]](https://github.com/TykTechnologies/tyk/pull/6691/files#diff-6f565750150d990575c808f1ca8f38483160dc6edf05f1534cd0bedb27c2e6c8R33-R36) ```diff -if streamingConfig.Enabled && d.Spec.isStreamingAPI() { - d.Logger().Warnf("Warning: %s", MessageStreamingOnlySupportedInEE) - return true +if streamingConfig.Enabled { + logStreamingAttempt() + if d.Spec.isStreamingAPI() { + return true + } } ``` Suggestion importance[1-10]: 5 Why: Refactoring the logging into a separate method could improve the maintainability of the code by separating concerns, although it's a moderate improvement as it primarily affects readability and not the functionality directly.	5

[sonarcloud[bot]]

Quality Gate failed

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

See analysis details on SonarCloud