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-18509] Stop panic if collection index path is wrong #2726

Merged
merged 1 commit into from
Jul 2, 2021

Conversation

mbwhite
Copy link
Member

@mbwhite mbwhite commented Jun 30, 2021

Signed-off-by: Matthew B White whitemat@uk.ibm.com

Type of change

  • Bug fix

@mbwhite mbwhite requested a review from a team as a code owner June 30, 2021 09:16
@manish-sethi manish-sethi changed the title [FAB-18509] Stop panic of collection index path is wrong [FAB-18509] Stop panic if collection index path is wrong Jun 30, 2021
@@ -420,21 +420,26 @@ const (
// Example for chaincode indexes:
// "META-INF/statedb/couchdb/indexes/indexColorSortName.json"
chaincodeIndexDirDepth = 3
pathLengthForCCindexes = 5
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
pathLengthForCCindexes = 5
pathLengthForCCindexes = 4

Path that comes here is of directory only. The file name is not part of this.

Copy link
Member Author

Choose a reason for hiding this comment

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

thanks for the review Manish - this is the output I got from the unit test - with printing the args to the getIndexInfo function. Includes the index file name

=== RUN   TestGetIndexInfo
[getIndexInfo] indexPath=META-INF/statedb/couchdb/indexes/indexColorSortName.json
[getIndexInfo] indexPath=META-INF/statedb/couchdb/collections/collectionMarbles/indexes/indexCollMarbles.json
[getIndexInfo] indexPath=META-INF/statedb/couchdb/indexColorSortName.json
[getIndexInfo] indexPath=META-INF/statedb/couchdb/collections/indexes/indexCollMarbles.json
[getIndexInfo] indexPath=META-INF/statedb/couchdb/collections/indexCollMarbles.json
[getIndexInfo] indexPath=META-INF/statedb/
--- PASS: TestGetIndexInfo (0.00s)

Copy link
Contributor

Choose a reason for hiding this comment

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

@mbwhite Existing TestGetIndexInfo() has a bug too. It should not pass the path including the index def, i.e., *.json. The input to getIndexInfo() comes from the HandleDeployChaincode() which passes only the directory excluding the actual filename.

We need to fix

  1. TestGetIndexInfo()
  2. Comments (provided in the const block)
  3. getIndexInfo()

Copy link
Contributor

Choose a reason for hiding this comment

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

Some tests in kvledger package and integration test also fail after making getIndexInfo() to consider the full path including the index file.

Copy link
Member Author

Choose a reason for hiding this comment

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

@cendhu ah.. that makes more sense now thanks... what can we say about the directory path that is passed in?
The code as-is is uses '/' to split the path, is it safe to assume the input will not have leading or trailing directory separators?

(aside I assume as well that Fabric is solely intended to work on a Linux filesystem? / not being the universal file separator :-) )

Copy link
Contributor

Choose a reason for hiding this comment

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

@mbwhite

func ExtractFileEntries(tarBytes []byte, databaseType string) (map[string][]*TarFileEntry, error) {
This function extracts files entries from the chaincode package (tar bytes). Hence, I assume no leading or trailing directory separator unless there is a bug in ExtractFileEntries(). I do not see a unit test for ExtractFileEntries(). Not sure whether it is tested somewhere and I miss it. Hence, it is better to verify that.

As far as I know, even on Windows, we recommend docker to be used (https://hyperledger-fabric.readthedocs.io/en/release-1.4/prereqs.html) and our docker base image is alpine linux. Hence, / as a file separator is okay.

Copy link
Member Author

Choose a reason for hiding this comment

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

After a bit more research I believe there are some in db_test.go. My reading of that function is that it later functions can safely assume there are no leading or trailing /

My background in cross-platform development (pre Docker) has made me question whenever I see anything about a filesystem 'hard coded' like a /

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. Thanks for confirming.

collectionDirDepth = 3
collectionNameDepth = 4
collectionIndexDirDepth = 5
pathLengthForCollectionindexes = 7
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
pathLengthForCollectionindexes = 7
pathLengthForCollectionindexes = 6

same as above

@mbwhite mbwhite force-pushed the index-dir-checks branch 3 times, most recently from 9866539 to d5fedc0 Compare July 1, 2021 10:38
@mbwhite mbwhite requested a review from manish-sethi July 1, 2021 10:39
@mbwhite
Copy link
Member Author

mbwhite commented Jul 2, 2021

/ci-run

Signed-off-by: Matthew B White <whitemat@uk.ibm.com>
@mbwhite mbwhite force-pushed the index-dir-checks branch from 589a8af to 600fc7c Compare July 2, 2021 12:53
@manish-sethi manish-sethi merged commit 1249da2 into hyperledger:main Jul 2, 2021
@manish-sethi
Copy link
Contributor

@Mergifyio backport release-2.2

@manish-sethi
Copy link
Contributor

@Mergifyio backport release-2.3

@mergify
Copy link

mergify bot commented Jul 7, 2021

Command backport release-2.2: success

Backports have been created

mergify bot pushed a commit that referenced this pull request Jul 7, 2021
Signed-off-by: Matthew B White <whitemat@uk.ibm.com>
(cherry picked from commit 1249da2)
@mergify
Copy link

mergify bot commented Jul 7, 2021

Command backport release-2.3: success

Backports have been created

mergify bot pushed a commit that referenced this pull request Jul 7, 2021
Signed-off-by: Matthew B White <whitemat@uk.ibm.com>
(cherry picked from commit 1249da2)
manish-sethi pushed a commit that referenced this pull request Jul 12, 2021
Signed-off-by: Matthew B White <whitemat@uk.ibm.com>
(cherry picked from commit 1249da2)
manish-sethi pushed a commit that referenced this pull request Jul 12, 2021
Signed-off-by: Matthew B White <whitemat@uk.ibm.com>
(cherry picked from commit 1249da2)
C0rWin pushed a commit to C0rWin/fabric that referenced this pull request Sep 24, 2022
…#2726) (hyperledger#2745)

Signed-off-by: Matthew B White <whitemat@uk.ibm.com>
(cherry picked from commit 1249da2)
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.

3 participants