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

Disallowing user to use incomplete fully qualified Check names in config file #4456

Closed
romani opened this Issue Jun 14, 2017 · 1 comment

Comments

Projects
None yet
1 participant
@romani
Member

romani commented Jun 14, 2017

base on #4414 , #4387

code quality question was raised at #4430 (review) , see discussion from thsi comment and down for details.

What to do:

  1. no changes to functionality is expected , except for disallowing to user use names like com.puppycrawl.tools.checkstyle.checks.naming.ConstantName, if user use fully qualified name, it should be absolutely correct ! Expected fully qualified Check name is com.puppycrawl.tools.checkstyle.checks.naming.ConstantNameCheck

  2. Additionally there should refactoring to avoid logic that is based on catching reflection exceptions.
    We do have all information in collection like thirdPartyNameToFullModuleNames and NAME_TO_FULL_MODULE_NAME (at keys and values). So we could predict cases where will not have success to load class by reflection at createObject(code)

It is expected to have code like below(deliver to user all possible classpath issues):

    private Object createObject(String className) throws CheckstyleException {
        Object instance = null;
            try {
               Class<?> clazz = Class.forName(className, true, moduleClassLoader);

                final Constructor<?> declaredConstructor = clazz.getDeclaredConstructor();
                declaredConstructor.setAccessible(true);
                instance = declaredConstructor.newInstance();
            }
            catch (final ReflectiveOperationException  | NoClassDefFoundError  ex) {
                throw new CheckstyleException("Unable to instantiate " + className, ex);
            }
        return instance;
}

code:


        if (instance == null) {
            instance = createObject(name);
        }
        final String nameCheck = name + CHECK_SUFFIX;
        if (instance == null) {
            instance = createObject(nameCheck);
        }
        if (instance == null) {
 .....

should be extended to validate collection and call createObject only when we sure about success to load class.

@romani romani added the approved label Jun 14, 2017

@romani romani added the bug label Jun 14, 2017

djydewang added a commit to djydewang/checkstyle that referenced this issue Jul 15, 2017

djydewang added a commit to djydewang/checkstyle that referenced this issue Aug 14, 2017

djydewang added a commit to djydewang/checkstyle that referenced this issue Sep 25, 2017

djydewang added a commit to djydewang/checkstyle that referenced this issue Sep 25, 2017

djydewang added a commit to djydewang/checkstyle that referenced this issue Sep 25, 2017

djydewang added a commit to djydewang/checkstyle that referenced this issue Oct 3, 2017

djydewang added a commit to djydewang/checkstyle that referenced this issue Oct 4, 2017

djydewang added a commit to djydewang/checkstyle that referenced this issue Oct 10, 2017

djydewang added a commit to djydewang/checkstyle that referenced this issue Oct 12, 2017

djydewang added a commit to djydewang/checkstyle that referenced this issue Oct 12, 2017

djydewang added a commit to djydewang/checkstyle that referenced this issue Oct 12, 2017

djydewang added a commit to djydewang/checkstyle that referenced this issue Oct 23, 2017

romani added a commit that referenced this issue Oct 28, 2017

@romani romani added breaking compatibility and removed bug labels Oct 28, 2017

@romani romani added this to the 8.4 milestone Oct 28, 2017

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Oct 28, 2017

Member

@djydewang , thanks a lot for elegant fix!

Member

romani commented Oct 28, 2017

@djydewang , thanks a lot for elegant fix!

@romani romani closed this Oct 28, 2017

romani added a commit to sevntu-checkstyle/checkstyle-samples that referenced this issue Oct 29, 2017

romani added a commit to sevntu-checkstyle/sevntu.checkstyle that referenced this issue Oct 29, 2017

romani added a commit to sevntu-checkstyle/sevntu.checkstyle that referenced this issue Oct 29, 2017

timurt added a commit to timurt/checkstyle that referenced this issue Dec 19, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment