Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
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
18 changes: 16 additions & 2 deletions lib/src/robot/client.dart
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import 'package:flutter/foundation.dart';
import 'package:grpc/grpc_connection_interface.dart';
import 'package:logger/logger.dart';

import '../exceptions.dart';
import '../gen/common/v1/common.pb.dart';
import '../gen/robot/v1/robot.pb.dart';
import '../gen/robot/v1/robot.pbgrpc.dart' as rpb;
Expand Down Expand Up @@ -253,9 +254,22 @@ class RobotClient {
}
}

/// Get a connected resource by its [ResourceName].
/// Get a resource client by its [ResourceName].
///
/// If the resource is already cached in the manager, it will be returned directly.
/// Otherwise, a new client will be created on-demand using the [Registry].
/// This allows resource clients to be created whether or not the resource
/// currently exists on the server. Errors will surface when the client
/// attempts to communicate with the resource.
T getResource<T>(ResourceName name) {
return _manager.getResource<T>(name);
try {
return _manager.getResource<T>(name);
} on ResourceNotFoundException {
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

lookupSubtype throws a generic Exception (message: "Subtype not registered in registry") when the subtype is unknown — not a ResourceNotFoundException. This changes the error contract: callers who catch ResourceNotFoundException to handle "resource not available" will miss the case where the subtype is unregistered. Consider re-throwing as ResourceNotFoundException, or documenting clearly that an unregistered subtype propagates a raw Exception.

final registration = Registry.instance.lookupSubtype(Subtype.fromResourceName(name));
final resource = registration.rpcClientCreator(name.name, _channel);
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

After a reconnect, refresh() replaces _manager with a new ResourceManager built only from server-returned resources. On-demand resources added to the old _manager by this path are not carried over to the new manager, and _resetResourceChannel is never called for them (it only iterates responseNames). Any client reference the caller is holding after a reconnect will have a stale channel and will silently fail until they call getResource again (which re-creates on-demand). This is an inconsistency vs. server-known resources, which do get their channels reset. At minimum the docstring should note this, and the reconnect test in the test plan should be exercised.

_manager.register(name, resource);
return resource as T;
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

resource as T is an unchecked cast at runtime. If the caller passes a mismatched type (e.g. getResource<Camera>(motorName)), this throws a TypeError here rather than at _manager.getResource<T>. This is consistent with the manager's own cast, but worth noting: there is no guard if T does not extend Resource. This is the same exposure that already existed in the manager path, so no change required — just confirming it is intentional.

}
}

/// Get a WebRTC stream client with the given name.
Expand Down
149 changes: 149 additions & 0 deletions test/unit_test/robot/client_test.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,149 @@
import 'package:flutter_test/flutter_test.dart';
import 'package:grpc/grpc.dart';
import 'package:viam_sdk/src/components/motor/client.dart';
import 'package:viam_sdk/src/components/motor/motor.dart';
import 'package:viam_sdk/src/components/motor/service.dart';
import 'package:viam_sdk/src/exceptions.dart';
import 'package:viam_sdk/src/gen/common/v1/common.pb.dart';
import 'package:viam_sdk/src/resource/base.dart';
import 'package:viam_sdk/src/resource/manager.dart';
import 'package:viam_sdk/src/resource/registry.dart';

import '../../test_utils.dart';
import '../components/motor_test.dart';

void main() {
group('RobotClient getResource on-demand creation', () {
// These tests verify the on-demand resource client creation logic
// used by RobotClient.getResource(). Since RobotClient requires a
// live connection, we test the same code path through its components:
// ResourceManager + Registry.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

The test group title says "RobotClient getResource on-demand creation" but none of the tests call RobotClient.getResource() — they reproduce its internal logic inline via ResourceManager + Registry. The actual method under test is never exercised. This means a regression in how RobotClient.getResource assembles those calls (e.g. wrong field used as the name, wrong channel passed) would not be caught. Ideally these tests should be integration-style tests that call robot.getResource<T>(name) on a real or fake RobotClient.

test('creates resource client on-demand for registered subtype', () {
final manager = ResourceManager();
final channel = ClientChannel(
'localhost',
port: 9999,
options: const ChannelOptions(credentials: ChannelCredentials.insecure()),
);

addTearDown(() => channel.shutdown());

final name = Motor.getResourceName('my_motor');

// Resource is not in the manager
expect(() => manager.getResource<Motor>(name), throwsA(isA<ResourceNotFoundException>()));

// Simulate on-demand creation (same logic as RobotClient.getResource)
final registration = Registry.instance.lookupSubtype(Subtype.fromResourceName(name));
final resource = registration.rpcClientCreator(name.name, channel);
manager.register(name, resource);

// Resource client was created with correct type and name
final result = manager.getResource<Motor>(name);
expect(result, isA<MotorClient>());
expect(result.name, 'my_motor');
});

test('returns cached resource on subsequent calls', () {
final manager = ResourceManager();
final channel = ClientChannel(
'localhost',
port: 9999,
options: const ChannelOptions(credentials: ChannelCredentials.insecure()),
);

addTearDown(() => channel.shutdown());

final name = Motor.getResourceName('my_motor');

// Create and register on-demand
final registration = Registry.instance.lookupSubtype(Subtype.fromResourceName(name));
final resource = registration.rpcClientCreator(name.name, channel);
manager.register(name, resource);

// Second call returns the same cached instance
final first = manager.getResource<Motor>(name);
final second = manager.getResource<Motor>(name);
expect(identical(first, second), true);
});

test('throws for unregistered subtype', () {
final unknownName = ResourceName()
..namespace = resourceNamespaceRDK
..type = resourceTypeComponent
..subtype = 'unknown_type'
..name = 'foo';

expect(
() => Registry.instance.lookupSubtype(Subtype.fromResourceName(unknownName)),
throwsA(isA<Exception>()),
);
});

test('on-demand client works end-to-end with real server', () async {
// Set up a real Motor gRPC server
const name = 'test_motor';
final motor = FakeMotor(name);
final manager = ResourceManager();
manager.register(Motor.getResourceName(name), motor);
final service = MotorService(manager);
final server = Server.create(services: [service]);
await serveServerAtUnusedPort(server);
final channel = ClientChannel(
'localhost',
port: server.port!,
options: const ChannelOptions(credentials: ChannelCredentials.insecure()),
);

addTearDown(() async {
await channel.shutdown();
await server.shutdown();
});

// Create a fresh manager (simulating a resource NOT returned by refresh)
final clientManager = ResourceManager();
final resourceName = Motor.getResourceName(name);

// Resource is not in the client manager
expect(() => clientManager.getResource<Motor>(resourceName), throwsA(isA<ResourceNotFoundException>()));

// On-demand creation (same logic as RobotClient.getResource)
final registration = Registry.instance.lookupSubtype(Subtype.fromResourceName(resourceName));
final resource = registration.rpcClientCreator(resourceName.name, channel);
clientManager.register(resourceName, resource);

// The on-demand created client can communicate with the server
final client = clientManager.getResource<Motor>(resourceName);
expect(client, isA<MotorClient>());
expect(await client.position(), 0);
await client.setPower(0.5);
expect(motor.power, 0.5);
});

test('on-demand client for resource not on server fails at call time, not creation time', () async {
final channel = ClientChannel(
'localhost',
port: 9999,
options: const ChannelOptions(credentials: ChannelCredentials.insecure()),
);

addTearDown(() => channel.shutdown());

final manager = ResourceManager();
final resourceName = Motor.getResourceName('nonexistent_motor');

// On-demand creation succeeds (no server check)
final registration = Registry.instance.lookupSubtype(Subtype.fromResourceName(resourceName));
final resource = registration.rpcClientCreator(resourceName.name, channel);
manager.register(resourceName, resource);

final client = manager.getResource<Motor>(resourceName);
expect(client, isA<MotorClient>());
expect(client.name, 'nonexistent_motor');

// Error only surfaces when actually calling the resource
await expectLater(client.position(), throwsA(anything));
});
});
}
Loading