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-11913] Implement OAS contract for analytics plugin #6861

Merged
merged 6 commits into from
Feb 6, 2025

Conversation

kofoworola
Copy link
Contributor

@kofoworola kofoworola commented Feb 5, 2025

User description

TT-11913
Summary [OAS] Custom Analytics plugins
Type Story Story
Status In Dev
Points N/A
Labels QA_Fail

Description

TT-11913

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

PR Type

Enhancement, Tests


Description

  • Replaced TrafficLogs plugins with CustomAnalyticsPlugins.

  • Added CustomAnalyticsPlugins type with Fill and ExtractTo methods.

  • Updated test cases to validate CustomAnalyticsPlugins functionality.

  • Removed unused TrafficLogs middleware section from MiddlewareSection.


Changes walkthrough 📝

Relevant files
Enhancement
api_definitions.go
Removed `TrafficLogs` middleware section.                               

apidef/api_definitions.go

  • Removed TrafficLogs middleware section from MiddlewareSection.
+0/-1     
middleware.go
Introduced `CustomAnalyticsPlugins` for analytics.             

apidef/oas/middleware.go

  • Replaced Plugins in TrafficLogs with CustomAnalyticsPlugins.
  • Added CustomAnalyticsPlugins type with Fill and ExtractTo methods.
  • Updated logic for handling TrafficLogs plugins.
  • +40/-10 
    Tests
    middleware_test.go
    Updated tests for `CustomAnalyticsPlugins`.                           

    apidef/oas/middleware_test.go

  • Updated test cases to use CustomAnalyticsPlugins.
  • Added assertions for CustomAnalyticsPlugins properties.
  • +7/-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 5, 2025

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

    @kofoworola kofoworola requested review from titpetric and a team and removed request for buraksezer February 5, 2025 06:02
    Copy link
    Contributor

    github-actions bot commented Feb 5, 2025

    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

    Possible Issue

    The Fill and ExtractTo methods for CustomAnalyticsPlugins rely on the first plugin in the list. This could lead to unexpected behavior if multiple plugins are configured. Ensure this logic aligns with the intended functionality.

    func (c *CustomAnalyticsPlugins) Fill(api apidef.APIDefinition) {
    	if api.AnalyticsPlugin.Enabled {
    		customPlugins := []CustomPlugin{
    			{
    				Enabled:      api.AnalyticsPlugin.Enabled,
    				FunctionName: api.AnalyticsPlugin.FuncName,
    				Path:         api.AnalyticsPlugin.PluginPath,
    			},
    		}
    		*c = customPlugins
    	}
    }
    
    // ExtractTo extracts CustomAnalyticsPlugins into AnalyticsPlugin of supplied api.
    func (c *CustomAnalyticsPlugins) ExtractTo(api *apidef.APIDefinition) {
    	if len(*c) > 0 {
    		// extract the first item in the customAnalyticsPlugin into apidef
    		plugin := (*c)[0]
    		api.AnalyticsPlugin.Enabled = plugin.Enabled
    		api.AnalyticsPlugin.FuncName = plugin.FunctionName
    		api.AnalyticsPlugin.PluginPath = plugin.Path
    	}
    Code Duplication

    The logic for initializing and handling CustomAnalyticsPlugins appears duplicated across multiple methods (Fill, ExtractTo, etc.). Consider refactoring to reduce redundancy and improve maintainability.

    func (t *TrafficLogs) Fill(api apidef.APIDefinition) {
    	t.Enabled = !api.DoNotTrack
    	t.TagHeaders = api.TagHeaders
    	t.CustomRetentionPeriod = ReadableDuration(time.Duration(api.ExpireAnalyticsAfter) * time.Second)
    
    	if t.Plugins == nil {
    		t.Plugins = make(CustomAnalyticsPlugins, 0)
    	}
    	t.Plugins.Fill(api)
    	if ShouldOmit(t.Plugins) {
    		t.Plugins = nil
    	}
    }
    
    // ExtractTo extracts *TrafficLogs into *apidef.APIDefinition.
    func (t *TrafficLogs) ExtractTo(api *apidef.APIDefinition) {
    	api.DoNotTrack = !t.Enabled
    	api.TagHeaders = t.TagHeaders
    	api.ExpireAnalyticsAfter = int64(t.CustomRetentionPeriod.Seconds())
    
    	if t.Plugins == nil {
    		t.Plugins = make(CustomAnalyticsPlugins, 0)
    		defer func() {
    			t.Plugins = nil
    		}()
    	}
    	t.Plugins.ExtractTo(api)
    }
    
    // CustomAnalyticsPlugins is a list of CustomPlugin objects for analytics.
    type CustomAnalyticsPlugins []CustomPlugin
    
    // Fill fills CustomAnalyticsPlugins from AnalyticsPlugin in the supplied api.
    func (c *CustomAnalyticsPlugins) Fill(api apidef.APIDefinition) {
    	if api.AnalyticsPlugin.Enabled {
    		customPlugins := []CustomPlugin{
    			{
    				Enabled:      api.AnalyticsPlugin.Enabled,
    				FunctionName: api.AnalyticsPlugin.FuncName,
    				Path:         api.AnalyticsPlugin.PluginPath,
    			},
    		}
    		*c = customPlugins
    	}
    }
    
    // ExtractTo extracts CustomAnalyticsPlugins into AnalyticsPlugin of supplied api.
    func (c *CustomAnalyticsPlugins) ExtractTo(api *apidef.APIDefinition) {
    	if len(*c) > 0 {
    		// extract the first item in the customAnalyticsPlugin into apidef
    		plugin := (*c)[0]
    		api.AnalyticsPlugin.Enabled = plugin.Enabled
    		api.AnalyticsPlugin.FuncName = plugin.FunctionName
    		api.AnalyticsPlugin.PluginPath = plugin.Path
    	}

    Copy link
    Contributor

    github-actions bot commented Feb 5, 2025

    API Changes

    --- prev.txt	2025-02-06 10:58:27.760336687 +0000
    +++ current.txt	2025-02-06 10:58:22.886279372 +0000
    @@ -323,10 +323,15 @@
         enabled in OAS API definition.
     
     type AnalyticsPluginConfig struct {
    -	Enabled    bool   `bson:"enable" json:"enable,omitempty"`
    +	// Enabled activates the custom plugin
    +	Enabled bool `bson:"enable" json:"enable,omitempty"`
    +	// PluginPath is the path to the shared object file or path to js code.
     	PluginPath string `bson:"plugin_path" json:"plugin_path,omitempty"`
    -	FuncName   string `bson:"func_name" json:"func_name,omitempty"`
    +	// FunctionName is the name of the method.
    +	FuncName string `bson:"func_name" json:"func_name,omitempty"`
     }
    +    AnalyticsPluginConfig holds the configuration for the analytics custom
    +    function plugins
     
     type AuthConfig struct {
     	Name              string          `mapstructure:"name" bson:"name" json:"name"`
    @@ -888,7 +893,6 @@
     	PostKeyAuth []MiddlewareDefinition `bson:"post_key_auth" json:"post_key_auth"`
     	AuthCheck   MiddlewareDefinition   `bson:"auth_check" json:"auth_check"`
     	Response    []MiddlewareDefinition `bson:"response" json:"response"`
    -	TrafficLogs []MiddlewareDefinition `bson:"traffic_logs" json:"traffic_logs"`
     	Driver      MiddlewareDriver       `bson:"driver" json:"driver"`
     	IdExtractor MiddlewareIdExtractor  `bson:"id_extractor" json:"id_extractor"`
     }
    @@ -2444,6 +2448,16 @@
     func (c *ContextVariables) Fill(api apidef.APIDefinition)
         Fill fills *ContextVariables from apidef.APIDefinition.
     
    +type CustomAnalyticsPlugins []CustomPlugin
    +    CustomAnalyticsPlugins is a list of CustomPlugin objects for analytics.
    +
    +func (c *CustomAnalyticsPlugins) ExtractTo(api *apidef.APIDefinition)
    +    ExtractTo extracts CustomAnalyticsPlugins into AnalyticsPlugin of supplied
    +    api.
    +
    +func (c *CustomAnalyticsPlugins) Fill(api apidef.APIDefinition)
    +    Fill fills CustomAnalyticsPlugins from AnalyticsPlugin in the supplied api.
    +
     type CustomKeyLifetime struct {
     	// Enabled enables custom maximum retention for keys for the API
     	//
    @@ -4088,7 +4102,7 @@
     	CustomRetentionPeriod ReadableDuration `bson:"customRetentionPeriod,omitempty" json:"customRetentionPeriod,omitempty"`
     	// Plugins configures custom plugins to allow for extensive modifications to analytics records
     	// The plugins would be executed in the order of configuration in the list.
    -	Plugins CustomPlugins `bson:"plugins,omitempty" json:"plugins,omitempty"`
    +	Plugins CustomAnalyticsPlugins `bson:"plugins,omitempty" json:"plugins,omitempty"`
     }
         TrafficLogs holds configuration about API log analytics.
     

    Copy link
    Contributor

    github-actions bot commented Feb 5, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Avoid nil pointer dereference in Fill

    Add a check in the CustomAnalyticsPlugins.Fill method to ensure that
    api.AnalyticsPlugin is not nil before accessing its fields, to avoid potential
    runtime panics.

    apidef/oas/middleware.go [1626-1636]

    -if api.AnalyticsPlugin.Enabled {
    +if api.AnalyticsPlugin != nil && api.AnalyticsPlugin.Enabled {
         customPlugins := []CustomPlugin{
             {
                 Enabled:      api.AnalyticsPlugin.Enabled,
                 FunctionName: api.AnalyticsPlugin.FuncName,
                 Path:         api.AnalyticsPlugin.PluginPath,
             },
         }
         *c = customPlugins
     }
    Suggestion importance[1-10]: 9

    Why: The suggestion adds a crucial check to ensure that api.AnalyticsPlugin is not nil before accessing its fields, preventing potential runtime panics. This is a significant improvement in robustness and error handling.

    9
    Prevent unintended resetting of Plugins

    Ensure that the defer statement inside the ExtractTo method for TrafficLogs does not
    unintentionally reset the Plugins field to nil after it has been populated and used,
    as this could lead to unexpected behavior.

    apidef/oas/middleware.go [1613-1617]

     if t.Plugins == nil {
         t.Plugins = make(CustomAnalyticsPlugins, 0)
    -    defer func() {
    +}
    +defer func() {
    +    if len(t.Plugins) == 0 {
             t.Plugins = nil
    -    }()
    -}
    +    }
    +}()
    Suggestion importance[1-10]: 8

    Why: The suggestion addresses a potential issue where the defer statement could unintentionally reset the Plugins field to nil after it has been populated and used. The improved code ensures that the Plugins field is only reset to nil if it remains empty, preventing unexpected behavior.

    8
    General
    Add test for nil Plugins

    Include a test case to verify the behavior of TrafficLogs.ExtractTo when Plugins is
    nil, ensuring it correctly handles this scenario without errors.

    apidef/oas/middleware_test.go [249-258]

    -t.Run("with custom analytics plugin", func(t *testing.T) {
    +t.Run("with nil plugins", func(t *testing.T) {
         t.Parallel()
         expectedTrafficLogsPlugin := TrafficLogs{
             Enabled:    true,
             TagHeaders: []string{},
    -        Plugins: CustomAnalyticsPlugins{
    -            {
    -                Enabled:      true,
    -                FunctionName: "CustomAnalyticsPlugin",
    -                Path:         "/path/to/plugin",
    -            },
    -        },
    +        Plugins:    nil,
         }
    +    api := apidef.APIDefinition{}
    +    expectedTrafficLogsPlugin.ExtractTo(&api)
    +    assert.Nil(t, api.AnalyticsPlugin.FuncName)
    +})
    Suggestion importance[1-10]: 7

    Why: Adding a test case for handling nil Plugins ensures that the TrafficLogs.ExtractTo method behaves correctly in this scenario, improving test coverage and reliability. However, the impact is limited to testing and does not directly affect the production code.

    7

    @kofoworola kofoworola force-pushed the feat/analytics_plugin branch from 30a2ad6 to df933be Compare February 6, 2025 08:41
    @kofoworola kofoworola enabled auto-merge (squash) February 6, 2025 10:15
    Copy link

    sonarqubecloud bot commented Feb 6, 2025

    Quality Gate Failed Quality Gate failed

    Failed conditions
    E Security Rating on New Code (required ≥ A)

    See analysis details on SonarQube Cloud

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

    @kofoworola kofoworola merged commit 4487d74 into master Feb 6, 2025
    26 of 41 checks passed
    @kofoworola kofoworola deleted the feat/analytics_plugin branch February 6, 2025 11:20
    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.

    4 participants