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

Implement XpathFilter #4422

Closed
MEZk opened this Issue Jun 5, 2017 · 14 comments

Comments

@MEZk
Contributor

MEZk commented Jun 5, 2017

We need to create XpathFilter. XpathFilter should be a place for the suppression by xpath algorithm. It should implement the AstFIlter interface (AST-based filter interface).

Details of the implementation

  1. XpathFilter should be able to reuse the logic of suppressing by module id, line and column number, file similar to SuppressElement.
  2. XpathFilter should have a private field xpathQuery. The type of the filed is String. The field stores xpath-expression passed from SuppressionLoader to XpathFilter by invoking setXpathQuery;
  3. The filter should have accept method which receives an audit event;
  4. Inside accept method the filter should extract root AST node from audit event object, and AST node which caused a violation;
  5. XPath-to-AST mapper should extract the node which should be suppressed starting from the root. If this node is equal to the node which caused the violation, then the filter should deny the audit event.
@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jul 7, 2017

Member

XpathFilter should be able to reuse the logic of suppressing by module id

is not a requirement. Code duplication is ok if that is reasonable. It is better to reuse some code, but it Filter is separate code could be reused only in scope of class like FilterUtils

The filter should have accept method which receives an audit event;

yes, event contains information for Filter to do matching , as discussed before lineNum, colNum, TokenType should be enough for Filter

Inside accept method the filter should extract root AST node from audit event object, and AST node which caused a violation;

AST filter should be hosted under TreeWalker, TreeWalker will initialize AstFilter with Root token, AstFilter will find violation DetailAST by lineNum, colNum, TokenType, and do matching by xpath ......

Member

romani commented Jul 7, 2017

XpathFilter should be able to reuse the logic of suppressing by module id

is not a requirement. Code duplication is ok if that is reasonable. It is better to reuse some code, but it Filter is separate code could be reused only in scope of class like FilterUtils

The filter should have accept method which receives an audit event;

yes, event contains information for Filter to do matching , as discussed before lineNum, colNum, TokenType should be enough for Filter

Inside accept method the filter should extract root AST node from audit event object, and AST node which caused a violation;

AST filter should be hosted under TreeWalker, TreeWalker will initialize AstFilter with Root token, AstFilter will find violation DetailAST by lineNum, colNum, TokenType, and do matching by xpath ......

@MEZk

This comment has been minimized.

Show comment
Hide comment
@MEZk

MEZk Jul 9, 2017

Contributor

AST filter should be hosted under TreeWalker, TreeWalker will initialize AstFilter with Root token, AstFilter will find violation DetailAST by lineNum, colNum, TokenType, and do matching by xpath ......

Yes, you are right. The original description in my previous comment was written before our Skype discussion. AstFilter should be initialized with AST root node. AuditEvent will provide the line, column number and token type to the filter. This information should be enough for the filter to match by xpath.

Contributor

MEZk commented Jul 9, 2017

AST filter should be hosted under TreeWalker, TreeWalker will initialize AstFilter with Root token, AstFilter will find violation DetailAST by lineNum, colNum, TokenType, and do matching by xpath ......

Yes, you are right. The original description in my previous comment was written before our Skype discussion. AstFilter should be initialized with AST root node. AuditEvent will provide the line, column number and token type to the filter. This information should be enough for the filter to match by xpath.

@timurt

This comment has been minimized.

Show comment
Hide comment
@timurt

timurt Jul 9, 2017

Collaborator

@MEZk

AuditEvent will provide the line, column number and token type to the filter. This information should be enough for the filter to match by xpath.

So there is no need in storing reference from Xpath node to corresponding Ast node, we also can store primitives, yes?

Collaborator

timurt commented Jul 9, 2017

@MEZk

AuditEvent will provide the line, column number and token type to the filter. This information should be enough for the filter to match by xpath.

So there is no need in storing reference from Xpath node to corresponding Ast node, we also can store primitives, yes?

@MEZk

This comment has been minimized.

Show comment
Hide comment
@MEZk

MEZk Jul 9, 2017

Contributor

@timurt
No. My previous comment was about AuditEvent and LocalizedMessage only.

Contributor

MEZk commented Jul 9, 2017

@timurt
No. My previous comment was about AuditEvent and LocalizedMessage only.

@MEZk

This comment has been minimized.

Show comment
Hide comment
@MEZk

MEZk Jul 13, 2017

Contributor

@timurt
Summary, XpathFilter should implement AstFilter interface (see #4714). Line, column number, token type, and all information, necessary for filtering, can be received from TreeWalkerAuditEvent. You also should add

private DetailAST root;

to TreeWalkerAuditEvent. The AST have to be passed to the event in the TreeWalker#processFiltered.

Contributor

MEZk commented Jul 13, 2017

@timurt
Summary, XpathFilter should implement AstFilter interface (see #4714). Line, column number, token type, and all information, necessary for filtering, can be received from TreeWalkerAuditEvent. You also should add

private DetailAST root;

to TreeWalkerAuditEvent. The AST have to be passed to the event in the TreeWalker#processFiltered.

@MEZk MEZk moved this from To Do to In Progress in Flexible Suppression Model Jul 26, 2017

timurt added a commit to timurt/checkstyle that referenced this issue Jul 28, 2017

@timurt

This comment has been minimized.

Show comment
Hide comment
@timurt

timurt Jul 28, 2017

Collaborator

@MEZk @rnveach
Here is my commit

Collaborator

timurt commented Jul 28, 2017

@MEZk @rnveach
Here is my commit

@MEZk

This comment has been minimized.

Show comment
Hide comment
@MEZk

MEZk Aug 3, 2017

Contributor

@timurt
I reviewed your work. Please, fix the points ASAP and let me know about the progress.

Contributor

MEZk commented Aug 3, 2017

@timurt
I reviewed your work. Please, fix the points ASAP and let me know about the progress.

@MEZk

This comment has been minimized.

Show comment
Hide comment
@MEZk

MEZk Aug 5, 2017

Contributor

@timurt
Ping

Contributor

MEZk commented Aug 5, 2017

@timurt
Ping

@MEZk

This comment has been minimized.

Show comment
Hide comment
@MEZk

MEZk Aug 6, 2017

Contributor

@timurt
Ping.

Contributor

MEZk commented Aug 6, 2017

@timurt
Ping.

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

@timurt

This comment has been minimized.

Show comment
Hide comment
@timurt

timurt Aug 6, 2017

Collaborator

@MEZk
Sorry for delay, here is my commit, fixed all mentioned points except one,

use java EqualsVerifier.forClass

Got this error

Caused by: java.lang.SecurityException: class "net.sf.saxon.expr.StaticContext$$DynamicSubclass"'s signer information does not match signer information of other classes in the same package

Collaborator

timurt commented Aug 6, 2017

@MEZk
Sorry for delay, here is my commit, fixed all mentioned points except one,

use java EqualsVerifier.forClass

Got this error

Caused by: java.lang.SecurityException: class "net.sf.saxon.expr.StaticContext$$DynamicSubclass"'s signer information does not match signer information of other classes in the same package

@MEZk

This comment has been minimized.

Show comment
Hide comment
@MEZk

MEZk Aug 7, 2017

Contributor

@timurt

Got this error

Problem with jar-certificate policy again.
Implement uts manually based on the equals and hashcode implementation rules.
See "Item 8: Obey the general contract when overriding equals", Effective Java.

Contributor

MEZk commented Aug 7, 2017

@timurt

Got this error

Problem with jar-certificate policy again.
Implement uts manually based on the equals and hashcode implementation rules.
See "Item 8: Obey the general contract when overriding equals", Effective Java.

timurt added a commit to timurt/checkstyle that referenced this issue Aug 8, 2017

@timurt

This comment has been minimized.

Show comment
Hide comment
@timurt

timurt Aug 8, 2017

Collaborator

@MEZk
I read it, added UTs for transitivity and consistency
Here is commit

Collaborator

timurt commented Aug 8, 2017

@MEZk
I read it, added UTs for transitivity and consistency
Here is commit

timurt added a commit to timurt/checkstyle that referenced this issue Aug 9, 2017

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

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

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

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

@MEZk MEZk added the approved label Aug 13, 2017

timurt added a commit to timurt/checkstyle that referenced this issue Aug 16, 2017

timurt added a commit to timurt/checkstyle that referenced this issue Aug 16, 2017

timurt added a commit to timurt/checkstyle that referenced this issue Aug 16, 2017

timurt added a commit to timurt/checkstyle that referenced this issue Aug 17, 2017

timurt added a commit to timurt/checkstyle that referenced this issue Aug 17, 2017

timurt added a commit to timurt/checkstyle that referenced this issue Aug 17, 2017

timurt added a commit to timurt/checkstyle that referenced this issue Aug 17, 2017

timurt added a commit to timurt/checkstyle that referenced this issue Aug 20, 2017

timurt added a commit to timurt/checkstyle that referenced this issue Aug 20, 2017

timurt added a commit to timurt/checkstyle that referenced this issue Aug 20, 2017

timurt added a commit to timurt/checkstyle that referenced this issue Aug 20, 2017

timurt added a commit to timurt/checkstyle that referenced this issue Aug 22, 2017

timurt added a commit to timurt/checkstyle that referenced this issue Aug 23, 2017

rnveach added a commit that referenced this issue Aug 23, 2017

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Aug 23, 2017

Member

Fix was merged.

Member

rnveach commented Aug 23, 2017

Fix was merged.

@rnveach rnveach closed this Aug 23, 2017

@rnveach rnveach added this to the 8.2 milestone Aug 23, 2017

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Aug 23, 2017

Member

I added miscellaneous since the filter isn't officially released.

Member

rnveach commented Aug 23, 2017

I added miscellaneous since the filter isn't officially released.

@MEZk MEZk moved this from In Progress to Done in Flexible Suppression Model Aug 23, 2017

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