Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[TT-13201] Streams Definition Validator #6656

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

buraksezer
Copy link
Contributor

@buraksezer buraksezer commented Oct 22, 2024

User description

TT-13201
Summary Streams definition validator
Type Story Story
Status In Dev
Points N/A
Labels -

PR for TT-13201

We decided to copy and modify the existing OAS validator. It shares the same API and adds extra functionality to validate Bento configurations. The Bento schema only includes what we support in our current Tyk Streams implementation. Additional properties are allowed.

It's also possible to implement new validators for Bento that implements bento.ConfigValidator interface. Currently, we only have a default validator.

This one works with bento.DefaultValidator:

var document = []byte(<OAS Document that includes Bento configuration in the streams section>)
err := streams.ValidateOASObject(document, "3.0.3")

This is how we can inject a Bento validator:

var document = []byte(<OAS Document that includes Bento configuration in the streams section>)
err := ValidateOASObjectWithBentoConfigValidator(document, "3.0.3", bento.AsyncAPIValidator)

Here is a sample for invalid configuration. auto_replay_nacks must be a boolean, not a string.

{
    "streams": {
        "test-kafka-stream": {
            "input": {
                "label": "",
                "kafka": {
                    "addresses": [],
                    "topics": [],
                    "target_version": "2.1.0",
                    "consumer_group": "",
                    "checkpoint_limit": 1024,
                    "auto_replay_nacks": "true"
                }
            }
        }
    }
}

streams.ValidateOASObject(document, "3.0.3") call should return for this document:

test-kafka-stream: input.kafka.auto_replay_nacks: Invalid type. Expected: boolean, given: string

PR Type

Enhancement, Tests


Description

  • Implemented a new Tyk Streams OAS validator with support for Bento configuration validation.
  • Added comprehensive tests for validating Tyk Streams OAS objects, including valid and invalid scenarios.
  • Defined and implemented a default Bento configuration validator and its associated schema.
  • Added test data for various OAS templates to support testing efforts.

Changes walkthrough 📝

Relevant files
Tests
4 files
validator_test.go
Add tests for Tyk Streams OAS validation                                 

apidef/streams/validator_test.go

  • Added tests for validating Tyk Streams OAS objects.
  • Included tests for valid and invalid OAS objects.
  • Added tests for loading OAS schemas and default version handling.
  • +373/-0 
    validator_test.go
    Add tests for Bento configuration validation                         

    apidef/streams/bento/validator_test.go

  • Added tests for Bento configuration validation.
  • Included tests for valid and invalid configurations.
  • Tested additional properties handling in configurations.
  • +88/-0   
    non-empty-x-tyk-ext-oas-template.json
    Add test data for non-empty x-tyk-streaming template         

    apidef/streams/testdata/non-empty-x-tyk-ext-oas-template.json

  • Added test data for non-empty x-tyk-streaming OAS template.
  • Included basic structure for Tyk streaming configurations.
  • +19/-0   
    empty-x-tyk-ext-oas-template.json
    Add test data for empty x-tyk-streams template                     

    apidef/streams/testdata/empty-x-tyk-ext-oas-template.json

  • Added test data for empty x-tyk-streams OAS template.
  • Provided minimal structure for testing purposes.
  • +9/-0     
    Enhancement
    4 files
    validator.go
    Implement Tyk Streams OAS validation logic                             

    apidef/streams/validator.go

  • Implemented validation logic for Tyk Streams OAS objects.
  • Integrated Bento configuration validation.
  • Added functions to load and manage OAS schemas.
  • +273/-0 
    validator.go
    Implement Bento configuration validation                                 

    apidef/streams/bento/validator.go

  • Defined interface for Bento configuration validation.
  • Implemented default Bento configuration validator.
  • Added schema loading for Bento configurations.
  • +108/-0 
    root.go
    Define structure for Tyk streaming configurations               

    apidef/streams/root.go

  • Defined structure for Tyk streaming configurations.
  • Included fields for Info, Server, Streams, and Middleware.
  • +17/-0   
    bento-v1.2.0-supported-schema.json
    Add JSON schema for Bento v1.2.0 configurations                   

    apidef/streams/bento/schema/bento-v1.2.0-supported-schema.json

  • Added JSON schema for Bento v1.2.0.
  • Defined input and output properties for Bento configurations.
  • Included processor and scanner definitions.
  • +2607/-0
    Additional files (token-limit)
    2 files
    x-tyk-streaming.json
    ...                                                                                                           

    apidef/streams/schema/x-tyk-streaming.json

    ...

    +2041/-0
    3.0.json
    ...                                                                                                           

    apidef/streams/schema/3.0.json

    ...

    +1662/-0

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

    @buger
    Copy link
    Member

    buger commented Oct 22, 2024

    This PR is too huge for one to review 💔

    Additions 7197 🙅‍♀️
    Expected ⬇️ 800

    Consider breaking it down into multiple small PRs.

    Check out this guide to learn more about PR best-practices.

    @buger
    Copy link
    Member

    buger commented Oct 22, 2024

    I'm a bot and I 👍 this PR title. 🤖

    @buger
    Copy link
    Member

    buger commented Oct 22, 2024

    This PR is too huge for one to review 💔

    Additions 7197 🙅‍♀️
    Expected ⬇️ 800

    Consider breaking it down into multiple small PRs.

    Check out this guide to learn more about PR best-practices.

    @buger
    Copy link
    Member

    buger commented Oct 22, 2024

    I'm a bot and I 👍 this PR title. 🤖

    @buraksezer buraksezer requested a review from buger October 22, 2024 08:18
    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Error Handling
    The error handling in the loadSchemas function could be improved by providing more specific error messages related to the context of each failure, especially in the nested loops where files are read and JSON schemas are manipulated.

    Concurrency
    Ensure that the use of sync.Once in loadSchemas and other similar functions is correctly managing concurrency, particularly around the initialization and use of shared resources like oasJSONSchemas and bentoValidators.

    Test Coverage
    The tests might be missing negative cases and edge cases for functions like validateJSON and validateBentoConfiguration. It's crucial to ensure that these functions handle unexpected inputs and errors gracefully.

    Copy link
    Contributor

    github-actions bot commented Oct 22, 2024

    API Changes

    --- prev.txt	2024-10-22 08:50:35.435661481 +0000
    +++ current.txt	2024-10-22 08:50:29.231621573 +0000
    @@ -5034,6 +5034,80 @@
     func (x *XTykAPIGateway) Fill(api apidef.APIDefinition)
         Fill fills *XTykAPIGateway from apidef.APIDefinition.
     
    +# Package: ./apidef/streams
    +
    +package streams // import "github.com/TykTechnologies/tyk/apidef/streams"
    +
    +
    +CONSTANTS
    +
    +const (
    +	// ExtensionTykStreaming is the OAS schema key for the Tyk Streams extension.
    +	ExtensionTykStreaming = "x-tyk-streaming"
    +)
    +
    +FUNCTIONS
    +
    +func GetOASSchema(version string) ([]byte, error)
    +    GetOASSchema returns an oas schema for a particular version.
    +
    +func ValidateOASObject(documentBody []byte, oasVersion string) error
    +    ValidateOASObject validates a Tyk Streams document against a particular OAS
    +    version.
    +
    +func ValidateOASObjectWithBentoConfigValidator(documentBody []byte, oasVersion string, bentoValidatorKind bento.ValidatorKind) error
    +    ValidateOASObjectWithBentoConfigValidator validates a Tyk Streams document
    +    against a particular OAS version and takes an optional ConfigValidator
    +
    +func ValidateOASTemplate(documentBody []byte, oasVersion string) error
    +    ValidateOASTemplate checks a Tyk Streams OAS API template for necessary
    +    fields, acknowledging that some standard Tyk OAS API fields are optional in
    +    templates.
    +
    +func ValidateOASTemplateWithBentoValidator(documentBody []byte, oasVersion string, bentoValidatorKind bento.ValidatorKind) error
    +
    +TYPES
    +
    +type XTykStreaming struct {
    +	// Info contains the main metadata for the API definition.
    +	Info oas.Info `bson:"info" json:"info"` // required
    +	// Server contains the configurations related to the server.
    +	Server oas.Server `bson:"server" json:"server"` // required
    +	// Streams contains the configurations related to Tyk Streams
    +	Streams map[string]interface{} `bson:"streams" json:"streams"` // required
    +	// Middleware contains the configurations related to the Tyk middleware.
    +	Middleware *oas.Middleware `bson:"middleware,omitempty" json:"middleware,omitempty"`
    +}
    +    XTykStreaming represents the structure for Tyk streaming configurations.
    +
    +# Package: ./apidef/streams/bento
    +
    +package bento // import "github.com/TykTechnologies/tyk/apidef/streams/bento"
    +
    +
    +CONSTANTS
    +
    +const (
    +	DefaultBentoConfigSchemaName string        = "bento-v1.2.0-supported-schema.json"
    +	DefaultValidator             ValidatorKind = "default-validator"
    +)
    +
    +TYPES
    +
    +type ConfigValidator interface {
    +	Validate(document []byte) error
    +}
    +
    +type DefaultConfigValidator struct {
    +	// Has unexported fields.
    +}
    +
    +func NewDefaultConfigValidator() (*DefaultConfigValidator, error)
    +
    +func (v *DefaultConfigValidator) Validate(document []byte) error
    +
    +type ValidatorKind string
    +
     # Package: ./certs
     
     package certs // import "github.com/TykTechnologies/tyk/certs"

    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Enhancement
    Restrict type fields to predefined values to ensure data integrity

    Add validation for type fields to ensure they only accept predefined string values,
    enhancing schema robustness and preventing unexpected types.

    apidef/streams/bento/schema/bento-v1.2.0-supported-schema.json [21-22]

     "type": {
    -  "type": "string"
    +  "type": "string",
    +  "enum": ["type1", "type2", "type3"]
     }
    Suggestion importance[1-10]: 8

    Why: Adding an enum to restrict type fields to predefined values enhances data integrity and prevents unexpected types, which is a significant improvement for schema robustness.

    8
    Enhance error reporting by wrapping errors with additional context

    Replace the direct return of err with a wrapped error that provides more context
    about the failure point.

    apidef/streams/bento/validator.go [81-82]

     if err != nil {
    -    return err
    +    return fmt.Errorf("failed to load Bento schemas: %w", err)
     }
    Suggestion importance[1-10]: 7

    Why: Wrapping the error with additional context improves error traceability and debugging, making it a valuable enhancement to the code's robustness.

    7
    Possible issue
    Populate or remove the empty properties object under plugin to avoid potential schema validation issues

    Ensure that the properties object under plugin is either populated with relevant
    sub-properties or removed if not needed, as it is currently an empty object which
    might be unintentional.

    apidef/streams/bento/schema/bento-v1.2.0-supported-schema.json [595-597]

     "plugin": {
       "additionalProperties": false,
    -  "properties": {},
    +  "properties": {
    +    "name": {
    +      "type": "string"
    +    },
    +    "version": {
    +      "type": "string"
    +    }
    +  },
       "type": "object"
     }
    Suggestion importance[1-10]: 7

    Why: The suggestion to populate or remove the empty properties object under plugin is valid as it addresses potential schema validation issues. Adding relevant sub-properties enhances the schema's utility and prevents unintended empty objects.

    7
    Best practice
    Define a default timeout value to ensure consistent behavior across different environments

    Consider defining a default value for timeout properties to ensure consistent
    behavior in environments where this value may not be explicitly set.

    apidef/streams/bento/schema/bento-v1.2.0-supported-schema.json [272-273]

     "timeout": {
    -  "type": "string"
    +  "type": "string",
    +  "default": "30s"
     }
    Suggestion importance[1-10]: 6

    Why: Defining a default value for timeout properties is a good practice to ensure consistent behavior, especially in environments where this value may not be explicitly set. However, the specific default value should be carefully considered.

    6

    Copy link

    sonarcloud bot commented Oct 22, 2024

    Quality Gate Failed Quality Gate failed

    Failed conditions
    0.0% Coverage on New Code (required ≥ 80%)
    C Reliability Rating on New Code (required ≥ A)

    See analysis details on SonarCloud

    Catch issues before they fail your Quality Gate with our IDE extension SonarLint

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    2 participants