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

[testing] Validation schema strictness improvements #6898

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

Conversation

titpetric
Copy link
Contributor

@titpetric titpetric commented Feb 20, 2025

PR Type

Enhancement, Tests


Description

  • Introduced strict schema validation for OAS with an optional toggle.

  • Updated WebhookEvent struct to enforce stricter validation.

  • Enhanced test coverage for schema loading and validation.

  • Adjusted JSON schema definitions to improve validation accuracy.


Changes walkthrough 📝

Relevant files
Enhancement
event.go
Enforce stricter validation in `WebhookEvent` struct         

apidef/oas/event.go

  • Added stricter validation for BodyTemplate in WebhookEvent.
  • Improved documentation for CoolDownPeriod field.
  • +5/-1     
    validator.go
    Add strict mode toggle for OAS schema loading                       

    apidef/oas/validator.go

  • Introduced LoadOASSchema with a strict mode toggle.
  • Refactored schema loading logic to support strict validation.
  • +12/-3   
    build.sh
    Update build script for strict schema generation                 

    apidef/oas/schema/build.sh

  • Modified script to include additional properties for event handlers.
  • Enhanced strict schema generation logic.
  • +8/-1     
    x-tyk-api-gateway.json
    Update JSON schema definitions for validation accuracy     

    apidef/oas/schema/x-tyk-api-gateway.json

  • Adjusted cooldownPeriod to reference X-Tyk-ReadableDuration.
  • Fixed type property to directly reference X-Tyk-EventType.
  • +2/-5     
    x-tyk-api-gateway.strict.json
    Adjust strict schema for enhanced validation                         

    apidef/oas/schema/x-tyk-api-gateway.strict.json

  • Enabled additionalProperties for X-Tyk-EventHandlers.
  • Adjusted cooldownPeriod to reference X-Tyk-ReadableDuration.
  • +3/-6     
    Tests
    fixtures_test.go
    Add schema validation and logging improvements in tests   

    apidef/oas/fixtures_test.go

  • Added schema loading validation in TestFixtures.
  • Improved logging for changed keys during migration.
  • +5/-1     
    linter_test.go
    Update linter test to use strict schema                                   

    apidef/oas/linter_test.go

  • Updated schema file reference to use strict schema.
  • Removed unused imports for cleaner code.
  • +1/-2     
    validator_test.go
    Update tests for schema loading with strict mode                 

    apidef/oas/validator_test.go

  • Updated tests to validate schema loading with strict mode disabled.
  • Enhanced test coverage for schema-related functions.
  • +3/-3     

    Need help?
  • Type /help how to ... in the comments thread for any questions about PR-Agent usage.
  • Check out the documentation for more information.
  • @buger
    Copy link
    Member

    buger commented Feb 20, 2025

    A JIRA Issue ID is missing from your branch name, PR title and PR description! 🦄

    Your branch: test/schema-strictness-and-validation

    Your PR title: [testing] Validation schema strictness improvements

    Your PR description: null

    If this is your first time contributing to this repository - welcome!


    Please refer to jira-lint to get started.

    Without the JIRA Issue ID in your branch name you would lose out on automatic updates to JIRA via SCM; some GitHub status checks might fail.

    Valid sample branch names:

    ‣ feature/shiny-new-feature--mojo-10'
    ‣ 'chore/changelogUpdate_mojo-123'
    ‣ 'bugfix/fix-some-strange-bug_GAL-2345'

    Copy link
    Contributor

    github-actions bot commented Feb 20, 2025

    API Changes

    --- prev.txt	2025-02-20 16:23:14.384734736 +0000
    +++ current.txt	2025-02-20 16:23:10.275734710 +0000
    @@ -855,10 +855,13 @@
     type JSVMEventHandlerConf struct {
     	// Disabled indicates whether the event handler is inactive.
     	Disabled bool `bson:"disabled" json:"disabled"`
    +
     	// ID is the optional unique identifier for the event handler.
     	ID string `bson:"id" json:"id"`
    -	// MethodName specifies the JavaScript function name to be executed.
    -	MethodName string `bson:"name" json:"name"`
    +
    +	// Name specifies the JavaScript function name to be executed.
    +	Name string `bson:"name" json:"name"`
    +
     	// Path refers to the file path of the JavaScript source code for the handler.
     	Path string `bson:"path" json:"path"`
     }
    @@ -1518,18 +1521,25 @@
     type WebHookHandlerConf struct {
     	// Disabled enables/disables this webhook.
     	Disabled bool `bson:"disabled" json:"disabled"`
    +
     	// ID optional ID of the webhook, to be used in pro mode.
     	ID string `bson:"id" json:"id"`
    +
     	// Name is the name of webhook.
     	Name string `bson:"name" json:"name"`
    +
     	// The method to use for the webhook.
     	Method string `bson:"method" json:"method"`
    +
     	// The target path on which to send the request.
     	TargetPath string `bson:"target_path" json:"target_path"`
    +
     	// The template to load in order to format the request.
     	TemplatePath string `bson:"template_path" json:"template_path"`
    +
     	// Headers to set when firing the webhook.
     	HeaderList map[string]string `bson:"header_map" json:"header_map"`
    +
     	// The cool-down for the event so it does not trigger again (in seconds).
     	EventTimeout int64 `bson:"event_timeout" json:"event_timeout"`
     }
    @@ -2025,6 +2035,10 @@
         GetValidationOptionsFromConfig retrieves validation options based on the
         configuration settings.
     
    +func LoadOASSchema(_ testing.TB, strict bool) error
    +    LoadOASSchema is a testing hook to increase schema strictness. This allows
    +    reporting unrecognized fields during development.
    +
     func MigrateAndFillOAS(api *apidef.APIDefinition) (APIDef, []APIDef, error)
         MigrateAndFillOAS migrates classic APIs to OAS-compatible forms. Then,
         it fills an OAS with it. To be able to make it a valid OAS, it adds some
    @@ -4723,8 +4737,10 @@
     type WebhookEvent struct {
     	// URL is the target URL for the webhook.
     	URL string `json:"url" bson:"url"`
    +
     	// Method is the HTTP method for the webhook.
     	Method string `json:"method" bson:"method"`
    +
     	// CoolDownPeriod defines cool-down for the event, so it does not trigger again.
     	// It uses shorthand notation.
     	// The value of CoolDownPeriod is a string that specifies the interval in a compact form,
    @@ -4742,8 +4758,10 @@
     	// It's important to format the string correctly, as invalid formats will
     	// be considered as 0s/empty.
     	CoolDownPeriod ReadableDuration `json:"cooldownPeriod" bson:"cooldownPeriod"`
    +
     	// BodyTemplate is the template to be used for request payload.
     	BodyTemplate string `json:"bodyTemplate,omitempty" bson:"bodyTemplate,omitempty"`
    +
     	// Headers are the list of request headers to be used.
     	Headers Headers `json:"headers,omitempty" bson:"headers,omitempty"`
     }

    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

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

    Schema Field Changes

    The BodyTemplate field in WebhookEvent has been updated to remove the omitempty tag. This change may have implications for serialization and deserialization behavior, especially for empty values. Ensure this change aligns with the intended functionality.

    // BodyTemplate is the template to be used for request payload.
    BodyTemplate string `json:"bodyTemplate" bson:"bodyTemplate"`
    Schema Loading Logic

    The LoadOASSchema function introduces a strict mode toggle. Ensure this logic is robust and correctly handles schema loading for both strict and non-strict modes.

    func LoadOASSchema(strict bool) error {
    	return loadOASSchema(strict)
    }
    
    func loadOASSchema(strict bool) error {
    	schemaFile := "schema/%s.json"
    	if strict {
    		schemaFile = "schema/%s.strict.json"
    	}
    
    	load := func() error {
    		xTykAPIGwSchema, err := schemaDir.ReadFile(fmt.Sprintf(schemaFile, ExtensionTykAPIGateway))
    		if err != nil {
    Schema Generation Script

    The build.sh script now modifies the schema to include stricter validation rules. Verify that the script correctly handles edge cases and does not introduce unintended schema changes.

    input="x-tyk-api-gateway.json"
    output="x-tyk-api-gateway.strict.json"
    
    cat $input \
    	| jq -r '(.additionalProperties = false) | (.definitions |= map_values(. + {"additionalProperties": false}))' \
    	| jq -r '.definitions["X-Tyk-EventHandlers"].additionalProperties = true' \
    	> $output

    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Improve error handling for schema loading

    Add error handling for cases where schemaDir.ReadFile fails to ensure the
    application does not proceed with invalid or missing schema files.

    apidef/oas/validator.go [53]

     xTykAPIGwSchema, err := schemaDir.ReadFile(fmt.Sprintf(schemaFile, ExtensionTykAPIGateway))
    +if err != nil {
    +    return fmt.Errorf("failed to read schema file: %w", err)
    +}
    Suggestion importance[1-10]: 9

    __

    Why: The suggestion enhances error handling for the schemaDir.ReadFile function, ensuring that the application does not proceed with invalid or missing schema files. This is a critical improvement for robustness and aligns with the existing error handling patterns in the PR.

    High
    Add error handling to schema script

    Add error handling for the cat and jq commands to ensure the script fails gracefully
    if the input file is missing or the commands fail.

    apidef/oas/schema/build.sh [11-14]

    -cat $input \
    +if ! cat $input \
     	| jq -r '(.additionalProperties = false) | (.definitions |= map_values(. + {"additionalProperties": false}))' \
     	| jq -r '.definitions["X-Tyk-EventHandlers"].additionalProperties = true' \
    -	> $output
    +	> $output; then
    +    echo "Error: Failed to process schema file" >&2
    +    exit 1
    +fi
    Suggestion importance[1-10]: 8

    __

    Why: The suggestion introduces error handling for the cat and jq commands in the script, ensuring that the script fails gracefully if the input file is missing or the commands fail. This is a valuable improvement for script reliability and aligns with best practices.

    Medium

    @@ -278,7 +280,9 @@ func TestFixtures(t *testing.T) {
    if tc.Debug || len(tc.Output) == 0 {
    keys := slices.Sorted(maps.Keys(result))

    t.Log("Changed keys after migration:")
    if len(keys) > 0 {
    t.Log("Changed keys after migration:")
    Copy link
    Contributor

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    is it changed keys after migration or just keys after mgiration?

    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.

    3 participants