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.

Sign up for GitHub

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
wants to merge 1 commit into from
Closed

Handle absent start file when collecting files #1085

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion ....passage.lic.base/src/org/eclipse/passage/lic/base/conditions/mining/LocalConditions.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
*******************************************************************************/
package org.eclipse.passage.lic.base.conditions.mining;

import java.nio.file.Files;
import java.nio.file.Path;
import java.util.Collection;
import java.util.Collections;
Expand Down Expand Up @@ -102,7 +103,12 @@ private ServiceInvocationResult<Collection<ConditionPack>> noLicenses(LicensedPr
}

private Collection<Path> licenses(LicensedProduct product) throws LicensingException {
return new FileCollection(base(product), scope).get();
Supplier<Path> base = base(product);
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 •
edited
Loading

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 •
edited
Loading

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

}
}

protected abstract Supplier<Path> base(LicensedProduct product);
Expand Down
9 changes: 6 additions & 3 deletions bundles/org.eclipse.passage.lic.base/src/org/eclipse/passage/lic/base/io/FileCollection.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

try (Stream<Path> all = files(base.get())) {
return filtered(all);
} catch (IOException e) {
throw new LicensingException(BaseMessages.getString("FileCollection.failure"), e); //$NON-NLS-1$
}
}

private Stream<Path> filesIn(Path path) throws IOException {
private Stream<Path> files(Path path) throws IOException {
if (!Files.exists(path)) {
throw new IllegalArgumentException("Root of file collection does not exist"); //$NON-NLS-1$
}
return Files.walk(path) //
.filter(Files::isRegularFile);
}
Expand Down