APIGOV-32644 - stop publishing servers when possible (#1034)

* compare string slices unordered lists

* when strip servers is set calculate the hash with and without servers, when both change publish without servers

* do not retain original spec definition after stripping servers

* add some test for new functionality

* more tests

* update tag comparison for revision

Author: jcollins-axway

pkg/apic/apiservice_test.go

@@ -350,29 +350,128 @@ func Test_PublishServiceError(t *testing.T) {
 func Test_processRevision(t *testing.T) {
 	client, httpClient := GetTestServiceClient()
 
-	// tests for updating existing revision
-	httpClient.SetResponses([]api.MockResponse{
-		{
-			FileName: "./testdata/servicerevision.json", // for call to update the serviceRevision
-			RespCode: http.StatusOK,
+	testCases := map[string]struct {
+		skip            bool
+		httpResponses   []api.MockResponse
+		serviceBody     ServiceBody
+		expectedRevName string
+	}{
+		"publish new revision": {
+			httpResponses: []api.MockResponse{
+				{
+					FileName: "./testdata/servicerevision.json", // for call to update the serviceRevision
+					RespCode: http.StatusOK,
+				},
+				{
+					FileName: "./testdata/servicerevision.json", // for call to update the serviceRevision x-agent-details
+					RespCode: http.StatusOK,
+				},
+				{
+					FileName: "./testdata/servicerevision.json", // for call to update the serviceRevision
+					RespCode: http.StatusOK,
+				},
+				{
+					FileName: "./testdata/servicerevision.json", // for call to update the serviceRevision x-agent-details
+					RespCode: http.StatusOK,
+				},
+			},
+			serviceBody: ServiceBody{
+				APIName:          "daleapi",
+				Documentation:    []byte("\"docs\""),
+				Image:            "abcde",
+				ImageContentType: "image/jpeg",
+				ResourceType:     Oas2,
+				RestAPIID:        "12345",
+			},
+			expectedRevName: "daleapi",
 		},
-		{
-			FileName: "./testdata/servicerevision.json", // for call to update the serviceRevision x-agent-details
-			RespCode: http.StatusOK,
+		"skip publish when no changes": {
+			httpResponses: []api.MockResponse{
+				{
+					RespData: `[{"name": "daleapi","tags": ["tag1","tag2"]}]`,
+					RespCode: http.StatusOK,
+				},
+			},
+			serviceBody: ServiceBody{
+				APIName:          "daleapi",
+				Documentation:    []byte("\"docs\""),
+				Image:            "abcde",
+				ImageContentType: "image/jpeg",
+				ResourceType:     Oas2,
+				RestAPIID:        "12345",
+				specHash:         "abc123",
+				specHashes: map[string]interface{}{
+					"abc123": "daleapi",
+				},
+				serviceContext: serviceContext{
+					serviceAction: updateAPI,
+				},
+			},
+			expectedRevName: "daleapi",
 		},
-		{
-			FileName: "./testdata/servicerevision.json", // for call to update the serviceRevision
-			RespCode: http.StatusOK,
+		"skip publish when previous revision found": {
+			httpResponses: []api.MockResponse{
+				{
+					RespData: `[{"name": "daleapi","tags": ["tag1","tag2"]},{"name": "daleapi-1","tags": ["tag1","tag2"]}]`,
+					RespCode: http.StatusOK,
+				},
+			},
+			serviceBody: ServiceBody{
+				APIName:          "daleapi",
+				Documentation:    []byte("\"docs\""),
+				Image:            "abcde",
+				ImageContentType: "image/jpeg",
+				ResourceType:     Oas2,
+				RestAPIID:        "12345",
+				specHash:         "abc123",
+				specHashes: map[string]interface{}{
+					"abc123": "daleapi-1",
+				},
+				serviceContext: serviceContext{
+					serviceAction: updateAPI,
+				},
+			},
+			expectedRevName: "daleapi-1",
 		},
-		{
-			FileName: "./testdata/servicerevision.json", // for call to update the serviceRevision x-agent-details
-			RespCode: http.StatusOK,
+		"find revision match using original spec hash": {
+			httpResponses: []api.MockResponse{
+				{
+					RespData: `[{"name": "daleapi","tags": ["tag1","tag2"]},{"name": "daleapi-1","tags": ["tag1","tag2"]}]`,
+					RespCode: http.StatusOK,
+				},
+			},
+			serviceBody: ServiceBody{
+				APIName:          "daleapi",
+				Documentation:    []byte("\"docs\""),
+				Image:            "abcde",
+				ImageContentType: "image/jpeg",
+				ResourceType:     Oas2,
+				RestAPIID:        "12345",
+				specHash:         "abc1234",
+				originalSpecHash: "abc123",
+				specHashes: map[string]interface{}{
+					"abc123": "daleapi-1",
+				},
+				serviceContext: serviceContext{
+					serviceAction: updateAPI,
+				},
+			},
+			expectedRevName: "daleapi-1",
 		},
-	})
-	cloneServiceBody := serviceBody
-	// Normal Revision
-	client.processRevision(&cloneServiceBody)
-	assert.NotEqual(t, "", cloneServiceBody.serviceContext.revisionName)
+	}
+	for name, tc := range testCases {
+		t.Run(name, func(t *testing.T) {
+			if tc.skip {
+				return
+			}
+			// tests for updating existing revision
+			httpClient.SetResponses(tc.httpResponses)
+
+			client.processRevision(&tc.serviceBody)
+			assert.NotEqual(t, "", tc.serviceBody.serviceContext.revisionName)
+			assert.Equal(t, tc.expectedRevName, tc.serviceBody.serviceContext.revisionName)
+		})
+	}
 }
 
 func TestDeleteServiceByAPIID(t *testing.T) {

pkg/apic/apiservicerevision.go

@@ -7,9 +7,7 @@ import (
 	"errors"
 	"fmt"
 	"net/http"
-	"reflect"
 	"regexp"
-	"sort"
 	"strconv"
 	"strings"
 	"text/template"
@@ -165,7 +163,15 @@ func (c *ServiceClient) getRevisionsIfUpdating(serviceBody *ServiceBody) ([]*man
 
 // checkAndUpdateExistingRevision checks if a revision with the same hash exists and updates tags if needed
 func (c *ServiceClient) checkAndUpdateExistingRevision(serviceBody *ServiceBody, apiServiceRevisions []*management.APIServiceRevision) (bool, error) {
+	// attempt to use the stripped spec hash
 	revName, found := serviceBody.specHashes[serviceBody.specHash]
+	if !found && serviceBody.originalSpecHash != "" {
+		// check if the original spec hash matches an existing revision,
+		// this is to cover the case where the spec content has not changed since the last publish,
+		// but the hash has changed due to non-content related changes (e.g. stripping servers)
+		revName, found = serviceBody.specHashes[serviceBody.originalSpecHash]
+	}
+
 	if !found {
 		return false, nil
 	}
@@ -205,24 +211,13 @@ func (c *ServiceClient) checkAndUpdateExistingRevision(serviceBody *ServiceBody,
 // different, return the updated tags
 func (c *ServiceClient) getUpdatedTagKeys(serviceBodyTags map[string]interface{}, revisionTags []string) []string {
 	// Extract values from map and convert to []string
-	var mapValues []string
-	for _, v := range serviceBodyTags {
-		if strVal, ok := v.(string); ok {
-			mapValues = append(mapValues, strVal)
-		}
-	}
-
-	// Sort both slices to allow unordered comparison
-	sort.Strings(mapValues)
-	sort.Strings(revisionTags)
+	tags := mapToTagsArray(serviceBodyTags, c.cfg.GetTagsToPublish())
 
 	// Compare
-	if reflect.DeepEqual(mapValues, revisionTags) {
+	if util.StringSlicesEqualUnordered(tags, revisionTags) {
 		return []string{} // return empty string slice if equal
 	}
 
-	// If not equal, return the keys from serviceBodyTags
-	tags := mapToTagsArray(serviceBodyTags, c.cfg.GetTagsToPublish())
 	return tags
 }
 

pkg/apic/client.go

@@ -4,7 +4,6 @@ import (
 	"encoding/json"
 	"fmt"
 	"net/http"
-	"slices"
 	"strconv"
 	"strings"
 	"sync"
@@ -870,7 +869,7 @@ func (c *ServiceClient) updateSpecORCreateResourceInstance(data *apiv1.ResourceI
 		method = coreapi.PUT
 
 		// check if either hash, tags or title have changed and mark for update
-		equalTags := slices.Equal(existingRI.GetTags(), data.GetTags())
+		equalTags := util.StringSlicesEqualUnordered(existingRI.GetTags(), data.GetTags())
 		oldHash, _ := util.GetAgentDetailsValue(existingRI, defs.AttrSpecHash)
 		newHash, _ := util.GetAgentDetailsValue(data, defs.AttrSpecHash)
 		if oldHash == newHash && existingRI.Title == data.Title && equalTags {

pkg/apic/servicebody.go

@@ -14,59 +14,61 @@ type APIKeyInfo struct {
 
 // ServiceBody - details about a service to create
 type ServiceBody struct {
-	NameToPush                string
-	APIName                   string
-	RestAPIID                 string
-	PrimaryKey                string
-	URL                       string
-	Stage                     string
-	StageDescriptor           string
-	StageDisplayName          string
-	Description               string
-	Version                   string
-	AuthPolicy                string
-	authPolicies              []string
-	apiKeyInfo                []APIKeyInfo
-	scopes                    map[string]string
-	SpecDefinition            []byte
-	Documentation             []byte
-	Tags                      map[string]interface{}
-	Image                     string
-	ImageContentType          string
-	CreatedBy                 string
-	ResourceContentType       string
-	ResourceType              string
-	SubscriptionName          string
-	APIUpdateSeverity         string
-	State                     string
-	Status                    string
-	ServiceAttributes         map[string]string
-	RevisionAttributes        map[string]string
-	InstanceAttributes        map[string]string
-	ServiceAgentDetails       map[string]interface{}
-	InstanceAgentDetails      map[string]interface{}
-	RevisionAgentDetails      map[string]interface{}
-	serviceContext            serviceContext
-	Endpoints                 []EndpointDefinition
-	UnstructuredProps         *UnstructuredProperties
-	TeamName                  string
-	teamID                    string
-	credentialRequestPolicies []string
-	ardName                   string
-	uniqueARD                 bool
-	ignoreSpecBasesCreds      bool
-	stripOASExtensions        bool
-	specHash                  string
-	specVersion               string
-	accessRequestDefinition   *management.AccessRequestDefinition
-	specHashes                map[string]interface{} // map of hash values to revision names
-	requestDefinitionsAllowed bool                   // used to validate if the instance can have request definitions or not. Use case example - v7 unpublished, remove request definitions
-	dataplaneType             DataplaneType
-	isDesignDataplane         bool
-	referencedServiceName     string
-	referencedInstanceName    string
-	logger                    log.FieldLogger
-	instanceLifecycle         *management.ApiServiceInstanceLifecycle
+	NameToPush                   string
+	APIName                      string
+	RestAPIID                    string
+	PrimaryKey                   string
+	URL                          string
+	Stage                        string
+	StageDescriptor              string
+	StageDisplayName             string
+	Description                  string
+	Version                      string
+	AuthPolicy                   string
+	authPolicies                 []string
+	apiKeyInfo                   []APIKeyInfo
+	scopes                       map[string]string
+	SpecDefinition               []byte
+	Documentation                []byte
+	Tags                         map[string]interface{}
+	Image                        string
+	ImageContentType             string
+	CreatedBy                    string
+	ResourceContentType          string
+	ResourceType                 string
+	SubscriptionName             string
+	APIUpdateSeverity            string
+	State                        string
+	Status                       string
+	ServiceAttributes            map[string]string
+	RevisionAttributes           map[string]string
+	InstanceAttributes           map[string]string
+	ServiceAgentDetails          map[string]interface{}
+	InstanceAgentDetails         map[string]interface{}
+	RevisionAgentDetails         map[string]interface{}
+	serviceContext               serviceContext
+	Endpoints                    []EndpointDefinition
+	UnstructuredProps            *UnstructuredProperties
+	TeamName                     string
+	teamID                       string
+	credentialRequestPolicies    []string
+	ardName                      string
+	uniqueARD                    bool
+	ignoreSpecBasesCreds         bool
+	stripOASExtensions           bool
+	stripOASServersBeforePublish bool
+	specHash                     string
+	specVersion                  string
+	accessRequestDefinition      *management.AccessRequestDefinition
+	specHashes                   map[string]interface{} // map of hash values to revision names
+	requestDefinitionsAllowed    bool                   // used to validate if the instance can have request definitions or not. Use case example - v7 unpublished, remove request definitions
+	dataplaneType                DataplaneType
+	isDesignDataplane            bool
+	referencedServiceName        string
+	referencedInstanceName       string
+	logger                       log.FieldLogger
+	instanceLifecycle            *management.ApiServiceInstanceLifecycle
+	originalSpecHash             string
 }
 
 // SetAccessRequestDefinitionName - set the name of the access request definition for this service body

pkg/apic/servicebuilder.go

@@ -56,6 +56,7 @@ type ServiceBuilder interface {
 	SetAccessRequestDefinitionName(accessRequestDefName string, isUnique bool) ServiceBuilder
 	SetIgnoreSpecBasedCreds(ignore bool) ServiceBuilder
 	SetStripOASExtensions(strip bool) ServiceBuilder
+	SetStripOASServersBeforePublish() ServiceBuilder
 
 	SetUnstructuredType(assetType string) ServiceBuilder
 	SetUnstructuredContentType(contentType string) ServiceBuilder
@@ -379,6 +380,14 @@ func (b *serviceBodyBuilder) Build() (ServiceBody, error) {
 		val.StripExtensions()
 	}
 
+	if b.serviceBody.stripOASServersBeforePublish {
+		b.serviceBody.originalSpecHash = b.serviceBody.specHash
+		val.stripEndpoints()
+		b.serviceBody.SpecDefinition = val.GetSpecBytes()
+		newHash, _ := util.ComputeHash(val.GetSpecBytes())
+		b.serviceBody.specHash = fmt.Sprintf("%v", newHash)
+	}
+
 	// only set ard name based on spec if not already set, use first auth we find
 	if b.serviceBody.ardName != "" {
 		return b.serviceBody, nil
@@ -426,6 +435,11 @@ func (b *serviceBodyBuilder) SetIgnoreSpecBasedCreds(ignore bool) ServiceBuilder
 	return b
 }
 
+func (b *serviceBodyBuilder) SetStripOASServersBeforePublish() ServiceBuilder {
+	b.serviceBody.stripOASServersBeforePublish = true
+	return b
+}
+
 func (b *serviceBodyBuilder) SetStripOASExtensions(strip bool) ServiceBuilder {
 	b.serviceBody.stripOASExtensions = strip
 	return b