Skip to content
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

[FAB-18508] ledger utility always outputs txNum #2724

Merged
merged 1 commit into from
Jul 2, 2021

Conversation

shimos
Copy link
Contributor

@shimos shimos commented Jun 29, 2021

Type of change

  • Bug fix

Description

This patch fixes a bug reported in FAB-18508, and makes the ledger troubleshooting utility output always the record "txNum".

Additional details

The bug occurred when the target transaction is the first transaction of a block (txNum == 0). This patch removes the omitempty attributes from all the members of a record.

Note that this patch also changes the output when a key is missing in either snapshot. Instead of an empty object ({}) for the missing snapshot, the entire key (either "snapshotrecord1" or "snapshotrecord2") will be omitted.
(Refer to the changes in the test code)

Related issues

https://jira.hyperledger.org/projects/FAB/issues/FAB-18508

@shimos shimos requested a review from a team as a code owner June 29, 2021 08:26
Copy link
Contributor

@manish-sethi manish-sethi left a comment

Choose a reason for hiding this comment

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

Thanks @shimos for your contribution. Left you a comment.

@@ -498,13 +484,11 @@ func TestJSONArrayFileWriter(t *testing.T) {
"value" : "green",
"blockNum" : 472,
"txNum" : 61
},
"snapshotrecord2" : {}
Copy link
Contributor

Choose a reason for hiding this comment

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

This was an intentional entry that was chosen to represent a missing record. Unless there is a strong reason, I would prefer to keep it as is.

@shimos shimos force-pushed the fab-18508 branch 2 times, most recently from df24901 to 97d4df5 Compare July 1, 2021 02:56
@shimos
Copy link
Contributor Author

shimos commented Jul 1, 2021

@manish-sethi Thank you for the review. I have revised the patch to keep the JSON structure.

Comment on lines 191 to 192
Record1 *snapshotRecord `json:"snapshotrecord1,omitempty"`
Record2 *snapshotRecord `json:"snapshotrecord2,omitempty"`
Namespace string `json:"namespace,omitempty"`
Key string `json:"key,omitempty"`
Record1 interface{} `json:"snapshotrecord1,omitempty"`
Record2 interface{} `json:"snapshotrecord2,omitempty"`
Copy link
Contributor

@manish-sethi manish-sethi Jul 1, 2021

Choose a reason for hiding this comment

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

It was better to use the concrete type, as before. On a second thought, as an alternate we could remove "omitempty" in the json annotation for Record1 and Record2. That will force to print a nil object as null in the final JSON -- which I feel would be even clearer than {}.

Sorry for an additional round of change on this.

@shimos shimos force-pushed the fab-18508 branch 2 times, most recently from 57defb8 to b12bac5 Compare July 2, 2021 01:37
This patch fixes a bug reported in FAB-18508, and makes the ledger
troubleshooting utility output always the record "txNum".

The bug occurred when the target transaction is the first transaction of
a block (i.e. txNum == 0). This patch removes the `omitempty` attributes
from all the members of a record.

Note that this patch also changes the output when a key is missing in
either snapshot. For a missing snapshot, 'null' will be output instead
of an empty object ({}).

Signed-off-by: Taku Shimosawa <taku.shimosawa.uf@hitachi.com>
@shimos
Copy link
Contributor Author

shimos commented Jul 2, 2021

@manish-sethi ok, so I have reverted to the first patch, and deleted 'omitempty' from snapshotrecord1 and snapshotrecord2 so that it will produce null for the missing key rather than deleting the key itself.

@manish-sethi
Copy link
Contributor

Thanks @shimos for your contribution!

@manish-sethi manish-sethi merged commit 9c3d459 into hyperledger:main Jul 2, 2021
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