From 5457f57fb09b27d88a96d00792b9cb171ce9c29d Mon Sep 17 00:00:00 2001 From: Danny Tuppeny Date: Mon, 9 Sep 2024 10:51:21 +0100 Subject: [PATCH] Deprecate DTDConnectionInfo and replace with a class that can handle separate private/exposed URIs (#8291) This replaces `DTDConnectionInfo` with `DTDInfo` (unsure if a better name?) and marks the former as deprecated. The new class has two URIs: - `localUri` which is the URI used for backend services on the same machine as the DTD process (this is usually a `localhost` URI) - `exposedUri` which is the URI that can be used from the frontend where the user is (this may or may not be the same as localUri depending on whether they're in a remote/web IDE session) The DevTools web API returns the exposed URI (since it will be used by frontend tools) and other places that used the URI for connecting directly use the local one. On its own, this change doesn't do anything, the intention is to roll this into the SDK and update the DevTools server code in dartdev/DDS to accept an additional optional URI which will then provide these back to the server code here (for the web API handler and extension search code). This is work towards fixing some of the issues in https://github.com/Dart-Code/Dart-Code/issues/5158. --- packages/devtools_shared/CHANGELOG.md | 1 + packages/devtools_shared/lib/src/common.dart | 34 +++++++++++++++++++ .../lib/src/extensions/extension_manager.dart | 6 ++-- .../server/handlers/_devtools_extensions.dart | 2 +- .../lib/src/server/handlers/_dtd.dart | 7 ++-- .../lib/src/server/handlers/_general.dart | 8 ++--- .../lib/src/server/server_api.dart | 2 +- .../devtools_shared/test/helpers/helpers.dart | 13 +++---- .../server/devtools_extensions_api_test.dart | 14 ++++---- .../test/server/dtd_api_test.dart | 6 ++-- .../test/server/general_api_test.dart | 18 +++++----- .../test/utils/file_utils_test.dart | 10 +++--- 12 files changed, 80 insertions(+), 41 deletions(-) diff --git a/packages/devtools_shared/CHANGELOG.md b/packages/devtools_shared/CHANGELOG.md index dbbcac39337..36e4c891225 100644 --- a/packages/devtools_shared/CHANGELOG.md +++ b/packages/devtools_shared/CHANGELOG.md @@ -15,6 +15,7 @@ * Deprecate `apiIncrementSurveyShownCount` in favor of `SurveyApi.incrementSurveyShownCount`. * Support Chrome's new headless mode in the integration test runner. * Add `PreferencesApi` to get and set preference values. +* Deprecate `DTDConnectionInfo` in favor of `DtdInfo` which supports tracking two URIs for DTD to better support web/remote environments. # 10.0.2 * Update dependency `web_socket_channel: '>=2.4.0 <4.0.0'`. diff --git a/packages/devtools_shared/lib/src/common.dart b/packages/devtools_shared/lib/src/common.dart index 88ea352eedd..5dd8acfd418 100644 --- a/packages/devtools_shared/lib/src/common.dart +++ b/packages/devtools_shared/lib/src/common.dart @@ -3,4 +3,38 @@ // found in the LICENSE file. /// Describes an instance of the Dart Tooling Daemon. +@Deprecated('Use DtdInfo instead') typedef DTDConnectionInfo = ({String? uri, String? secret}); + +/// Information about a Dart Tooling Daemon instance. +class DtdInfo { + DtdInfo( + this.localUri, { + Uri? exposedUri, + this.secret, + }) : exposedUri = exposedUri ?? localUri; + + /// The URI for connecting to DTD from the backend. + /// + /// This is usually a `http://localhost/` address that is accessible to tools + /// running in the same location as the DTD process. It may NOT be accessible + /// to frontends that run in another location - for example the DevTools + /// frontend running in a browser (or embedded in an IDE) in a remote/web IDE + /// session. + final Uri localUri; + + /// The exposed URI for connecting to DTD from the frontend. + /// + /// In a remote session, this can be an address provided by the hosting + /// infrastructure/proxy that tunnels through to the backend running the DTD + /// process which might look like `https://foo-123.cloud-ide.foo/`. + /// + /// For a non-remote session, this will be the same value as [localUri]. + final Uri exposedUri; + + /// The secret token that allows calling privileged DTD APIs. + /// + /// This may not always be available if DTD was spawned by another process + /// and it should never be shared with external code. + final String? secret; +} diff --git a/packages/devtools_shared/lib/src/extensions/extension_manager.dart b/packages/devtools_shared/lib/src/extensions/extension_manager.dart index 8ef91bd44d9..ecc9a5f4528 100644 --- a/packages/devtools_shared/lib/src/extensions/extension_manager.dart +++ b/packages/devtools_shared/lib/src/extensions/extension_manager.dart @@ -59,7 +59,7 @@ class ExtensionsManager { Future serveAvailableExtensions( String? rootFileUriString, List logs, - DTDConnectionInfo? dtd, + DtdInfo? dtd, ) async { logs.add( 'ExtensionsManager.serveAvailableExtensions for ' @@ -86,11 +86,11 @@ class ExtensionsManager { // Find all static extensions for the project roots, which are derived from // the Dart Tooling Daemon, and add them to [devtoolsExtensions]. - final dtdUri = dtd?.uri; + final dtdUri = dtd?.localUri; if (dtdUri != null) { DartToolingDaemon? dartToolingDaemon; try { - dartToolingDaemon = await DartToolingDaemon.connect(Uri.parse(dtdUri)); + dartToolingDaemon = await DartToolingDaemon.connect(dtdUri); final projectRoots = await dartToolingDaemon.getProjectRoots( depth: staticExtensionsSearchDepth, ); diff --git a/packages/devtools_shared/lib/src/server/handlers/_devtools_extensions.dart b/packages/devtools_shared/lib/src/server/handlers/_devtools_extensions.dart index 04f48fd109c..33ceeb4af20 100644 --- a/packages/devtools_shared/lib/src/server/handlers/_devtools_extensions.dart +++ b/packages/devtools_shared/lib/src/server/handlers/_devtools_extensions.dart @@ -11,7 +11,7 @@ abstract class _ExtensionsApiHandler { ServerApi api, Map queryParams, ExtensionsManager extensionsManager, - DTDConnectionInfo? dtd, + DtdInfo? dtd, ) async { final missingRequiredParams = ServerApi._checkRequiredParameters( [ExtensionsApi.packageRootUriPropertyName], diff --git a/packages/devtools_shared/lib/src/server/handlers/_dtd.dart b/packages/devtools_shared/lib/src/server/handlers/_dtd.dart index cfe5c9cbf82..2fe7d1e795f 100644 --- a/packages/devtools_shared/lib/src/server/handlers/_dtd.dart +++ b/packages/devtools_shared/lib/src/server/handlers/_dtd.dart @@ -9,10 +9,13 @@ part of '../server_api.dart'; abstract class _DtdApiHandler { static shelf.Response handleGetDtdUri( ServerApi api, - DTDConnectionInfo? dtd, + DtdInfo? dtd, ) { return ServerApi._encodeResponse( - {DtdApi.uriPropertyName: dtd?.uri}, + { + // Always provide the exposed URI to callers of the web API. + DtdApi.uriPropertyName: dtd?.exposedUri.toString(), + }, api: api, ); } diff --git a/packages/devtools_shared/lib/src/server/handlers/_general.dart b/packages/devtools_shared/lib/src/server/handlers/_general.dart index d5491b3727c..d309b771701 100644 --- a/packages/devtools_shared/lib/src/server/handlers/_general.dart +++ b/packages/devtools_shared/lib/src/server/handlers/_general.dart @@ -22,7 +22,7 @@ abstract class Handler { static Future handleNotifyForVmServiceConnection( ServerApi api, Map queryParams, - DTDConnectionInfo? dtd, + DtdInfo? dtd, ) async { final missingRequiredParams = ServerApi._checkRequiredParameters( const [apiParameterValueKey, apiParameterVmServiceConnected], @@ -32,7 +32,7 @@ abstract class Handler { ); if (missingRequiredParams != null) return missingRequiredParams; - final dtdUri = dtd?.uri; + final dtdUri = dtd?.localUri; final dtdSecret = dtd?.secret; if (dtdUri == null || dtdSecret == null) { // If DevTools server did not start DTD, there is nothing for us to do. @@ -62,7 +62,7 @@ abstract class Handler { DartToolingDaemon? dartToolingDaemon; try { - dartToolingDaemon = await DartToolingDaemon.connect(Uri.parse(dtd!.uri!)); + dartToolingDaemon = await DartToolingDaemon.connect(dtd!.localUri); final detectRootResponse = await detectRootPackageForVmService( vmServiceUriAsString: vmServiceUriAsString, @@ -152,7 +152,7 @@ abstract class Handler { @visibleForTesting static Future updateDtdWorkspaceRoots( DartToolingDaemon dtd, { - required DTDConnectionInfo dtdConnectionInfo, + required DtdInfo dtdConnectionInfo, required Uri rootFromVmService, required bool connected, required ServerApi api, diff --git a/packages/devtools_shared/lib/src/server/server_api.dart b/packages/devtools_shared/lib/src/server/server_api.dart index 46a38810272..291d6e73897 100644 --- a/packages/devtools_shared/lib/src/server/server_api.dart +++ b/packages/devtools_shared/lib/src/server/server_api.dart @@ -58,7 +58,7 @@ class ServerApi { required ExtensionsManager extensionsManager, required DeeplinkManager deeplinkManager, ServerApi? api, - DTDConnectionInfo? dtd, + DtdInfo? dtd, }) { api ??= ServerApi(); final queryParams = request.requestedUri.queryParameters; diff --git a/packages/devtools_shared/test/helpers/helpers.dart b/packages/devtools_shared/test/helpers/helpers.dart index af9a2d49b3c..d5fbb435779 100644 --- a/packages/devtools_shared/test/helpers/helpers.dart +++ b/packages/devtools_shared/test/helpers/helpers.dart @@ -10,9 +10,8 @@ import 'package:devtools_shared/devtools_shared.dart'; import 'package:path/path.dart' as path; typedef TestDtdConnectionInfo = ({ - String? uri, - String? secret, - Process? dtdProcess, + DtdInfo? info, + Process? process, }); /// Helper method to start DTD for the purpose of testing. @@ -23,8 +22,7 @@ Future startDtd() async { Process? dtdProcess; StreamSubscription? dtdStoutSubscription; - TestDtdConnectionInfo onFailure() => - (uri: null, secret: null, dtdProcess: dtdProcess); + TestDtdConnectionInfo onFailure() => (info: null, process: dtdProcess); try { dtdProcess = await Process.start( @@ -44,7 +42,10 @@ Future startDtd() async { } }) { completer.complete( - (uri: uri, secret: secret, dtdProcess: dtdProcess), + ( + info: DtdInfo(Uri.parse(uri), secret: secret), + process: dtdProcess + ), ); } else { completer.complete(onFailure()); diff --git a/packages/devtools_shared/test/server/devtools_extensions_api_test.dart b/packages/devtools_shared/test/server/devtools_extensions_api_test.dart index 13dcf6950ea..7e14ae77d08 100644 --- a/packages/devtools_shared/test/server/devtools_extensions_api_test.dart +++ b/packages/devtools_shared/test/server/devtools_extensions_api_test.dart @@ -28,14 +28,14 @@ void main() { setUp(() async { extensionsManager = ExtensionsManager(); dtd = await startDtd(); - expect(dtd!.uri, isNotNull, reason: 'Error starting DTD for test'); - testDtdConnection = await DartToolingDaemon.connect(Uri.parse(dtd!.uri!)); + expect(dtd!.info, isNotNull, reason: 'Error starting DTD for test'); + testDtdConnection = await DartToolingDaemon.connect(dtd!.info!.localUri); }); tearDown(() async { await testDtdConnection?.close(); - dtd?.dtdProcess?.kill(); - await dtd?.dtdProcess?.exitCode; + dtd?.process?.kill(); + await dtd?.process?.exitCode; dtd = null; await extensionTestManager.reset(); @@ -50,7 +50,7 @@ void main() { includeBadExtension: includeBadExtension, ); await testDtdConnection!.setIDEWorkspaceRoots( - dtd!.secret!, + dtd!.info!.secret!, [extensionTestManager.packagesRootUri], ); } @@ -75,7 +75,7 @@ void main() { request, extensionsManager: manager, deeplinkManager: FakeDeeplinkManager(), - dtd: (uri: dtd!.uri, secret: dtd!.secret), + dtd: dtd!.info, ); } @@ -252,7 +252,7 @@ class _TestExtensionsManager extends ExtensionsManager { Future serveAvailableExtensions( String? rootFileUriString, List logs, - DTDConnectionInfo? dtd, + DtdInfo? dtd, ) async { await super.serveAvailableExtensions(rootFileUriString, logs, dtd); throw Exception('Fake exception for test'); diff --git a/packages/devtools_shared/test/server/dtd_api_test.dart b/packages/devtools_shared/test/server/dtd_api_test.dart index 020f2c9ef20..724b7b93b1c 100644 --- a/packages/devtools_shared/test/server/dtd_api_test.dart +++ b/packages/devtools_shared/test/server/dtd_api_test.dart @@ -16,7 +16,7 @@ import '../fakes.dart'; void main() { group('$DtdApi', () { test('handle ${DtdApi.apiGetDtdUri} succeeds', () async { - const dtdUri = 'ws://dtd:uri'; + final dtdUri = Uri.parse('ws://dtd/uri'); final request = Request( 'get', Uri( @@ -29,12 +29,12 @@ void main() { request, extensionsManager: ExtensionsManager(), deeplinkManager: FakeDeeplinkManager(), - dtd: (uri: dtdUri, secret: null), + dtd: DtdInfo(dtdUri), ); expect(response.statusCode, HttpStatus.ok); expect( await response.readAsString(), - jsonEncode({DtdApi.uriPropertyName: dtdUri}), + jsonEncode({DtdApi.uriPropertyName: dtdUri.toString()}), ); }); }); diff --git a/packages/devtools_shared/test/server/general_api_test.dart b/packages/devtools_shared/test/server/general_api_test.dart index e2c21e08e2c..27bb7429e37 100644 --- a/packages/devtools_shared/test/server/general_api_test.dart +++ b/packages/devtools_shared/test/server/general_api_test.dart @@ -20,7 +20,7 @@ void main() { group('General DevTools server API', () { group(apiNotifyForVmServiceConnection, () { Future sendNotifyRequest({ - required DTDConnectionInfo dtd, + required DtdInfo? dtd, Map? queryParameters, // ignore: avoid-redundant-async, returning FutureOr. }) async { @@ -45,7 +45,7 @@ void main() { 'succeeds when DTD is not available', () async { final response = await sendNotifyRequest( - dtd: (uri: null, secret: null), + dtd: null, queryParameters: { apiParameterValueKey: 'fake_uri', apiParameterVmServiceConnected: 'true', @@ -60,7 +60,7 @@ void main() { 'returns badRequest for invalid VM service argument', () async { final response = await sendNotifyRequest( - dtd: (uri: 'ws://dtd:uri', secret: 'fake_secret'), + dtd: DtdInfo(Uri.parse('ws://dtd/uri'), secret: 'fake_secret'), queryParameters: { apiParameterValueKey: 'fake_uri', apiParameterVmServiceConnected: 'true', @@ -77,7 +77,7 @@ void main() { 'returns badRequest for invalid $apiParameterVmServiceConnected argument', () async { final response = await sendNotifyRequest( - dtd: (uri: 'ws://dtd:uri', secret: 'fake_secret'), + dtd: DtdInfo(Uri.parse('ws://dtd/uri'), secret: 'fake_secret'), queryParameters: { apiParameterValueKey: 'ws://127.0.0.1:8181/LEpVqqD7E_Y=/ws', apiParameterVmServiceConnected: 'bad_arg', @@ -98,15 +98,15 @@ void main() { setUp(() async { dtd = await startDtd(); - expect(dtd!.uri, isNotNull, reason: 'Error starting DTD for test'); + expect(dtd!.info, isNotNull, reason: 'Error starting DTD for test'); testDtdConnection = - await DartToolingDaemon.connect(Uri.parse(dtd!.uri!)); + await DartToolingDaemon.connect(dtd!.info!.localUri); }); tearDown(() async { await testDtdConnection?.close(); - dtd?.dtdProcess?.kill(); - await dtd?.dtdProcess?.exitCode; + dtd?.process?.kill(); + await dtd?.process?.exitCode; dtd = null; }); @@ -117,7 +117,7 @@ void main() { }) async { await server.Handler.updateDtdWorkspaceRoots( testDtdConnection!, - dtdConnectionInfo: (uri: dtd!.uri, secret: dtd!.secret), + dtdConnectionInfo: dtd!.info!, rootFromVmService: root, connected: connected, api: ServerApi(), diff --git a/packages/devtools_shared/test/utils/file_utils_test.dart b/packages/devtools_shared/test/utils/file_utils_test.dart index 99a87b6130d..8fd57a5b9bf 100644 --- a/packages/devtools_shared/test/utils/file_utils_test.dart +++ b/packages/devtools_shared/test/utils/file_utils_test.dart @@ -39,21 +39,21 @@ void main() { setUp(() async { dtd = await startDtd(); - expect(dtd!.uri, isNotNull, reason: 'Error starting DTD for test'); - testDtdConnection = await DartToolingDaemon.connect(Uri.parse(dtd!.uri!)); + expect(dtd!.info, isNotNull, reason: 'Error starting DTD for test'); + testDtdConnection = await DartToolingDaemon.connect(dtd!.info!.localUri); _setupTestDirectoryStructure(); await testDtdConnection!.setIDEWorkspaceRoots( - dtd!.secret!, + dtd!.info!.secret!, [Uri.parse(projectRoot)], ); }); tearDown(() async { await testDtdConnection?.close(); - dtd?.dtdProcess?.kill(); - await dtd?.dtdProcess?.exitCode; + dtd?.process?.kill(); + await dtd?.process?.exitCode; dtd = null; });