From d815e2cb569f78208ae3112ea92cb8e8a1fd8875 Mon Sep 17 00:00:00 2001 From: Giordano Ricci Date: Tue, 20 Sep 2022 11:13:33 +0100 Subject: [PATCH] Correlations: Return 200 instead of 404 for empt correlation lists (#55242) * return 200 instead of 404 for empty correlations list * handle empty list response & improve consistency --- pkg/services/correlations/api.go | 6 +- pkg/services/correlations/database.go | 8 -- .../correlations/correlations_read_test.go | 16 +-- .../correlations/CorrelationsPage.test.tsx | 7 +- .../correlations/CorrelationsPage.tsx | 97 +++++++++---------- .../correlations/Forms/AddCorrelationForm.tsx | 25 ++--- .../Forms/EditCorrelationForm.tsx | 25 ++--- .../features/correlations/useCorrelations.ts | 10 +- 8 files changed, 82 insertions(+), 112 deletions(-) diff --git a/pkg/services/correlations/api.go b/pkg/services/correlations/api.go index 506345e10d7..fb68009faef 100644 --- a/pkg/services/correlations/api.go +++ b/pkg/services/correlations/api.go @@ -224,7 +224,7 @@ func (s *CorrelationsService) getCorrelationHandler(c *models.ReqContext) respon return response.Error(http.StatusNotFound, "Source data source not found", err) } - return response.Error(http.StatusInternalServerError, "Failed to update correlation", err) + return response.Error(http.StatusInternalServerError, "Failed to get correlation", err) } return response.JSON(http.StatusOK, correlation) @@ -270,7 +270,7 @@ func (s *CorrelationsService) getCorrelationsBySourceUIDHandler(c *models.ReqCon return response.Error(http.StatusNotFound, "Source data source not found", err) } - return response.Error(http.StatusInternalServerError, "Failed to update correlation", err) + return response.Error(http.StatusInternalServerError, "Failed to get correlations", err) } return response.JSON(http.StatusOK, correlations) @@ -309,7 +309,7 @@ func (s *CorrelationsService) getCorrelationsHandler(c *models.ReqContext) respo return response.Error(http.StatusNotFound, "No correlation found", err) } - return response.Error(http.StatusInternalServerError, "Failed to update correlation", err) + return response.Error(http.StatusInternalServerError, "Failed to get correlations", err) } return response.JSON(http.StatusOK, correlations) diff --git a/pkg/services/correlations/database.go b/pkg/services/correlations/database.go index 679a3224527..7d98a59c1a1 100644 --- a/pkg/services/correlations/database.go +++ b/pkg/services/correlations/database.go @@ -192,10 +192,6 @@ func (s CorrelationsService) getCorrelationsBySourceUID(ctx context.Context, cmd return []Correlation{}, err } - if len(correlations) == 0 { - return []Correlation{}, ErrCorrelationNotFound - } - return correlations, nil } @@ -209,10 +205,6 @@ func (s CorrelationsService) getCorrelations(ctx context.Context, cmd GetCorrela return []Correlation{}, err } - if len(correlations) == 0 { - return []Correlation{}, ErrCorrelationNotFound - } - return correlations, nil } diff --git a/pkg/tests/api/correlations/correlations_read_test.go b/pkg/tests/api/correlations/correlations_read_test.go index e37b18c428b..15b180412c0 100644 --- a/pkg/tests/api/correlations/correlations_read_test.go +++ b/pkg/tests/api/correlations/correlations_read_test.go @@ -42,21 +42,21 @@ func TestIntegrationReadCorrelation(t *testing.T) { t.Run("Get all correlations", func(t *testing.T) { // Running this here before creating a correlation in order to test this path. - t.Run("If no correlation exists it should return 404", func(t *testing.T) { + t.Run("If no correlation exists it should return 200", func(t *testing.T) { res := ctx.Get(GetParams{ url: "/api/datasources/correlations", user: adminUser, }) - require.Equal(t, http.StatusNotFound, res.StatusCode) + require.Equal(t, http.StatusOK, res.StatusCode) responseBody, err := io.ReadAll(res.Body) require.NoError(t, err) - var response errorResponseBody + var response []correlations.Correlation err = json.Unmarshal(responseBody, &response) require.NoError(t, err) - require.Equal(t, "No correlation found", response.Message) + require.Len(t, response, 0) require.NoError(t, res.Body.Close()) }) @@ -183,21 +183,21 @@ func TestIntegrationReadCorrelation(t *testing.T) { require.NoError(t, res.Body.Close()) }) - t.Run("If no correlation exists it should return 404", func(t *testing.T) { + t.Run("If no correlation exists it should return 200", func(t *testing.T) { res := ctx.Get(GetParams{ url: fmt.Sprintf("/api/datasources/uid/%s/correlations", dsWithoutCorrelations.Uid), user: adminUser, }) - require.Equal(t, http.StatusNotFound, res.StatusCode) + require.Equal(t, http.StatusOK, res.StatusCode) responseBody, err := io.ReadAll(res.Body) require.NoError(t, err) - var response errorResponseBody + var response []correlations.Correlation err = json.Unmarshal(responseBody, &response) require.NoError(t, err) - require.Equal(t, "No correlation found", response.Message) + require.Len(t, response, 0) require.NoError(t, res.Body.Close()) }) diff --git a/public/app/features/correlations/CorrelationsPage.test.tsx b/public/app/features/correlations/CorrelationsPage.test.tsx index ca693634920..b3ac6243f94 100644 --- a/public/app/features/correlations/CorrelationsPage.test.tsx +++ b/public/app/features/correlations/CorrelationsPage.test.tsx @@ -119,11 +119,8 @@ const renderWithContext = async ( }, fetch: (options: BackendSrvRequest) => { return new Observable((s) => { - if (correlations.length) { - s.next(merge(createFetchResponse({ url: options.url, data: correlations }))); - } else { - s.error(merge(createFetchError({ config: { url: options.url }, status: 404 }))); - } + s.next(merge(createFetchResponse({ url: options.url, data: correlations }))); + s.complete(); }); }, diff --git a/public/app/features/correlations/CorrelationsPage.tsx b/public/app/features/correlations/CorrelationsPage.tsx index 1d0be3684da..a8a17e2046c 100644 --- a/public/app/features/correlations/CorrelationsPage.tsx +++ b/public/app/features/correlations/CorrelationsPage.tsx @@ -4,6 +4,7 @@ import React, { memo, useCallback, useEffect, useMemo, useState } from 'react'; import { CellProps, SortByFn } from 'react-table'; import { GrafanaTheme2 } from '@grafana/data'; +import { isFetchError } from '@grafana/runtime'; import { Badge, Button, DeleteButton, HorizontalGroup, LoadingPlaceholder, useStyles2, Alert } from '@grafana/ui'; import { Page } from 'app/core/components/Page/Page'; import { contextSrv } from 'app/core/core'; @@ -14,7 +15,6 @@ import { AddCorrelationForm } from './Forms/AddCorrelationForm'; import { EditCorrelationForm } from './Forms/EditCorrelationForm'; import { EmptyCorrelationsCTA } from './components/EmptyCorrelationsCTA'; import { Column, Table } from './components/Table'; -import { RemoveCorrelationParams } from './types'; import { CorrelationData, useCorrelations } from './useCorrelations'; const sortDatasource: SortByFn = (a, b, column) => @@ -30,10 +30,13 @@ const loaderWrapper = css` export default function CorrelationsPage() { const navModel = useNavModel('correlations'); const [isAdding, setIsAdding] = useState(false); - const { remove, get } = useCorrelations(); + const { + remove, + get: { execute: fetchCorrelations, ...get }, + } = useCorrelations(); useEffect(() => { - get.execute(); + fetchCorrelations(); // we only want to fetch data on first render // eslint-disable-next-line react-hooks/exhaustive-deps }, []); @@ -41,21 +44,15 @@ export default function CorrelationsPage() { const canWriteCorrelations = contextSrv.hasPermission(AccessControlAction.DataSourcesWrite); const handleAdd = useCallback(() => { - get.execute(); + fetchCorrelations(); setIsAdding(false); - }, [get]); + }, [fetchCorrelations]); - const handleUpdate = useCallback(() => { - get.execute(); - }, [get]); - - const handleRemove = useCallback<(params: RemoveCorrelationParams) => void>( - async (correlation) => { - await remove.execute(correlation); - get.execute(); - }, - [remove, get] - ); + useEffect(() => { + if (!remove.error && !remove.loading && remove.value) { + fetchCorrelations(); + } + }, [remove.error, remove.loading, remove.value, fetchCorrelations]); const RowActions = useCallback( ({ @@ -69,11 +66,11 @@ export default function CorrelationsPage() { !readOnly && ( handleRemove({ sourceUID, uid })} + onConfirm={() => remove.execute({ sourceUID, uid })} closeOnConfirm /> ), - [handleRemove] + [remove] ); const columns = useMemo>>( @@ -107,7 +104,7 @@ export default function CorrelationsPage() { const data = useMemo(() => get.value, [get.value]); - const showEmptyListCTA = data?.length === 0 && !isAdding && (!get.error || get.error.status === 404); + const showEmptyListCTA = data?.length === 0 && !isAdding && !get.error; return ( @@ -126,42 +123,42 @@ export default function CorrelationsPage() { - {!data && get.loading && ( -
- -
- )} +
+ {!data && get.loading && ( +
+ +
+ )} - {showEmptyListCTA && setIsAdding(true)} />} + {showEmptyListCTA && setIsAdding(true)} />} - { - // This error is not actionable, it'd be nice to have a recovery button - get.error && get.error.status !== 404 && ( - - - {get.error.data.message || + { + // This error is not actionable, it'd be nice to have a recovery button + get.error && ( + + {(isFetchError(get.error) && get.error.data?.message) || 'An unknown error occurred while fetching correlation data. Please try again.'} - - - ) - } + + ) + } - {isAdding && setIsAdding(false)} onCreated={handleAdd} />} + {isAdding && setIsAdding(false)} onCreated={handleAdd} />} - {data && data.length >= 1 && ( - ( - - )} - columns={columns} - data={data} - getRowId={(correlation) => `${correlation.source.uid}-${correlation.uid}`} - /> - )} + {data && data.length >= 1 && ( +
( + + )} + columns={columns} + data={data} + getRowId={(correlation) => `${correlation.source.uid}-${correlation.uid}`} + /> + )} + ); diff --git a/public/app/features/correlations/Forms/AddCorrelationForm.tsx b/public/app/features/correlations/Forms/AddCorrelationForm.tsx index 21460a4d1bd..48c60350362 100644 --- a/public/app/features/correlations/Forms/AddCorrelationForm.tsx +++ b/public/app/features/correlations/Forms/AddCorrelationForm.tsx @@ -1,5 +1,5 @@ import { css } from '@emotion/css'; -import React, { useCallback } from 'react'; +import React, { useEffect } from 'react'; import { Controller } from 'react-hook-form'; import { DataSourceInstanceSettings, GrafanaTheme2 } from '@grafana/data'; @@ -47,17 +47,17 @@ const withDsUID = (fn: Function) => (ds: DataSourceInstanceSettings) => fn(ds.ui export const AddCorrelationForm = ({ onClose, onCreated }: Props) => { const styles = useStyles2(getStyles); - const { create } = useCorrelations(); + const { + create: { execute, loading, error, value }, + } = useCorrelations(); - const onSubmit = useCallback( - async (correlation) => { - await create.execute(correlation); + useEffect(() => { + if (!error && !loading && value) { onCreated(); - }, - [create, onCreated] - ); + } + }, [error, loading, value, onCreated]); - const { control, handleSubmit, register, errors } = useCorrelationForm({ onSubmit }); + const { control, handleSubmit, register, errors } = useCorrelationForm({ onSubmit: execute }); return ( @@ -108,12 +108,7 @@ export const AddCorrelationForm = ({ onClose, onCreated }: Props) => { - diff --git a/public/app/features/correlations/Forms/EditCorrelationForm.tsx b/public/app/features/correlations/Forms/EditCorrelationForm.tsx index 3dffdb6ccb7..1290b61177e 100644 --- a/public/app/features/correlations/Forms/EditCorrelationForm.tsx +++ b/public/app/features/correlations/Forms/EditCorrelationForm.tsx @@ -1,4 +1,4 @@ -import React, { useCallback } from 'react'; +import React, { useEffect } from 'react'; import { Button, HorizontalGroup } from '@grafana/ui'; @@ -15,17 +15,17 @@ interface Props { } export const EditCorrelationForm = ({ onUpdated, defaultValues, readOnly = false }: Props) => { - const { update } = useCorrelations(); + const { + update: { execute, loading, error, value }, + } = useCorrelations(); - const onSubmit = useCallback( - async (correlation) => { - await update.execute(correlation); + useEffect(() => { + if (!error && !loading && value) { onUpdated(); - }, - [update, onUpdated] - ); + } + }, [error, loading, value, onUpdated]); - const { handleSubmit, register } = useCorrelationForm({ onSubmit, defaultValues }); + const { handleSubmit, register } = useCorrelationForm({ onSubmit: execute, defaultValues }); return (
e.preventDefault() : handleSubmit}> @@ -35,12 +35,7 @@ export const EditCorrelationForm = ({ onUpdated, defaultValues, readOnly = false {!readOnly && ( - diff --git a/public/app/features/correlations/useCorrelations.ts b/public/app/features/correlations/useCorrelations.ts index 627fdd0f4c1..88aad32fb06 100644 --- a/public/app/features/correlations/useCorrelations.ts +++ b/public/app/features/correlations/useCorrelations.ts @@ -1,9 +1,8 @@ -import { useState } from 'react'; import { useAsyncFn } from 'react-use'; import { lastValueFrom } from 'rxjs'; import { DataSourceInstanceSettings } from '@grafana/data'; -import { getDataSourceSrv, FetchResponse, FetchError } from '@grafana/runtime'; +import { getDataSourceSrv, FetchResponse } from '@grafana/runtime'; import { useGrafana } from 'app/core/context/GrafanaContext'; import { Correlation, CreateCorrelationParams, RemoveCorrelationParams, UpdateCorrelationParams } from './types'; @@ -32,17 +31,13 @@ function getData(response: FetchResponse) { */ export const useCorrelations = () => { const { backend } = useGrafana(); - const [error, setError] = useState(null); const [getInfo, get] = useAsyncFn<() => Promise>( () => lastValueFrom( backend.fetch({ url: '/api/datasources/correlations', method: 'GET', showErrorAlert: false }) ) - .then(getData, (e) => { - setError(e); - return []; - }) + .then(getData) .then(toEnrichedCorrelationsData), [backend] ); @@ -78,7 +73,6 @@ export const useCorrelations = () => { get: { execute: get, ...getInfo, - error, }, remove: { execute: remove,