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

Copy statedb/couchdb recursively discarding non .json files to create index for public and private data assets. #108

Merged
merged 7 commits into from
Mar 27, 2024

Conversation

davidfdr
Copy link
Contributor

Related to issue: #106

Problem when private data indexing is needed.

… index for public and private data assets.

Signed-off-by: David F D Reis <[email protected]>
@davidfdr davidfdr changed the title Copy statedb/couchdb recursively discarding non .json files to create index for public and private data assets. [WIP] Copy statedb/couchdb recursively discarding non .json files to create index for public and private data assets. Mar 25, 2024
@davidfdr davidfdr marked this pull request as ready for review March 25, 2024 21:54
@davidfdr davidfdr changed the title [WIP] Copy statedb/couchdb recursively discarding non .json files to create index for public and private data assets. Copy statedb/couchdb recursively discarding non .json files to create index for public and private data assets. Mar 26, 2024
Copy link
Member

@jt-nti jt-nti left a comment

Choose a reason for hiding this comment

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

Thanks for the pull request! It would be great to get it merged- there are a few lint errors to fix up, and I've added some tests in PR #109 which aren't passing yet. (I had been wondering about using filepath.Rel(indexSrcDir, src) and the filepath.Match function to get them passing.)

Also, the skip function gets a FileInfo which should simplify things a bit.

Thanks again!

internal/util/copy.go Outdated Show resolved Hide resolved
@davidfdr davidfdr requested a review from jt-nti March 26, 2024 20:59
Copy link
Member

@jt-nti jt-nti left a comment

Choose a reason for hiding this comment

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

Thanks for getting the linting and tests passing! Just a few minor comments to tidy things up and I think it'll be ready to merge.

internal/util/copy.go Show resolved Hide resolved
}

if info.IsDir() {
return skipFolder(logger, indexSrcDir, src)
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be better to check for any errors here instead of just passing them on, e.g.

skip, err := skipFolder(logger, indexSrcDir, src)
if err != nil {
  return skip, fmt.Errorf(
			"<suitable error message with strings for dir/src being checked>: %w",
			...,
			err,
		)
}

@@ -83,6 +99,45 @@ func CopyIndexFiles(logger *log.CmdLogger, src, dest string) error {
return nil
}

// skipFolder checks if the folder will need to be skipped during indexes copy.
func skipFolder(logger *log.CmdLogger, indexSrcDir, src string) (bool, error) {
path, _ := filepath.Rel(indexSrcDir, src)
Copy link
Member

Choose a reason for hiding this comment

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

Needs to handle any errors. I would also be tempted to do this outside the function and just pass in the path, although maybe the error handling would be neater if it stays here. Either would be fine.

func skipFolder(logger *log.CmdLogger, indexSrcDir, src string) (bool, error) {
path, _ := filepath.Rel(indexSrcDir, src)

matchContainsPublicIndexFolder, _ := filepath.Match("index*", path)
Copy link
Member

Choose a reason for hiding this comment

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

The pattern should be more specific, i.e. indexes/*, otherwise things like "indexed/" would also match.

path, _ := filepath.Rel(indexSrcDir, src)

matchContainsPublicIndexFolder, _ := filepath.Match("index*", path)
matchContainsPrivateDataCollectionFolder, _ := filepath.Match("collections*", path)
Copy link
Member

Choose a reason for hiding this comment

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

Similarly collections/* although that's what you have for matchPrivateDataCollectionFolder. I don't think you need both unless I've misunderstood.

Copy link
Contributor Author

@davidfdr davidfdr Mar 27, 2024

Choose a reason for hiding this comment

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

The first folder after couchdb can be only collections or indexes.

matchContainsPublicIndexFolder, _ := filepath.Match("index*", path)
matchContainsPrivateDataCollectionFolder, _ := filepath.Match("collections*", path)
matchPrivateDataCollectionFolder, _ := filepath.Match("collections/*", path)
matchPrivateDataCollectionIndexFolder, _ := filepath.Match("collections/*/indexes", path)
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if this pattern should be, collections/*/indexes/*

Copy link
Contributor Author

@davidfdr davidfdr Mar 27, 2024

Choose a reason for hiding this comment

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

collections/*/indexes/* will permit this collections/somefolderCollectionName/indexes/anotherfolder correct? And this is not good right?

matchContainsPrivateDataCollectionFolder, _ := filepath.Match("collections*", path)
matchPrivateDataCollectionFolder, _ := filepath.Match("collections/*", path)
matchPrivateDataCollectionIndexFolder, _ := filepath.Match("collections/*/indexes", path)
relativeFoldersLength := len(strings.Split(path, "/"))
Copy link
Member

Choose a reason for hiding this comment

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

This is stretching my knowledge of platform specifics a bit (I'm wondering about the / in the match patterns!) but splitting the path using filepath.Separator would probably be better.

Copy link
Member

@jt-nti jt-nti left a comment

Choose a reason for hiding this comment

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

Thanks very much for persevering with the PR! Getting a fix merged for the private data collection indexes is fantastic.

@jt-nti jt-nti merged commit fdc5486 into hyperledger-labs:main Mar 27, 2024
9 checks passed
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