Copilot commented on code in PR #3395:
URL: https://github.com/apache/dubbo-go/pull/3395#discussion_r3392665247
##########
metadata/client.go:
##########
@@ -45,18 +45,25 @@ const defaultTimeout = "5s" // s
func GetMetadataFromMetadataReport(revision string, instance
registry.ServiceInstance, registryId string) (*info.MetadataInfo, error) {
report := GetMetadataReportByRegistry(registryId)
if report == nil {
- return nil, perrors.Errorf("no metadata report instance found
for registryId=%s, please check metadata-report configuration", registryId)
+ return nil, perrors.Errorf("[Metadata-Report] no metadata
report instance found for registryId=%s, please check metadata-report
configuration", registryId)
}
- return report.GetAppMetadata(instance.GetServiceName(), revision)
+ meta, err := report.GetAppMetadata(instance.GetServiceName(), revision)
+ if err != nil {
+ return nil, perrors.Wrapf(err, "[Metadata-Report] failed to get
app metadata app=%s revision=%s", instance.GetServiceName(), revision)
+ }
+ return meta, nil
}
func GetMetadataFromRpc(revision string, instance registry.ServiceInstance)
(*info.MetadataInfo, error) {
url := buildStandardMetadataServiceURL(instance)
+ if url == nil {
+ return nil, perrors.New("[Metadata-URL] metadata service URL
params missing: protocol is empty")
+ }
url.SetParam(constant.TimeoutKey, defaultTimeout)
p := extension.GetProtocol(url.Protocol)
invoker := p.Refer(url)
Review Comment:
GetMetadataFromRpc only guards a nil URL (missing protocol). If the metadata
service URL params include a protocol but omit the port,
buildStandardMetadataServiceURL returns a non-nil URL with an empty Port and
the call proceeds to Refer/Invoke, producing confusing downstream errors. Add
an explicit port-empty guard with a [Metadata-URL] error so missing port is
reported at the URL layer as intended.
##########
registry/servicediscovery/service_instances_changed_listener_impl_test.go:
##########
@@ -372,3 +374,216 @@ func (c *capturingNotifyListener) NotifyAll(events
[]*registry.ServiceEvent, cal
callback()
}
}
+
+func TestGetMetadataInfo_CacheKeyFormat(t *testing.T) {
+ // Ensure cache is initialized (normally done by
NewServiceInstancesChangedListener)
+ _ = NewServiceInstancesChangedListener(testApp, constant.DefaultKey,
gxset.NewSet(testApp))
+
+ revision := "rev-cache-key-test"
+ // Pre-populate cache with the expected composite key
+ expectedKey := testApp + ":" + constant.DefaultKey + ":" + revision
+ expectedMeta := newTestMetadataInfo(t, revision, 20000, "dev")
+ metaCache.Set(expectedKey, expectedMeta)
+ t.Cleanup(func() {
+ metaCache.Delete(expectedKey)
+ })
+
+ instance := ®istry.DefaultServiceInstance{
+ ID: "127.0.0.1:20000",
+ ServiceName: testApp,
+ Host: "127.0.0.1",
+ Port: 20000,
+ Enable: true,
+ Healthy: true,
+ Metadata: map[string]string{
+ constant.ExportedServicesRevisionPropertyName: revision,
+ constant.ServiceInstanceEndpoints:
`[{"port":20000,"protocol":"tri"}]`,
+ },
+ }
+
+ // Should hit the cache and return the pre-populated metadata
+ meta, err := GetMetadataInfo(testApp, instance, revision,
constant.DefaultKey)
+ assert.NoError(t, err)
+ assert.Equal(t, expectedMeta, meta)
+}
+
+func TestGetMetadataInfo_LocalStorageGoesDirectlyToRPC(t *testing.T) {
+ // Ensure cache is initialized
+ _ = NewServiceInstancesChangedListener(testApp, constant.DefaultKey,
gxset.NewSet(testApp))
+
+ // Instance with no MetadataStorageTypePropertyName (i.e. local/default
path)
+ // should go directly to RPC without touching the metadata report.
+ // RPC will fail with a URL error because there are no URL params —
that's
+ // enough to confirm the correct branch was taken.
+ instance := ®istry.DefaultServiceInstance{
+ ID: "127.0.0.1:20003",
+ ServiceName: testApp,
+ Host: "127.0.0.1",
+ Port: 20003,
+ Enable: true,
+ Healthy: true,
+ Metadata: map[string]string{
+ constant.ExportedServicesRevisionPropertyName:
"rev-local-rpc",
+ // MetadataStorageTypePropertyName intentionally absent
→ local path
+ constant.ServiceInstanceEndpoints:
`[{"port":20003,"protocol":"tri"}]`,
+ },
+ }
+
+ _, err := GetMetadataInfo(testApp, instance, "rev-local-rpc",
constant.DefaultKey)
+ require.Error(t, err)
+ // Must be a URL/RPC error, not a report error, confirming the local
path
+ // skips the report entirely and goes straight to RPC.
+ assert.Contains(t, err.Error(), "[Metadata-URL]",
+ "local storage path should go directly to RPC, not touch the
metadata report")
+}
+
+func TestGetMetadataInfo_FallbackToRPC(t *testing.T) {
+ // Ensure cache is initialized
+ _ = NewServiceInstancesChangedListener(testApp, constant.DefaultKey,
gxset.NewSet(testApp))
+
+ // remote storage type without a report registered → report will fail
+ // should fall through to RPC, which will fail with url error (no URL
params)
+ instance := ®istry.DefaultServiceInstance{
+ ID: "127.0.0.1:20002",
+ ServiceName: testApp,
+ Host: "127.0.0.1",
+ Port: 20002,
+ Enable: true,
+ Healthy: true,
+ Metadata: map[string]string{
+ constant.ExportedServicesRevisionPropertyName:
"rev-fallback-to-rpc",
+ constant.MetadataStorageTypePropertyName:
constant.RemoteMetadataStorageType,
+ constant.ServiceInstanceEndpoints:
`[{"port":20002,"protocol":"tri"}]`,
+ },
+ }
+
+ _, err := GetMetadataInfo(testApp, instance, "rev-fallback-to-rpc",
constant.DefaultKey)
+ require.Error(t, err)
+ // Both report and RPC fail: error should be a combined Fallback error
containing both causes.
+ // [Metadata-Fallback] prefix proves the fallback path was taken.
+ assert.Contains(t, err.Error(), "[Metadata-Fallback]",
+ "fallback path should produce a [Metadata-Fallback] error")
+ assert.Contains(t, err.Error(), "[Metadata-URL]",
+ "fallback error should include the RPC/URL failure cause")
+}
+
+// TestGetMetadataInfo_ReportReturnsNil_RPCSucceeds verifies the path where
the metadata
+// report returns (nil, nil) — no error but no data — and RPC provides a valid
result.
+// The result should be cached and returned successfully.
+func TestGetMetadataInfo_ReportReturnsNil_RPCSucceeds(t *testing.T) {
Review Comment:
This test’s name/comment says “RPC succeeds” and “result should be cached”,
but the test actually asserts an error and does not exercise a successful RPC
fallback or caching. As written it only verifies that a (nil, nil) report
result triggers the fallback path and that the resulting error is not tagged as
a report error. Rename/update the comment so the test intent matches what it
actually validates (or alternatively implement a mock protocol so the RPC
fallback truly succeeds).
##########
registry/servicediscovery/service_instances_changed_listener_impl_test.go:
##########
@@ -372,3 +374,216 @@ func (c *capturingNotifyListener) NotifyAll(events
[]*registry.ServiceEvent, cal
callback()
}
}
+
+func TestGetMetadataInfo_CacheKeyFormat(t *testing.T) {
+ // Ensure cache is initialized (normally done by
NewServiceInstancesChangedListener)
+ _ = NewServiceInstancesChangedListener(testApp, constant.DefaultKey,
gxset.NewSet(testApp))
+
+ revision := "rev-cache-key-test"
+ // Pre-populate cache with the expected composite key
+ expectedKey := testApp + ":" + constant.DefaultKey + ":" + revision
+ expectedMeta := newTestMetadataInfo(t, revision, 20000, "dev")
+ metaCache.Set(expectedKey, expectedMeta)
+ t.Cleanup(func() {
+ metaCache.Delete(expectedKey)
+ })
+
+ instance := ®istry.DefaultServiceInstance{
+ ID: "127.0.0.1:20000",
+ ServiceName: testApp,
+ Host: "127.0.0.1",
+ Port: 20000,
+ Enable: true,
+ Healthy: true,
+ Metadata: map[string]string{
+ constant.ExportedServicesRevisionPropertyName: revision,
+ constant.ServiceInstanceEndpoints:
`[{"port":20000,"protocol":"tri"}]`,
+ },
+ }
+
+ // Should hit the cache and return the pre-populated metadata
+ meta, err := GetMetadataInfo(testApp, instance, revision,
constant.DefaultKey)
+ assert.NoError(t, err)
+ assert.Equal(t, expectedMeta, meta)
+}
+
+func TestGetMetadataInfo_LocalStorageGoesDirectlyToRPC(t *testing.T) {
+ // Ensure cache is initialized
+ _ = NewServiceInstancesChangedListener(testApp, constant.DefaultKey,
gxset.NewSet(testApp))
+
+ // Instance with no MetadataStorageTypePropertyName (i.e. local/default
path)
+ // should go directly to RPC without touching the metadata report.
+ // RPC will fail with a URL error because there are no URL params —
that's
+ // enough to confirm the correct branch was taken.
+ instance := ®istry.DefaultServiceInstance{
+ ID: "127.0.0.1:20003",
+ ServiceName: testApp,
+ Host: "127.0.0.1",
+ Port: 20003,
+ Enable: true,
+ Healthy: true,
+ Metadata: map[string]string{
+ constant.ExportedServicesRevisionPropertyName:
"rev-local-rpc",
+ // MetadataStorageTypePropertyName intentionally absent
→ local path
+ constant.ServiceInstanceEndpoints:
`[{"port":20003,"protocol":"tri"}]`,
+ },
+ }
+
+ _, err := GetMetadataInfo(testApp, instance, "rev-local-rpc",
constant.DefaultKey)
+ require.Error(t, err)
+ // Must be a URL/RPC error, not a report error, confirming the local
path
+ // skips the report entirely and goes straight to RPC.
+ assert.Contains(t, err.Error(), "[Metadata-URL]",
+ "local storage path should go directly to RPC, not touch the
metadata report")
+}
+
+func TestGetMetadataInfo_FallbackToRPC(t *testing.T) {
+ // Ensure cache is initialized
+ _ = NewServiceInstancesChangedListener(testApp, constant.DefaultKey,
gxset.NewSet(testApp))
+
+ // remote storage type without a report registered → report will fail
+ // should fall through to RPC, which will fail with url error (no URL
params)
+ instance := ®istry.DefaultServiceInstance{
+ ID: "127.0.0.1:20002",
+ ServiceName: testApp,
+ Host: "127.0.0.1",
+ Port: 20002,
+ Enable: true,
+ Healthy: true,
+ Metadata: map[string]string{
+ constant.ExportedServicesRevisionPropertyName:
"rev-fallback-to-rpc",
+ constant.MetadataStorageTypePropertyName:
constant.RemoteMetadataStorageType,
+ constant.ServiceInstanceEndpoints:
`[{"port":20002,"protocol":"tri"}]`,
+ },
+ }
+
+ _, err := GetMetadataInfo(testApp, instance, "rev-fallback-to-rpc",
constant.DefaultKey)
+ require.Error(t, err)
+ // Both report and RPC fail: error should be a combined Fallback error
containing both causes.
+ // [Metadata-Fallback] prefix proves the fallback path was taken.
+ assert.Contains(t, err.Error(), "[Metadata-Fallback]",
+ "fallback path should produce a [Metadata-Fallback] error")
+ assert.Contains(t, err.Error(), "[Metadata-URL]",
+ "fallback error should include the RPC/URL failure cause")
+}
+
+// TestGetMetadataInfo_ReportReturnsNil_RPCSucceeds verifies the path where
the metadata
+// report returns (nil, nil) — no error but no data — and RPC provides a valid
result.
+// The result should be cached and returned successfully.
+func TestGetMetadataInfo_ReportReturnsNil_RPCSucceeds(t *testing.T) {
+ const regID = "report-nil-rpc-ok"
+ const revision = "rev-report-nil-rpc-ok"
+
+ // Register a mock report that returns (nil, nil) — success with no
data.
+ mockReport := new(listenerMockMetadataReport)
+ extension.SetMetadataReportFactory(regID, func()
metadatareport.MetadataReportFactory {
+ return mockReport
+ })
+ opts := metadata.NewReportOptions(
+ metadata.WithRegistryId(regID),
+ metadata.WithProtocol(regID),
+ metadata.WithAddress("127.0.0.1"),
+ )
+ require.NoError(t, opts.Init())
+ t.Cleanup(metadata.ClearMetadataReportInstances)
+ t.Cleanup(func() {
+ if metaCache != nil {
+ metaCache.Delete(testApp + ":" + regID + ":" + revision)
+ }
+ })
+
+ mockReport.On("GetAppMetadata").Return((*info.MetadataInfo)(nil),
nil).Once()
+
+ // Pre-populate the cache with metadata so the RPC path (which would
normally
+ // fail with a URL error) instead hits the cache via
GetMetadataFromRpc's own
+ // internal path — but GetMetadataFromRpc doesn't use the cache.
+ // Instead, provide a valid RPC endpoint by pre-seeding the cache at a
key
+ // the RPC path doesn't use, and rely on the fact that the instance
below has
+ // no URL params → RPC will fail. We instead test via the cache
shortcut:
+ // seed the cache after the report-nil trigger so the fallback to RPC is
+ // confirmed by the error shape.
+ //
+ // The cleanest proof of this path uses a real mock protocol. Since that
+ // requires more wiring, we verify the fallback was attempted by
confirming
+ // the returned error comes from the RPC layer (not the report layer).
+ instance := ®istry.DefaultServiceInstance{
+ ID: "127.0.0.1:20098",
+ ServiceName: testApp,
+ Host: "127.0.0.1",
+ Port: 20098,
+ Enable: true,
+ Healthy: true,
+ Metadata: map[string]string{
+ constant.ExportedServicesRevisionPropertyName: revision,
+ constant.MetadataStorageTypePropertyName:
constant.RemoteMetadataStorageType,
+ // No URL params — RPC will fail at the
URL-construction stage
+ },
+ }
+ _ = NewServiceInstancesChangedListener(testApp, regID,
gxset.NewSet(testApp))
+
+ _, err := GetMetadataInfo(testApp, instance, revision, regID)
+ require.Error(t, err)
+ // The report returned nil (no error), so the fallback was triggered.
+ // RPC then fails because no URL params are present.
+ // The error must mention the nil-metadata fallback, not a report error.
+ assert.Contains(t, err.Error(), "[Metadata-Fallback]",
+ "nil report result should trigger fallback and produce a
[Metadata-Fallback] error")
+ assert.NotContains(t, err.Error(), "[Metadata-Report]",
+ "error must not look like a report error — the report returned
nil, not an error")
+ mockReport.AssertExpectations(t)
+}
+
+// TestGetMetadataInfo_RPCReturnsNilMetadata verifies that when RPC succeeds
but returns
+// nil metadata, GetMetadataInfo returns a descriptive [Metadata-RPC] error
rather than
+// silently caching and returning nil.
+func TestGetMetadataInfo_RPCReturnsNilMetadata(t *testing.T) {
+ // This path is exercised via the cache: pre-seed with a typed nil
*info.MetadataInfo.
+ // GetMetadataInfo will hit the cache fast-path, get nil, and the
caller (OnEvent) handles
+ // it. The nil guard on lines 298-301 and 311-313 is exercised when RPC
itself returns nil.
+ // Since controlling what GetMetadataFromRpc returns requires a mock
protocol (separate
+ // package), we verify the guard indirectly: the cache fast-path with a
nil entry must
+ // NOT trigger a panic, and the caller must receive (nil, nil) so it
can skip gracefully.
+ _ = NewServiceInstancesChangedListener(testApp, constant.DefaultKey,
gxset.NewSet(testApp))
Review Comment:
This test is named/claimed to verify “RPC returns nil metadata”, but it
never performs an RPC call— it only stores a typed nil *info.MetadataInfo in
metaCache and asserts it doesn’t panic. That’s a useful regression test, but
the current name/comment are misleading and don’t validate the new nil-metadata
guard in the RPC result path. Rename/update the comment to reflect the cache
typed-nil behavior (or add a protocol mock to make RPC actually return nil).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
