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

Handle absent start file when collecting files #1085

Closed

Conversation

HannesWell
Copy link
Contributor

In PR #1071 simplified some of the IO operations in passage.
Unfortunately I did not consider the case that the start-file of a walk can be not existing.
This PR aims to fix that case.

@codecov
Copy link

codecov bot commented May 27, 2022

Codecov Report

Merging #1085 (513544c) into master (cd43e81) will decrease coverage by 0.00%.
The diff coverage is 42.85%.

@@             Coverage Diff              @@
##             master    #1085      +/-   ##
============================================
- Coverage     33.08%   33.07%   -0.01%     
  Complexity      346      346              
============================================
  Files          1156     1156              
  Lines         25612    25617       +5     
  Branches       1588     1589       +1     
============================================
+ Hits           8473     8474       +1     
- Misses        16635    16637       +2     
- Partials        504      506       +2     
Impacted Files Coverage Δ
...rg/eclipse/passage/lic/base/io/FileCollection.java 75.00% <33.33%> (-10.72%) ⬇️
...ge/lic/base/conditions/mining/LocalConditions.java 44.44% <50.00%> (-1.02%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cd43e81...513544c. Read the comment docs.

Copy link
Contributor

@eparovyshnaya eparovyshnaya left a comment

Choose a reason for hiding this comment

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

Dear @HannesWell
thank you very much for your contribution.

Indeed there is an issue here, but it must be solved outside of this utility scope.

We do not apply fail-safe approach to core functionality, so the solution should be redone. There are also code quality issues.

return Files.walk(path) //
.filter(Files::isRegularFile);
static Stream<Path> filesIn(Path path) throws IOException {
Stream<Path> paths = Files.exists(path) ? Files.walk(path) : Stream.empty();
Copy link
Contributor

@eparovyshnaya eparovyshnaya May 27, 2022

Choose a reason for hiding this comment

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

Not existing directory cannot be mapped to empty set of files. For one reason, there is no way to tell the difference between erroneous situation (absent folder) and quite valid one (indeed no files of desired extension).

FileCollection must not take responsibility for this kind of semantics, as it is dedicated to a single very much clear purpose.

The case of invalid path definitely corresponds to a development error, thus IllegalArgumentException must be thrown, coupled with the class documentation update.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Understand. After checking the previous code again the reason for the current failure is that before the mentioned PR the IOException caught in FileCollection accidentally was not propagated as LicensingException. Throwing a LicensingException was at least the intention before. From the IOException alone it cannot be distinguished (reliably) why it occurred. So if you relly want an IllegalArgumentException here the existence of the start file/directory has to be checked initially.

I also checked all non-test call-sides of FileCollection.get()and think that LocalConditions.licenses() is the only caller where a non-existing start is a valid case.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let me put it in a simpler way: fail safe is dangerous practice, Passage core has been designed on fail fast.

In this case not-existing path is a developer's error. It must be fixed by adding some ensutring code to FileCollection client.

FileCollection has clear meaning and responsibility. It must not divise answers for illegal state that can be easily avoided by caller side.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me put it in a simpler way: fail safe is dangerous practice, Passage core has been designed on fail fast.

In this case not-existing path is a developer's error. It must be fixed by adding some ensutring code to FileCollection client.

I understand that and just wanted to point out, that this was not done before. But of course it can be improved. :)

The latest commit throws an IllegalArgumentException if the collection root does not exist.

@HannesWell HannesWell force-pushed the fixFileCollecting branch from 76ba5ed to ad9bb5f Compare May 27, 2022 16:17
@HannesWell HannesWell force-pushed the fixFileCollecting branch from ad9bb5f to 513544c Compare May 27, 2022 18:18
Copy link
Contributor

@eparovyshnaya eparovyshnaya left a comment

Choose a reason for hiding this comment

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

Path supplier ois the party responsible for the valid output.

LocalConditions as well as FileCollection must simply rely on the supplier at the respect

@@ -41,15 +41,18 @@ public FileCollection(Supplier<Path> base, PassageFileExtension extension) {
this.extensions = extension;
}

public Collection<Path> get() throws LicensingException {
try (Stream<Path> all = filesIn(base.get())) {
public Collection<Path> get() throws LicensingException, IllegalArgumentException {
Copy link
Contributor

Choose a reason for hiding this comment

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

IllegalArgumentException is RuntimeException

is must not be declared as it is a part of interface

IllegalArgumentException means development, not runtime error.

if (Files.exists(base.get())) {
return new FileCollection(base, scope).get();
} else {
return Collections.emptyList();
Copy link
Contributor

Choose a reason for hiding this comment

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

Again, empty set is legal, where incorrect configuration (wrong path) is not.

Supplier must provide existing path, and LocalConditions must rely on it.

base(LicensedProduct declaration can benefit from doc mentioning the fact though.

Copy link
Contributor Author

@HannesWell HannesWell May 29, 2022

Choose a reason for hiding this comment

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

Supplier must provide existing path, and LocalConditions must rely on it.

I'm not sure if this is really intended? If I understand it correctly in one application multiple different local-conditions can be specified so that a user can choose where to place a license file. For example in <user-home>/.passage/<path-to-product-version> or within the installation location.
I think it is more user-friend if it is sufficient to choose one location to put the license file into than to be forced to place it into every available location.
If that is the case, it is perfectly legal that for example UserHomeCondition.base() method returns a path that does not exists (if the user put the license file into the installation location maybe not even <user-home>/.passage must exists).

And I don't know a better place where to check that. Please suggest one, if you do.

Alternatively the supplier could return an Optional<Path> and sub-classes would have to return an empty Optional if the path does not exists. But I suspect that this requires more code since each sub-class where it is not an error if the path is absent has to check the existence in its base() implementation.

Copy link
Contributor

@eparovyshnaya eparovyshnaya May 30, 2022

Choose a reason for hiding this comment

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

Condition mining does only mining.
Condition mining cannot do so, when equipped with not-existing path to mine over.

Equip condition mining with existing path and be sure you get all conditions from the location.
There are several MinedConditions implementations, each configure existing base path.

If you implement custom version of MinedConditions on base of LocalConditions, just make sure it never gets invalid base path.

If you are not ready to guarantee this, you can create your own implementation of pure MinedConditions interface and rely on your custom assumptions in it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Condition mining does only mining. Condition mining cannot do so, when equipped with not-existing path to mine over.

IMHO it should also be valid and would make sense to mine over a non-existing path and consider it as empty.

Equip condition mining with existing path and be sure you get all conditions from the location. There are several MinedConditions implementations, each configure existing base path.

I cannot confirm that. For example the UserHomeResidentConditions uses paths, that do not necessarily exist (at least I cannot find any code path that ensures the existence).

And for example if I launch Passages License-Operator in version 2.4.0-M3 for the very first time, i.e. without having a license file and without having the agreements yet accepted so that <user-home>/.passage/org.eclipse.passage.loc.operator.product/2.4.0/ does not exist, I get the following errors:

image

IMHO it is valid that for example <user-home>/.passage/org.eclipse.passage.loc.operator.product/2.4.0/ does not exist at the very first start and I think it would lead to a bad user experience to require that a user has to create those folders manually in advance.
And in general the path searched by the UserHomeResidentConditions could remain empty if the license file is placed at an alternative location and the product has no agreements. Therefore I think it would be quite inconvenient to require its existence.

Copy link
Contributor

Choose a reason for hiding this comment

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

Have created personal ticket for Passage Operator startup issue: #1088

@eparovyshnaya
Copy link
Contributor

Here I'm closing the request.
Thank you again, @HannesWell, for your time.

@HannesWell
Copy link
Contributor Author

Here I'm closing the request. Thank you again, @HannesWell, for your time.

In favour of the general usability and even for Passage's own products I'm strongly convinced the changes suggested for LocalConditions should be considered.

@eparovyshnaya
Copy link
Contributor

As verbosely described above, proposed changes violate not only code design principles of the Passage core, but also archtectural integrity.

To highlight again: Passage is absolutely open for extensions. It publicly declares all the interfaces that allow you to implement your own mechanics on any other code and architecture quality standards.

@HannesWell HannesWell deleted the fixFileCollecting branch May 31, 2022 12:10
@HannesWell
Copy link
Contributor Author

To highlight again: Passage is absolutely open for extensions. It publicly declares all the interfaces that allow you to implement your own mechanics on any other code and architecture quality standards.

Actually code-quality is important for me, but I have to admit that I'm not used to the code and architecture-style used in Passage. I fully respect it, but it makes it harder for meet the code-style requirements for contributions.
Nevertheless I'm interested in your solution of #1088.

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