Skip to content

Commit c1969b6

Browse files
rolandgroenrolandgroen
committed
Merge branch 'main' into feat/exclude-admin-directories
* main: fix: #241 mCSD update client caches admin directories in memory indefinitely (#242) # Conflicts: # component/mcsd/component.go
2 parents cc2778d + df71fea commit c1969b6

File tree

5 files changed

+293
-28
lines changed

5 files changed

+293
-28
lines changed

‎component/mcsd/component.go‎

Lines changed: 46 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -73,6 +73,7 @@ type administrationDirectory struct {
7373
fhirBaseURL string
7474
resourceTypes []string
7575
discover bool
76+
sourceURL string // The fullUrl from the Bundle entry that created this Endpoint, used for unregistration on DELETE
7677
}
7778

7879
type DirectoryUpdateReport struct {
@@ -95,7 +96,7 @@ func New(config Config) (*Component, error) {
9596
updateMux: &sync.RWMutex{},
9697
}
9798
for _, rootDirectory := range config.AdministrationDirectories {
98-
if err := result.registerAdministrationDirectory(context.Background(), rootDirectory.FHIRBaseURL, rootDirectoryResourceTypes, true); err != nil {
99+
if err := result.registerAdministrationDirectory(context.Background(), rootDirectory.FHIRBaseURL, rootDirectoryResourceTypes, true, ""); err != nil {
99100
return nil, fmt.Errorf("register root administration directory (url=%s): %w", rootDirectory.FHIRBaseURL, err)
100101
}
101102
}
@@ -125,8 +126,7 @@ func (c *Component) RegisterHttpHandlers(publicMux, internalMux *http.ServeMux)
125126
})
126127
}
127128

128-
func (c *Component) registerAdministrationDirectory(ctx context.Context, fhirBaseURL string, resourceTypes []string, discover bool) error {
129-
129+
func (c *Component) registerAdministrationDirectory(ctx context.Context, fhirBaseURL string, resourceTypes []string, discover bool, sourceURL string) error {
130130
// Must be a valid http or https URL
131131
parsedFHIRBaseURL, err := url.Parse(fhirBaseURL)
132132
if err != nil {
@@ -156,11 +156,40 @@ func (c *Component) registerAdministrationDirectory(ctx context.Context, fhirBas
156156
resourceTypes: resourceTypes,
157157
fhirBaseURL: fhirBaseURL,
158158
discover: discover,
159+
sourceURL: sourceURL,
159160
})
160161
log.Ctx(ctx).Info().Str("fhir_server", fhirBaseURL).Msgf("Registered mCSD Directory (discover=%v)", discover)
161162
return nil
162163
}
163164

165+
// unregisterAdministrationDirectory removes an administration directory from the list by its fullUrl.
166+
// This is called when an Endpoint is deleted to prevent it from being fetched in future updates.
167+
// The fullUrl parameter is the Bundle entry fullUrl that was used when the Endpoint was registered.
168+
func (c *Component) unregisterAdministrationDirectory(ctx context.Context, fullUrl string) {
169+
initialCount := len(c.administrationDirectories)
170+
c.administrationDirectories = slices.DeleteFunc(c.administrationDirectories, func(dir administrationDirectory) bool {
171+
return dir.sourceURL == fullUrl
172+
})
173+
if len(c.administrationDirectories) < initialCount {
174+
log.Ctx(ctx).Info().Str("full_url", fullUrl).Msg("Unregistered mCSD Directory after Endpoint deletion")
175+
}
176+
}
177+
178+
// processEndpointDeletes processes DELETE operations for Endpoints and unregisters them from administrationDirectories.
179+
// This prevents deleted Endpoints from being fetched in future updates, fixing issue #241.
180+
func (c *Component) processEndpointDeletes(ctx context.Context, entries []fhir.BundleEntry) {
181+
for _, entry := range entries {
182+
if entry.Request != nil && entry.Request.Method == fhir.HTTPVerbDELETE && entry.FullUrl != nil {
183+
parts := strings.Split(entry.Request.Url, "/")
184+
if len(parts) >= 2 && parts[0] == "Endpoint" {
185+
// Unregister the administration directory using the fullUrl
186+
// The fullUrl uniquely identifies the resource that was deleted
187+
c.unregisterAdministrationDirectory(ctx, *entry.FullUrl)
188+
}
189+
}
190+
}
191+
}
192+
164193
func (c *Component) update(ctx context.Context) (UpdateReport, error) {
165194
c.updateMux.Lock()
166195
defer c.updateMux.Unlock()
@@ -232,6 +261,11 @@ func (c *Component) updateFromDirectory(ctx context.Context, fhirBaseURLRaw stri
232261
// _history can return multiple versions of the same resource, but transaction bundles must have unique resources
233262
deduplicatedEntries := deduplicateHistoryEntries(entries)
234263

264+
// Pre-process Endpoint DELETEs to unregister administration directories
265+
if allowDiscovery {
266+
c.processEndpointDeletes(ctx, deduplicatedEntries)
267+
}
268+
235269
// Build transaction with deterministic conditional references
236270
tx := fhir.Bundle{
237271
Type: fhir.BundleTypeTransaction,
@@ -251,6 +285,8 @@ func (c *Component) updateFromDirectory(ctx context.Context, fhirBaseURLRaw stri
251285
report.Warnings = append(report.Warnings, fmt.Sprintf("entry #%d: %s", i, err.Error()))
252286
continue
253287
}
288+
289+
// Handle Endpoint discovery and registration
254290
if allowDiscovery && resourceType == "Endpoint" && entry.Resource != nil {
255291
var endpoint fhir.Endpoint
256292
if err := json.Unmarshal(entry.Resource, &endpoint); err != nil {
@@ -264,8 +300,14 @@ func (c *Component) updateFromDirectory(ctx context.Context, fhirBaseURLRaw stri
264300
}
265301

266302
if coding.CodablesIncludesCode(endpoint.PayloadType, payloadCoding) {
303+
// Use the fullUrl from the Bundle entry to track this Endpoint
304+
// The fullUrl uniquely identifies the resource and can be used for later unregistration
305+
if entry.FullUrl == nil {
306+
report.Warnings = append(report.Warnings, fmt.Sprintf("entry #%d: Endpoint missing fullUrl", i))
307+
continue
308+
}
267309
log.Ctx(ctx).Debug().Msgf("Discovered mCSD Directory: %s", endpoint.Address)
268-
err := c.registerAdministrationDirectory(ctx, endpoint.Address, directoryResourceTypes, false)
310+
err := c.registerAdministrationDirectory(ctx, endpoint.Address, directoryResourceTypes, false, *entry.FullUrl)
269311
if err != nil {
270312
report.Warnings = append(report.Warnings, fmt.Sprintf("entry #%d: failed to register discovered mCSD Directory at %s: %s", i, endpoint.Address, err.Error()))
271313
}

‎component/mcsd/component_test.go‎

Lines changed: 164 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -78,8 +78,10 @@ func TestComponent_update_regression(t *testing.T) {
7878

7979
require.NoError(t, err)
8080
require.NotNil(t, report)
81-
assert.Empty(t, report[server.URL].Warnings)
82-
assert.NotNil(t, report[server.URL].Warnings, "expected an empty slice")
81+
// Expect warnings about Location resources (root directories only support Organization and Endpoint)
82+
// The regression data contains Location resources which should produce warnings
83+
assert.NotEmpty(t, report[server.URL].Warnings, "expected warnings about Location resources not being allowed")
84+
assert.Contains(t, report[server.URL].Warnings[0], "Location not allowed", "expected warning about Location not being allowed in root directory")
8385
assert.Empty(t, report[server.URL].Errors)
8486
assert.NotNil(t, report[server.URL].Errors, "expected an empty slice")
8587
}
@@ -582,7 +584,11 @@ func TestComponent_updateFromDirectory(t *testing.T) {
582584
defer server.Close()
583585

584586
capturingClient := &test.StubFHIRClient{}
585-
component, err := New(Config{})
587+
component, err := New(Config{
588+
QueryDirectory: DirectoryConfig{
589+
FHIRBaseURL: "http://example.com/local/fhir",
590+
},
591+
})
586592
require.NoError(t, err)
587593

588594
component.fhirClientFn = func(baseURL *url.URL) fhirclient.Client {
@@ -604,6 +610,149 @@ func TestComponent_updateFromDirectory(t *testing.T) {
604610
orgs := capturingClient.CreatedResources["Organization"]
605611
require.Len(t, orgs, 0, "Should have 0 Organizations after deduplication (DELETE is most recent operation)")
606612
})
613+
614+
t.Run("handles DELETE operations for Endpoints and unregisters from administrationDirectories", func(t *testing.T) {
615+
// This test verifies that when an Endpoint is deleted (DELETE operation in _history),
616+
// it is properly removed from the query directory and unregistered from administrationDirectories.
617+
// This fixes issue #241 where deleted Endpoints were cached indefinitely.
618+
619+
ctx := context.Background()
620+
621+
// Create test data with an Endpoint that will be deleted
622+
initialBundle := `{
623+
"resourceType": "Bundle",
624+
"type": "history",
625+
"entry": [{
626+
"fullUrl": "http://test.example.org/fhir/Endpoint/test-endpoint",
627+
"resource": {
628+
"resourceType": "Endpoint",
629+
"id": "test-endpoint",
630+
"status": "active",
631+
"payloadType": [{
632+
"coding": [{
633+
"system": "http://nuts-foundation.github.io/nl-generic-functions-ig/CodeSystem/nl-gf-data-exchange-capabilities",
634+
"code": "http://nuts-foundation.github.io/nl-generic-functions-ig/CapabilityStatement/nl-gf-admin-directory-update-client"
635+
}]
636+
}],
637+
"address": "https://directory.example.org/fhir"
638+
},
639+
"request": {
640+
"method": "POST",
641+
"url": "Endpoint/test-endpoint"
642+
}
643+
}]
644+
}`
645+
646+
// Create bundle with DELETE operation for the same Endpoint
647+
deleteBundle := `{
648+
"resourceType": "Bundle",
649+
"type": "history",
650+
"entry": [{
651+
"fullUrl": "http://test.example.org/fhir/Endpoint/test-endpoint",
652+
"request": {
653+
"method": "DELETE",
654+
"url": "Endpoint/test-endpoint"
655+
}
656+
}]
657+
}`
658+
659+
// Create a mock server that returns the initial bundle first, then the delete bundle
660+
callCount := 0
661+
mux := http.NewServeMux()
662+
server := httptest.NewServer(mux)
663+
defer server.Close()
664+
665+
mux.HandleFunc("/fhir/Endpoint/_history", func(w http.ResponseWriter, r *http.Request) {
666+
w.Header().Set("Content-Type", "application/json")
667+
if callCount == 0 {
668+
w.Write([]byte(initialBundle))
669+
} else {
670+
w.Write([]byte(deleteBundle))
671+
}
672+
callCount++
673+
})
674+
mux.HandleFunc("/fhir/Organization/_history", func(w http.ResponseWriter, r *http.Request) {
675+
w.Header().Set("Content-Type", "application/json")
676+
w.Write([]byte(`{"resourceType": "Bundle", "type": "history", "entry": []}`))
677+
})
678+
mux.HandleFunc("/fhir/Location/_history", func(w http.ResponseWriter, r *http.Request) {
679+
w.Header().Set("Content-Type", "application/json")
680+
w.Write([]byte(`{"resourceType": "Bundle", "type": "history", "entry": []}`))
681+
})
682+
mux.HandleFunc("/fhir/HealthcareService/_history", func(w http.ResponseWriter, r *http.Request) {
683+
w.Header().Set("Content-Type", "application/json")
684+
w.Write([]byte(`{"resourceType": "Bundle", "type": "history", "entry": []}`))
685+
})
686+
687+
component, err := New(Config{
688+
QueryDirectory: DirectoryConfig{
689+
FHIRBaseURL: "http://example.com/local/fhir",
690+
},
691+
AdministrationDirectories: map[string]DirectoryConfig{
692+
"test-dir": {
693+
FHIRBaseURL: server.URL + "/fhir",
694+
},
695+
},
696+
})
697+
require.NoError(t, err)
698+
699+
// Mock FHIR client that tracks operations
700+
capturingClient := &test.StubFHIRClient{}
701+
component.fhirClientFn = func(baseURL *url.URL) fhirclient.Client {
702+
if baseURL.String() == server.URL+"/fhir" {
703+
return fhirclient.New(baseURL, http.DefaultClient, &fhirclient.Config{UsePostSearch: false})
704+
}
705+
if baseURL.String() == "http://example.com/local/fhir" {
706+
return capturingClient
707+
}
708+
return &test.StubFHIRClient{Error: errors.New("unknown URL")}
709+
}
710+
711+
// First update - should discover and register the Endpoint
712+
report1, err := component.updateFromDirectory(ctx, server.URL+"/fhir", []string{"Endpoint"}, true)
713+
require.NoError(t, err)
714+
require.Empty(t, report1.Errors)
715+
require.Equal(t, 1, report1.CountCreated, "Should have created 1 Endpoint")
716+
717+
// Verify Endpoint was created in query directory
718+
require.NotNil(t, capturingClient.CreatedResources)
719+
require.Len(t, capturingClient.CreatedResources["Endpoint"], 1, "Endpoint should be created in query directory")
720+
721+
// Verify Endpoint was discovered and registered with correct fullUrl
722+
initialAdminDirCount := len(component.administrationDirectories)
723+
foundEndpoint := false
724+
var registeredFullUrl string
725+
for _, dir := range component.administrationDirectories {
726+
if dir.fhirBaseURL == "https://directory.example.org/fhir" {
727+
foundEndpoint = true
728+
registeredFullUrl = dir.sourceURL
729+
break
730+
}
731+
}
732+
assert.True(t, foundEndpoint, "Endpoint should be registered as administration directory")
733+
assert.Equal(t, "http://test.example.org/fhir/Endpoint/test-endpoint", registeredFullUrl, "Registered Endpoint should have fullUrl from Bundle entry")
734+
735+
// Second update - should process DELETE and unregister the Endpoint
736+
report2, err := component.updateFromDirectory(ctx, server.URL+"/fhir", []string{"Endpoint"}, true)
737+
require.NoError(t, err)
738+
require.Empty(t, report2.Errors)
739+
740+
// Verify DELETE was processed and Endpoint was unregistered
741+
afterDeleteCount := len(component.administrationDirectories)
742+
assert.Less(t, afterDeleteCount, initialAdminDirCount, "Deleted Endpoint should be unregistered")
743+
744+
deletedEndpointStillExists := false
745+
for _, dir := range component.administrationDirectories {
746+
if dir.fhirBaseURL == "https://directory.example.org/fhir" {
747+
deletedEndpointStillExists = true
748+
break
749+
}
750+
}
751+
assert.False(t, deletedEndpointStillExists, "Deleted Endpoint should not remain in administrationDirectories")
752+
753+
// Verify DELETE was sent to query directory
754+
assert.Equal(t, 1, report2.CountDeleted, "Should have 1 deleted resource")
755+
})
607756
}
608757

609758
func startMockServer(t *testing.T, filesToServe map[string]string) *httptest.Server {
@@ -639,7 +788,7 @@ func TestComponent_registerAdministrationDirectory(t *testing.T) {
639788
})
640789
require.NoError(t, err)
641790

642-
err = component.registerAdministrationDirectory(context.Background(), "http://example.com/fhir", []string{"Organization"}, false)
791+
err = component.registerAdministrationDirectory(context.Background(), "http://example.com/fhir", []string{"Organization"}, false, "")
643792

644793
require.NoError(t, err, "Should not error when URL is excluded, just skip registration")
645794
assert.Len(t, component.administrationDirectories, 0, "No directories should be registered")
@@ -654,7 +803,7 @@ func TestComponent_registerAdministrationDirectory(t *testing.T) {
654803
require.NoError(t, err)
655804

656805
// Try to register with trailing slash - should still be excluded
657-
err = component.registerAdministrationDirectory(context.Background(), "http://example.com/fhir/", []string{"Organization"}, false)
806+
err = component.registerAdministrationDirectory(context.Background(), "http://example.com/fhir/", []string{"Organization"}, false, "")
658807

659808
require.NoError(t, err, "Should not error when URL is excluded, just skip registration")
660809
assert.Len(t, component.administrationDirectories, 0, "No directories should be registered")
@@ -669,7 +818,7 @@ func TestComponent_registerAdministrationDirectory(t *testing.T) {
669818
require.NoError(t, err)
670819

671820
// Try to register without trailing slash - should still be excluded due to trimming
672-
err = component.registerAdministrationDirectory(context.Background(), "http://example.com/fhir", []string{"Organization"}, false)
821+
err = component.registerAdministrationDirectory(context.Background(), "http://example.com/fhir", []string{"Organization"}, false, "")
673822

674823
require.NoError(t, err, "Should not error when URL is excluded, just skip registration")
675824
assert.Len(t, component.administrationDirectories, 0, "No directories should be registered")
@@ -683,7 +832,7 @@ func TestComponent_registerAdministrationDirectory(t *testing.T) {
683832
})
684833
require.NoError(t, err)
685834

686-
err = component.registerAdministrationDirectory(context.Background(), "http://example.com/fhir/", []string{"Organization"}, false)
835+
err = component.registerAdministrationDirectory(context.Background(), "http://example.com/fhir/", []string{"Organization"}, false, "")
687836

688837
require.NoError(t, err, "Should not error when URL is excluded, just skip registration")
689838
assert.Len(t, component.administrationDirectories, 0, "No directories should be registered")
@@ -697,7 +846,7 @@ func TestComponent_registerAdministrationDirectory(t *testing.T) {
697846
})
698847
require.NoError(t, err)
699848

700-
err = component.registerAdministrationDirectory(context.Background(), "http://allowed.com/fhir", []string{"Organization"}, false)
849+
err = component.registerAdministrationDirectory(context.Background(), "http://allowed.com/fhir", []string{"Organization"}, false, "")
701850

702851
require.NoError(t, err)
703852
assert.Len(t, component.administrationDirectories, 1, "Directory should be registered")
@@ -717,7 +866,7 @@ func TestComponent_registerAdministrationDirectory(t *testing.T) {
717866
require.NoError(t, err)
718867

719868
// Try to register the same URL as admin directory - should be excluded
720-
err = component.registerAdministrationDirectory(context.Background(), ownFHIRBaseURL, []string{"Organization"}, true)
869+
err = component.registerAdministrationDirectory(context.Background(), ownFHIRBaseURL, []string{"Organization"}, true, "")
721870

722871
require.NoError(t, err, "Should not error when URL is excluded, just skip registration")
723872
assert.Len(t, component.administrationDirectories, 0, "Own directory should not be registered as admin directory")
@@ -734,12 +883,12 @@ func TestComponent_registerAdministrationDirectory(t *testing.T) {
734883
require.NoError(t, err)
735884

736885
// Try to register excluded directories
737-
err1 := component.registerAdministrationDirectory(context.Background(), "http://excluded1.com/fhir", []string{"Organization"}, false)
738-
err2 := component.registerAdministrationDirectory(context.Background(), "http://excluded2.com/fhir", []string{"Organization"}, false)
739-
err3 := component.registerAdministrationDirectory(context.Background(), "http://excluded3.com/fhir", []string{"Organization"}, false)
886+
err1 := component.registerAdministrationDirectory(context.Background(), "http://excluded1.com/fhir", []string{"Organization"}, false, "")
887+
err2 := component.registerAdministrationDirectory(context.Background(), "http://excluded2.com/fhir", []string{"Organization"}, false, "")
888+
err3 := component.registerAdministrationDirectory(context.Background(), "http://excluded3.com/fhir", []string{"Organization"}, false, "")
740889

741890
// Register an allowed directory
742-
err4 := component.registerAdministrationDirectory(context.Background(), "http://allowed.com/fhir", []string{"Organization"}, false)
891+
err4 := component.registerAdministrationDirectory(context.Background(), "http://allowed.com/fhir", []string{"Organization"}, false, "")
743892

744893
require.NoError(t, err1, "Should not error when URL is excluded, just skip registration")
745894
require.NoError(t, err2, "Should not error when URL is excluded, just skip registration")
@@ -754,7 +903,7 @@ func TestComponent_registerAdministrationDirectory(t *testing.T) {
754903
})
755904
require.NoError(t, err)
756905

757-
err = component.registerAdministrationDirectory(context.Background(), "http://example.com/fhir", []string{"Organization"}, false)
906+
err = component.registerAdministrationDirectory(context.Background(), "http://example.com/fhir", []string{"Organization"}, false, "")
758907

759908
require.NoError(t, err)
760909
assert.Len(t, component.administrationDirectories, 1, "Directory should be registered when exclusion list is empty")
@@ -769,7 +918,7 @@ func TestComponent_registerAdministrationDirectory(t *testing.T) {
769918
require.NoError(t, err)
770919

771920
// Invalid URL should return error, not silently skip
772-
err = component.registerAdministrationDirectory(context.Background(), "not-a-valid-url", []string{"Organization"}, false)
921+
err = component.registerAdministrationDirectory(context.Background(), "not-a-valid-url", []string{"Organization"}, false, "")
773922

774923
require.Error(t, err)
775924
assert.Contains(t, err.Error(), "invalid FHIR base URL")

0 commit comments

Comments
 (0)