From e93fc9c00332ecf2bd7db9b9d18392d101519f10 Mon Sep 17 00:00:00 2001 From: Andres Martinez Gotor Date: Tue, 15 Apr 2025 15:32:46 +0200 Subject: [PATCH] UserStorage: Improve error handling (#104025) --- .../src/utils/userStorage.test.tsx | 36 +++++++++++++------ .../grafana-runtime/src/utils/userStorage.tsx | 36 ++++++++++++++----- pkg/registry/apis/userstorage/register.go | 2 +- .../apis/userstorage/register_test.go | 2 +- 4 files changed, 55 insertions(+), 21 deletions(-) diff --git a/packages/grafana-runtime/src/utils/userStorage.test.tsx b/packages/grafana-runtime/src/utils/userStorage.test.tsx index 91fe2374508..6e267a83332 100644 --- a/packages/grafana-runtime/src/utils/userStorage.test.tsx +++ b/packages/grafana-runtime/src/utils/userStorage.test.tsx @@ -79,13 +79,16 @@ describe('userStorage', () => { it('creates a new user storage if it does not exist', async () => { request.mockReturnValueOnce(Promise.reject({ status: 404 } as FetchError)); + request.mockReturnValueOnce(Promise.resolve({ status: 200 } as FetchResponse)); const storage = usePluginUserStorage(); await storage.setItem('key', 'value'); - expect(request).toHaveBeenCalledWith({ - url: '/apis/userstorage.grafana.app/v0alpha1/namespaces/default/user-storage/plugin-id:abc', - method: 'GET', - showErrorAlert: false, - }); + expect(request).toHaveBeenCalledWith( + expect.objectContaining({ + url: '/apis/userstorage.grafana.app/v0alpha1/namespaces/default/user-storage/plugin-id:abc', + method: 'GET', + showErrorAlert: false, + }) + ); expect(request).toHaveBeenCalledWith( expect.objectContaining({ url: '/apis/userstorage.grafana.app/v0alpha1/namespaces/default/user-storage/', @@ -98,6 +101,17 @@ describe('userStorage', () => { }, }) ); + expect(localStorage.setItem).not.toHaveBeenCalled(); + }); + + it('falls back to localStorage if the user storage fails to be created', async () => { + // Get fails with not found + request.mockReturnValueOnce(Promise.reject({ status: 404 } as FetchError)); + // Create fails with forbidden + request.mockReturnValueOnce(Promise.reject({ status: 403 } as FetchError)); + const storage = usePluginUserStorage(); + await storage.setItem('key', 'value'); + expect(localStorage.setItem).toHaveBeenCalledWith('plugin-id:abc:key', 'value'); }); it('updates the user storage if it exists', async () => { @@ -109,11 +123,13 @@ describe('userStorage', () => { ); const storage = usePluginUserStorage(); await storage.setItem('key', 'new-value'); - expect(request).toHaveBeenCalledWith({ - url: '/apis/userstorage.grafana.app/v0alpha1/namespaces/default/user-storage/plugin-id:abc', - method: 'GET', - showErrorAlert: false, - }); + expect(request).toHaveBeenCalledWith( + expect.objectContaining({ + url: '/apis/userstorage.grafana.app/v0alpha1/namespaces/default/user-storage/plugin-id:abc', + method: 'GET', + showErrorAlert: false, + }) + ); expect(request).toHaveBeenCalledWith( expect.objectContaining({ url: '/apis/userstorage.grafana.app/v0alpha1/namespaces/default/user-storage/plugin-id:abc', diff --git a/packages/grafana-runtime/src/utils/userStorage.tsx b/packages/grafana-runtime/src/utils/userStorage.tsx index 245322e0a68..466bb5e8e7e 100644 --- a/packages/grafana-runtime/src/utils/userStorage.tsx +++ b/packages/grafana-runtime/src/utils/userStorage.tsx @@ -27,6 +27,7 @@ async function apiRequest(requestOptions: RequestOptions) { ...requestOptions, url: baseURL + requestOptions.url, data: requestOptions.body, + showErrorAlert: false, }) ); return { data: responseData, meta }; @@ -60,17 +61,19 @@ export class UserStorage { const userStorage = await apiRequest<{ spec: UserStorageSpec }>({ url: `/${this.resourceName}`, method: 'GET', - showErrorAlert: false, + manageError: (error) => { + if (get(error, 'status') === 404) { + this.storageSpec = null; + return { error: null }; + } + return { error }; + }, }); if ('error' in userStorage) { - if (get(userStorage, 'error.status') !== 404) { - console.error('Failed to get user storage', userStorage.error); - } - // No user storage found, return null - this.storageSpec = null; - } else { - this.storageSpec = userStorage.data.spec; + return userStorage.error; } + this.storageSpec = userStorage.data.spec; + return; } async getItem(key: string): Promise { @@ -96,7 +99,12 @@ export class UserStorage { const newData = { data: { [key]: value } }; // Ensure this.storageSpec is initialized - await this.init(); + const error = await this.init(); + if (error) { + // Fallback to localStorage + localStorage.setItem(`${this.resourceName}:${key}`, value); + return; + } if (!this.storageSpec) { // No user storage found, create a new one @@ -107,6 +115,11 @@ export class UserStorage { metadata: { name: this.resourceName, labels: { user: this.userUID, service: this.service } }, spec: newData, }, + manageError: (error) => { + // Fallback to localStorage + localStorage.setItem(`${this.resourceName}:${key}`, value); + return { error }; + }, }); this.storageSpec = newData; return; @@ -119,6 +132,11 @@ export class UserStorage { url: `/${this.resourceName}`, method: 'PATCH', body: { spec: newData }, + manageError: (error) => { + // Fallback to localStorage + localStorage.setItem(`${this.resourceName}:${key}`, value); + return { error }; + }, }); } } diff --git a/pkg/registry/apis/userstorage/register.go b/pkg/registry/apis/userstorage/register.go index 5da9b7c7da5..c09bf723d77 100644 --- a/pkg/registry/apis/userstorage/register.go +++ b/pkg/registry/apis/userstorage/register.go @@ -94,7 +94,7 @@ func (b *UserStorageAPIBuilder) GetAuthorizer() authorizer.Authorizer { switch attr.GetVerb() { case "create": // Create requests are validated later since we don't have access to the resource name - return authorizer.DecisionNoOpinion, "", nil + return authorizer.DecisionAllow, "", nil case "get", "delete", "patch", "update": // Only allow the user to access their own settings if !compareResourceNameAndUserUID(attr.GetName(), u) { diff --git a/pkg/registry/apis/userstorage/register_test.go b/pkg/registry/apis/userstorage/register_test.go index 885c62e9b95..ae96d89138c 100644 --- a/pkg/registry/apis/userstorage/register_test.go +++ b/pkg/registry/apis/userstorage/register_test.go @@ -43,7 +43,7 @@ func TestAuthorizer(t *testing.T) { requesterID: "123", objectName: "", verb: "create", - decision: authorizer.DecisionNoOpinion, + decision: authorizer.DecisionAllow, }, { name: "forbidden action",