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

Add tests to verify yaml conversions work #6819

Merged
merged 1 commit into from
Jan 8, 2025
Merged

Conversation

jeffy-mathew
Copy link
Contributor

@jeffy-mathew jeffy-mathew commented Jan 7, 2025

User description

Description

This PR adds a test to verify that no additional yaml struct tags aren't required on OAS structs to support yaml while defining Tyk OAS APIs.

kin-openapi supports parsing yaml openapi specifications making use of json struct tags.
kin-openapi/go.mod at cea0a13b906a708102947f95b9b09d631ff60976 · getkin/kin-openapi it can be seen that kin-openapi is using GitHub - oasdiff/yaml: A better way to marshal and unmarshal YAML in Golang which explains here that it converts yaml ↔︎ json internally. This means we don’t have to have additional struct tags for yaml.

Related Issue

https://tyktech.atlassian.net/browse/TT-13783

Motivation and Context

How This Has Been Tested

Screenshots (if appropriate)

Types of changes

  • 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)

Checklist

  • I ensured that the documentation is up to date
  • I explained why this PR updates go.mod in detail with reasoning why it's required
  • I would like a code coverage CI quality gate exception and have explained why

PR Type

Tests, Enhancement


Description

  • Added a new test TestYaml to validate YAML and JSON conversions.

  • Introduced dependency on github.com/oasdiff/yaml for YAML handling.

  • Updated go.mod and go.sum to include new YAML-related dependencies.

  • Enhanced test coverage for OpenAPI and Tyk extension integration.


Changes walkthrough 📝

Relevant files
Tests
oas_test.go
Added `TestYaml` for YAML and JSON conversion validation 

apidef/oas/oas_test.go

  • Added a new test TestYaml for YAML and JSON conversion.
  • Validated Tyk extension integration with OpenAPI structures.
  • Used yaml.JSONToYAML for JSON to YAML conversion.
  • Ensured equality between original and converted objects.
  • +54/-0   
    Dependencies
    go.mod
    Updated dependencies for YAML handling                                     

    go.mod

  • Added github.com/oasdiff/yaml as a direct dependency.
  • Added github.com/oasdiff/yaml3 as an indirect dependency.
  • +2/-0     
    go.sum
    Updated checksums for new YAML dependencies                           

    go.sum

  • Added checksums for github.com/oasdiff/yaml and
    github.com/oasdiff/yaml3.
  • Updated dependency metadata for YAML-related libraries.
  • +4/-0     

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

    @buger
    Copy link
    Member

    buger commented Jan 7, 2025

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

    Your branch: exp/yaml-conversions

    Your PR title: Add tests to verify yaml conversions work

    Your PR description:

    Description

    Related Issue

    Motivation and Context

    How This Has Been Tested

    Screenshots (if appropriate)

    Types of changes

    • 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)

    Checklist

    • I ensured that the documentation is up to date
    • I explained why this PR updates go.mod in detail with reasoning why it's required
    • I would like a code coverage CI quality gate exception and have explained why

    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 Jan 7, 2025

    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

    Possible Issue

    The test TestYaml includes type assertions (e.g., assert.True(t, ok)) for converting int to float64 in multiple places. This could lead to runtime errors if the type assertion fails. Consider adding more robust type checking or handling unexpected types gracefully.

    for k, v := range tykExt.Server.Authentication.SecuritySchemes {
    	intVal, ok := v.(int)
    	assert.True(t, ok)
    	tykExt.Server.Authentication.SecuritySchemes[k] = float64(intVal)
    }
    
    for k, v := range tykExt.Middleware.Global.PluginConfig.Data.Value {
    	intVal, ok := v.(int)
    	assert.True(t, ok)
    	tykExt.Middleware.Global.PluginConfig.Data.Value[k] = float64(intVal)
    Dependency Validation

    The new dependency github.com/oasdiff/yaml has been introduced. Ensure that this dependency is stable, maintained, and does not introduce any potential compatibility or security issues.

    "github.com/oasdiff/yaml"

    Copy link
    Contributor

    github-actions bot commented Jan 7, 2025

    API Changes

    no api changes detected

    Copy link
    Contributor

    github-actions bot commented Jan 7, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Add a runtime check to ensure map values are of the expected type before type assertion

    Ensure that the tykExt.Server.Authentication.SecuritySchemes map only contains
    integer values before attempting type assertion to avoid runtime panics.

    apidef/oas/oas_test.go [1333-1336]

     for k, v := range tykExt.Server.Authentication.SecuritySchemes {
         intVal, ok := v.(int)
    -    assert.True(t, ok)
    +    if !ok {
    +        t.Fatalf("unexpected value type in SecuritySchemes for key %v: %T", k, v)
    +    }
         tykExt.Server.Authentication.SecuritySchemes[k] = float64(intVal)
     }
    Suggestion importance[1-10]: 9

    Why: The suggestion adds a runtime check to ensure that the map values are of the expected type before performing a type assertion. This prevents potential runtime panics and improves the robustness of the test. The improved code is accurate and directly addresses the issue in the existing code.

    9

    @jeffy-mathew jeffy-mathew requested a review from titpetric January 8, 2025 15:33
    @jeffy-mathew jeffy-mathew enabled auto-merge (squash) January 8, 2025 15:33
    Copy link

    sonarqubecloud bot commented Jan 8, 2025

    Quality Gate Failed Quality Gate failed

    Failed conditions
    1 Security Hotspot

    See analysis details on SonarQube Cloud

    @jeffy-mathew jeffy-mathew merged commit 4304645 into master Jan 8, 2025
    44 of 46 checks passed
    @jeffy-mathew jeffy-mathew deleted the exp/yaml-conversions branch January 8, 2025 20:22
    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