diff --git a/import-control.xml b/import-control.xml index af553fe9e..db12c3902 100755 --- a/import-control.xml +++ b/import-control.xml @@ -15,23 +15,24 @@ - - - - - + + + + + - + + - - @@ -42,25 +43,14 @@ - - - - - - - - - - - \ No newline at end of file diff --git a/src/checkstyle/com/puppycrawl/tools/checkstyle/checks/imports/Guard.java b/src/checkstyle/com/puppycrawl/tools/checkstyle/checks/imports/Guard.java index 3432ca09b..368b1a688 100755 --- a/src/checkstyle/com/puppycrawl/tools/checkstyle/checks/imports/Guard.java +++ b/src/checkstyle/com/puppycrawl/tools/checkstyle/checks/imports/Guard.java @@ -28,29 +28,96 @@ class Guard private final boolean mAllowed; /** Package to control access to. */ private final String mPkgName; + /** Package to control access to. */ + private final String mClassName; + + /** + * Indicates if must be an exact match. Only valid if guard using a + * package. + */ + private final boolean mExactMatch; + /** Indicates if the guard only applies to this package. */ + private final boolean mLocalOnly; /** * Constructs an instance. - * @param aAllowed whether the package is allowed. - * @param aPkgName the package name. + * @param aAllow whether to allow access. + * @param aLocalOnly whether guard is to be applied locally only + * @param aPkgName the package to apply guard on. + * @param aExactMatch whether the package must match exactly. */ - Guard(final boolean aAllowed, final String aPkgName) + Guard(final boolean aAllow, final boolean aLocalOnly, + final String aPkgName, final boolean aExactMatch) { - mAllowed = aAllowed; + mAllowed = aAllow; + mLocalOnly = aLocalOnly; mPkgName = aPkgName; + mClassName = null; + mExactMatch = aExactMatch; + } + + /** + * Constructs an instance. + * @param aAllow whether to allow access. + * @param aLocalOnly whether guard is to be applied locally only + * @param aClassName the class to apply guard on. + */ + Guard(final boolean aAllow, final boolean aLocalOnly, + final String aClassName) + { + mAllowed = aAllow; + mLocalOnly = aLocalOnly; + mPkgName = null; + mClassName = aClassName; + mExactMatch = true; // not used. } /** * Verifies whether a package name be used. - * @param aName the package to check. + * @param aForImport the package to check. + * @param aInPkg the package doing the import. * @return a result {@link AccessResult} indicating whether it can be used. */ - AccessResult verify(final String aName) + AccessResult verifyImport(final String aForImport, final String aInPkg) { - assert aName != null; - if (!aName.startsWith(mPkgName + ".")) { - return AccessResult.UNKNOWN; + assert aForImport != null; + if (mClassName != null) { + final boolean classMatch = mClassName.equals(aForImport); + return calculateResult(classMatch); } - return mAllowed ? AccessResult.ALLOWED : AccessResult.DISALLOWED; + + // Must be checking a package. First check that we actually match + // the package. Then check if matched and we must be an exact match. + // In this case, the text after the first "." must not contain + // another "." as this indicates that it is not an exact match. + assert mPkgName != null; + //boolean pkgMatch = aForImport.startsWith(mPkgName + "."); + boolean pkgMatch = aForImport.startsWith(mPkgName + "."); + if (pkgMatch && mExactMatch) { + pkgMatch = (aForImport.indexOf('.', (mPkgName.length() + 1)) == -1); + } + return calculateResult(pkgMatch); + } + + /** + * @return returns whether the guard is to only be applied locally. + */ + boolean isLocalOnly() + { + return mLocalOnly; + } + + /** + * Returns the appropriate {@link AccessResult} based on whether there + * was a match and if the guard is to allow access. + * @param aMatched indicates whether there was a match. + * @return An appropriate {@link AccessResult}. + */ + private AccessResult calculateResult(final boolean aMatched) + { + if (aMatched) { + return mAllowed ? AccessResult.ALLOWED : AccessResult.DISALLOWED; + } + return AccessResult.UNKNOWN; } } diff --git a/src/checkstyle/com/puppycrawl/tools/checkstyle/checks/imports/ImportControlCheck.java b/src/checkstyle/com/puppycrawl/tools/checkstyle/checks/imports/ImportControlCheck.java index 64f73e03a..e03a56fee 100755 --- a/src/checkstyle/com/puppycrawl/tools/checkstyle/checks/imports/ImportControlCheck.java +++ b/src/checkstyle/com/puppycrawl/tools/checkstyle/checks/imports/ImportControlCheck.java @@ -52,6 +52,9 @@ public class ImportControlCheck extends Check { /** The root package controller. */ private PkgControl mRoot; + /** The package doing the import. */ + private String mInPkg; + /** * The package controller for the current file. Used for performance * optimisation. @@ -75,7 +78,8 @@ public class ImportControlCheck extends Check log(nameAST, "import.control.missing.file"); } else { - mCurrentLeaf = mRoot.locateFinest(full.getText()); + mInPkg = full.getText(); + mCurrentLeaf = mRoot.locateFinest(mInPkg); } } else if (mCurrentLeaf != null) { @@ -87,7 +91,8 @@ public class ImportControlCheck extends Check imp = FullIdent.createFullIdent((DetailAST) aAST .getFirstChild().getNextSibling()); } - final AccessResult access = mCurrentLeaf.checkAccess(imp.getText()); + final AccessResult access = mCurrentLeaf.checkAccess(imp.getText(), + mInPkg); if (!AccessResult.ALLOWED.equals(access)) { log(aAST, "import.control.disallowed", imp.getText()); } diff --git a/src/checkstyle/com/puppycrawl/tools/checkstyle/checks/imports/ImportControlLoader.java b/src/checkstyle/com/puppycrawl/tools/checkstyle/checks/imports/ImportControlLoader.java index 23e2d5d89..d316277ca 100755 --- a/src/checkstyle/com/puppycrawl/tools/checkstyle/checks/imports/ImportControlLoader.java +++ b/src/checkstyle/com/puppycrawl/tools/checkstyle/checks/imports/ImportControlLoader.java @@ -59,7 +59,9 @@ final class ImportControlLoader extends AbstractLoader /** {@inheritDoc} */ public void startElement(final String aNamespaceURI, - final String aLocalName, final String aQName, final Attributes aAtts) + final String aLocalName, + final String aQName, + final Attributes aAtts) throws SAXException { if (aQName.equals("import-control")) { @@ -73,9 +75,25 @@ final class ImportControlLoader extends AbstractLoader } else if (aQName.equals("allow") || aQName.equals("disallow")) { assert mStack.size() > 0; - final String pkg = safeGet(aAtts, "pkg"); + // Need to handle either "pkg" or "class" attribute. + // May have "exact-match" for "pkg" + // May have "local-only" + final boolean isAllow = aQName.equals("allow"); + final boolean isLocalOnly = (aAtts.getValue("local-only") != null); + final String pkg = aAtts.getValue("pkg"); + final Guard g; + if (pkg != null) { + final boolean exactMatch = + (aAtts.getValue("exact-match") != null); + g = new Guard(isAllow, isLocalOnly, pkg, exactMatch); + } + else { + final String clazz = safeGet(aAtts, "class"); + g = new Guard(isAllow, isLocalOnly, clazz); + } + final PkgControl pc = (PkgControl) mStack.peek(); - pc.addGuard(new Guard(aQName.equals("allow"), pkg)); + pc.addGuard(g); } } diff --git a/src/checkstyle/com/puppycrawl/tools/checkstyle/checks/imports/PkgControl.java b/src/checkstyle/com/puppycrawl/tools/checkstyle/checks/imports/PkgControl.java index 48b475ed2..82aa6f175 100755 --- a/src/checkstyle/com/puppycrawl/tools/checkstyle/checks/imports/PkgControl.java +++ b/src/checkstyle/com/puppycrawl/tools/checkstyle/checks/imports/PkgControl.java @@ -117,12 +117,13 @@ class PkgControl * its parent looking for a match. This will recurse looking for match. * If there is no clear result then {@link AccessResult#UNKNOWN} is * returned. - * @param aForPkg the package to check on. + * @param aForImport the package to check on. + * @param aInPkg the package doing the import. * @return an {@link AccessResult}. */ - AccessResult checkAccess(final String aForPkg) + AccessResult checkAccess(final String aForImport, final String aInPkg) { - AccessResult retVal = localCheckAccess(aForPkg); + AccessResult retVal = localCheckAccess(aForImport, aInPkg); if (retVal != AccessResult.UNKNOWN) { return retVal; } @@ -131,21 +132,27 @@ class PkgControl return AccessResult.DISALLOWED; } - return mParent.checkAccess(aForPkg); + return mParent.checkAccess(aForImport, aInPkg); } /** * Checks whether any of the guards for this node control access to * a specified package. - * @param aForPkg the package to check. + * @param aForImport the package to check. + * @param aInPkg the package doing the import. * @return an {@link AccessResult}. */ - private AccessResult localCheckAccess(final String aForPkg) + private AccessResult localCheckAccess(final String aForImport, + final String aInPkg) { final Iterator it = mGuards.iterator(); while (it.hasNext()) { final Guard g = (Guard) it.next(); - final AccessResult result = g.verify(aForPkg); + // Check if a Guard is only meant to be applied locally. + if (g.isLocalOnly() && !mFullPackage.equals(aInPkg)) { + continue; + } + final AccessResult result = g.verifyImport(aForImport, aInPkg); if (result != AccessResult.UNKNOWN) { return result; } diff --git a/src/checkstyle/com/puppycrawl/tools/checkstyle/checks/imports/import_control_1_0.dtd b/src/checkstyle/com/puppycrawl/tools/checkstyle/checks/imports/import_control_1_0.dtd index 9f9ecd2c5..102957db8 100755 --- a/src/checkstyle/com/puppycrawl/tools/checkstyle/checks/imports/import_control_1_0.dtd +++ b/src/checkstyle/com/puppycrawl/tools/checkstyle/checks/imports/import_control_1_0.dtd @@ -1,4 +1,4 @@ - + @@ -10,12 +10,16 @@ - + + + %attlist.guard;> - + %attlist.guard;> diff --git a/src/tests/com/puppycrawl/tools/checkstyle/checks/imports/GuardTest.java b/src/tests/com/puppycrawl/tools/checkstyle/checks/imports/GuardTest.java index 06884cc34..3fe207279 100755 --- a/src/tests/com/puppycrawl/tools/checkstyle/checks/imports/GuardTest.java +++ b/src/tests/com/puppycrawl/tools/checkstyle/checks/imports/GuardTest.java @@ -6,13 +6,38 @@ import junit.framework.TestCase; public class GuardTest extends TestCase { - public void testGuard() + public void testPkgGuard1() { - final Guard g = new Guard(true, "pkg"); + final Guard g = new Guard(true, false, "pkg", false); assertNotNull(g); - assertEquals(AccessResult.UNKNOWN, g.verify("asda")); - assertEquals(AccessResult.UNKNOWN, g.verify("p")); - assertEquals(AccessResult.ALLOWED, g.verify("pkg.a")); - assertEquals(AccessResult.UNKNOWN, g.verify("pkg")); + assertEquals(AccessResult.UNKNOWN, g.verifyImport("asda", "ignored")); + assertEquals(AccessResult.UNKNOWN, g.verifyImport("p", "ignored")); + assertEquals(AccessResult.UNKNOWN, g.verifyImport("pkga", "ignored")); + assertEquals(AccessResult.ALLOWED, g.verifyImport("pkg.a", "ignored")); + assertEquals(AccessResult.ALLOWED, g.verifyImport("pkg.a.b", "ignored")); + assertEquals(AccessResult.UNKNOWN, g.verifyImport("pkg", "ignored")); + } + + public void testPkgGuard2() + { + final Guard g = new Guard(true, false, "pkg", true); + assertNotNull(g); + assertEquals(AccessResult.UNKNOWN, g.verifyImport("asda", "ignored")); + assertEquals(AccessResult.UNKNOWN, g.verifyImport("p", "ignored")); + assertEquals(AccessResult.ALLOWED, g.verifyImport("pkg.a", "ignored")); + assertEquals(AccessResult.UNKNOWN, g.verifyImport("pkg.a.b", "ignored")); + assertEquals(AccessResult.UNKNOWN, g.verifyImport("pkg", "ignored")); + } + + public void testClassGuard() + { + final Guard g = new Guard(true, false, "pkg.a"); + assertNotNull(g); + assertEquals(AccessResult.UNKNOWN, g.verifyImport("asda", "ignored")); + assertEquals(AccessResult.UNKNOWN, g.verifyImport("p", "ignored")); + assertEquals(AccessResult.UNKNOWN, g.verifyImport("pkga", "ignored")); + assertEquals(AccessResult.ALLOWED, g.verifyImport("pkg.a", "ignored")); + assertEquals(AccessResult.UNKNOWN, g.verifyImport("pkg.a.b", "ignored")); + assertEquals(AccessResult.UNKNOWN, g.verifyImport("pkg", "ignored")); } } diff --git a/src/tests/com/puppycrawl/tools/checkstyle/checks/imports/PkgControlTest.java b/src/tests/com/puppycrawl/tools/checkstyle/checks/imports/PkgControlTest.java index 33c4b9c65..5323e34ea 100755 --- a/src/tests/com/puppycrawl/tools/checkstyle/checks/imports/PkgControlTest.java +++ b/src/tests/com/puppycrawl/tools/checkstyle/checks/imports/PkgControlTest.java @@ -13,11 +13,11 @@ public class PkgControlTest extends TestCase protected void setUp() throws Exception { super.setUp(); - pcRoot.addGuard(new Guard(false, "org.springframework")); - pcRoot.addGuard(new Guard(false, "org.hibernate")); - pcRoot.addGuard(new Guard(true, "org.apache.commons")); + pcRoot.addGuard(new Guard(false, false, "org.springframework", false)); + pcRoot.addGuard(new Guard(false, false, "org.hibernate", false)); + pcRoot.addGuard(new Guard(true, false, "org.apache.commons", false)); - pcCommon.addGuard(new Guard(true, "org.hibernate")); + pcCommon.addGuard(new Guard(true, false, "org.hibernate", false)); } public void testFullPkg() @@ -37,17 +37,19 @@ public class PkgControlTest extends TestCase public void testCheckAccess() { - assertEquals(AccessResult.DISALLOWED, pcCommon - .checkAccess("org.springframework.something")); + assertEquals(AccessResult.DISALLOWED, pcCommon.checkAccess( + "org.springframework.something", + "com.kazgroup.courtlink.common")); assertEquals(AccessResult.ALLOWED, pcCommon - .checkAccess("org.apache.commons.something")); - assertEquals(AccessResult.DISALLOWED, pcCommon - .checkAccess("org.apache.commons")); - assertEquals(AccessResult.ALLOWED, pcCommon - .checkAccess("org.hibernate.something")); - assertEquals(AccessResult.DISALLOWED, pcCommon - .checkAccess("com.badpackage.something")); - assertEquals(AccessResult.DISALLOWED, pcRoot - .checkAccess("org.hibernate.something")); + .checkAccess("org.apache.commons.something", + "com.kazgroup.courtlink.common")); + assertEquals(AccessResult.DISALLOWED, pcCommon.checkAccess( + "org.apache.commons", "com.kazgroup.courtlink.common")); + assertEquals(AccessResult.ALLOWED, pcCommon.checkAccess( + "org.hibernate.something", "com.kazgroup.courtlink.common")); + assertEquals(AccessResult.DISALLOWED, pcCommon.checkAccess( + "com.badpackage.something", "com.kazgroup.courtlink.common")); + assertEquals(AccessResult.DISALLOWED, pcRoot.checkAccess( + "org.hibernate.something", "com.kazgroup.courtlink")); } }