Skip to content

Commit 96ca580

Browse files
runcomruncom
committed
fix: remove mutexes around ownerinfo/rvinfo
Signed-off-by: Antonio Murdaca <[email protected]>
1 parent e3238a7 commit 96ca580

File tree

4 files changed

+225
-24
lines changed

4 files changed

+225
-24
lines changed

‎api/handlers/ownerinfo.go‎

Lines changed: 9 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -8,22 +8,20 @@ import (
88
"io"
99
"log/slog"
1010
"net/http"
11-
"sync"
1211

1312
"github.com/fido-device-onboard/go-fdo-server/internal/db"
1413
"gorm.io/gorm"
1514
)
1615

1716
func OwnerInfoHandler(w http.ResponseWriter, r *http.Request) {
18-
var mu sync.Mutex
1917
slog.Debug("Received OwnerInfo request", "method", r.Method, "path", r.URL.Path)
2018
switch r.Method {
2119
case http.MethodGet:
2220
getOwnerInfo(w, r)
2321
case http.MethodPost:
24-
createOwnerInfo(w, r, &mu)
22+
createOwnerInfo(w, r)
2523
case http.MethodPut:
26-
updateOwnerInfo(w, r, &mu)
24+
updateOwnerInfo(w, r)
2725
default:
2826
slog.Debug("Method not allowed", "method", r.Method, "path", r.URL.Path)
2927
http.Error(w, "Method not allowed", http.StatusMethodNotAllowed)
@@ -48,10 +46,7 @@ func getOwnerInfo(w http.ResponseWriter, _ *http.Request) {
4846
w.Write(ownerInfoJSON)
4947
}
5048

51-
func createOwnerInfo(w http.ResponseWriter, r *http.Request, mu *sync.Mutex) {
52-
mu.Lock()
53-
defer mu.Unlock()
54-
49+
func createOwnerInfo(w http.ResponseWriter, r *http.Request) {
5550
ownerInfo, err := io.ReadAll(r.Body)
5651
if err != nil {
5752
slog.Debug("Error reading body", "error", err)
@@ -70,6 +65,11 @@ func createOwnerInfo(w http.ResponseWriter, r *http.Request, mu *sync.Mutex) {
7065
}
7166

7267
if err := db.InsertOwnerInfo(ownerInfo); err != nil {
68+
if errors.Is(err, gorm.ErrDuplicatedKey) {
69+
slog.Debug("ownerInfo already exists (constraint)", "error", err)
70+
http.Error(w, "ownerInfo already exists", http.StatusConflict)
71+
return
72+
}
7373
slog.Debug("Error inserting ownerInfo", "error", err)
7474
http.Error(w, "Error inserting ownerInfo", http.StatusInternalServerError)
7575
return
@@ -82,10 +82,7 @@ func createOwnerInfo(w http.ResponseWriter, r *http.Request, mu *sync.Mutex) {
8282
w.Write(ownerInfo)
8383
}
8484

85-
func updateOwnerInfo(w http.ResponseWriter, r *http.Request, mu *sync.Mutex) {
86-
mu.Lock()
87-
defer mu.Unlock()
88-
85+
func updateOwnerInfo(w http.ResponseWriter, r *http.Request) {
8986
ownerInfo, err := io.ReadAll(r.Body)
9087
if err != nil {
9188
slog.Debug("Error reading body", "error", err)

‎api/handlers/rvinfo.go‎

Lines changed: 9 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -8,23 +8,21 @@ import (
88
"io"
99
"log/slog"
1010
"net/http"
11-
"sync"
1211

1312
"github.com/fido-device-onboard/go-fdo-server/internal/db"
1413
"gorm.io/gorm"
1514
)
1615

1716
func RvInfoHandler() http.HandlerFunc {
18-
var mu sync.Mutex
1917
return func(w http.ResponseWriter, r *http.Request) {
2018
slog.Debug("Received RV request", "method", r.Method, "path", r.URL.Path)
2119
switch r.Method {
2220
case http.MethodGet:
2321
getRvInfo(w, r)
2422
case http.MethodPost:
25-
createRvInfo(w, r, &mu)
23+
createRvInfo(w, r)
2624
case http.MethodPut:
27-
updateRvInfo(w, r, &mu)
25+
updateRvInfo(w, r)
2826
default:
2927
slog.Debug("Method not allowed", "method", r.Method, "path", r.URL.Path)
3028
http.Error(w, "Method not allowed", http.StatusMethodNotAllowed)
@@ -50,10 +48,7 @@ func getRvInfo(w http.ResponseWriter, _ *http.Request) {
5048
w.Write(rvInfoJSON)
5149
}
5250

53-
func createRvInfo(w http.ResponseWriter, r *http.Request, mu *sync.Mutex) {
54-
mu.Lock()
55-
defer mu.Unlock()
56-
51+
func createRvInfo(w http.ResponseWriter, r *http.Request) {
5752
rvInfo, err := io.ReadAll(r.Body)
5853
if err != nil {
5954
slog.Debug("Error reading body", "error", err)
@@ -72,6 +67,11 @@ func createRvInfo(w http.ResponseWriter, r *http.Request, mu *sync.Mutex) {
7267
}
7368

7469
if err := db.InsertRvInfo(rvInfo); err != nil {
70+
if errors.Is(err, gorm.ErrDuplicatedKey) {
71+
slog.Debug("rvInfo already exists (constraint)", "error", err)
72+
http.Error(w, "rvInfo already exists", http.StatusConflict)
73+
return
74+
}
7575
slog.Debug("Error inserting rvInfo", "error", err)
7676
http.Error(w, "Error inserting rvInfo", http.StatusInternalServerError)
7777
return
@@ -84,10 +84,7 @@ func createRvInfo(w http.ResponseWriter, r *http.Request, mu *sync.Mutex) {
8484
w.Write(rvInfo)
8585
}
8686

87-
func updateRvInfo(w http.ResponseWriter, r *http.Request, mu *sync.Mutex) {
88-
mu.Lock()
89-
defer mu.Unlock()
90-
87+
func updateRvInfo(w http.ResponseWriter, r *http.Request) {
9188
rvInfo, err := io.ReadAll(r.Body)
9289
if err != nil {
9390
slog.Debug("Error reading body", "error", err)

‎api/handlersTest/ownerinfo_test.go‎

Lines changed: 104 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,104 @@
1+
// SPDX-FileCopyrightText: (C) 2024 Intel Corporation
2+
// SPDX-License-Identifier: Apache 2.0
3+
package handlersTest
4+
5+
import (
6+
"bytes"
7+
"net/http"
8+
"net/http/httptest"
9+
"sync"
10+
"testing"
11+
12+
"github.com/fido-device-onboard/go-fdo-server/api/handlers"
13+
)
14+
15+
func TestOwnerInfo_PostConflictOnDuplicate(t *testing.T) {
16+
setupTestDB(t)
17+
18+
handler := http.HandlerFunc(handlers.OwnerInfoHandler)
19+
body := []byte(`[{"dns":"owner.example","port":"8082","protocol":"http"}]`)
20+
21+
var wg sync.WaitGroup
22+
wg.Add(2)
23+
statuses := make(chan int, 2)
24+
doPost := func() {
25+
defer wg.Done()
26+
req := httptest.NewRequest(http.MethodPost, "/api/v1/owner/redirect", bytes.NewReader(body))
27+
rec := httptest.NewRecorder()
28+
handler.ServeHTTP(rec, req)
29+
statuses <- rec.Code
30+
}
31+
go doPost()
32+
go doPost()
33+
wg.Wait()
34+
close(statuses)
35+
36+
var have201, have409 bool
37+
for s := range statuses {
38+
if s == http.StatusCreated {
39+
have201 = true
40+
}
41+
if s == http.StatusConflict {
42+
have409 = true
43+
}
44+
if s != http.StatusCreated && s != http.StatusConflict {
45+
t.Fatalf("unexpected status: %d", s)
46+
}
47+
}
48+
if !have201 || !have409 {
49+
t.Fatalf("expected one 201 and one 409, got have201=%v have409=%v", have201, have409)
50+
}
51+
}
52+
53+
func TestOwnerInfo_Put404ThenCreateThenUpdateAndGet(t *testing.T) {
54+
setupTestDB(t)
55+
56+
get := func() *httptest.ResponseRecorder {
57+
req := httptest.NewRequest(http.MethodGet, "/api/v1/owner/redirect", nil)
58+
rec := httptest.NewRecorder()
59+
handlers.OwnerInfoHandler(rec, req)
60+
return rec
61+
}
62+
put := func(body []byte) *httptest.ResponseRecorder {
63+
req := httptest.NewRequest(http.MethodPut, "/api/v1/owner/redirect", bytes.NewReader(body))
64+
rec := httptest.NewRecorder()
65+
handlers.OwnerInfoHandler(rec, req)
66+
return rec
67+
}
68+
post := func(body []byte) *httptest.ResponseRecorder {
69+
req := httptest.NewRequest(http.MethodPost, "/api/v1/owner/redirect", bytes.NewReader(body))
70+
rec := httptest.NewRecorder()
71+
handlers.OwnerInfoHandler(rec, req)
72+
return rec
73+
}
74+
75+
// PUT before create -> 404
76+
putBody := []byte(`[{"dns":"owner.example","port":"8082","protocol":"http"}]`)
77+
if rec := put(putBody); rec.Code != http.StatusNotFound {
78+
t.Fatalf("expected 404 on PUT before create, got %d", rec.Code)
79+
}
80+
81+
// POST create -> 201
82+
postBody := []byte(`[{"dns":"owner.example","port":"8082","protocol":"http"}]`)
83+
if rec := post(postBody); rec.Code != http.StatusCreated {
84+
t.Fatalf("expected 201 on POST create, got %d", rec.Code)
85+
}
86+
87+
// PUT update -> 200
88+
updateBody := []byte(`[{"dns":"owner-updated","port":"9090","protocol":"http"}]`)
89+
if rec := put(updateBody); rec.Code != http.StatusOK {
90+
t.Fatalf("expected 200 on PUT update, got %d", rec.Code)
91+
}
92+
93+
// GET -> updated value present
94+
rec := get()
95+
if rec.Code != http.StatusOK {
96+
t.Fatalf("expected 200 on GET, got %d", rec.Code)
97+
}
98+
if got := rec.Body.String(); got != string(updateBody) {
99+
t.Fatalf("expected body %q, got %q", string(updateBody), got)
100+
}
101+
102+
}
103+
104+

‎api/handlersTest/rvinfo_test.go‎

Lines changed: 103 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,103 @@
1+
// SPDX-FileCopyrightText: (C) 2024 Intel Corporation
2+
// SPDX-License-Identifier: Apache 2.0
3+
package handlersTest
4+
5+
import (
6+
"bytes"
7+
"net/http"
8+
"net/http/httptest"
9+
"sync"
10+
"testing"
11+
12+
"github.com/fido-device-onboard/go-fdo-server/api/handlers"
13+
)
14+
15+
func TestRvInfo_PostConflictOnDuplicate(t *testing.T) {
16+
setupTestDB(t)
17+
18+
handler := handlers.RvInfoHandler()
19+
body := []byte(`[{"dns":"rv.example","device_port":"8082","owner_port":"8082","protocol":"http"}]`)
20+
21+
var wg sync.WaitGroup
22+
wg.Add(2)
23+
statuses := make(chan int, 2)
24+
doPost := func() {
25+
defer wg.Done()
26+
req := httptest.NewRequest(http.MethodPost, "/api/v1/rv", bytes.NewReader(body))
27+
rec := httptest.NewRecorder()
28+
handler.ServeHTTP(rec, req)
29+
statuses <- rec.Code
30+
}
31+
go doPost()
32+
go doPost()
33+
wg.Wait()
34+
close(statuses)
35+
36+
var have201, have409 bool
37+
for s := range statuses {
38+
if s == http.StatusCreated {
39+
have201 = true
40+
}
41+
if s == http.StatusConflict {
42+
have409 = true
43+
}
44+
if s != http.StatusCreated && s != http.StatusConflict {
45+
t.Fatalf("unexpected status: %d", s)
46+
}
47+
}
48+
if !have201 || !have409 {
49+
t.Fatalf("expected one 201 and one 409, got have201=%v have409=%v", have201, have409)
50+
}
51+
}
52+
53+
func TestRvInfo_Put404ThenCreateThenUpdateAndGet(t *testing.T) {
54+
setupTestDB(t)
55+
56+
get := func() *httptest.ResponseRecorder {
57+
req := httptest.NewRequest(http.MethodGet, "/api/v1/rv", nil)
58+
rec := httptest.NewRecorder()
59+
handlers.RvInfoHandler()(rec, req)
60+
return rec
61+
}
62+
put := func(body []byte) *httptest.ResponseRecorder {
63+
req := httptest.NewRequest(http.MethodPut, "/api/v1/rv", bytes.NewReader(body))
64+
rec := httptest.NewRecorder()
65+
handlers.RvInfoHandler()(rec, req)
66+
return rec
67+
}
68+
post := func(body []byte) *httptest.ResponseRecorder {
69+
req := httptest.NewRequest(http.MethodPost, "/api/v1/rv", bytes.NewReader(body))
70+
rec := httptest.NewRecorder()
71+
handlers.RvInfoHandler()(rec, req)
72+
return rec
73+
}
74+
75+
// PUT before create -> 404
76+
putBody := []byte(`[{"dns":"rv.example","device_port":"8082","owner_port":"8082","protocol":"http"}]`)
77+
if rec := put(putBody); rec.Code != http.StatusNotFound {
78+
t.Fatalf("expected 404 on PUT before create, got %d", rec.Code)
79+
}
80+
81+
// POST create -> 201
82+
postBody := []byte(`[{"dns":"rv.example","device_port":"8082","owner_port":"8082","protocol":"http"}]`)
83+
if rec := post(postBody); rec.Code != http.StatusCreated {
84+
t.Fatalf("expected 201 on POST create, got %d", rec.Code)
85+
}
86+
87+
// PUT update -> 200
88+
updateBody := []byte(`[{"dns":"rv-updated","device_port":"9090","owner_port":"9090","protocol":"http"}]`)
89+
if rec := put(updateBody); rec.Code != http.StatusOK {
90+
t.Fatalf("expected 200 on PUT update, got %d", rec.Code)
91+
}
92+
93+
// GET -> updated value present
94+
rec := get()
95+
if rec.Code != http.StatusOK {
96+
t.Fatalf("expected 200 on GET, got %d", rec.Code)
97+
}
98+
if got := rec.Body.String(); got != string(updateBody) {
99+
t.Fatalf("expected body %q, got %q", string(updateBody), got)
100+
}
101+
}
102+
103+

0 commit comments

Comments
 (0)