-
Notifications
You must be signed in to change notification settings - Fork 23
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
rework directory scanner to ensure it can enforce exclusions and it uses path #54
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,29 @@ | ||
package org.apache.maven.shared.utils.io; | ||
|
||
/* | ||
* Licensed to the Apache Software Foundation (ASF) under one | ||
* or more contributor license agreements. See the NOTICE file | ||
* distributed with this work for additional information | ||
* regarding copyright ownership. The ASF licenses this file | ||
* to you under the Apache License, Version 2.0 (the | ||
* "License"); you may not use this file except in compliance | ||
* with the License. You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, | ||
* software distributed under the License is distributed on an | ||
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
* KIND, either express or implied. See the License for the | ||
* specific language governing permissions and limitations | ||
* under the License. | ||
*/ | ||
|
||
/** | ||
* Enables a {@link ScanConductor} to access the {@link DirectoryScanner} configuration. | ||
* It is set once the scanner is initialized. | ||
*/ | ||
public interface ScannerAware | ||
{ | ||
void setDirectoryScanner( DirectoryScanner scanner ); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,58 @@ | ||
package org.apache.maven.shared.utils.io.conductor; | ||
|
||
/* | ||
* Licensed to the Apache Software Foundation (ASF) under one | ||
* or more contributor license agreements. See the NOTICE file | ||
* distributed with this work for additional information | ||
* regarding copyright ownership. The ASF licenses this file | ||
* to you under the Apache License, Version 2.0 (the | ||
* "License"); you may not use this file except in compliance | ||
* with the License. You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, | ||
* software distributed under the License is distributed on an | ||
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
* KIND, either express or implied. See the License for the | ||
* specific language governing permissions and limitations | ||
* under the License. | ||
*/ | ||
|
||
import org.apache.maven.shared.utils.io.DirectoryScanner; | ||
import org.apache.maven.shared.utils.io.MatchPatterns; | ||
import org.apache.maven.shared.utils.io.ScanConductor; | ||
import org.apache.maven.shared.utils.io.ScannerAware; | ||
|
||
import java.io.File; | ||
|
||
/** | ||
* If an exclude is defined on a folder it does not visit of the children | ||
* even if some include can match children. | ||
*/ | ||
public class EnforceExcludesOverIncludes implements ScanConductor, ScannerAware | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wouldn't it make sense if this class would directly implement https://docs.oracle.com/javase/7/docs/api/java/nio/file/DirectoryStream.Filter.html instead? That way one could get rid of the ScanConductor interface... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree with @kwin let's make it slim |
||
{ | ||
private MatchPatterns excludes; | ||
|
||
@Override | ||
public ScanAction visitDirectory( final String name, final File directory ) | ||
{ | ||
if ( excludes.matches( name, true ) ) | ||
{ | ||
return ScanAction.NO_RECURSE; | ||
} | ||
return ScanAction.CONTINUE; | ||
} | ||
|
||
@Override | ||
public ScanAction visitFile( final String name, final File file ) | ||
{ | ||
return ScanAction.CONTINUE; | ||
} | ||
|
||
@Override | ||
public void setDirectoryScanner( final DirectoryScanner scanner ) | ||
{ | ||
excludes = scanner.getExcludesPatterns(); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -162,7 +162,7 @@ public void followSymlinksFalse() | |
ds.scan(); | ||
List<String> included = Arrays.asList( ds.getIncludedFiles() ); | ||
assertAlwaysIncluded( included ); | ||
assertEquals( 9, included.size() ); | ||
assertEquals( included.toString(), 9, included.size() ); | ||
List<String> includedDirs = Arrays.asList( ds.getIncludedDirectories() ); | ||
assertTrue( includedDirs.contains( "" ) ); | ||
assertTrue( includedDirs.contains( "aRegularDir" ) ); | ||
|
@@ -243,14 +243,6 @@ public void testSimpleExcludes() | |
/* expExclDirs */ NONE ); | ||
} | ||
|
||
public void testIsSymbolicLink() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why is this test removed? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No more relevant since there we use directly Files API and it was testing an internal |
||
throws IOException | ||
{ | ||
File file = new File( "." ); | ||
DirectoryScanner ds = new DirectoryScanner(); | ||
ds.isSymbolicLink( file, "abc" ); | ||
} | ||
|
||
/** | ||
* Performs a scan and test for the given parameters if not null. | ||
*/ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,92 @@ | ||
package org.apache.maven.shared.utils.io.conductor; | ||
|
||
/* | ||
* Licensed to the Apache Software Foundation (ASF) under one | ||
* or more contributor license agreements. See the NOTICE file | ||
* distributed with this work for additional information | ||
* regarding copyright ownership. The ASF licenses this file | ||
* to you under the Apache License, Version 2.0 (the | ||
* "License"); you may not use this file except in compliance | ||
* with the License. You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, | ||
* software distributed under the License is distributed on an | ||
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
* KIND, either express or implied. See the License for the | ||
* specific language governing permissions and limitations | ||
* under the License. | ||
*/ | ||
|
||
import org.apache.maven.shared.utils.io.DirectoryScanner; | ||
import org.junit.Rule; | ||
import org.junit.Test; | ||
import org.junit.rules.TemporaryFolder; | ||
|
||
import java.io.File; | ||
import java.io.FileWriter; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. danger: this class depends on platform default encoding. Avoid. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it is fine, it is just used to "touch" the file, not write content |
||
import java.io.IOException; | ||
import java.util.ArrayList; | ||
import java.util.Collections; | ||
import java.util.List; | ||
|
||
import static java.util.Arrays.asList; | ||
import static java.util.Collections.singletonList; | ||
import static org.junit.Assert.assertEquals; | ||
|
||
public class EnforceExcludesOverIncludesTest | ||
{ | ||
@Rule | ||
public final TemporaryFolder folder = new TemporaryFolder(); | ||
|
||
@Test | ||
public void dontVisitChildren() throws IOException | ||
{ | ||
createFakeFiles(); | ||
|
||
DirectoryScanner scanner = new DirectoryScanner(); | ||
scanner.setScanConductor(new EnforceExcludesOverIncludes()); | ||
scanner.setExcludes( "**/target/**" ); | ||
scanner.setIncludes( "**" ); // files in target will match include but our conductor will bypass them anyway | ||
scanner.setBasedir( folder.getRoot() ); | ||
scanner.scan(); | ||
assertResultEquals( asList( "bar", "sub/other" ), scanner.getIncludedFiles() ); | ||
assertResultEquals( singletonList( "target" ), scanner.getExcludedDirectories() ); | ||
} | ||
|
||
@Test // we don't set the conductor to ensure we have a "control" test to compare to the other | ||
public void controlTest() throws IOException | ||
{ | ||
createFakeFiles(); | ||
|
||
DirectoryScanner scanner = new DirectoryScanner(); | ||
scanner.setExcludes( "**/target/**" ); | ||
scanner.setIncludes( "**" ); // files in target will match include but our conductor will bypass them anyway | ||
scanner.setBasedir( folder.getRoot() ); | ||
scanner.scan(); | ||
assertResultEquals( asList( "bar", "sub/other" ), scanner.getIncludedFiles() ); | ||
assertResultEquals( asList( "target", "target/nested" ), scanner.getExcludedDirectories() ); | ||
} | ||
|
||
private void createFakeFiles() throws IOException | ||
{ | ||
touch(new File(folder.getRoot(), "bar")); | ||
touch(new File(folder.getRoot(), "sub/other")); | ||
touch(new File(folder.getRoot(), "target/foo")); | ||
touch(new File(folder.getRoot(), "target/nested/dummy")); | ||
} | ||
|
||
private void assertResultEquals( final List<String> expected, final String[] actual ) | ||
{ | ||
final List<String> actualList = new ArrayList<>( asList( actual ) ); | ||
Collections.sort( actualList ); | ||
assertEquals( expected, actualList ); | ||
} | ||
|
||
private void touch( final File file ) throws IOException | ||
{ | ||
file.getParentFile().mkdirs(); | ||
file.createNewFile(); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please retain this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this comment is wrong, it shouldn't have been merged IMO