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

Skip registration for files and ignore files with insufficient rights #15

Merged
merged 7 commits into from
May 29, 2024

Conversation

sven1103
Copy link
Contributor

Currently the registration fails silently if the dataset to register
is not wrapped in a directory.

Also, the dataset registration fails, when the application has not sufficient rights to execute and write to the dataset folder (e.g. wrong access group, etc).

@github-actions github-actions bot added the fix label May 29, 2024
@KochTobi KochTobi requested review from KochTobi and a team May 29, 2024 07:26
KochTobi
KochTobi previously approved these changes May 29, 2024
Copy link
Member

@KochTobi KochTobi left a comment

Choose a reason for hiding this comment

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

LGTM
One question about the directory check

private List<RegistrationRequest> createRequests(File[] files, Path userDirectory) {
if (files == null || files.length == 0) {
return new ArrayList<>();
}
return Arrays.stream(files).filter(file -> !file.isHidden())
return Arrays.stream(files).filter(file -> !file.isHidden()).filter(File::isDirectory)
Copy link
Member

Choose a reason for hiding this comment

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

I would argue that the filtering for directories should take place earlier. In the detectDataForRegistration there is a variable userProcessDirectories containing both files that are and file that are not directories?
Why is the check in line 74 not sufficient?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point!

I added another filter method and apply all filters before the createRequest method, since it violates the definition of a function.

sven1103 added 2 commits May 29, 2024 09:45
…m:qbicsoftware/data-processing into fix/registration-data-must-be-a-directory
if (file.isHidden()) {
return false;
}
return !file.isFile();
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
return !file.isFile();
return file.isDirectory();

Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
0.0% Coverage on New Code (required ≥ 80%)

See analysis details on SonarCloud

Copy link
Member

@KochTobi KochTobi left a comment

Choose a reason for hiding this comment

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

LGTM

@sven1103 sven1103 merged commit d3ae472 into main May 29, 2024
7 of 8 checks passed
@sven1103 sven1103 deleted the fix/registration-data-must-be-a-directory branch May 29, 2024 08:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants