From 7a7aa610a144d714397018159f7b119d739a0d8d Mon Sep 17 00:00:00 2001 From: Casey Waldren Date: Tue, 5 Mar 2024 11:27:36 -0800 Subject: [PATCH 1/4] refactor: remove unused baseURI variables related to ServiceEndpoints configuration --- internal/endpoints/configure_endpoints.go | 30 +++----- .../endpoints/configure_endpoints_test.go | 73 +++++++++++++++++++ ldcomponents/polling_data_source_builder.go | 4 +- ldcomponents/send_events.go | 4 +- ldcomponents/streaming_data_source_builder.go | 4 +- 5 files changed, 88 insertions(+), 27 deletions(-) create mode 100644 internal/endpoints/configure_endpoints_test.go diff --git a/internal/endpoints/configure_endpoints.go b/internal/endpoints/configure_endpoints.go index a33cab71..8e38dffb 100644 --- a/internal/endpoints/configure_endpoints.go +++ b/internal/endpoints/configure_endpoints.go @@ -48,11 +48,8 @@ func getCustom(serviceEndpoints interfaces.ServiceEndpoints, serviceType Service } // IsCustom returns true if the service endpoint has been overridden with a non-default value. -func IsCustom(serviceEndpoints interfaces.ServiceEndpoints, serviceType ServiceType, overrideValue string) bool { - uri := overrideValue - if uri == "" { - uri = getCustom(serviceEndpoints, serviceType) - } +func IsCustom(serviceEndpoints interfaces.ServiceEndpoints, serviceType ServiceType) bool { + uri := getCustom(serviceEndpoints, serviceType) return uri != "" && strings.TrimSuffix(uri, "/") != strings.TrimSuffix(DefaultBaseURI(serviceType), "/") } @@ -74,23 +71,20 @@ func DefaultBaseURI(serviceType ServiceType) string { func SelectBaseURI( serviceEndpoints interfaces.ServiceEndpoints, serviceType ServiceType, - overrideValue string, loggers ldlog.Loggers, ) string { - configuredBaseURI := overrideValue - if configuredBaseURI == "" { - if anyCustom(serviceEndpoints) { - configuredBaseURI = getCustom(serviceEndpoints, serviceType) - if configuredBaseURI == "" { - loggers.Errorf( - "You have set custom ServiceEndpoints without specifying the %s base URI; connections may not work properly", - serviceType, - ) - configuredBaseURI = DefaultBaseURI(serviceType) - } - } else { + var configuredBaseURI string + if anyCustom(serviceEndpoints) { + configuredBaseURI = getCustom(serviceEndpoints, serviceType) + if configuredBaseURI == "" { + loggers.Errorf( + "You have set custom ServiceEndpoints without specifying the %s base URI; connections may not work properly", + serviceType, + ) configuredBaseURI = DefaultBaseURI(serviceType) } + } else { + configuredBaseURI = DefaultBaseURI(serviceType) } return strings.TrimRight(configuredBaseURI, "/") } diff --git a/internal/endpoints/configure_endpoints_test.go b/internal/endpoints/configure_endpoints_test.go new file mode 100644 index 00000000..e6172a1f --- /dev/null +++ b/internal/endpoints/configure_endpoints_test.go @@ -0,0 +1,73 @@ +package endpoints + +import ( + "fmt" + "github.com/launchdarkly/go-sdk-common/v3/ldlog" + "github.com/launchdarkly/go-sdk-common/v3/ldlogtest" + "github.com/launchdarkly/go-server-sdk/v7/interfaces" + "github.com/stretchr/testify/assert" + "strings" + "testing" +) + +func TestDefaultURISelectedIfNoCustomURISpecified(t *testing.T) { + logger := ldlogtest.NewMockLog() + endpoints := interfaces.ServiceEndpoints{} + services := []ServiceType{StreamingService, PollingService, EventsService} + for _, service := range services { + assert.Equal(t, strings.TrimSuffix(DefaultBaseURI(service), "/"), SelectBaseURI(endpoints, service, logger.Loggers)) + } +} + +func TestSelectCustomURIs(t *testing.T) { + logger := ldlogtest.NewMockLog() + const customURI = "http://custom_uri" + + cases := []struct { + endpoints interfaces.ServiceEndpoints + service ServiceType + }{ + {interfaces.ServiceEndpoints{Polling: customURI}, PollingService}, + {interfaces.ServiceEndpoints{Streaming: customURI}, StreamingService}, + {interfaces.ServiceEndpoints{Events: customURI}, EventsService}, + } + + for _, c := range cases { + assert.Equal(t, customURI, SelectBaseURI(c.endpoints, c.service, logger.Loggers)) + } + + assert.Empty(t, logger.GetOutput(ldlog.Error)) +} + +func TestLogErrorIfAtLeastOneButNotAllCustomURISpecified(t *testing.T) { + logger := ldlogtest.NewMockLog() + const customURI = "http://custom_uri" + + cases := []struct { + endpoints interfaces.ServiceEndpoints + service ServiceType + }{ + {interfaces.ServiceEndpoints{Streaming: customURI}, PollingService}, + {interfaces.ServiceEndpoints{Events: customURI}, PollingService}, + {interfaces.ServiceEndpoints{Streaming: customURI, Events: customURI}, PollingService}, + + {interfaces.ServiceEndpoints{Polling: customURI}, StreamingService}, + {interfaces.ServiceEndpoints{Events: customURI}, StreamingService}, + {interfaces.ServiceEndpoints{Polling: customURI, Events: customURI}, StreamingService}, + + {interfaces.ServiceEndpoints{Streaming: customURI}, EventsService}, + {interfaces.ServiceEndpoints{Polling: customURI}, EventsService}, + {interfaces.ServiceEndpoints{Streaming: customURI, Polling: customURI}, EventsService}, + } + + // Even if the configuration is considered to be likely malformed, we should still return the proper default URI for + // the service that wasn't configured. + for _, c := range cases { + assert.Equal(t, strings.TrimSuffix(DefaultBaseURI(c.service), "/"), SelectBaseURI(c.endpoints, c.service, logger.Loggers)) + } + + // For each service that wasn't configured, we should see a log message indicating that. + for _, c := range cases { + logger.AssertMessageMatch(t, true, ldlog.Error, fmt.Sprintf("You have set custom ServiceEndpoints without specifying the %s base URI", c.service)) + } +} diff --git a/ldcomponents/polling_data_source_builder.go b/ldcomponents/polling_data_source_builder.go index 21a9d678..72f4cf3e 100644 --- a/ldcomponents/polling_data_source_builder.go +++ b/ldcomponents/polling_data_source_builder.go @@ -20,7 +20,6 @@ const DefaultPollInterval = 30 * time.Second // // See [PollingDataSource] for usage. type PollingDataSourceBuilder struct { - baseURI string pollInterval time.Duration filterKey ldvalue.OptionalString } @@ -90,7 +89,6 @@ func (b *PollingDataSourceBuilder) Build(context subsystems.ClientContext) (subs configuredBaseURI := endpoints.SelectBaseURI( context.GetServiceEndpoints(), endpoints.PollingService, - b.baseURI, context.GetLogging().Loggers, ) cfg := datasource.PollingConfig{ @@ -107,7 +105,7 @@ func (b *PollingDataSourceBuilder) DescribeConfiguration(context subsystems.Clie return ldvalue.ObjectBuild(). SetBool("streamingDisabled", true). SetBool("customBaseURI", - endpoints.IsCustom(context.GetServiceEndpoints(), endpoints.PollingService, b.baseURI)). + endpoints.IsCustom(context.GetServiceEndpoints(), endpoints.PollingService)). Set("pollingIntervalMillis", durationToMillisValue(b.pollInterval)). SetBool("usingRelayDaemon", false). Build() diff --git a/ldcomponents/send_events.go b/ldcomponents/send_events.go index 790d30e2..6576791e 100644 --- a/ldcomponents/send_events.go +++ b/ldcomponents/send_events.go @@ -34,7 +34,6 @@ const ( // See [SendEvents] for usage. type EventProcessorBuilder struct { allAttributesPrivate bool - baseURI string capacity int diagnosticRecordingInterval time.Duration flushInterval time.Duration @@ -74,7 +73,6 @@ func (b *EventProcessorBuilder) Build( configuredBaseURI := endpoints.SelectBaseURI( context.GetServiceEndpoints(), endpoints.EventsService, - b.baseURI, loggers, ) @@ -206,7 +204,7 @@ func (b *EventProcessorBuilder) DescribeConfiguration(context subsystems.ClientC return ldvalue.ObjectBuild(). Set("allAttributesPrivate", ldvalue.Bool(b.allAttributesPrivate)). Set("customEventsURI", ldvalue.Bool( - endpoints.IsCustom(context.GetServiceEndpoints(), endpoints.EventsService, b.baseURI))). + endpoints.IsCustom(context.GetServiceEndpoints(), endpoints.EventsService))). Set("diagnosticRecordingIntervalMillis", durationToMillisValue(b.diagnosticRecordingInterval)). Set("eventsCapacity", ldvalue.Int(b.capacity)). Set("eventsFlushIntervalMillis", durationToMillisValue(b.flushInterval)). diff --git a/ldcomponents/streaming_data_source_builder.go b/ldcomponents/streaming_data_source_builder.go index 8378ed33..ade67b04 100644 --- a/ldcomponents/streaming_data_source_builder.go +++ b/ldcomponents/streaming_data_source_builder.go @@ -20,7 +20,6 @@ const DefaultInitialReconnectDelay = time.Second // // See StreamingDataSource for usage. type StreamingDataSourceBuilder struct { - baseURI string initialReconnectDelay time.Duration filterKey ldvalue.OptionalString } @@ -81,7 +80,6 @@ func (b *StreamingDataSourceBuilder) Build(context subsystems.ClientContext) (su configuredBaseURI := endpoints.SelectBaseURI( context.GetServiceEndpoints(), endpoints.StreamingService, - b.baseURI, context.GetLogging().Loggers, ) cfg := datasource.StreamConfig{ @@ -101,7 +99,7 @@ func (b *StreamingDataSourceBuilder) DescribeConfiguration(context subsystems.Cl return ldvalue.ObjectBuild(). SetBool("streamingDisabled", false). SetBool("customStreamURI", - endpoints.IsCustom(context.GetServiceEndpoints(), endpoints.StreamingService, b.baseURI)). + endpoints.IsCustom(context.GetServiceEndpoints(), endpoints.StreamingService)). Set("reconnectTimeMillis", durationToMillisValue(b.initialReconnectDelay)). SetBool("usingRelayDaemon", false). Build() From ae2bb13170b2fd0f6b9f0766c5f35897bb4fdd93 Mon Sep 17 00:00:00 2001 From: Casey Waldren Date: Tue, 5 Mar 2024 12:11:25 -0800 Subject: [PATCH 2/4] fix: do not emit error when RelayProxyEndpointsWithoutEvents is used --- interfaces/service_endpoints.go | 28 +++++++++++++-- internal/endpoints/configure_endpoints.go | 10 +++--- .../endpoints/configure_endpoints_test.go | 35 +++++++++++++------ ldcomponents/service_endpoints.go | 5 +-- 4 files changed, 59 insertions(+), 19 deletions(-) diff --git a/interfaces/service_endpoints.go b/interfaces/service_endpoints.go index 5fe3c582..dbcb6e87 100644 --- a/interfaces/service_endpoints.go +++ b/interfaces/service_endpoints.go @@ -7,9 +7,31 @@ package interfaces // values such as Streaming, or use the helper method // [github.com/launchdarkly/go-server-sdk/v7/ldcomponents.RelayProxyEndpoints]. // +// Important note: if one or more URI is set to a custom value, then all URIs should be set to custom values. +// Otherwise, the SDK will emit an error-level log to surface this potential misconfiguration, while using default values for +// the unset URIs. +// +// There are some scenarios where it is desirable to set only some of the fields, but this is not recommended for general +// usage. If you're scenario requires it, you can call [WithPartialSpecification] to suppress the +// error message. +// // See Config.ServiceEndpoints for more details. type ServiceEndpoints struct { - Streaming string - Polling string - Events string + Streaming string + Polling string + Events string + allowPartialSpecification bool +} + +// WithPartialSpecification returns a copy of this ServiceEndpoints that will not trigger an error-level log message +// if only some, but not all the fields are set to custom values. This is an advanced configuration and likely not necessary +// for most use-cases. +func (s ServiceEndpoints) WithPartialSpecification() ServiceEndpoints { + s.allowPartialSpecification = true + return s +} + +// PartialSpecificationRequested returns true if this ServiceEndpoints should not be treated as malformed if some, but not all fields are set. +func (s ServiceEndpoints) PartialSpecificationRequested() bool { + return s.allowPartialSpecification } diff --git a/internal/endpoints/configure_endpoints.go b/internal/endpoints/configure_endpoints.go index 8e38dffb..85dfac1e 100644 --- a/internal/endpoints/configure_endpoints.go +++ b/internal/endpoints/configure_endpoints.go @@ -77,10 +77,12 @@ func SelectBaseURI( if anyCustom(serviceEndpoints) { configuredBaseURI = getCustom(serviceEndpoints, serviceType) if configuredBaseURI == "" { - loggers.Errorf( - "You have set custom ServiceEndpoints without specifying the %s base URI; connections may not work properly", - serviceType, - ) + if !serviceEndpoints.PartialSpecificationRequested() { + loggers.Errorf( + "You have set custom ServiceEndpoints without specifying the %s base URI; connections may not work properly", + serviceType, + ) + } configuredBaseURI = DefaultBaseURI(serviceType) } } else { diff --git a/internal/endpoints/configure_endpoints_test.go b/internal/endpoints/configure_endpoints_test.go index e6172a1f..fe61c314 100644 --- a/internal/endpoints/configure_endpoints_test.go +++ b/internal/endpoints/configure_endpoints_test.go @@ -40,7 +40,7 @@ func TestSelectCustomURIs(t *testing.T) { } func TestLogErrorIfAtLeastOneButNotAllCustomURISpecified(t *testing.T) { - logger := ldlogtest.NewMockLog() + const customURI = "http://custom_uri" cases := []struct { @@ -60,14 +60,29 @@ func TestLogErrorIfAtLeastOneButNotAllCustomURISpecified(t *testing.T) { {interfaces.ServiceEndpoints{Streaming: customURI, Polling: customURI}, EventsService}, } - // Even if the configuration is considered to be likely malformed, we should still return the proper default URI for - // the service that wasn't configured. - for _, c := range cases { - assert.Equal(t, strings.TrimSuffix(DefaultBaseURI(c.service), "/"), SelectBaseURI(c.endpoints, c.service, logger.Loggers)) - } + t.Run("without explicit partial specification", func(t *testing.T) { + logger := ldlogtest.NewMockLog() + + // Even if the configuration is considered to be likely malformed, we should still return the proper default URI for + // the service that wasn't configured. + for _, c := range cases { + assert.Equal(t, strings.TrimSuffix(DefaultBaseURI(c.service), "/"), SelectBaseURI(c.endpoints, c.service, logger.Loggers)) + } + + // For each service that wasn't configured, we should see a log message indicating that. + for _, c := range cases { + logger.AssertMessageMatch(t, true, ldlog.Error, fmt.Sprintf("You have set custom ServiceEndpoints without specifying the %s base URI", c.service)) + } + }) + + t.Run("with partial specification", func(t *testing.T) { + logger := ldlogtest.NewMockLog() + + for _, c := range cases { + endpoints := c.endpoints.WithPartialSpecification() + assert.Equal(t, strings.TrimSuffix(DefaultBaseURI(c.service), "/"), SelectBaseURI(endpoints, c.service, logger.Loggers)) + } + assert.Empty(t, logger.GetOutput(ldlog.Error)) + }) - // For each service that wasn't configured, we should see a log message indicating that. - for _, c := range cases { - logger.AssertMessageMatch(t, true, ldlog.Error, fmt.Sprintf("You have set custom ServiceEndpoints without specifying the %s base URI", c.service)) - } } diff --git a/ldcomponents/service_endpoints.go b/ldcomponents/service_endpoints.go index 3d633c8f..40d6ee61 100644 --- a/ldcomponents/service_endpoints.go +++ b/ldcomponents/service_endpoints.go @@ -31,7 +31,8 @@ func RelayProxyEndpoints(relayProxyBaseURI string) interfaces.ServiceEndpoints { } // RelayProxyEndpointsWithoutEvents specifies a single base URI for a Relay Proxy instance, telling -// the SDK to use the Relay Proxy for all services except analytics events. +// the SDK to use the Relay Proxy for all services except analytics events. Note that this does not disable events, it +// instead means events (if enabled) will be sent directly to LaunchDarkly. // // When using the LaunchDarkly Relay Proxy (https://docs.launchdarkly.com/home/relay-proxy), the SDK // only needs to know the single base URI of the Relay Proxy, which will provide all of the proxied @@ -51,5 +52,5 @@ func RelayProxyEndpointsWithoutEvents(relayProxyBaseURI string) interfaces.Servi return interfaces.ServiceEndpoints{ Streaming: relayProxyBaseURI, Polling: relayProxyBaseURI, - } + }.WithPartialSpecification() } From 0e8e1172e6b596e8f87e7152029a34ec17daa63e Mon Sep 17 00:00:00 2001 From: Casey Waldren Date: Wed, 13 Mar 2024 10:58:00 -0700 Subject: [PATCH 3/4] attempt to shorten doc line length to appease linter --- interfaces/service_endpoints.go | 29 +++++++++++++++++------------ 1 file changed, 17 insertions(+), 12 deletions(-) diff --git a/interfaces/service_endpoints.go b/interfaces/service_endpoints.go index dbcb6e87..728abef3 100644 --- a/interfaces/service_endpoints.go +++ b/interfaces/service_endpoints.go @@ -2,17 +2,20 @@ package interfaces // ServiceEndpoints allow configuration of custom service URIs. // -// If you want to set non-default values for any of these fields, set the ServiceEndpoints field -// in the SDK's [github.com/launchdarkly/go-server-sdk/v7.Config] struct. You may set individual -// values such as Streaming, or use the helper method +// If you want to set non-default values for any of these fields, +// set the ServiceEndpoints field in the SDK's +// [github.com/launchdarkly/go-server-sdk/v7.Config] struct. +// You may set individual values such as Streaming, or use the helper method // [github.com/launchdarkly/go-server-sdk/v7/ldcomponents.RelayProxyEndpoints]. // -// Important note: if one or more URI is set to a custom value, then all URIs should be set to custom values. -// Otherwise, the SDK will emit an error-level log to surface this potential misconfiguration, while using default values for -// the unset URIs. +// Important note: if one or more URI is set to a custom value, then +// all URIs should be set to custom values. Otherwise, the SDK will emit +// an error-level log to surface this potential misconfiguration, while +// using default values for the unset URIs. // -// There are some scenarios where it is desirable to set only some of the fields, but this is not recommended for general -// usage. If you're scenario requires it, you can call [WithPartialSpecification] to suppress the +// There are some scenarios where it is desirable to set only some of the +// fields, but this is not recommended for general usage. If you're scenario +// requires it, you can call [WithPartialSpecification] to suppress the // error message. // // See Config.ServiceEndpoints for more details. @@ -23,15 +26,17 @@ type ServiceEndpoints struct { allowPartialSpecification bool } -// WithPartialSpecification returns a copy of this ServiceEndpoints that will not trigger an error-level log message -// if only some, but not all the fields are set to custom values. This is an advanced configuration and likely not necessary -// for most use-cases. +// WithPartialSpecification returns a copy of this ServiceEndpoints that will +// not trigger an error-level log message if only some, but not all the fields +// are set to custom values. This is an advanced configuration and likely not +// necessary for most use-cases. func (s ServiceEndpoints) WithPartialSpecification() ServiceEndpoints { s.allowPartialSpecification = true return s } -// PartialSpecificationRequested returns true if this ServiceEndpoints should not be treated as malformed if some, but not all fields are set. +// PartialSpecificationRequested returns true if this ServiceEndpoints should not +// be treated as malformed if some, but not all fields are set. func (s ServiceEndpoints) PartialSpecificationRequested() bool { return s.allowPartialSpecification } From 1d4ccf8f2e28c16b7b4d55f939662c67473ce987 Mon Sep 17 00:00:00 2001 From: Casey Waldren Date: Thu, 14 Mar 2024 10:29:41 -0700 Subject: [PATCH 4/4] Update interfaces/service_endpoints.go Co-authored-by: Matthew M. Keeler --- interfaces/service_endpoints.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/interfaces/service_endpoints.go b/interfaces/service_endpoints.go index 728abef3..85c1385b 100644 --- a/interfaces/service_endpoints.go +++ b/interfaces/service_endpoints.go @@ -14,7 +14,7 @@ package interfaces // using default values for the unset URIs. // // There are some scenarios where it is desirable to set only some of the -// fields, but this is not recommended for general usage. If you're scenario +// fields, but this is not recommended for general usage. If your scenario // requires it, you can call [WithPartialSpecification] to suppress the // error message. //