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

ImportControl: blacklist mode #3451

Closed
romani opened this Issue Sep 19, 2016 · 6 comments

Comments

Projects
None yet
3 participants
@romani
Member

romani commented Sep 19, 2016

base on #3358

I can't figure out how to put ImportControl into blacklist mode, aka except everything but the things I've listed. (Using allow pkg="" seems to allow everything and ignore disallow blocks.)

We need to think about this....... there might be some problems with priority of rule apply.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani
Member

romani commented Sep 20, 2016

@rnveach rnveach added the medium label Mar 2, 2017

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Apr 13, 2017

Member

@timurt , please help us with this issue.
Issue is not certain , so it requires investigation in behavior first.
If "blacklist mode" is already possible, most likely we need to update documentation to make it clear.
if "blacklist mode" is not possible - please provide your ideas for design first on how to make it happen. Please do not code before approval of design.

Member

romani commented Apr 13, 2017

@timurt , please help us with this issue.
Issue is not certain , so it requires investigation in behavior first.
If "blacklist mode" is already possible, most likely we need to update documentation to make it clear.
if "blacklist mode" is not possible - please provide your ideas for design first on how to make it happen. Please do not code before approval of design.

@timurt

This comment has been minimized.

Show comment
Hide comment
@timurt

timurt Apr 16, 2017

Collaborator

@romani Ok, first of all let me describe behaviour shortly:

  1. What is not allowed explicitly - is disallowed by default.
  2. We start checking from the closest to the current package described inside config file, and moving towards the root.

except everything but the things I've listed

I have found one way we can do this, but I am not sure is it logically correct
We always start checking allows and disallows in the current package, if there is not certain rule, we go up to the parent and continue checking until we reach the root.
Solution I found is to store disallows in the current package and have 'allow everything' rule in the parent package.

<import-control pkg="com.puppycrawl.tools.checkstyle.checks">
  <allow class=".*" regex="true"/>
  <subpackage name="imports">
    <disallow class="java\..*\.Image" local-only="true" regex="true"/>
  </subpackage>
</import-control>

The main disadvantage of such implementation is 'allow everything' rule will be used by all subpackages of this parent. Here is example

<import-control pkg="com.puppycrawl.tools.checkstyle.checks">
  <allow class=".*" regex="true"/>
  <subpackage name="imports">
    <disallow class="java\..*\.Image" local-only="true" regex="true"/>
  </subpackage>
  <subpackage name="metrics">
    <allow pkg="javax.swing" exact-match="true"/>
  </subpackage>
</import-control>

What if we want javax.swing to be the only allowed package for classes inside com.puppycrawl.tools.checkstyle.checks.metrics, but by this implementation all classes are allowed inside this package.
I am not sure, but maybe we can avoid it by using several ImportControl checks.

Additional attributes

What I suggest, as I mentioned before there is rule 'What is not allowed explicitly - is disallowed by default.'. What if we add new property for subpackage tag, lets call it strategy which will have three possible values: allowed, disallowed, parent. This property will specify strategy in a case if no suitable allow/disallow is found.
allowed value means - if there is no certain allow/disallow rule in current package, then import is allowed by default
disallowed value means - if there is no certain allow/disallow rule in current package, then import is disallowed by default
parent value means - if there is no certain allow/disallow rule in current package, then we should go up and check parent.
By default parent strategy should be used.

For import-control we should add flag property which will identify what to do in a case when no suitable rule was found. Lets call it disallowed, by default it has true value. disallowed="false" means
what is not disallowed explicitly - is allowed by default.

Example

<import-control pkg="com.puppycrawl.tools.checkstyle.checks" disallowed="false">
  <allow class="java.awt.Image"/>
  <allow class="java.io.File" local-only="true"/>
  <subpackage name="imports" strategy="parent">
    <disallow class="java.awt.Image" local-only="true"/>
    <allow pkg="javax.swing" exact-match="true"/>
    <allow pkg="java.io" exact-match="true" local-only="true"/>
  </subpackage>
</import-control>

In this example we can see if we could not find suitable allow/disallow inside subpackage name="imports" then we go to the parent. If there is no suitable allow/disallow inside parent package, then import is allowed by default

Collaborator

timurt commented Apr 16, 2017

@romani Ok, first of all let me describe behaviour shortly:

  1. What is not allowed explicitly - is disallowed by default.
  2. We start checking from the closest to the current package described inside config file, and moving towards the root.

except everything but the things I've listed

I have found one way we can do this, but I am not sure is it logically correct
We always start checking allows and disallows in the current package, if there is not certain rule, we go up to the parent and continue checking until we reach the root.
Solution I found is to store disallows in the current package and have 'allow everything' rule in the parent package.

<import-control pkg="com.puppycrawl.tools.checkstyle.checks">
  <allow class=".*" regex="true"/>
  <subpackage name="imports">
    <disallow class="java\..*\.Image" local-only="true" regex="true"/>
  </subpackage>
</import-control>

The main disadvantage of such implementation is 'allow everything' rule will be used by all subpackages of this parent. Here is example

<import-control pkg="com.puppycrawl.tools.checkstyle.checks">
  <allow class=".*" regex="true"/>
  <subpackage name="imports">
    <disallow class="java\..*\.Image" local-only="true" regex="true"/>
  </subpackage>
  <subpackage name="metrics">
    <allow pkg="javax.swing" exact-match="true"/>
  </subpackage>
</import-control>

What if we want javax.swing to be the only allowed package for classes inside com.puppycrawl.tools.checkstyle.checks.metrics, but by this implementation all classes are allowed inside this package.
I am not sure, but maybe we can avoid it by using several ImportControl checks.

Additional attributes

What I suggest, as I mentioned before there is rule 'What is not allowed explicitly - is disallowed by default.'. What if we add new property for subpackage tag, lets call it strategy which will have three possible values: allowed, disallowed, parent. This property will specify strategy in a case if no suitable allow/disallow is found.
allowed value means - if there is no certain allow/disallow rule in current package, then import is allowed by default
disallowed value means - if there is no certain allow/disallow rule in current package, then import is disallowed by default
parent value means - if there is no certain allow/disallow rule in current package, then we should go up and check parent.
By default parent strategy should be used.

For import-control we should add flag property which will identify what to do in a case when no suitable rule was found. Lets call it disallowed, by default it has true value. disallowed="false" means
what is not disallowed explicitly - is allowed by default.

Example

<import-control pkg="com.puppycrawl.tools.checkstyle.checks" disallowed="false">
  <allow class="java.awt.Image"/>
  <allow class="java.io.File" local-only="true"/>
  <subpackage name="imports" strategy="parent">
    <disallow class="java.awt.Image" local-only="true"/>
    <allow pkg="javax.swing" exact-match="true"/>
    <allow pkg="java.io" exact-match="true" local-only="true"/>
  </subpackage>
</import-control>

In this example we can see if we could not find suitable allow/disallow inside subpackage name="imports" then we go to the parent. If there is no suitable allow/disallow inside parent package, then import is allowed by default

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Apr 21, 2017

Member

@timurt ,

first of all let me describe behaviour shortly:

confirmed, good summary, please do PR right now to update xdoc to explain users how it works now.
Please also explain user need for disallow tag - it is required to allow bigger package "java.util" and disallow "java.util.stream.Stream".

Solution I found is to store disallows in the current package and have 'allow everything' rule in the parent package.

You solution is completely correct, please add new subsection to xdoc "Example of blacklist mode" and show user example.
Please change example a bit and show user how to disallow usage of "sun." packages on root level to make it close to life (well known example). In addition, make subpackage name="dao" and disallow "swing" usage in it.
Please do this in same PR as previous point or separate.

but by this implementation all classes are allowed inside this package.
I am not sure, but maybe we can avoid it by using several ImportControl checks.

It completely ok, user is responsible for config he is creating. If he decided to go "black list" mode, and define only disallowed imports. He should not expect that mix of modes will work. Config is either "black list " (allow all at root, disallow certain) or it is "white list" (allow certain).
Usage of few config will result in conflicts between them.

Member

romani commented Apr 21, 2017

@timurt ,

first of all let me describe behaviour shortly:

confirmed, good summary, please do PR right now to update xdoc to explain users how it works now.
Please also explain user need for disallow tag - it is required to allow bigger package "java.util" and disallow "java.util.stream.Stream".

Solution I found is to store disallows in the current package and have 'allow everything' rule in the parent package.

You solution is completely correct, please add new subsection to xdoc "Example of blacklist mode" and show user example.
Please change example a bit and show user how to disallow usage of "sun." packages on root level to make it close to life (well known example). In addition, make subpackage name="dao" and disallow "swing" usage in it.
Please do this in same PR as previous point or separate.

but by this implementation all classes are allowed inside this package.
I am not sure, but maybe we can avoid it by using several ImportControl checks.

It completely ok, user is responsible for config he is creating. If he decided to go "black list" mode, and define only disallowed imports. He should not expect that mix of modes will work. Config is either "black list " (allow all at root, disallow certain) or it is "white list" (allow certain).
Usage of few config will result in conflicts between them.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Apr 21, 2017

Member

What I suggest, as I mentioned before there is rule 'What is not allowed explicitly - is disallowed by default.'. What if we add new property for subpackage tag, lets call it strategy

I like your idea ..... with minor correction.
Name of the property should be strategyOnMismatch with values allowed, disallowed, delegateToParent(should be default to keep compatibility) .

Rationale of your request is to "allow independent rules for subpackages". It will allow define completely isolated rules for special packages, for example:

  • to certainly keep control of api package;
  • control that domain packages are POJOs and use only jdk standard types/collections ;
  • or package that is extracted to separate artifact .

This extension is not related to blacklist mode, so please make summary from my and your post and create new issue.

This issue should should address only backlist mode.

Member

romani commented Apr 21, 2017

What I suggest, as I mentioned before there is rule 'What is not allowed explicitly - is disallowed by default.'. What if we add new property for subpackage tag, lets call it strategy

I like your idea ..... with minor correction.
Name of the property should be strategyOnMismatch with values allowed, disallowed, delegateToParent(should be default to keep compatibility) .

Rationale of your request is to "allow independent rules for subpackages". It will allow define completely isolated rules for special packages, for example:

  • to certainly keep control of api package;
  • control that domain packages are POJOs and use only jdk standard types/collections ;
  • or package that is extracted to separate artifact .

This extension is not related to blacklist mode, so please make summary from my and your post and create new issue.

This issue should should address only backlist mode.

timurt pushed a commit to timurt/checkstyle that referenced this issue Apr 23, 2017

timurt pushed a commit to timurt/checkstyle that referenced this issue Apr 24, 2017

timurt added a commit to timurt/checkstyle that referenced this issue May 6, 2017

timurt added a commit to timurt/checkstyle that referenced this issue May 6, 2017

timurt added a commit to timurt/checkstyle that referenced this issue May 12, 2017

timurt added a commit to timurt/checkstyle that referenced this issue May 12, 2017

rnveach added a commit that referenced this issue May 14, 2017

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach May 14, 2017

Member

Fix is merged

Member

rnveach commented May 14, 2017

Fix is merged

@rnveach rnveach closed this May 14, 2017

@rnveach rnveach added this to the 7.8 milestone May 14, 2017

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