From b2574e779791683ba2ed74b4662c067a0e8396af Mon Sep 17 00:00:00 2001 From: Naman Verma Date: Fri, 24 Jul 2026 16:24:47 +0530 Subject: [PATCH 1/4] fix: remove requirement for links to be always present (#12260) * fix: remove requirement for links to be always present * fix: make links use defined type * test: add integration test for link omission roundtrip * test: change not in to None for links in panel --------- Co-authored-by: Ashwin Bhatkal --- docs/api/openapi.yml | 4 ++-- .../api/generated/services/sigNoz.schemas.ts | 8 ++++---- .../dashboard/useCreateExportDashboard.ts | 1 - .../PanelEditor/ConfigPane/sectionRegistry.tsx | 2 +- .../drilldown/resolvePanelContextLinks.ts | 4 ++-- .../DrilldownMenu/DrilldownAggregateMenu.tsx | 4 ++-- .../DashboardContainer/patchOps.ts | 1 - .../NewDashboardModal/BlankDashboardPanel.tsx | 1 - .../dashboardtypes/perses_dashboard_data.go | 18 +----------------- .../dashboardtypes/perses_dashboard_test.go | 4 ++-- pkg/types/dashboardtypes/perses_replicas.go | 12 +----------- .../tests/dashboard/03_v2_dashboard.py | 8 ++++---- 12 files changed, 19 insertions(+), 48 deletions(-) diff --git a/docs/api/openapi.yml b/docs/api/openapi.yml index 2cf99067828..c4fcdf4da88 100644 --- a/docs/api/openapi.yml +++ b/docs/api/openapi.yml @@ -2748,6 +2748,7 @@ components: links: items: $ref: '#/components/schemas/DashboardtypesLink' + nullable: true type: array panels: additionalProperties: @@ -2764,7 +2765,6 @@ components: - variables - panels - layouts - - links type: object DashboardtypesDashboardView: properties: @@ -3401,6 +3401,7 @@ components: links: items: $ref: '#/components/schemas/DashboardtypesLink' + nullable: true type: array plugin: $ref: '#/components/schemas/DashboardtypesPanelPlugin' @@ -3412,7 +3413,6 @@ components: - display - plugin - queries - - links type: object DashboardtypesPatchOp: enum: diff --git a/frontend/src/api/generated/services/sigNoz.schemas.ts b/frontend/src/api/generated/services/sigNoz.schemas.ts index a4b98f371f0..c60763031ae 100644 --- a/frontend/src/api/generated/services/sigNoz.schemas.ts +++ b/frontend/src/api/generated/services/sigNoz.schemas.ts @@ -4626,9 +4626,9 @@ export interface DashboardtypesQueryDTO { export interface DashboardtypesPanelSpecDTO { display: DashboardtypesDisplayDTO; /** - * @type array + * @type array,null */ - links: DashboardtypesLinkDTO[]; + links?: DashboardtypesLinkDTO[] | null; plugin: DashboardtypesPanelPluginDTO; /** * @type array @@ -4821,9 +4821,9 @@ export interface DashboardtypesDashboardSpecDTO { */ layouts: DashboardtypesLayoutDTO[]; /** - * @type array + * @type array,null */ - links: DashboardtypesLinkDTO[]; + links?: DashboardtypesLinkDTO[] | null; /** * @type object */ diff --git a/frontend/src/hooks/dashboard/useCreateExportDashboard.ts b/frontend/src/hooks/dashboard/useCreateExportDashboard.ts index ecc5864e806..f135247889a 100644 --- a/frontend/src/hooks/dashboard/useCreateExportDashboard.ts +++ b/frontend/src/hooks/dashboard/useCreateExportDashboard.ts @@ -55,7 +55,6 @@ export function useCreateExportDashboard({ layouts: [], panels: {}, variables: [], - links: [], }, }), { diff --git a/frontend/src/pages/DashboardPageV2/DashboardContainer/PanelEditor/ConfigPane/sectionRegistry.tsx b/frontend/src/pages/DashboardPageV2/DashboardContainer/PanelEditor/ConfigPane/sectionRegistry.tsx index 746ff03f71a..bfc592d82f7 100644 --- a/frontend/src/pages/DashboardPageV2/DashboardContainer/PanelEditor/ConfigPane/sectionRegistry.tsx +++ b/frontend/src/pages/DashboardPageV2/DashboardContainer/PanelEditor/ConfigPane/sectionRegistry.tsx @@ -120,7 +120,7 @@ export const SECTION_REGISTRY: { [SectionKind.ContextLinks]: { Component: ContextLinksSection, // Panel-level slice (spec.links), not under the plugin spec — no cast needed. - get: (spec): DashboardtypesLinkDTO[] => spec.links, + get: (spec): DashboardtypesLinkDTO[] => spec.links || [], update: (spec, links): PanelSpec => ({ ...spec, links }), }, // One editor for every threshold variant (label / comparison / table); the kind's diff --git a/frontend/src/pages/DashboardPageV2/DashboardContainer/Panels/utils/drilldown/resolvePanelContextLinks.ts b/frontend/src/pages/DashboardPageV2/DashboardContainer/Panels/utils/drilldown/resolvePanelContextLinks.ts index 58843eaa165..c56a0a7cc16 100644 --- a/frontend/src/pages/DashboardPageV2/DashboardContainer/Panels/utils/drilldown/resolvePanelContextLinks.ts +++ b/frontend/src/pages/DashboardPageV2/DashboardContainer/Panels/utils/drilldown/resolvePanelContextLinks.ts @@ -1,4 +1,4 @@ -import type { DashboardtypesLinkDTO } from 'api/generated/services/sigNoz.schemas'; +import type { DashboardtypesPanelSpecDTO } from 'api/generated/services/sigNoz.schemas'; import { resolveTexts } from 'hooks/dashboard/useContextVariables'; import { resolveContextLinkUrl } from './resolveContextLinkUrl'; @@ -15,7 +15,7 @@ export interface ResolvedDrilldownLink { * label + URL, drops links without a URL, and skips substitution when `renderVariables === false`. */ export function resolvePanelContextLinks( - links: DashboardtypesLinkDTO[] | undefined, + links: DashboardtypesPanelSpecDTO['links'], processedVariables: Record, ): ResolvedDrilldownLink[] { const usable = (links ?? []).filter((link) => !!link.url); diff --git a/frontend/src/pages/DashboardPageV2/DashboardContainer/PanelsAndSectionsLayout/Panel/DrilldownMenu/DrilldownAggregateMenu.tsx b/frontend/src/pages/DashboardPageV2/DashboardContainer/PanelsAndSectionsLayout/Panel/DrilldownMenu/DrilldownAggregateMenu.tsx index b2f2deab384..56326af3457 100644 --- a/frontend/src/pages/DashboardPageV2/DashboardContainer/PanelsAndSectionsLayout/Panel/DrilldownMenu/DrilldownAggregateMenu.tsx +++ b/frontend/src/pages/DashboardPageV2/DashboardContainer/PanelsAndSectionsLayout/Panel/DrilldownMenu/DrilldownAggregateMenu.tsx @@ -8,7 +8,7 @@ import { ScrollText, } from '@signozhq/icons'; import logEvent from 'api/common/logEvent'; -import type { DashboardtypesLinkDTO } from 'api/generated/services/sigNoz.schemas'; +import type { DashboardtypesPanelSpecDTO } from 'api/generated/services/sigNoz.schemas'; import { getAggregateColumnHeader } from 'container/QueryTable/Drilldown/drilldownUtils'; import ContextMenu from 'periscope/components/ContextMenu'; import type { DrilldownContext } from 'pages/DashboardPageV2/DashboardContainer/Panels/types/drilldown'; @@ -29,7 +29,7 @@ interface DrilldownAggregateMenuProps { /** While dashboard variables resolve, the actions show a spinner and are disabled. */ isResolving?: boolean; /** Panel's context links; resolved against the clicked point + variables here. */ - links: DashboardtypesLinkDTO[] | undefined; + links: DashboardtypesPanelSpecDTO['links']; /** Whether the clicked point exposes group-by fields to bind to dashboard variables. */ canSetDashboardVariables: boolean; onViewLogs: () => void; diff --git a/frontend/src/pages/DashboardPageV2/DashboardContainer/patchOps.ts b/frontend/src/pages/DashboardPageV2/DashboardContainer/patchOps.ts index 58bd49f63d3..fcf137bb0b9 100644 --- a/frontend/src/pages/DashboardPageV2/DashboardContainer/patchOps.ts +++ b/frontend/src/pages/DashboardPageV2/DashboardContainer/patchOps.ts @@ -49,7 +49,6 @@ export function createDefaultPanel( spec: pluginSpec, } as DashboardtypesPanelPluginDTO, queries, - links: [], }, }; } diff --git a/frontend/src/pages/DashboardsListPageV2/components/NewDashboardModal/BlankDashboardPanel.tsx b/frontend/src/pages/DashboardsListPageV2/components/NewDashboardModal/BlankDashboardPanel.tsx index 445154f7fe0..7eca2197a2f 100644 --- a/frontend/src/pages/DashboardsListPageV2/components/NewDashboardModal/BlankDashboardPanel.tsx +++ b/frontend/src/pages/DashboardsListPageV2/components/NewDashboardModal/BlankDashboardPanel.tsx @@ -62,7 +62,6 @@ function BlankDashboardPanel({ onClose }: Props): JSX.Element { layouts: [], panels: {}, variables: [], - links: [], }, }); void logEvent(DashboardListEvents.DashboardCreated, { diff --git a/pkg/types/dashboardtypes/perses_dashboard_data.go b/pkg/types/dashboardtypes/perses_dashboard_data.go index d163e612430..589d884e661 100644 --- a/pkg/types/dashboardtypes/perses_dashboard_data.go +++ b/pkg/types/dashboardtypes/perses_dashboard_data.go @@ -26,7 +26,7 @@ type DashboardSpec struct { Layouts []Layout `json:"layouts" required:"true" nullable:"false"` Duration common.DurationString `json:"duration"` RefreshInterval common.DurationString `json:"refreshInterval"` - Links []Link `json:"links" required:"true" nullable:"false"` + Links []Link `json:"links,omitzero"` } // ══════════════════════════════════════════════ @@ -45,16 +45,6 @@ func (d *DashboardSpec) UnmarshalJSON(data []byte) error { return d.Validate() } -// validateLinks rejects a missing/null spec.links value: a typed client must -// send [] rather than omitting links, so its value round-trips faithfully. -// Panel links are the panel spec's concern, validated in validatePanels. -func (d *DashboardSpec) validateLinks() error { - if d.Links == nil { - return errors.NewInvalidInputf(ErrCodeDashboardInvalidInput, "spec.links is required; send [] when there are no links") - } - return nil -} - // ══════════════════════════════════════════════ // Cross-field validation // ══════════════════════════════════════════════ @@ -63,9 +53,6 @@ func (d *DashboardSpec) Validate() error { if err := d.Display.Validate("dashboard", "spec.display.name"); err != nil { return err } - if err := d.validateLinks(); err != nil { - return err - } if err := d.validateVariables(); err != nil { return err } @@ -117,9 +104,6 @@ func (d *DashboardSpec) validatePanels() error { if err := panel.Spec.Display.Validate("panel", path+".spec.display.name"); err != nil { return err } - if err := panel.Spec.validateLinks(path); err != nil { - return err - } panelKind := panel.Spec.Plugin.Kind if len(panel.Spec.Queries) != 1 { return errors.NewInvalidInputf(ErrCodeDashboardInvalidInput, "%s.spec.queries: panel must have one query", path) diff --git a/pkg/types/dashboardtypes/perses_dashboard_test.go b/pkg/types/dashboardtypes/perses_dashboard_test.go index 0ebbd34189e..d4a21650e7d 100644 --- a/pkg/types/dashboardtypes/perses_dashboard_test.go +++ b/pkg/types/dashboardtypes/perses_dashboard_test.go @@ -77,8 +77,8 @@ func TestUnmarshalErrorPreservesNestedMessage(t *testing.T) { } func TestValidateEmptySpec(t *testing.T) { - // no variables no panels - data := []byte(`{"links": []}`) + // no variables no panels no links + data := []byte(`{}`) _, err := unmarshalDashboard(data) assert.NoError(t, err, "expected valid") } diff --git a/pkg/types/dashboardtypes/perses_replicas.go b/pkg/types/dashboardtypes/perses_replicas.go index f9ed1b233c6..90bf8f2c01b 100644 --- a/pkg/types/dashboardtypes/perses_replicas.go +++ b/pkg/types/dashboardtypes/perses_replicas.go @@ -78,17 +78,7 @@ type PanelSpec struct { Display Display `json:"display" required:"true"` Plugin PanelPlugin `json:"plugin" required:"true"` Queries []Query `json:"queries" required:"true" nullable:"false"` - Links []Link `json:"links" required:"true" nullable:"false"` -} - -// validateLinks rejects a missing/null links field, where path is the panel's -// location (e.g. "spec.panels."). A typed client must send [] rather than -// omitting links, so its value round-trips faithfully. -func (s *PanelSpec) validateLinks(path string) error { - if s.Links == nil { - return errors.NewInvalidInputf(ErrCodeDashboardInvalidInput, "%s.spec.links is required; send [] when there are no links", path) - } - return nil + Links []Link `json:"links,omitzero"` } // Link replicates dashboard.Link (Perses) so its zero-valued fields survive the diff --git a/tests/integration/tests/dashboard/03_v2_dashboard.py b/tests/integration/tests/dashboard/03_v2_dashboard.py index 3cd4175dbdf..76ddc550e24 100644 --- a/tests/integration/tests/dashboard/03_v2_dashboard.py +++ b/tests/integration/tests/dashboard/03_v2_dashboard.py @@ -1803,7 +1803,6 @@ def test_dashboard_v2_roundtrip_preserves_zero_values( "kind": "Panel", "spec": { "display": {"name": "promql"}, - "links": [], "plugin": {"kind": "signoz/TimeSeriesPanel", "spec": {}}, "queries": [ { @@ -1919,9 +1918,10 @@ def threshold(panel_id: str) -> dict: for description, spec, key in absent_cases: assert key not in spec, description - # links is a required, non-nullable field: an explicit [] round-trips as [], - # so a typed client always reads a concrete array (never null or absent). - assert panels["timeseries"]["spec"]["links"] == [], "panel links round-trip as []" + # links is optional: an explicit [] round-trips as [], while an omitted + # links reads back as null. + assert panels["timeseries"]["spec"]["links"] == [], "explicit panel links round-trip as []" + assert panels["promql"]["spec"]["links"] is None, "omitted panel links read back as null" finally: requests.delete( signoz.self.host_configs["8080"].get(f"{BASE_URL}/{dashboard_id}"), From 9f5ad1ba267f42abedc5b09faa5340da89ca56a9 Mon Sep 17 00:00:00 2001 From: Swapnil Nakade Date: Fri, 24 Jul 2026 16:26:33 +0530 Subject: [PATCH 2/4] fix: list gcp accounts was missing config (#12175) * fix: list gcp accounts was missing config * refactor: updating cloudintegration integration tests * refactor: moving variables to test file * chore: removing verbose comments --- pkg/types/cloudintegrationtypes/account.go | 7 + tests/fixtures/cloudintegrations.py | 47 +++++- .../tests/cloudintegrations/04_accounts.py | 148 +++++++++++++----- 3 files changed, 157 insertions(+), 45 deletions(-) diff --git a/pkg/types/cloudintegrationtypes/account.go b/pkg/types/cloudintegrationtypes/account.go index 95502f601df..21afedf9eb4 100644 --- a/pkg/types/cloudintegrationtypes/account.go +++ b/pkg/types/cloudintegrationtypes/account.go @@ -137,6 +137,13 @@ func NewAccountFromStorable(storableAccount *StorableCloudIntegration) (*Account return nil, err } account.Config.Azure = azureConfig + case CloudProviderTypeGCP: + gcpConfig := new(GCPAccountConfig) + err := json.Unmarshal([]byte(storableAccount.Config), gcpConfig) + if err != nil { + return nil, err + } + account.Config.GCP = gcpConfig } if storableAccount.LastAgentReport != nil { diff --git a/tests/fixtures/cloudintegrations.py b/tests/fixtures/cloudintegrations.py index 0e171fa4de8..3d4e4cf19d6 100644 --- a/tests/fixtures/cloudintegrations.py +++ b/tests/fixtures/cloudintegrations.py @@ -1,6 +1,7 @@ """Fixtures for cloud integration tests.""" from collections.abc import Callable +from dataclasses import dataclass, field from http import HTTPStatus import pytest @@ -20,6 +21,29 @@ logger = setup_logger(__name__) +# Per-provider config shape. +@dataclass(frozen=True) +class ProviderAccountSpec: + # provider slug used in the URL path and config key (e.g. "aws", "gcp"). + provider: str + # params for the account created by default. + initial_params: dict + # params for the config an update (PUT) test sends. + updated_params: dict + # params -> the provider-keyed `config` block for a POST/PUT body. + build_config: Callable[[dict], dict] + # params -> the full config block the API is expected to return under + # config[provider] on GET/list. This may differ from what build_config sends: + # e.g. AWS accepts deploymentRegion on POST but the API does not echo it back. + expected_config: Callable[[dict], dict] + # id shown in parametrized test names; defaults to the provider slug. + id: str = field(default="") + + def __post_init__(self) -> None: + if not self.id: + object.__setattr__(self, "id", self.provider) + + @pytest.fixture(scope="function") def deprecated_create_cloud_integration_account( request: pytest.FixtureRequest, @@ -97,19 +121,26 @@ def _create( cloud_provider: str = "aws", deployment_region: str = "us-east-1", regions: list[str] | None = None, + config: dict | None = None, ) -> dict: - if regions is None: - regions = ["us-east-1"] - - endpoint = f"/api/v1/cloud_integrations/{cloud_provider}/accounts" - - request_payload = { - "config": { + # `config`, when given, is the fully-formed provider-keyed config block + # (e.g. built via a ProviderAccountSpec.build_config) and is used as-is. + # Otherwise fall back to the AWS shape built from deployment_region/regions, + # preserving existing AWS callers. + if config is None: + if regions is None: + regions = ["us-east-1"] + config = { cloud_provider: { "deploymentRegion": deployment_region, "regions": regions, } - }, + } + + endpoint = f"/api/v1/cloud_integrations/{cloud_provider}/accounts" + + request_payload = { + "config": config, "credentials": { "sigNozApiURL": "https://test-deployment.test.signoz.cloud", "sigNozApiKey": "test-api-key-789", diff --git a/tests/integration/tests/cloudintegrations/04_accounts.py b/tests/integration/tests/cloudintegrations/04_accounts.py index 043f9ad4c4b..85f1120ea26 100644 --- a/tests/integration/tests/cloudintegrations/04_accounts.py +++ b/tests/integration/tests/cloudintegrations/04_accounts.py @@ -2,16 +2,60 @@ from collections.abc import Callable from http import HTTPStatus +import pytest import requests from fixtures import types from fixtures.auth import USER_ADMIN_EMAIL, USER_ADMIN_PASSWORD, add_license -from fixtures.cloudintegrations import simulate_agent_checkin +from fixtures.cloudintegrations import ( + ProviderAccountSpec, + simulate_agent_checkin, +) from fixtures.logger import setup_logger logger = setup_logger(__name__) -CLOUD_PROVIDER = "aws" +AWS_ACCOUNT_SPEC = ProviderAccountSpec( + provider="aws", + initial_params={"deployment_region": "us-east-1", "regions": ["us-east-1"]}, + updated_params={"deployment_region": "us-east-1", "regions": ["us-east-1", "us-west-2", "eu-west-1"]}, + build_config=lambda p: {"aws": {"deploymentRegion": p["deployment_region"], "regions": p["regions"]}}, + expected_config=lambda p: {"regions": p["regions"]}, +) + +GCP_ACCOUNT_SPEC = ProviderAccountSpec( + provider="gcp", + initial_params={ + "deployment_project_id": "signoz-test-project", + "deployment_region": "us-central1", + "project_ids": ["signoz-test-project"], + }, + updated_params={ + "deployment_project_id": "signoz-test-project", + "deployment_region": "us-central1", + "project_ids": ["signoz-test-project", "signoz-test-project-2"], + }, + build_config=lambda p: { + "gcp": { + "deploymentProjectId": p["deployment_project_id"], + "deploymentRegion": p["deployment_region"], + "projectIds": p["project_ids"], + } + }, + expected_config=lambda p: { + "deploymentProjectId": p["deployment_project_id"], + "deploymentRegion": p["deployment_region"], + "projectIds": p["project_ids"], + }, +) + +PROVIDER_ACCOUNT_SPECS = [AWS_ACCOUNT_SPEC, GCP_ACCOUNT_SPEC] + +provider_spec = pytest.mark.parametrize( + "spec", + PROVIDER_ACCOUNT_SPECS, + ids=[s.id for s in PROVIDER_ACCOUNT_SPECS], +) def test_apply_license( @@ -24,16 +68,18 @@ def test_apply_license( add_license(signoz, make_http_mocks, get_token) +@provider_spec def test_list_accounts_empty( signoz: types.SigNoz, create_user_admin: types.Operation, # pylint: disable=unused-argument get_token: Callable[[str, str], str], + spec: ProviderAccountSpec, ) -> None: """List accounts returns an empty list when no accounts have checked in.""" admin_token = get_token(USER_ADMIN_EMAIL, USER_ADMIN_PASSWORD) response = requests.get( - signoz.self.host_configs["8080"].get(f"/api/v1/cloud_integrations/{CLOUD_PROVIDER}/accounts"), + signoz.self.host_configs["8080"].get(f"/api/v1/cloud_integrations/{spec.provider}/accounts"), headers={"Authorization": f"Bearer {admin_token}"}, timeout=10, ) @@ -46,24 +92,30 @@ def test_list_accounts_empty( assert len(data["accounts"]) == 0, "accounts list should be empty when no accounts have checked in" +@provider_spec def test_list_accounts_after_checkin( signoz: types.SigNoz, create_user_admin: types.Operation, # pylint: disable=unused-argument get_token: Callable[[str, str], str], create_cloud_integration_account: Callable, + spec: ProviderAccountSpec, ) -> None: - """List accounts returns an account after it has checked in.""" + """List accounts returns an account, with its provider config, after check-in.""" admin_token = get_token(USER_ADMIN_EMAIL, USER_ADMIN_PASSWORD) - account = create_cloud_integration_account(admin_token, CLOUD_PROVIDER, regions=["us-east-1"]) + account = create_cloud_integration_account( + admin_token, + spec.provider, + config=spec.build_config(spec.initial_params), + ) account_id = account["id"] provider_account_id = str(uuid.uuid4()) - checkin = simulate_agent_checkin(signoz, admin_token, CLOUD_PROVIDER, account_id, provider_account_id) + checkin = simulate_agent_checkin(signoz, admin_token, spec.provider, account_id, provider_account_id) assert checkin.status_code == HTTPStatus.OK, f"Check-in failed: {checkin.text}" response = requests.get( - signoz.self.host_configs["8080"].get(f"/api/v1/cloud_integrations/{CLOUD_PROVIDER}/accounts"), + signoz.self.host_configs["8080"].get(f"/api/v1/cloud_integrations/{spec.provider}/accounts"), headers={"Authorization": f"Bearer {admin_token}"}, timeout=10, ) @@ -74,25 +126,31 @@ def test_list_accounts_after_checkin( found = next((a for a in data["accounts"] if a["id"] == account_id), None) assert found is not None, f"Account {account_id} should appear in list after check-in" assert found["providerAccountId"] == provider_account_id, "providerAccountId should match" - assert found["config"]["aws"]["regions"] == ["us-east-1"], "regions should match account config" + assert found["config"][spec.provider] == spec.expected_config(spec.initial_params), "config should match account config" assert found["agentReport"] is not None, "agentReport should be present after check-in" assert found["removedAt"] is None, "removedAt should be null for a live account" +@provider_spec def test_get_account( signoz: types.SigNoz, create_user_admin: types.Operation, # pylint: disable=unused-argument get_token: Callable[[str, str], str], create_cloud_integration_account: Callable, + spec: ProviderAccountSpec, ) -> None: - """Get a specific account by ID returns the account with correct fields.""" + """Get a specific account by ID returns the account with its provider config.""" admin_token = get_token(USER_ADMIN_EMAIL, USER_ADMIN_PASSWORD) - account = create_cloud_integration_account(admin_token, CLOUD_PROVIDER, regions=["us-east-1", "eu-west-1"]) + account = create_cloud_integration_account( + admin_token, + spec.provider, + config=spec.build_config(spec.initial_params), + ) account_id = account["id"] response = requests.get( - signoz.self.host_configs["8080"].get(f"/api/v1/cloud_integrations/{CLOUD_PROVIDER}/accounts/{account_id}"), + signoz.self.host_configs["8080"].get(f"/api/v1/cloud_integrations/{spec.provider}/accounts/{account_id}"), headers={"Authorization": f"Bearer {admin_token}"}, timeout=10, ) @@ -101,23 +159,22 @@ def test_get_account( data = response.json()["data"] assert data["id"] == account_id, "id should match" - assert data["config"]["aws"]["regions"] == [ - "us-east-1", - "eu-west-1", - ], "regions should match" + assert data["config"][spec.provider] == spec.expected_config(spec.initial_params), "config should match" assert data["removedAt"] is None, "removedAt should be null" +@provider_spec def test_get_account_not_found( signoz: types.SigNoz, create_user_admin: types.Operation, # pylint: disable=unused-argument get_token: Callable[[str, str], str], + spec: ProviderAccountSpec, ) -> None: """Get a non-existent account returns 404.""" admin_token = get_token(USER_ADMIN_EMAIL, USER_ADMIN_PASSWORD) response = requests.get( - signoz.self.host_configs["8080"].get(f"/api/v1/cloud_integrations/{CLOUD_PROVIDER}/accounts/{uuid.uuid4()}"), + signoz.self.host_configs["8080"].get(f"/api/v1/cloud_integrations/{spec.provider}/accounts/{uuid.uuid4()}"), headers={"Authorization": f"Bearer {admin_token}"}, timeout=10, ) @@ -125,46 +182,52 @@ def test_get_account_not_found( assert response.status_code == HTTPStatus.NOT_FOUND, f"Expected 404, got {response.status_code}" +@provider_spec def test_update_account( signoz: types.SigNoz, create_user_admin: types.Operation, # pylint: disable=unused-argument get_token: Callable[[str, str], str], create_cloud_integration_account: Callable, + spec: ProviderAccountSpec, ) -> None: """Update account config and verify the change is persisted via GET.""" admin_token = get_token(USER_ADMIN_EMAIL, USER_ADMIN_PASSWORD) - account = create_cloud_integration_account(admin_token, CLOUD_PROVIDER, regions=["us-east-1"]) + account = create_cloud_integration_account( + admin_token, + spec.provider, + config=spec.build_config(spec.initial_params), + ) account_id = account["id"] - checkin = simulate_agent_checkin(signoz, admin_token, CLOUD_PROVIDER, account_id, str(uuid.uuid4())) + checkin = simulate_agent_checkin(signoz, admin_token, spec.provider, account_id, str(uuid.uuid4())) assert checkin.status_code == HTTPStatus.OK, f"Check-in failed: {checkin.text}" - updated_regions = ["us-east-1", "us-west-2", "eu-west-1"] - response = requests.put( - signoz.self.host_configs["8080"].get(f"/api/v1/cloud_integrations/{CLOUD_PROVIDER}/accounts/{account_id}"), + signoz.self.host_configs["8080"].get(f"/api/v1/cloud_integrations/{spec.provider}/accounts/{account_id}"), headers={"Authorization": f"Bearer {admin_token}"}, - json={"config": {"aws": {"regions": updated_regions}}}, + json={"config": spec.build_config(spec.updated_params)}, timeout=10, ) assert response.status_code == HTTPStatus.NO_CONTENT, f"Expected 204, got {response.status_code}" get_response = requests.get( - signoz.self.host_configs["8080"].get(f"/api/v1/cloud_integrations/{CLOUD_PROVIDER}/accounts/{account_id}"), + signoz.self.host_configs["8080"].get(f"/api/v1/cloud_integrations/{spec.provider}/accounts/{account_id}"), headers={"Authorization": f"Bearer {admin_token}"}, timeout=10, ) assert get_response.status_code == HTTPStatus.OK - assert get_response.json()["data"]["config"]["aws"]["regions"] == updated_regions, "Regions should reflect the update" + assert get_response.json()["data"]["config"][spec.provider] == spec.expected_config(spec.updated_params), "Config should reflect the update" +@provider_spec def test_update_account_after_checkin_preserves_connected_status( signoz: types.SigNoz, create_user_admin: types.Operation, # pylint: disable=unused-argument get_token: Callable[[str, str], str], create_cloud_integration_account: Callable, + spec: ProviderAccountSpec, ) -> None: """Updating config after agent check-in must not remove the account from the connected list. @@ -175,17 +238,21 @@ def test_update_account_after_checkin_preserves_connected_status( admin_token = get_token(USER_ADMIN_EMAIL, USER_ADMIN_PASSWORD) # 1. Create account - account = create_cloud_integration_account(admin_token, CLOUD_PROVIDER, regions=["us-east-1"]) + account = create_cloud_integration_account( + admin_token, + spec.provider, + config=spec.build_config(spec.initial_params), + ) account_id = account["id"] provider_account_id = str(uuid.uuid4()) # 2. Agent checks in — sets account_id and last_agent_report - checkin = simulate_agent_checkin(signoz, admin_token, CLOUD_PROVIDER, account_id, provider_account_id) + checkin = simulate_agent_checkin(signoz, admin_token, spec.provider, account_id, provider_account_id) assert checkin.status_code == HTTPStatus.OK, f"Check-in failed: {checkin.text}" # 3. Verify the account appears in the connected list list_response = requests.get( - signoz.self.host_configs["8080"].get(f"/api/v1/cloud_integrations/{CLOUD_PROVIDER}/accounts"), + signoz.self.host_configs["8080"].get(f"/api/v1/cloud_integrations/{spec.provider}/accounts"), headers={"Authorization": f"Bearer {admin_token}"}, timeout=10, ) @@ -195,18 +262,17 @@ def test_update_account_after_checkin_preserves_connected_status( assert found_before is not None, "Account should be listed after check-in" # 4. Update account config - updated_regions = ["us-east-1", "us-west-2"] update_response = requests.put( - signoz.self.host_configs["8080"].get(f"/api/v1/cloud_integrations/{CLOUD_PROVIDER}/accounts/{account_id}"), + signoz.self.host_configs["8080"].get(f"/api/v1/cloud_integrations/{spec.provider}/accounts/{account_id}"), headers={"Authorization": f"Bearer {admin_token}"}, - json={"config": {"aws": {"regions": updated_regions}}}, + json={"config": spec.build_config(spec.updated_params)}, timeout=10, ) assert update_response.status_code == HTTPStatus.NO_CONTENT, f"Expected 204, got {update_response.status_code}" # 5. Verify the account still appears in the connected list with correct fields list_response = requests.get( - signoz.self.host_configs["8080"].get(f"/api/v1/cloud_integrations/{CLOUD_PROVIDER}/accounts"), + signoz.self.host_configs["8080"].get(f"/api/v1/cloud_integrations/{spec.provider}/accounts"), headers={"Authorization": f"Bearer {admin_token}"}, timeout=10, ) @@ -216,27 +282,33 @@ def test_update_account_after_checkin_preserves_connected_status( assert found_after is not None, "Account must still be listed after config update (account_id should not be reset)" assert found_after["providerAccountId"] == provider_account_id, "providerAccountId should be preserved after update" assert found_after["agentReport"] is not None, "agentReport should be preserved after update" - assert found_after["config"]["aws"]["regions"] == updated_regions, "Config should reflect the update" + assert found_after["config"][spec.provider] == spec.expected_config(spec.updated_params), "Config should reflect the update" assert found_after["removedAt"] is None, "removedAt should still be null" +@provider_spec def test_disconnect_account( signoz: types.SigNoz, create_user_admin: types.Operation, # pylint: disable=unused-argument get_token: Callable[[str, str], str], create_cloud_integration_account: Callable, + spec: ProviderAccountSpec, ) -> None: """Disconnect an account removes it from the connected list.""" admin_token = get_token(USER_ADMIN_EMAIL, USER_ADMIN_PASSWORD) - account = create_cloud_integration_account(admin_token, CLOUD_PROVIDER) + account = create_cloud_integration_account( + admin_token, + spec.provider, + config=spec.build_config(spec.initial_params), + ) account_id = account["id"] - checkin = simulate_agent_checkin(signoz, admin_token, CLOUD_PROVIDER, account_id, str(uuid.uuid4())) + checkin = simulate_agent_checkin(signoz, admin_token, spec.provider, account_id, str(uuid.uuid4())) assert checkin.status_code == HTTPStatus.OK, f"Check-in failed: {checkin.text}" response = requests.delete( - signoz.self.host_configs["8080"].get(f"/api/v1/cloud_integrations/{CLOUD_PROVIDER}/accounts/{account_id}"), + signoz.self.host_configs["8080"].get(f"/api/v1/cloud_integrations/{spec.provider}/accounts/{account_id}"), headers={"Authorization": f"Bearer {admin_token}"}, timeout=10, ) @@ -244,7 +316,7 @@ def test_disconnect_account( assert response.status_code == HTTPStatus.NO_CONTENT, f"Expected 204, got {response.status_code}" list_response = requests.get( - signoz.self.host_configs["8080"].get(f"/api/v1/cloud_integrations/{CLOUD_PROVIDER}/accounts"), + signoz.self.host_configs["8080"].get(f"/api/v1/cloud_integrations/{spec.provider}/accounts"), headers={"Authorization": f"Bearer {admin_token}"}, timeout=10, ) @@ -252,16 +324,18 @@ def test_disconnect_account( assert not any(a["id"] == account_id for a in accounts), "Disconnected account should not appear in the connected list" +@provider_spec def test_disconnect_account_idempotent( signoz: types.SigNoz, create_user_admin: types.Operation, # pylint: disable=unused-argument get_token: Callable[[str, str], str], + spec: ProviderAccountSpec, ) -> None: """Disconnect on a non-existent account ID returns 204 (blind update, no existence check).""" admin_token = get_token(USER_ADMIN_EMAIL, USER_ADMIN_PASSWORD) response = requests.delete( - signoz.self.host_configs["8080"].get(f"/api/v1/cloud_integrations/{CLOUD_PROVIDER}/accounts/{uuid.uuid4()}"), + signoz.self.host_configs["8080"].get(f"/api/v1/cloud_integrations/{spec.provider}/accounts/{uuid.uuid4()}"), headers={"Authorization": f"Bearer {admin_token}"}, timeout=10, ) From 973429a22f944bed7a71758effd7c4314d3bdb28 Mon Sep 17 00:00:00 2001 From: Tushar Vats Date: Fri, 24 Jul 2026 18:26:56 +0530 Subject: [PATCH 3/4] fix(logs): stop hasToken and hasAny/hasAll 500s on separator and big-int needles (#12261) hasToken(body, ) returned a 500 whenever the needle contained a token separator or whitespace (ClickHouse code 36). Validate the needle up front in conditionForHasToken (the single choke point for both legacy and JSON modes) and return a clean 400 pointing at CONTAINS instead. hasAny/hasAll on a legacy body array returned a 500 for a quoted integer needle >= 2^32: clickhouse-go binds it as a concrete Array(UInt64), which has no supertype with the Array(Nullable(Int64)) extraction that the function must unify (code 386). CAST the needle array to Array(Int64) to match the extraction. Scalar has() and the scalar OR-fallback are value-level and unaffected. --- pkg/telemetrylogs/condition_builder.go | 34 ++++++++++++++++-- .../filter_expr_logs_body_json_test.go | 17 +++++++++ pkg/telemetrylogs/filter_expr_logs_test.go | 13 +++++++ .../03_body_array_functions.py | 15 ++++---- .../querierlogs/09_json_body_functions.py | 35 +++++++++---------- 5 files changed, 85 insertions(+), 29 deletions(-) diff --git a/pkg/telemetrylogs/condition_builder.go b/pkg/telemetrylogs/condition_builder.go index 045629057ee..40c97cecfdc 100644 --- a/pkg/telemetrylogs/condition_builder.go +++ b/pkg/telemetrylogs/condition_builder.go @@ -89,7 +89,8 @@ func (c *conditionBuilder) conditionForArrayFunction( for i, v := range list { vals[i] = legacyCoerceNeedle(v, elemType) } - arrayCond := fmt.Sprintf("%s(%s, %s)", operator.FunctionName(), arrayExpr, sb.Var(vals)) + // Pin the needle array type to the haystack; scalar fallback below coerces value-level. + arrayCond := fmt.Sprintf("%s(%s, %s)", operator.FunctionName(), arrayExpr, castNeedleArray(elemType, sb.Var(vals))) if !hasScalar { return arrayCond, nil } @@ -113,6 +114,27 @@ func (c *conditionBuilder) conditionForArrayFunction( return fmt.Sprintf("(%s OR ifNull(%s, false))", arrayCond, sb.And(sb.E(scalarExpr, typedNeedle), scalarGuard)), nil } +// castNeedleArray pins an Int64 needle array to Array(Int64) so it matches the Array(Nullable(Int64)) +// haystack; without it a needle >= 2^32 binds as Array(UInt64) and hasAny/hasAll error (code 386). +func castNeedleArray(elemType telemetrytypes.FieldDataType, arg string) string { + if elemType == telemetrytypes.FieldDataTypeInt64 { + return fmt.Sprintf("CAST(%s AS Array(Int64))", arg) + } + return arg +} + +// firstTokenSeparator returns the first char of s that hasToken treats as a token separator +// (anything other than an ASCII letter or digit), and whether one was found. +func firstTokenSeparator(s string) (string, bool) { + for _, r := range s { + isAlphaNum := (r >= '0' && r <= '9') || (r >= 'a' && r <= 'z') || (r >= 'A' && r <= 'Z') + if !isAlphaNum { + return string(r), true + } + } + return "", false +} + // conditionForHasToken builds a hasToken full-text search over the body column, resolving the // column from the key name + use_json_body flag. func (c *conditionBuilder) conditionForHasToken( @@ -129,11 +151,19 @@ func (c *conditionBuilder) conditionForHasToken( } // hasToken matches string tokens only. - if _, ok := needle.(string); !ok { + needleStr, ok := needle.(string) + if !ok { return "", errors.NewInvalidInputf(errors.CodeInvalidInput, "function `hasToken` expects value parameter to be a string").WithUrl(hasTokenFunctionDocURL) } + // A multi-token needle makes CH hasToken error (code 36); reject up front as a 400. Both modes flow here. + if sep, found := firstTokenSeparator(needleStr); found { + return "", errors.NewInvalidInputf(errors.CodeInvalidInput, + "function `hasToken` matches a single whole token, but %q contains the separator %q; use a substring filter (e.g. `body CONTAINS '%s'`) to search across separators", + needleStr, sep, needleStr).WithUrl(hasTokenFunctionDocURL) + } + bodyJSONEnabled := c.fl.BooleanOrEmpty(ctx, flagger.FeatureUseJSONBody, featuretypes.NewFlaggerEvaluationContext(orgID)) if !bodyJSONEnabled { diff --git a/pkg/telemetrylogs/filter_expr_logs_body_json_test.go b/pkg/telemetrylogs/filter_expr_logs_body_json_test.go index bf2fd09da11..fed7735c99d 100644 --- a/pkg/telemetrylogs/filter_expr_logs_body_json_test.go +++ b/pkg/telemetrylogs/filter_expr_logs_body_json_test.go @@ -104,6 +104,23 @@ func TestFilterExprLogsBodyJSON(t *testing.T) { expectedArgs: []any{int64(200), int64(200)}, expectedErrorContains: "", }, + { + // Big-int needle CAST to Array(Int64) to match the haystack (else 386). + category: "json", + query: `hasAny(body.ids, ['9007199254740993', '9007199254740994'])`, + shouldPass: true, + expectedQuery: `WHERE (hasAny(JSONExtract(JSON_QUERY(body, '$."ids"[*]'), 'Array(Nullable(Int64))'), CAST(? AS Array(Int64))) OR ifNull((JSONExtract(JSON_VALUE(body, '$."ids"'), 'Nullable(Int64)') IN (?, ?) AND JSONType(body, 'ids') NOT IN ('Array', 'Object', 'Null')), false))`, + expectedArgs: []any{[]any{int64(9007199254740993), int64(9007199254740994)}, int64(9007199254740993), int64(9007199254740994)}, + expectedErrorContains: "", + }, + { + category: "json", + query: `hasAll(body.ids, ['9007199254740993', '9007199254740994'])`, + shouldPass: true, + expectedQuery: `WHERE (hasAll(JSONExtract(JSON_QUERY(body, '$."ids"[*]'), 'Array(Nullable(Int64))'), CAST(? AS Array(Int64))) OR ifNull(((JSONExtract(JSON_VALUE(body, '$."ids"'), 'Nullable(Int64)') = ? AND JSONExtract(JSON_VALUE(body, '$."ids"'), 'Nullable(Int64)') = ?) AND JSONType(body, 'ids') NOT IN ('Array', 'Object', 'Null')), false))`, + expectedArgs: []any{[]any{int64(9007199254740993), int64(9007199254740994)}, int64(9007199254740993), int64(9007199254740994)}, + expectedErrorContains: "", + }, { category: "json", query: "body.message = hello", diff --git a/pkg/telemetrylogs/filter_expr_logs_test.go b/pkg/telemetrylogs/filter_expr_logs_test.go index 3ea1a2612e5..0f783c084d0 100644 --- a/pkg/telemetrylogs/filter_expr_logs_test.go +++ b/pkg/telemetrylogs/filter_expr_logs_test.go @@ -1561,6 +1561,19 @@ func TestFilterExprLogs(t *testing.T) { expectedArgs: []any{"download"}, expectedErrorContains: "function `hasToken` expects value parameter to be a string", }, + // A multi-token needle (separator/whitespace) is a clean 400, not a CH execution error. + { + category: "hasTokenUnderscoreNeedle", + query: "hasToken(body, \"user_id\")", + shouldPass: false, + expectedErrorContains: "function `hasToken` matches a single whole token", + }, + { + category: "hasTokenWhitespaceNeedle", + query: "hasToken(body, \"production node\")", + shouldPass: false, + expectedErrorContains: "function `hasToken` matches a single whole token", + }, // extra / mis-shaped value arguments are rejected, not silently dropped. { category: "hasExtraArgs", diff --git a/tests/integration/tests/querier_json_body/03_body_array_functions.py b/tests/integration/tests/querier_json_body/03_body_array_functions.py index ad5ff4e6302..72fc50723b4 100644 --- a/tests/integration/tests/querier_json_body/03_body_array_functions.py +++ b/tests/integration/tests/querier_json_body/03_body_array_functions.py @@ -1178,12 +1178,11 @@ def test_logs_json_body_hastoken_separator_needle_errors( export_json_types: Callable[[list[Logs]], None], needle: str, ) -> None: - """KNOWN BUG (currently 500) — same defect as the flag-off path. In JSON mode hasToken searches - body.message via ClickHouse hasToken(LOWER(body.message), LOWER(?)), which rejects a needle - containing whitespace or separator characters (`.`/`_`/`-`/space) with error code 36. The needle - is passed straight through, so the query fails at execution and the raw error surfaces as a 500 - instead of a 400 / graceful fallback. Flip to 400 (or a fallback match) once the needle is - validated.""" + """Same defect as the flag-off path. In JSON mode hasToken searches body.message via + ClickHouse hasToken(LOWER(body.message), LOWER(?)), which rejects a needle containing + whitespace or separator characters (`.`/`_`/`-`/space) with error code 36 at execution. The + needle is validated up front, so a multi-token needle is a clean 400 (hasToken matches a single + whole token) rather than a raw execution 500.""" now = datetime.now(tz=UTC) logs = [ Logs(timestamp=now - timedelta(seconds=1), resources={"service.name": "app-service"}, body_v2=json.dumps({"message": "client 192.168.1.1 user_id error-code production node"}), body_promoted="", severity_text="INFO"), @@ -1199,8 +1198,8 @@ def test_logs_json_body_hastoken_separator_needle_errors( request_type=RequestType.RAW, queries=[build_raw_query("A", "logs", order=[build_order_by("timestamp")], limit=100, filter_expression=f"hasToken(body, '{needle}')")], ) - assert response.status_code == HTTPStatus.INTERNAL_SERVER_ERROR, f"{needle}: expected 500, got {response.status_code}: {response.text}" - assert "Needle must not contain whitespace or separator characters" in response.text, response.text + assert response.status_code == HTTPStatus.BAD_REQUEST, f"{needle}: expected 400, got {response.status_code}: {response.text}" + assert "matches a single whole token" in response.text, response.text def test_logs_json_body_unregistered_path_warns( diff --git a/tests/integration/tests/querierlogs/09_json_body_functions.py b/tests/integration/tests/querierlogs/09_json_body_functions.py index faea6fe8dd5..216e0fb6322 100644 --- a/tests/integration/tests/querierlogs/09_json_body_functions.py +++ b/tests/integration/tests/querierlogs/09_json_body_functions.py @@ -1197,13 +1197,11 @@ def test_logs_json_body_hastoken_separator_needle_errors( insert_logs: Callable[[list[Logs]], None], needle: str, ) -> None: - """KNOWN BUG (currently 500). hasToken maps to ClickHouse hasToken(LOWER(body), LOWER(?)), - which rejects a needle containing whitespace or separator characters (`.`/`_`/`-`/space) with - error code 36 ("Needle must not contain whitespace or separator characters"). The querier - passes the user needle straight through, so the query fails during execution and the raw CH - error surfaces as a 500 rather than a 400 / graceful fallback. Common needles like an IP, - `user_id` or a UUID hit this. Flip this to 400 (or a fallback match) once the needle is - validated.""" + """hasToken maps to ClickHouse hasToken(LOWER(body), LOWER(?)), which rejects a needle + containing whitespace or separator characters (`.`/`_`/`-`/space) with error code 36 at + execution. The querier validates the needle up front and returns a clean 400 (hasToken matches + a single whole token) instead of letting the raw CH error surface as a 500. Common needles like + an IP, `user_id` or a UUID hit this.""" now = datetime.now(tz=UTC) insert_logs( [ @@ -1219,8 +1217,8 @@ def test_logs_json_body_hastoken_separator_needle_errors( request_type=RequestType.RAW, queries=[build_raw_query("A", "logs", order=[build_order_by("timestamp")], limit=100, filter_expression=f"hasToken(body, '{needle}')")], ) - assert response.status_code == HTTPStatus.INTERNAL_SERVER_ERROR, f"{needle}: expected 500, got {response.status_code}: {response.text}" - assert "Needle must not contain whitespace or separator characters" in response.text, response.text + assert response.status_code == HTTPStatus.BAD_REQUEST, f"{needle}: expected 400, got {response.status_code}: {response.text}" + assert "matches a single whole token" in response.text, response.text @pytest.mark.parametrize( @@ -1230,20 +1228,18 @@ def test_logs_json_body_hastoken_separator_needle_errors( pytest.param("hasAll", id="hasall"), ], ) -def test_logs_json_body_hasany_hasall_large_quoted_int_errors( +def test_logs_json_body_hasany_hasall_large_quoted_int_matches( signoz: types.SigNoz, create_user_admin: None, # pylint: disable=unused-argument get_token: Callable[[str, str], str], insert_logs: Callable[[list[Logs]], None], func: str, ) -> None: - """KNOWN BUG (currently 500). A quoted integer needle >= 2^32 in hasAny/hasAll fails with - ClickHouse code 386 ("no supertype for types UInt64, Int64") -> 500. The body array is - extracted as Array(Nullable(Int64)), but the needle array is bound as Array(UInt64) for values - that don't fit UInt32, and CH has no Int64/UInt64 supertype at the array-vs-array type level. - Single has() is unaffected because scalar-vs-array coercion is value-level (asserted below as a - contrast). When fixed (bind the needle array as Int64) this should return 200 with an exact - match like has().""" + """A quoted integer needle >= 2^32 in hasAny/hasAll used to 500: the body array is extracted as + Array(Nullable(Int64)) but the needle array bound as a concrete Array(UInt64), and CH has no + Int64/UInt64 supertype at the array-vs-array level (code 386). The needle array is now CAST to + Array(Int64) to match the extraction, so it returns 200 with an exact match like has() (asserted + below as a contrast).""" now = datetime.now(tz=UTC) insert_logs( [ @@ -1254,7 +1250,7 @@ def test_logs_json_body_hasany_hasall_large_quoted_int_errors( start_ms = int((now - timedelta(seconds=10)).timestamp() * 1000) end_ms = int(now.timestamp() * 1000) - # hasAny/hasAll with a quoted big int (>= 2^32) -> 500 (no supertype). + # hasAny/hasAll with a quoted big int (>= 2^32) -> 200 with an exact match (needle CAST to Int64). response = make_query_request( signoz, token, @@ -1263,7 +1259,8 @@ def test_logs_json_body_hasany_hasall_large_quoted_int_errors( request_type=RequestType.RAW, queries=[build_raw_query("A", "logs", order=[build_order_by("timestamp")], limit=100, filter_expression=f"{func}(body.id, ['9007199254740993'])")], ) - assert response.status_code == HTTPStatus.INTERNAL_SERVER_ERROR, f"{func}: expected 500, got {response.status_code}: {response.text}" + assert response.status_code == HTTPStatus.OK, f"{func}: expected 200, got {response.status_code}: {response.text}" + assert len(get_rows(response)) == 1, f"{func} with a quoted big int should match exactly one log" # Contrast: single has() with the same quoted big int works (exact Int64 match, 1 row). response = make_query_request( From ecb77af4cb240a273b3bfceb13dd272af1a899af Mon Sep 17 00:00:00 2001 From: Naman Verma Date: Fri, 24 Jul 2026 21:07:04 +0530 Subject: [PATCH 4/4] chore: fix schema and validations based on migration errors (#12263) * chore: fix schema based on migration errors * test: add rejection based integration tests for new validations * fix: remove datasource field from schema --- docs/api/openapi.yml | 47 ---------- .../api/generated/services/sigNoz.schemas.ts | 56 ----------- .../dashboardtypes/perses_dashboard_data.go | 25 ++--- .../perses_dashboard_patch_test.go | 2 +- .../dashboardtypes/perses_dashboard_test.go | 94 +++++++++---------- pkg/types/dashboardtypes/perses_drift_test.go | 27 +++--- .../dashboardtypes/perses_plugin_wrappers.go | 52 ---------- pkg/types/dashboardtypes/perses_replicas.go | 21 ++--- .../dashboardtypes/perses_signoz_plugins.go | 15 --- pkg/types/dashboardtypes/testdata/perses.json | 11 +-- .../testdata/perses_with_sections.json | 9 -- .../tests/dashboard/03_v2_dashboard.py | 70 +++++++++++++- 12 files changed, 152 insertions(+), 277 deletions(-) diff --git a/docs/api/openapi.yml b/docs/api/openapi.yml index c4fcdf4da88..e0806fda6a6 100644 --- a/docs/api/openapi.yml +++ b/docs/api/openapi.yml @@ -1592,13 +1592,6 @@ components: required: - config type: object - CommonDisplay: - properties: - description: - type: string - name: - type: string - type: object CommonJSONRef: properties: $ref: @@ -2732,11 +2725,6 @@ components: type: object DashboardtypesDashboardSpec: properties: - datasources: - additionalProperties: - $ref: '#/components/schemas/DashboardtypesDatasourceSpec' - nullable: true - type: object display: $ref: '#/components/schemas/DashboardtypesDisplay' duration: @@ -2801,39 +2789,6 @@ components: required: - version type: object - DashboardtypesDatasourcePlugin: - discriminator: - mapping: - signoz/Datasource: '#/components/schemas/DashboardtypesDatasourcePluginVariantGithubComSigNozSignozPkgTypesDashboardtypesSigNozDatasourceSpec' - propertyName: kind - oneOf: - - $ref: '#/components/schemas/DashboardtypesDatasourcePluginVariantGithubComSigNozSignozPkgTypesDashboardtypesSigNozDatasourceSpec' - type: object - DashboardtypesDatasourcePluginKind: - enum: - - signoz/Datasource - type: string - DashboardtypesDatasourcePluginVariantGithubComSigNozSignozPkgTypesDashboardtypesSigNozDatasourceSpec: - properties: - kind: - enum: - - signoz/Datasource - type: string - spec: - $ref: '#/components/schemas/DashboardtypesSigNozDatasourceSpec' - required: - - kind - - spec - type: object - DashboardtypesDatasourceSpec: - properties: - default: - type: boolean - display: - $ref: '#/components/schemas/CommonDisplay' - plugin: - $ref: '#/components/schemas/DashboardtypesDatasourcePlugin' - type: object DashboardtypesDisplay: properties: description: @@ -3610,8 +3565,6 @@ components: required: - queryValue type: object - DashboardtypesSigNozDatasourceSpec: - type: object DashboardtypesSource: enum: - user diff --git a/frontend/src/api/generated/services/sigNoz.schemas.ts b/frontend/src/api/generated/services/sigNoz.schemas.ts index c60763031ae..fd47d519641 100644 --- a/frontend/src/api/generated/services/sigNoz.schemas.ts +++ b/frontend/src/api/generated/services/sigNoz.schemas.ts @@ -3236,17 +3236,6 @@ export interface CloudintegrationtypesUpdatableServiceDTO { config: CloudintegrationtypesServiceConfigDTO; } -export interface CommonDisplayDTO { - /** - * @type string - */ - description?: string; - /** - * @type string - */ - name?: string; -} - export interface CommonJSONRefDTO { /** * @type string @@ -3990,44 +3979,6 @@ export interface DashboardtypesDashboardPanelRefDTO { panelName: string; } -export enum DashboardtypesDatasourcePluginVariantGithubComSigNozSignozPkgTypesDashboardtypesSigNozDatasourceSpecDTOKind { - 'signoz/Datasource' = 'signoz/Datasource', -} -export interface DashboardtypesSigNozDatasourceSpecDTO { - [key: string]: unknown; -} - -export interface DashboardtypesDatasourcePluginVariantGithubComSigNozSignozPkgTypesDashboardtypesSigNozDatasourceSpecDTO { - /** - * @enum signoz/Datasource - * @type string - */ - kind: DashboardtypesDatasourcePluginVariantGithubComSigNozSignozPkgTypesDashboardtypesSigNozDatasourceSpecDTOKind; - spec: DashboardtypesSigNozDatasourceSpecDTO; -} - -export type DashboardtypesDatasourcePluginDTO = - DashboardtypesDatasourcePluginVariantGithubComSigNozSignozPkgTypesDashboardtypesSigNozDatasourceSpecDTO; - -export interface DashboardtypesDatasourceSpecDTO { - /** - * @type boolean - */ - default?: boolean; - display?: CommonDisplayDTO; - plugin?: DashboardtypesDatasourcePluginDTO; -} - -export type DashboardtypesDashboardSpecDTODatasourcesAnyOf = { - [key: string]: DashboardtypesDatasourceSpecDTO; -}; - -/** - * @nullable - */ -export type DashboardtypesDashboardSpecDTODatasources = - DashboardtypesDashboardSpecDTODatasourcesAnyOf | null; - export enum DashboardtypesPanelKindDTO { Panel = 'Panel', } @@ -4807,10 +4758,6 @@ export type DashboardtypesVariableDTO = | DashboardtypesVariableEnvelopeGithubComSigNozSignozPkgTypesDashboardtypesTextVariableSpecDTO; export interface DashboardtypesDashboardSpecDTO { - /** - * @type object,null - */ - datasources?: DashboardtypesDashboardSpecDTODatasources; display: DashboardtypesDisplayDTO; /** * @type string @@ -4886,9 +4833,6 @@ export interface DashboardtypesDashboardViewDTO { updatedAt?: string; } -export enum DashboardtypesDatasourcePluginKindDTO { - 'signoz/Datasource' = 'signoz/Datasource', -} export interface TagtypesGettableTagDTO { /** * @type string diff --git a/pkg/types/dashboardtypes/perses_dashboard_data.go b/pkg/types/dashboardtypes/perses_dashboard_data.go index 589d884e661..b56e9f72adc 100644 --- a/pkg/types/dashboardtypes/perses_dashboard_data.go +++ b/pkg/types/dashboardtypes/perses_dashboard_data.go @@ -17,16 +17,17 @@ import ( // DashboardSpec is the SigNoz dashboard v2 spec shape. It mirrors // dashboard.Spec (Perses) field-for-field, except every common.Plugin // occurrence is replaced with a typed SigNoz plugin whose OpenAPI schema is a -// per-site discriminated oneOf. +// per-site discriminated oneOf. Perses's datasources field is deliberately +// dropped: SigNoz never reads it (queries carry their own signal/source), so +// the drift test allowlists it as an intentional omission. type DashboardSpec struct { - Display Display `json:"display" required:"true"` - Datasources map[string]*DatasourceSpec `json:"datasources,omitzero"` - Variables []Variable `json:"variables" required:"true" nullable:"false"` - Panels map[string]*Panel `json:"panels" required:"true" nullable:"false"` - Layouts []Layout `json:"layouts" required:"true" nullable:"false"` - Duration common.DurationString `json:"duration"` - RefreshInterval common.DurationString `json:"refreshInterval"` - Links []Link `json:"links,omitzero"` + Display Display `json:"display" required:"true"` + Variables []Variable `json:"variables" required:"true" nullable:"false"` + Panels map[string]*Panel `json:"panels" required:"true" nullable:"false"` + Layouts []Layout `json:"layouts" required:"true" nullable:"false"` + Duration common.DurationString `json:"duration"` + RefreshInterval common.DurationString `json:"refreshInterval"` + Links []Link `json:"links,omitzero"` } // ══════════════════════════════════════════════ @@ -106,7 +107,7 @@ func (d *DashboardSpec) validatePanels() error { } panelKind := panel.Spec.Plugin.Kind if len(panel.Spec.Queries) != 1 { - return errors.NewInvalidInputf(ErrCodeDashboardInvalidInput, "%s.spec.queries: panel must have one query", path) + return errors.NewInvalidInputf(ErrCodeDashboardInvalidInput, "%s.spec.queries: panel must have one query, found %d", path, len(panel.Spec.Queries)) } allowed := allowedQueryKinds[panelKind] for qi, q := range panel.Spec.Queries { @@ -269,8 +270,8 @@ func (d *DashboardSpec) validateLayouts() error { return errors.NewInternalf(errors.CodeInternal, "spec.layouts[%d].spec: unexpected layout spec type %T", li, layout.Spec) } if grid.Display != nil { - if n := utf8.RuneCountInString(grid.Display.Title); n > MaxDisplayNameLen { - return errors.NewInvalidInputf(ErrCodeDashboardInvalidInput, "spec.layouts[%d].spec.display.title: layout name must be at most %d characters, got %d", li, MaxDisplayNameLen, n) + if n := utf8.RuneCountInString(grid.Display.Title); n > MaxLayoutTitleLen { + return errors.NewInvalidInputf(ErrCodeDashboardInvalidInput, "spec.layouts[%d].spec.display.title: layout name must be at most %d characters, got %d", li, MaxLayoutTitleLen, n) } } if err := validateGridLayoutGeometry(grid, li); err != nil { diff --git a/pkg/types/dashboardtypes/perses_dashboard_patch_test.go b/pkg/types/dashboardtypes/perses_dashboard_patch_test.go index 826ef6fdf02..1002ef011a8 100644 --- a/pkg/types/dashboardtypes/perses_dashboard_patch_test.go +++ b/pkg/types/dashboardtypes/perses_dashboard_patch_test.go @@ -30,7 +30,7 @@ const basePostableJSON = `{ "spec": { "name": "service", "allowAllValue": true, - "allowMultiple": false, + "allowMultiple": true, "plugin": { "kind": "signoz/DynamicVariable", "spec": {"name": "service.name", "signal": "metrics"} diff --git a/pkg/types/dashboardtypes/perses_dashboard_test.go b/pkg/types/dashboardtypes/perses_dashboard_test.go index d4a21650e7d..9a48213ed5f 100644 --- a/pkg/types/dashboardtypes/perses_dashboard_test.go +++ b/pkg/types/dashboardtypes/perses_dashboard_test.go @@ -44,7 +44,7 @@ func TestInvalidateNotAJSON(t *testing.T) { // TestUnmarshalErrorPreservesNestedMessage guards the wrap on dec.Decode in // DashboardSpec.UnmarshalJSON. The wrap stamps a consistent type/code on // decode failures, but must not smother the rich messages produced by nested -// UnmarshalJSON methods (panel/query/variable/datasource plugin envelopes). +// UnmarshalJSON methods (panel/query/variable plugin envelopes). func TestUnmarshalErrorPreservesNestedMessage(t *testing.T) { data := []byte(`{ "panels": { @@ -91,7 +91,7 @@ func TestValidateOnlyVariables(t *testing.T) { "spec": { "name": "service", "allowAllValue": true, - "allowMultiple": false, + "allowMultiple": true, "plugin": { "kind": "signoz/DynamicVariable", "spec": { @@ -237,6 +237,12 @@ func TestInvalidateListVariableCrossFields(t *testing.T) { assert.Contains(t, err.Error(), "customAllValue cannot be set") }) + t.Run("allowAllValue without allowMultiple", func(t *testing.T) { + _, err := unmarshalDashboard(listVar(`"allowAllValue": true, "allowMultiple": false,`)) + require.Error(t, err) + assert.Contains(t, err.Error(), "allowAllValue cannot be set") + }) + t.Run("list defaultValue without allowMultiple", func(t *testing.T) { _, err := unmarshalDashboard(listVar(`"allowAllValue": false, "allowMultiple": false, "defaultValue": ["a", "b"],`)) require.Error(t, err) @@ -441,20 +447,6 @@ func TestInvalidateUnknownPluginKind(t *testing.T) { }`, wantContain: "FakeVariable", }, - { - name: "unknown datasource plugin", - data: `{ - "datasources": { - "ds1": { - "default": true, - "plugin": {"kind": "FakeDatasource", "spec": {}} - } - }, - "links": [], - "layouts": [] - }`, - wantContain: "FakeDatasource", - }, } for _, tt := range tests { @@ -1739,55 +1731,61 @@ func TestInvalidateDuplicatePanelReference(t *testing.T) { assert.Contains(t, err.Error(), "spec.layouts[0].spec.items[1].content") } -// Every display name — dashboard, panel, variable — and the grid layout title is -// bounded at MaxDisplayNameLen. The name is one over the limit in each case, and -// the message reads ": name must be at most ...", pairing the -// locatable path (like the other spec errors) with a human field label. +// Every display name — dashboard, panel, variable — is bounded at MaxDisplayNameLen, +// while the grid layout title has its own, larger bound (MaxLayoutTitleLen). The name +// is one over the relevant limit in each case, and the message reads ": +// name must be at most ...", pairing the locatable path (like the other spec +// errors) with a human field label. func TestInvalidateDisplayNameTooLong(t *testing.T) { - tooLong := strings.Repeat("x", MaxDisplayNameLen+1) - lengthMsg := fmt.Sprintf("must be at most %d characters, got %d", MaxDisplayNameLen, MaxDisplayNameLen+1) - testCases := []struct { - scenario string - dashboardJSON string - expectedPath string - expectedLabel string + scenario string + limit int + dashboardJSONFmt string + expectedPath string + expectedLabel string }{ { - scenario: "dashboard display name", - dashboardJSON: `{"display": {"name": "` + tooLong + `"}, "links": [], "layouts": []}`, - expectedLabel: "dashboard", - expectedPath: "spec.display.name", + scenario: "dashboard display name", + limit: MaxDisplayNameLen, + dashboardJSONFmt: `{"display": {"name": "%s"}, "links": [], "layouts": []}`, + expectedLabel: "dashboard", + expectedPath: "spec.display.name", }, { - scenario: "panel display name", - dashboardJSON: `{"panels": {"p1": {"kind": "Panel", "spec": {"links": [],"display": {"name": "` + tooLong + `"}, "plugin": {"kind": "signoz/TablePanel", "spec": {}}, "queries": []}}}, "links": [], "layouts": []}`, - expectedLabel: "panel", - expectedPath: "spec.panels.p1.spec.display.name", + scenario: "panel display name", + limit: MaxDisplayNameLen, + dashboardJSONFmt: `{"panels": {"p1": {"kind": "Panel", "spec": {"links": [], "display": {"name": "%s"}, "plugin": {"kind": "signoz/TablePanel", "spec": {}}, "queries": []}}}, "links": [], "layouts": []}`, + expectedLabel: "panel", + expectedPath: "spec.panels.p1.spec.display.name", }, { - scenario: "list variable display name", - dashboardJSON: `{"variables": [{"kind": "ListVariable", "spec": {"name": "svc", "display": {"name": "` + tooLong + `"}, "plugin": {"kind": "signoz/DynamicVariable", "spec": {"name": "service.name", "signal": "metrics"}}}}], "links": [], "layouts": []}`, - expectedLabel: "variable", - expectedPath: "spec.variables[0].spec.display.name", + scenario: "list variable display name", + limit: MaxDisplayNameLen, + dashboardJSONFmt: `{"variables": [{"kind": "ListVariable", "spec": {"name": "svc", "display": {"name": "%s"}, "plugin": {"kind": "signoz/DynamicVariable", "spec": {"name": "service.name", "signal": "metrics"}}}}], "links": [], "layouts": []}`, + expectedLabel: "variable", + expectedPath: "spec.variables[0].spec.display.name", }, { - scenario: "text variable display name", - dashboardJSON: `{"variables": [{"kind": "TextVariable", "spec": {"name": "mytext", "value": "v", "display": {"name": "` + tooLong + `"}}}], "links": [], "layouts": []}`, - expectedLabel: "variable", - expectedPath: "spec.variables[0].spec.display.name", + scenario: "text variable display name", + limit: MaxDisplayNameLen, + dashboardJSONFmt: `{"variables": [{"kind": "TextVariable", "spec": {"name": "mytext", "value": "v", "display": {"name": "%s"}}}], "links": [], "layouts": []}`, + expectedLabel: "variable", + expectedPath: "spec.variables[0].spec.display.name", }, { - scenario: "layout title", - dashboardJSON: `{"links": [], "layouts": [{"kind": "Grid", "spec": {"display": {"title": "` + tooLong + `"}, "items": []}}]}`, - expectedLabel: "layout", - expectedPath: "spec.layouts[0].spec.display.title", + scenario: "layout title", + limit: MaxLayoutTitleLen, + dashboardJSONFmt: `{"links": [], "layouts": [{"kind": "Grid", "spec": {"display": {"title": "%s"}, "items": []}}]}`, + expectedLabel: "layout", + expectedPath: "spec.layouts[0].spec.display.title", }, } for _, testCase := range testCases { t.Run(testCase.scenario, func(t *testing.T) { - _, err := unmarshalDashboard([]byte(testCase.dashboardJSON)) + tooLong := strings.Repeat("x", testCase.limit+1) + lengthMsg := fmt.Sprintf("must be at most %d characters, got %d", testCase.limit, testCase.limit+1) + _, err := unmarshalDashboard(fmt.Appendf(nil, testCase.dashboardJSONFmt, tooLong)) require.Error(t, err) // Message is ":