fix: Strip trailing slash for connections (#752)

Author: teeohhem

.changeset/clean-dingos-divide.md

@@ -0,0 +1,5 @@
+---
+"@hyperdx/app": patch
+---
+
+Removes trailing slash for connection urls

packages/app/src/__tests__/utils.test.ts

@@ -8,6 +8,7 @@ import {
   formatDate,
   formatNumber,
   getMetricTableName,
+  stripTrailingSlash,
   useQueryHistory,
 } from '../utils';
 
@@ -473,6 +474,48 @@ describe('useLocalStorage', () => {
   });
 });
 
+describe('stripTrailingSlash', () => {
+  it('should throw an error for nullish values', () => {
+    expect(() => stripTrailingSlash(null)).toThrow(
+      'URL must be a non-empty string',
+    );
+    expect(() => stripTrailingSlash(undefined)).toThrow(
+      'URL must be a non-empty string',
+    );
+  });
+
+  it('should throw an error for non-string values', () => {
+    expect(() => stripTrailingSlash(123 as any)).toThrow(
+      'URL must be a non-empty string',
+    );
+    expect(() => stripTrailingSlash({} as any)).toThrow(
+      'URL must be a non-empty string',
+    );
+  });
+
+  it('should remove trailing slash from URLs', () => {
+    expect(stripTrailingSlash('http://example.com/')).toBe(
+      'http://example.com',
+    );
+    expect(stripTrailingSlash('http://example.com/api/')).toBe(
+      'http://example.com/api',
+    );
+  });
+
+  it('should not modify URLs without trailing slash', () => {
+    expect(stripTrailingSlash('http://example.com')).toBe('http://example.com');
+    expect(stripTrailingSlash('http://example.com/api')).toBe(
+      'http://example.com/api',
+    );
+  });
+
+  it('should handle URLs with multiple trailing slashes', () => {
+    expect(stripTrailingSlash('http://example.com///')).toBe(
+      'http://example.com//',
+    );
+  });
+});
+
 describe('useQueryHistory', () => {
   const mockGetItem = jest.fn();
   const mockSetItem = jest.fn();

packages/app/src/components/ConnectionForm.tsx

@@ -13,6 +13,7 @@ import {
   useDeleteConnection,
   useUpdateConnection,
 } from '@/connection';
+import { stripTrailingSlash } from '@/utils';
 
 import ConfirmDeleteMenu from './ConfirmDeleteMenu';
 
@@ -32,9 +33,10 @@ function useTestConnection({
     useState<TestConnectionState | null>(null);
 
   const handleTestConnection = useCallback(async () => {
-    const host = getValues('host');
+    const hostValue = getValues('host');
     const username = getValues('username');
     const password = getValues('password');
+    const host = stripTrailingSlash(hostValue);
 
     if (testConnectionState) {
       return;
@@ -134,9 +136,15 @@ export function ConnectionForm({
   const deleteConnection = useDeleteConnection();
 
   const onSubmit = (data: Connection) => {
+    // Make sure we don't save a trailing slash in the host
+    const normalizedData = {
+      ...data,
+      host: stripTrailingSlash(data.host),
+    };
+
     if (isNew) {
       createConnection.mutate(
-        { connection: data },
+        { connection: normalizedData },
         {
           onSuccess: () => {
             notifications.show({
@@ -157,7 +165,7 @@ export function ConnectionForm({
       );
     } else {
       updateConnection.mutate(
-        { connection: data, id: connection.id },
+        { connection: normalizedData, id: connection.id },
         {
           onSuccess: () => {
             notifications.show({

packages/app/src/components/__tests__/ConnectionForm.test.tsx

@@ -0,0 +1,165 @@
+import React from 'react';
+import { fireEvent, render, screen, waitFor } from '@testing-library/react';
+
+import { Connection } from '../../connection';
+import { ConnectionForm } from '../ConnectionForm';
+
+import '@testing-library/jest-dom';
+
+// --- Mocks ---
+const mockCreateMutate = jest.fn();
+const mockUpdateMutate = jest.fn();
+jest.mock('@/connection', () => ({
+  ...jest.requireActual('@/connection'),
+  useCreateConnection: () => ({
+    mutate: mockCreateMutate,
+    isPending: false,
+  }),
+  useUpdateConnection: () => ({
+    mutate: mockUpdateMutate,
+    isPending: false,
+  }),
+
+  useDeleteConnection: () => ({
+    mutate: jest.fn(),
+    isPending: false,
+  }),
+}));
+
+jest.mock('@mantine/notifications', () => ({
+  notifications: {
+    show: jest.fn(),
+  },
+}));
+
+const mockTestConnectionMutateAsync = jest.fn();
+jest.mock('@/api', () => ({
+  ...(jest.requireActual('@/api') ?? {}),
+  useTestConnection: () => ({
+    mutateAsync: mockTestConnectionMutateAsync.mockResolvedValue({
+      success: true,
+    }),
+  }),
+}));
+
+// --- Test Suite ---
+
+describe('ConnectionForm', () => {
+  const baseConnection: Connection = {
+    id: '',
+    name: 'Test Connection',
+    host: 'http://localhost:8123',
+    username: 'default',
+    password: '',
+  };
+
+  beforeEach(() => {
+    mockCreateMutate.mockClear();
+    mockUpdateMutate.mockClear();
+    mockTestConnectionMutateAsync.mockClear();
+    (
+      jest.requireMock('@mantine/notifications') as any
+    ).notifications.show.mockClear();
+  });
+
+  it('should save connection with trailing slash removed from host when creating', async () => {
+    renderWithMantine(
+      <ConnectionForm connection={baseConnection} isNew={true} />,
+    );
+
+    const hostInput = screen.getByPlaceholderText('http://localhost:8123');
+    const nameInput = screen.getByPlaceholderText('My Clickhouse Server');
+    const submitButton = screen.getByRole('button', { name: 'Create' });
+
+    await fireEvent.change(nameInput, { target: { value: 'Test Name' } });
+    await fireEvent.change(hostInput, {
+      target: { value: 'http://example.com:8123/' },
+    }); // Host with trailing slash
+
+    fireEvent.click(submitButton);
+
+    await waitFor(() => {
+      expect(mockCreateMutate).toHaveBeenCalledTimes(1);
+    });
+
+    // Check the arguments passed to the mutate function
+    expect(mockCreateMutate).toHaveBeenCalledWith(
+      expect.objectContaining({
+        connection: expect.objectContaining({
+          host: 'http://example.com:8123',
+          name: 'Test Name',
+        }),
+      }),
+      expect.anything(),
+    );
+  });
+
+  it('should save connection with trailing slash removed from host when updating', async () => {
+    const existingConnection = {
+      ...baseConnection,
+      id: 'existing-id',
+      host: 'http://old.com/',
+    };
+    renderWithMantine(
+      <ConnectionForm connection={existingConnection} isNew={false} />,
+    );
+
+    const hostInput = screen.getByPlaceholderText('http://localhost:8123');
+    const submitButton = screen.getByRole('button', { name: 'Save' });
+
+    // Update host
+    await fireEvent.change(hostInput, {
+      target: { value: 'http://updated.com:8123/' },
+    });
+
+    fireEvent.click(submitButton);
+
+    // Wait for mutate to be called and assert
+    await waitFor(() => {
+      expect(mockUpdateMutate).toHaveBeenCalledTimes(1);
+    });
+
+    // Check the arguments passed to the mutate function
+    expect(mockUpdateMutate).toHaveBeenCalledWith(
+      expect.objectContaining({
+        id: 'existing-id',
+        connection: expect.objectContaining({
+          host: 'http://updated.com:8123',
+        }),
+      }),
+      expect.anything(),
+    );
+  });
+
+  it('should use stripped host for test connection', async () => {
+    renderWithMantine(
+      <ConnectionForm connection={baseConnection} isNew={true} />,
+    );
+    const hostInput = screen.getByPlaceholderText('http://localhost:8123');
+
+    const nameInput = screen.getByPlaceholderText('My Clickhouse Server');
+    const testButton = screen.getByRole('button', { name: 'Test Connection' });
+
+    await fireEvent.change(nameInput, { target: { value: 'Test Name' } });
+    await fireEvent.change(hostInput, {
+      target: { value: 'http://test.com:8123/' },
+    });
+
+    // Ensure form state is valid before clicking test
+    await waitFor(() => expect(testButton).not.toBeDisabled());
+
+    fireEvent.click(testButton);
+
+    await waitFor(() =>
+      expect(mockTestConnectionMutateAsync).toHaveBeenCalled(),
+    );
+
+    // Assert that the mock API call received the stripped host
+    expect(mockTestConnectionMutateAsync).toHaveBeenCalledTimes(1);
+    expect(mockTestConnectionMutateAsync).toHaveBeenCalledWith(
+      expect.objectContaining({
+        host: 'http://test.com:8123',
+      }),
+    );
+  });
+});

packages/app/src/utils.ts

@@ -747,3 +747,11 @@ export function getMetricTableName(
 export function toArray<T>(obj?: T | T[]): T[] {
   return !obj ? [] : Array.isArray(obj) ? obj : [obj];
 }
+
+// Helper function to remove trailing slash
+export const stripTrailingSlash = (url: string | undefined | null): string => {
+  if (!url || typeof url !== 'string') {
+    throw new Error('URL must be a non-empty string');
+  }
+  return url.endsWith('/') ? url.slice(0, -1) : url;
+};

smoke-tests/otel-collector/data/auto-parse/default/input.json

@@ -1,53 +1,53 @@
 {
-    "resourceLogs": [
+  "resourceLogs": [
+    {
+      "resource": {
+        "attributes": [
+          {
+            "key": "suite-id",
+            "value": {
+              "stringValue": "auto-parse"
+            }
+          },
+          {
+            "key": "test-id",
+            "value": {
+              "stringValue": "default"
+            }
+          }
+        ]
+      },
+      "scopeLogs": [
         {
-            "resource": {
-                "attributes": [
-                    {
-                        "key": "suite-id",
-                        "value": {
-                            "stringValue": "auto-parse"
-                        }
-                    },
-                    {
-                        "key": "test-id",
-                        "value": {
-                            "stringValue": "default"
-                        }
-                    }
-                ]
+          "scope": {},
+          "logRecords": [
+            {
+              "timeUnixNano": "1901999580000000000",
+              "body": {
+                "stringValue": "[note] this is very much not JSON even though it starts with an array char"
+              }
             },
-            "scopeLogs": [
-                {
-                    "scope": {},
-                    "logRecords": [
-                        {
-                            "timeUnixNano": "1901999580000000000",
-                            "body": {
-                                "stringValue": "[note] this is very much not JSON even though it starts with an array char"
-                            }
-                        },
-                        {
-                            "timeUnixNano": "1901999580000000001",
-                            "body": {
-                                "stringValue": "{note} this is very much not JSON even though it starts with an object char"
-                            }
-                        },
-                        {
-                            "timeUnixNano": "1901999580000000002",
-                            "body": {
-                                "stringValue": "NOTE: this is very much not JSON"
-                            }
-                        },
-                        {
-                            "timeUnixNano": "1901999580000000003",
-                            "body": {
-                                "stringValue": "this has some {Key {Value { '{' } } invalid JSON in it"
-                            }
-                        }
-                    ]
-                }
-            ]
+            {
+              "timeUnixNano": "1901999580000000001",
+              "body": {
+                "stringValue": "{note} this is very much not JSON even though it starts with an object char"
+              }
+            },
+            {
+              "timeUnixNano": "1901999580000000002",
+              "body": {
+                "stringValue": "NOTE: this is very much not JSON"
+              }
+            },
+            {
+              "timeUnixNano": "1901999580000000003",
+              "body": {
+                "stringValue": "this has some {Key {Value { '{' } } invalid JSON in it"
+              }
+            }
+          ]
         }
-    ]
-}
+      ]
+    }
+  ]
+}