Skip to content

Implement and test getMultipleStates method for Stub API. #454

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.

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

ArunRawat404
Copy link

@ArunRawat404 ArunRawat404 commented May 23, 2025

Issue

#453

Description

This PR implements the getMultipleStates method to ChaincodeStub and includes the following updates:

  1. Implementation of getMultipleStates(...keys) method to retrieve multiple state values from the given keys.
  2. Handler support for this via handleGetMultipleStates and handleOneSendGetMultipleStates in handler.js.
  3. Unit tests for getMultipleStates to verify:
    • It returns correct results when keys are provided.
    • It returns an empty array when no keys are passed.
    • It throws on handler rejection.

This method mirrors functionality found in fabric-chaincode-go

@ArunRawat404 ArunRawat404 requested a review from a team as a code owner May 23, 2025 18:14
@ArunRawat404 ArunRawat404 force-pushed the feature/getMultipleStates-implementation branch from 53f29da to 1c30228 Compare May 23, 2025 18:23
Copy link
Member

@bestbeforetoday bestbeforetoday left a comment

Choose a reason for hiding this comment

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

Thank you for the nice clear code. A few comments where it might be possible to optimize the implementation.

The stub unit tests fit exactly the style of the existing tests, which is great. This change is lacking tests of the handler though, which is where all the complex logic lives.

It should be possible to drive unit tests of the stub with a real handler implementation behind it, with stubbing/mocking done underneath the handler to avoid actual gRPC calls. This would test both the stub and accompanying handler implementation together, which would give more confidence that they work together and avoid the need for separate stub and handler unit tests.

Ideally I would mock out the gRPC client stubs or the gRPC client itself so that it receives and returns appropriate gRPC messages. This would be a much more significant change. Don't worry about that for now unless you really want to.

Comment on lines +394 to +406
if (keys.length === 0) {
return [];
}

const responses = [];

if (!this.usePeerGetMultipleKeys) {
for (const key of keys) {
const resp = await this.handleGetState(collection, key, channel_id, txId);
responses.push(resp);
}
return responses;
}
Copy link
Member

Choose a reason for hiding this comment

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

It might be possible to simplify this, similar to the following:

Suggested change
if (keys.length === 0) {
return [];
}
const responses = [];
if (!this.usePeerGetMultipleKeys) {
for (const key of keys) {
const resp = await this.handleGetState(collection, key, channel_id, txId);
responses.push(resp);
}
return responses;
}
if (!this.usePeerGetMultipleKeys) {
const promises = keys.map((key) =>
this.handleGetState(collection, key, channel_id, tx_Id)
);
return Promise.all(promises);
}

return responses;
}

let remainingKeys = [...keys];
Copy link
Member

Choose a reason for hiding this comment

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

I guess you are doing this to take a copy and avoid modifying the keys array passed in by the caller? I don't think this will be necessary since slice will return a new array rather than modifying the original array. If it is a concern, it might be worth adding a unit test to confirm that the caller's array is not modified.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, remainingKeys is not needed as slice don't modify the original array.

Comment on lines +409 to +421
while (remainingKeys.length > this.maxSizeGetMultipleKeys) {
const batch = remainingKeys.slice(0, this.maxSizeGetMultipleKeys);
const resp = await this.handleOneSendGetMultipleStates(collection, batch, channel_id, txId);
responses.push(...resp);
remainingKeys = remainingKeys.slice(this.maxSizeGetMultipleKeys);
}

if (remainingKeys.length > 0) {
const resp = await this.handleOneSendGetMultipleStates(collection, remainingKeys, channel_id, txId);
responses.push(...resp);
}

return responses.map(r => (r.length === 0 ? null : r));
Copy link
Member

@bestbeforetoday bestbeforetoday May 26, 2025

Choose a reason for hiding this comment

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

slice handles out of bounds indexes cleanly so this should not need the multiple loops that the Go implementation uses. You might be able to simplify the implementation similar to the following:

Suggested change
while (remainingKeys.length > this.maxSizeGetMultipleKeys) {
const batch = remainingKeys.slice(0, this.maxSizeGetMultipleKeys);
const resp = await this.handleOneSendGetMultipleStates(collection, batch, channel_id, txId);
responses.push(...resp);
remainingKeys = remainingKeys.slice(this.maxSizeGetMultipleKeys);
}
if (remainingKeys.length > 0) {
const resp = await this.handleOneSendGetMultipleStates(collection, remainingKeys, channel_id, txId);
responses.push(...resp);
}
return responses.map(r => (r.length === 0 ? null : r));
const batchPromises = [];
for (
;
keys.length > 0;
keys = keys.slice(this.maxSizeGetMultipleKeys)
) {
const batch = keys.slice(0, this.maxSizeGetMultipleKeys);
const promise = this.handleOneSendGetMultipleStates(
collection,
batch,
channel_id,
txId
);
batchPromises.push(promise);
}
const batchResponses = await Promise.all(batchPromises);
return batchResponses.flat();

@@ -612,6 +612,56 @@ describe('Stub', () => {
});
});

describe.only('getMultipleStates', () => {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
describe.only('getMultipleStates', () => {
describe('getMultipleStates', () => {

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants