Skip to content

feat(connect-form): Allow saving invalid but well-formed strings #2861

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 10 commits into from
Mar 9, 2022
Merged

feat(connect-form): Allow saving invalid but well-formed strings #2861

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
1f85c36
always use loose validation
mcasimir Mar 4, 2022
35c301e
rephrase TLS error messages in a more positive way
mcasimir Mar 4, 2022
4e643ca
only update lastUsed if saved previously
mcasimir Mar 4, 2022
56bba3d
fix tests
mcasimir Mar 4, 2022
cb26e4b
remove .only
mcasimir Mar 4, 2022
44a1057
don't update connection list if save fails
mcasimir Mar 4, 2022
c1717ff
fix requested changes
mcasimir Mar 4, 2022
d3bc06e
don't check for lastUsed
mcasimir Mar 4, 2022
c6019a1
strict validation of well formed uri
mcasimir Mar 9, 2022
b766f0f
revert change to tls warnings
mcasimir Mar 9, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/compass-connect/src/modules/telemetry.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ async function getHostInformation(host) {

async function getConnectionData(connectionInfo) {
const {connectionOptions: {connectionString, sshTunnel}} = connectionInfo;
const connectionStringData = new ConnectionString(connectionString);
const connectionStringData = new ConnectionString(connectionString, {looseValidation: true});
const hostName = connectionStringData.hosts[0];
const searchParams = connectionStringData.searchParams;

Expand Down
13 changes: 9 additions & 4 deletions packages/compass-connections/src/components/connections.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,23 +7,28 @@ import {
fireEvent,
} from '@testing-library/react';
import { expect } from 'chai';
import type { ConnectionInfo, ConnectionOptions } from 'mongodb-data-service';
import type {
ConnectionInfo,
ConnectionOptions,
ConnectionStorage,
} from 'mongodb-data-service';
import { v4 as uuid } from 'uuid';
import sinon from 'sinon';

import Connections from './connections';
import type { ConnectionStore } from '../stores/connections-store';
import { ToastArea } from '@mongodb-js/compass-components';

function getMockConnectionStorage(
mockConnections: ConnectionInfo[]
): ConnectionStore {
): ConnectionStorage {
return {
loadAll: () => {
return Promise.resolve(mockConnections);
},
save: () => Promise.resolve(),
delete: () => Promise.resolve(),
load: (id: string) =>
Promise.resolve(mockConnections.find((conn) => conn.id === id)),
};
}

Expand Down Expand Up @@ -113,7 +118,7 @@ describe('Connections Component', function () {

describe('when rendered with saved connections in storage', function () {
let mockConnectFn: sinon.SinonSpy;
let mockStorage: ConnectionStore;
let mockStorage: ConnectionStorage;
let savedConnectionId: string;
let savedConnectionWithAppNameId: string;
let saveConnectionSpy: sinon.SinonSpy;
Expand Down
3 changes: 1 addition & 2 deletions packages/compass-connections/src/components/connections.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ import { createLoggerAndTelemetry } from '@mongodb-js/compass-logging';
import ResizableSidebar from './resizeable-sidebar';
import FormHelp from './form-help/form-help';
import Connecting from './connecting/connecting';
import type { ConnectionStore } from '../stores/connections-store';
import { useConnections } from '../stores/connections-store';
import { cloneDeep } from 'lodash';

Expand Down Expand Up @@ -65,7 +64,7 @@ function Connections({
connectionInfo: ConnectionInfo,
dataService: DataService
) => void;
connectionStorage?: ConnectionStore;
connectionStorage?: ConnectionStorage;
appName: string;
connectFn?: (connectionOptions: ConnectionOptions) => Promise<DataService>;
}): React.ReactElement {
Expand Down
4 changes: 3 additions & 1 deletion packages/compass-connections/src/modules/telemetry.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,9 @@ async function getConnectionData({
}: Pick<ConnectionInfo, 'connectionOptions'>): Promise<
Record<string, unknown>
> {
const connectionStringData = new ConnectionString(connectionString);
const connectionStringData = new ConnectionString(connectionString, {
looseValidation: true,
});
const hostName = connectionStringData.hosts[0];
const searchParams =
connectionStringData.typedSearchParams<MongoClientOptions>();
Expand Down
57 changes: 48 additions & 9 deletions packages/compass-connections/src/stores/connections-store.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@ import type { RenderResult } from '@testing-library/react-hooks';
import { renderHook, act } from '@testing-library/react-hooks';
import sinon from 'sinon';

import type { ConnectionStore } from './connections-store';
import { useConnections } from './connections-store';
import type { ConnectionStorage } from 'mongodb-data-service';

const noop = (): any => {
/* no-op */
Expand Down Expand Up @@ -33,20 +33,23 @@ const mockConnections = [
];

describe('use-connections hook', function () {
let mockConnectionStorage: ConnectionStore;
let mockConnectionStorage: ConnectionStorage;
let loadAllSpy: sinon.SinonSpy;
let saveSpy: sinon.SinonSpy;
let deleteSpy: sinon.SinonSpy;
let loadSpy: sinon.SinonSpy;

beforeEach(function () {
loadAllSpy = sinon.spy();
saveSpy = sinon.spy();
deleteSpy = sinon.spy();
loadSpy = sinon.spy();

mockConnectionStorage = {
loadAll: loadAllSpy,
save: saveSpy,
delete: deleteSpy,
load: loadSpy,
};
});

Expand Down Expand Up @@ -98,7 +101,7 @@ describe('use-connections hook', function () {
await result.current.saveConnection({
id: 'oranges',
connectionOptions: {
connectionString: 'aba',
connectionString: 'mongodb://aba',
},
favorite: {
name: 'not peaches',
Expand All @@ -118,7 +121,7 @@ describe('use-connections hook', function () {
expect(hookResult.current.state.connections[1]).to.deep.equal({
id: 'oranges',
connectionOptions: {
connectionString: 'aba',
connectionString: 'mongodb://aba',
},
favorite: {
name: 'not peaches',
Expand Down Expand Up @@ -152,7 +155,7 @@ describe('use-connections hook', function () {
await result.current.saveConnection({
id: 'pineapples',
connectionOptions: {
connectionString: '',
connectionString: 'mongodb://bacon',
},
favorite: {
name: 'bacon',
Expand All @@ -172,7 +175,7 @@ describe('use-connections hook', function () {
expect(hookResult.current.state.connections[0]).to.deep.equal({
id: 'pineapples',
connectionOptions: {
connectionString: '',
connectionString: 'mongodb://bacon',
},
favorite: {
name: 'bacon',
Expand All @@ -181,6 +184,42 @@ describe('use-connections hook', function () {
});
});

describe('saving an invalid connection', function () {
let hookResult: RenderResult<ReturnType<typeof useConnections>>;
beforeEach(async function () {
const { result } = renderHook(() =>
useConnections({
onConnected: noop,
connectionStorage: mockConnectionStorage,
connectFn: noop,
appName: 'Test App Name',
})
);

await act(async () => {
await result.current.saveConnection({
id: 'pineapples',
connectionOptions: {
connectionString: 'bacon',
},
favorite: {
name: 'bacon',
},
});
});

hookResult = result;
});

it('calls to save a connection on the store', function () {
expect(saveSpy.callCount).to.equal(0);
});

it('does not add the new connection to the current connections list', function () {
expect(hookResult.current.state.connections).to.be.undefined;
});
});

describe('saving the current active connection', function () {
let hookResult: RenderResult<ReturnType<typeof useConnections>>;
beforeEach(async function () {
Expand Down Expand Up @@ -209,7 +248,7 @@ describe('use-connections hook', function () {
await result.current.saveConnection({
id: 'turtle',
connectionOptions: {
connectionString: 'nice',
connectionString: 'mongodb://nice',
},
favorite: {
name: 'snakes! ah!',
Expand All @@ -225,7 +264,7 @@ describe('use-connections hook', function () {
expect(hookResult.current.state.activeConnectionInfo).to.deep.equal({
id: 'turtle',
connectionOptions: {
connectionString: 'nice',
connectionString: 'mongodb://nice',
},
favorite: {
name: 'snakes! ah!',
Expand All @@ -238,7 +277,7 @@ describe('use-connections hook', function () {
expect(hookResult.current.state.connections[0]).to.deep.equal({
id: 'turtle',
connectionOptions: {
connectionString: 'nice',
connectionString: 'mongodb://nice',
},
favorite: {
name: 'snakes! ah!',
Expand Down
45 changes: 33 additions & 12 deletions packages/compass-connections/src/stores/connections-store.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import type {
ConnectionInfo,
ConnectionOptions,
DataService,
ConnectionStorage,
} from 'mongodb-data-service';
import { getConnectionTitle } from 'mongodb-data-service';
import { useEffect, useReducer, useRef } from 'react';
Expand Down Expand Up @@ -30,10 +31,9 @@ export function createNewConnectionInfo(): ConnectionInfo {
},
};
}
export interface ConnectionStore {
loadAll: () => Promise<ConnectionInfo[]>;
save: (connectionInfo: ConnectionInfo) => Promise<void>;
delete: (connectionInfo: ConnectionInfo) => Promise<void>;

function ensureWellFormedConnectionString(connectionString: string) {
new ConnectionString(connectionString);
}

function setAppNameParamIfMissing(
Expand All @@ -43,7 +43,9 @@ function setAppNameParamIfMissing(
let connectionStringUrl;

try {
connectionStringUrl = new ConnectionString(connectionString);
connectionStringUrl = new ConnectionString(connectionString, {
looseValidation: true,
});
} catch (e) {
//
}
Expand Down Expand Up @@ -183,7 +185,7 @@ async function loadConnections(
type: 'set-connections';
connections: ConnectionInfo[];
}>,
connectionStorage: ConnectionStore
connectionStorage: ConnectionStorage
) {
try {
const loadedConnections = await connectionStorage.loadAll();
Expand All @@ -207,7 +209,7 @@ export function useConnections({
connectionInfo: ConnectionInfo,
dataService: DataService
) => void;
connectionStorage: ConnectionStore;
connectionStorage: ConnectionStorage;
connectFn: (connectionOptions: ConnectionOptions) => Promise<DataService>;
appName: string;
}): {
Expand All @@ -234,10 +236,17 @@ export function useConnections({
const connectedConnectionInfo = useRef<ConnectionInfo>();
const connectedDataService = useRef<DataService>();

async function saveConnectionInfo(connectionInfo: ConnectionInfo) {
async function saveConnectionInfo(
connectionInfo: ConnectionInfo
): Promise<boolean> {
try {
ensureWellFormedConnectionString(
connectionInfo?.connectionOptions?.connectionString
);
await connectionStorage.save(connectionInfo);
debug(`saved connection with id ${connectionInfo.id || ''}`);

return true;
} catch (err) {
debug(
`error saving connection with id ${connectionInfo.id || ''}: ${
Expand All @@ -252,6 +261,8 @@ export function useConnections({
(err as Error).message
}`,
});

return false;
}
}

Expand All @@ -263,9 +274,15 @@ export function useConnections({
try {
onConnected(connectionInfo, dataService);

// Update lastUsed date as now and save the connection.
connectionInfo.lastUsed = new Date();
await saveConnectionInfo(connectionInfo);
// if a connection has been saved already we only want to update the lastUsed
// attribute, otherwise we are going to save the entire connection info.
const connectionInfoToBeSaved =
(await connectionStorage.load(connectionInfo.id)) ?? connectionInfo;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to reload the entire connection here? Would it make sense to check for lastUsed instead?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need to reload for the update, but we could do a check first, something like this?

const connectionInfoToBeSaved = connectionInfo.lastUsed && (await connectionStorage.load(connectionInfo.id)) ?? connectionInfo;

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah gotcha we are reloading here to pull in the original state of the connection then so the changes that are currently there do not override. This is probably a nicer way to do it than keeping around a clone of the original state of the connection then, I think both have implications if a user is doing things in another window.

Copy link
Collaborator Author

@mcasimir mcasimir Mar 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are only replicating the old behavior here, i was surprised this is how the old connection model worked.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the suggestion I made for using lastUsed might not actually do anything then? Sorry!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah true, actually it would cause the entire thing to be saved on connect the first time, even if it was saved before, but just never used

Copy link
Collaborator Author

@mcasimir mcasimir Mar 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed it back. Let's wait for @addaleax before merging, It would be great to have some feedback cause she wasn't fond of relaxing the validation too much. We can easily drop the looseValidation from ensureWellFormedConnectionString in case


await saveConnectionInfo({
...cloneDeep(connectionInfoToBeSaved),
lastUsed: new Date(),
});
} catch (err) {
debug(
`error occurred connection with id ${connectionInfo.id || ''}: ${
Expand Down Expand Up @@ -379,7 +396,11 @@ export function useConnections({
});
},
async saveConnection(connectionInfo: ConnectionInfo) {
await saveConnectionInfo(connectionInfo);
const saved = await saveConnectionInfo(connectionInfo);

if (!saved) {
return;
}

const existingConnectionIndex = connections.findIndex(
(connection) => connection.id === connectionInfo.id
Expand Down
4 changes: 2 additions & 2 deletions packages/compass-e2e-tests/tests/connection.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,8 +104,8 @@ describe('SRV connectivity', function () {
expect(resolutionLogs).to.have.lengthOf(1);

const { from, to, resolutionDetails } = resolutionLogs[0];
const fromCS = new ConnectionString(from);
const toCS = new ConnectionString(to);
const fromCS = new ConnectionString(from, { looseValidation: true });
const toCS = new ConnectionString(to, { looseValidation: true });
fromCS.searchParams.delete('appname');
toCS.searchParams.delete('appname');
toCS.hosts.sort();
Expand Down
3 changes: 2 additions & 1 deletion packages/compass-export-to-language/src/stores/store.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,8 @@ function getCurrentlyConnectedUri(dataService) {

try {
connectionStringUrl = new ConnectionString(
dataService.getConnectionOptions().connectionString
dataService.getConnectionOptions().connectionString,
{looseValidation: true}
);
} catch (e) {
return '<uri>';
Expand Down
2 changes: 1 addition & 1 deletion packages/compass-metrics/src/modules/rules.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ const LOCALHOST = /(^localhost)|(^127\.0\.0\.1)/gi;

async function getCloudInfoFromDataService(dataService) {
try {
const url = new ConnectionString(dataService.getConnectionOptions().connectionString);
const url = new ConnectionString(dataService.getConnectionOptions().connectionString, {looseValidation: true});
const firstServerHostname = (url.hosts[0] || '').split(':')[0];
return await getCloudInfo(firstServerHostname);
} catch (e) {
Expand Down
11 changes: 3 additions & 8 deletions packages/connection-form/src/components/connect-form.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,16 +150,11 @@ function ConnectForm({
const callOnSaveConnectionClickedAndStoreErrors = useCallback(
async (connectionInfo: ConnectionInfo): Promise<void> => {
try {
const formErrors = validateConnectionOptionsErrors(
connectionInfo.connectionOptions,
{ looseValidation: false }
);
if (formErrors.length) {
setErrors(formErrors);
return;
}
await onSaveConnectionClicked?.(connectionInfo);
} catch (err) {
// save errors are already handled as toast notifications,
// keeping so we don't rely too much on far-away code and leave errors
// uncaught in case that code would change
setErrors([
{
message: `Unable to save connection: ${(err as Error).message}`,
Expand Down
13 changes: 13 additions & 0 deletions packages/connection-form/src/utils/validation.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -350,6 +350,19 @@ describe('validation', function () {
});
});

it('should not return warnings when certificate validation is enabled', function () {
[
'tlsInsecure',
'tlsAllowInvalidHostnames',
'tlsAllowInvalidCertificates',
].forEach((option) => {
const result = validateConnectionOptionsWarnings({
connectionString: `mongodb+srv://myserver.com?${option}=false`,
});
expect(result).to.deep.equal([]);
});
});

it('should return warnings if unknown readPreference', function () {
const result = validateConnectionOptionsWarnings({
connectionString: `mongodb://myserver.com?readPreference=invalidReadPreference`,
Expand Down
Loading