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
45 changes: 38 additions & 7 deletions packages/bruno-requests/src/grpc/grpc-client.js
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,8 @@ const normalizeWindowsNamedPipe = (pipePath) => {
const getParsedGrpcUrlObject = (url) => {
const addProtocolIfMissing = (str) => {
if (str.includes('://')) return str;
if (str.includes('localhost') || str.includes('127.0.0.1')) {
const lower = str.toLowerCase();
if (lower.includes('localhost') || lower.includes('127.0.0.1')) {
return `grpc://${str}`;
}
return `grpcs://${str}`;
Expand All @@ -133,11 +134,11 @@ const getParsedGrpcUrlObject = (url) => {
return { host: normalizeWindowsNamedPipe(url), path: '', protocol: 'pipe', isLocalTransport: true };
}

const urlObj = new URL(addProtocolIfMissing(url.toLowerCase()));
const urlObj = new URL(addProtocolIfMissing(url));

return {
host: urlObj.host,
protocol: urlObj.protocol.replace(':', ''),
protocol: urlObj.protocol.replace(':', '').toLowerCase(),
path: removeTrailingSlash(urlObj.pathname),
isLocalTransport: false
};
Expand Down Expand Up @@ -228,8 +229,14 @@ class GrpcClient {
* @param {grpc.ChannelOptions} options - channel options
* @returns {Promise<{ client: GrpcReflection, services: string[], callOptions: Object }>}
*/
async #getReflectionClient(host, credentials = ChannelCredentials.createInsecure(), metadata = null, options = {}) {
const makeClient = (version) => new GrpcReflection(host, credentials, options, version);
async #getReflectionClient(host, credentials = ChannelCredentials.createInsecure(), metadata = null, options = {}, pathPrefix = '') {
const makeClient = (version) => {
const client = new GrpcReflection(host, credentials, options, version);
if (pathPrefix) {
this.#applyPathPrefix(client, host, credentials, options, pathPrefix);
}
return client;
};
const callOptions = this.#createCallOptions(metadata);

let client;
Expand All @@ -250,6 +257,30 @@ class GrpcClient {
return { client, services, callOptions };
}

/**
* Replace a GrpcReflection instance's internal gRPC client with one
* whose method paths are prefixed with the URL subpath.
* This allows reflection to work when the gRPC server is hosted behind a URL subpath.
*/
#applyPathPrefix(reflectionInstance, host, credentials, options, prefix) {
const originalClient = reflectionInstance.client;
const serviceDef = originalClient.constructor?.service;
if (!serviceDef) return;

const prefixedDef = {};
for (const [name, def] of Object.entries(serviceDef)) {
prefixedDef[name] = { ...def, path: prefix + def.path };
}

const PrefixedClient = makeGenericClientConstructor(prefixedDef);
const prefixedClient = new PrefixedClient(host, credentials, options);
// Close the original client's channel to prevent leaks
if (typeof originalClient.close === 'function') {
originalClient.close();
}
reflectionInstance.client = prefixedClient;
}

/**
* Close a GrpcReflection client's underlying gRPC channel.
* GrpcReflection doesn't expose a close() method, so we access the
Expand Down Expand Up @@ -783,7 +814,7 @@ class GrpcClient {

let reflectionClient = null;
try {
const { client, services, callOptions } = await this.#getReflectionClient(targetHost, credentials, metadata, mergedChannelOptions);
const { client, services, callOptions } = await this.#getReflectionClient(targetHost, credentials, metadata, mergedChannelOptions, path);
reflectionClient = client;

const methods = [];
Expand Down Expand Up @@ -1003,7 +1034,7 @@ class GrpcClient {
} else if (protocol === 'pipe') {
console.warn('Windows named pipes are not directly supported by grpcurl');
parts.push('-plaintext');
} else if (url.startsWith('grpcs://') || url.startsWith('https://')) {
} else if (protocol === 'grpcs' || protocol === 'https') {
if (ca) {
/**
* Instead of using certificate that relies on CN, use SANs
Expand Down
92 changes: 91 additions & 1 deletion packages/bruno-requests/src/grpc/grpc-client.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
// Store captured values for assertions
let capturedChannelOptions = null;
let capturedHost = null;
let capturedRequestPath = null;

// Mock GrpcReflection to capture options
const mockListServices = jest.fn().mockResolvedValue(['test.Service']);
Expand Down Expand Up @@ -73,7 +74,10 @@ jest.mock('@grpc/grpc-js', () => {
const mockRpc = createMockRpc();
return {
close: jest.fn(),
makeUnaryRequest: jest.fn().mockReturnValue(mockRpc),
makeUnaryRequest: jest.fn().mockImplementation((path) => {
capturedRequestPath = path;
return mockRpc;
}),
makeClientStreamRequest: jest.fn().mockReturnValue(mockRpc),
makeServerStreamRequest: jest.fn().mockReturnValue(mockRpc),
makeBidiStreamRequest: jest.fn().mockReturnValue(mockRpc)
Expand Down Expand Up @@ -109,6 +113,7 @@ describe('GrpcClient', () => {
jest.clearAllMocks();
capturedChannelOptions = null;
capturedHost = null;
capturedRequestPath = null;
mockEventCallback = jest.fn();
grpcClient = new GrpcClient(mockEventCallback);
});
Expand Down Expand Up @@ -715,4 +720,89 @@ describe('GrpcClient', () => {
expect(capturedHost).toBe('myserver:50051');
});
});

describe('URL subpath support in startConnection', () => {
const baseCollection = {
uid: 'test-collection-uid',
pathname: '/test/path'
};

beforeEach(() => {
grpcClient.methods.set('/test.Service/TestMethod', {
path: '/test.Service/TestMethod',
requestStream: false,
responseStream: false,
requestSerialize: (val) => Buffer.from(JSON.stringify(val)),
responseDeserialize: (val) => JSON.parse(val.toString())
});
});

test('should include URL subpath in the gRPC request path', async () => {
const request = {
url: 'grpcs://myserver:443/my-subpath',
uid: 'test-request-uid',
method: '/test.Service/TestMethod',
headers: {},
body: { grpc: [{ content: '{}' }] }
};

await grpcClient.startConnection({
request,
collection: baseCollection
});

expect(capturedRequestPath).toBe('/my-subpath/test.Service/TestMethod');
});

test('should preserve URL subpath case sensitivity', async () => {
const request = {
url: 'grpcs://myserver:443/MySubPath',
uid: 'test-request-uid',
method: '/test.Service/TestMethod',
headers: {},
body: { grpc: [{ content: '{}' }] }
};

await grpcClient.startConnection({
request,
collection: baseCollection
});

expect(capturedRequestPath).toBe('/MySubPath/test.Service/TestMethod');
});

test('should work without subpath (standard URL)', async () => {
const request = {
url: 'grpc://myserver:50051',
uid: 'test-request-uid',
method: '/test.Service/TestMethod',
headers: {},
body: { grpc: [{ content: '{}' }] }
};

await grpcClient.startConnection({
request,
collection: baseCollection
});

expect(capturedRequestPath).toBe('/test.Service/TestMethod');
});

test('should connect to host without subpath in channel target', async () => {
const request = {
url: 'grpcs://myserver:443/my-subpath',
uid: 'test-request-uid',
method: '/test.Service/TestMethod',
headers: {},
body: { grpc: [{ content: '{}' }] }
};

await grpcClient.startConnection({
request,
collection: baseCollection
});

expect(capturedHost).toBe('myserver:443');
});
});
Comment on lines +724 to +807
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Add a reflection-specific subpath test as well.

These cases cover startConnection, but the new behavior in this PR also changes loadMethodsFromReflection() for subpath-hosted servers. Without one test around reflection on a URL like grpcs://host/prefix, method discovery can regress while this suite still passes.

As per coding guidelines, **/*.{test,spec}.{js,jsx,ts,tsx}: Add tests for any new functionality or meaningful changes.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/bruno-requests/src/grpc/grpc-client.spec.js` around lines 724 - 807,
The test suite adds URL subpath coverage for startConnection but misses a
similar test for reflection-based method discovery, so add a spec that calls
loadMethodsFromReflection with a request URL containing a subpath (e.g.,
'grpcs://host:443/prefix'), using the same baseCollection and mocked reflection
response, then assert that capturedRequestPath includes the subpath (e.g.,
'/prefix/grpc.reflection.v1alpha.ServerReflection/ServerReflectionInfo' or the
appropriate reflection method path), that capturedHost is 'host:443', and that
grpcClient loaded the expected methods; reference loadMethodsFromReflection,
startConnection, capturedRequestPath, capturedHost, and the mocked
grpcClient.methods.setup used in the file to locate where to add the new test.

});
Loading