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 xpath query generator #4901

Closed
MEZk opened this Issue Aug 5, 2017 · 10 comments

Comments

@MEZk
Contributor

MEZk commented Aug 5, 2017

We need to implement xpath query generator. The query generator should receive root ast, column and line number in order to generate xpath expression. The generator should be represented by the separate class.

Possible algorithm:

  1. Receive root ast, colum, line number.
  2. Traverse the tree from the root to the target nodes.
  3. Write Xpath starting from the root node to the target nodes.

Example. Generate xpath for all nodes with the lineNo = 2 and columnNo = 4.
AST:

CLASS_DEF -> CLASS_DEF [1:0]
|--MODIFIERS -> MODIFIERS [1:0]
|  `--LITERAL_PUBLIC -> public [1:0]
|--LITERAL_CLASS -> class [1:7]
|--IDENT -> TestClass [1:13]
`--OBJBLOCK -> OBJBLOCK [1:23]
    |--LCURLY -> { [1:23]
    |--METHOD_DEF -> METHOD_DEF [2:4]
    |  |--MODIFIERS -> MODIFIERS [2:4]
    |  |--TYPE -> TYPE [2:4]
    |  |  `--LITERAL_VOID -> void [2:4]
    |  |--IDENT -> foo [2:9]
    |  |--LPAREN -> ( [2:12]
    |  |--PARAMETERS -> PARAMETERS [2:13]
    |  |--RPAREN -> ) [2:13]
    |  `--SLIST -> { [2:15]
    |      `--RCURLY -> } [3:4]
    `--RCURLY -> } [4:0]

output should be all tokens for "[2:4]"

/CLASS_DEF[@text='TestClass']//OBJBLOCK//METHOD_DEF[@text='foo']
/CLASS_DEF[@text='TestClass']//OBJBLOCK//MODIFIERS
/CLASS_DEF[@text='TestClass']//OBJBLOCK//TYPE
/CLASS_DEF[@text='TestClass']//OBJBLOCK//LITERAL_VOID

Some examples of the implementation:
https://stackoverflow.com/questions/4746299/generate-get-xpath-from-xml-node-java

@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

This comment has been minimized.

Show comment
Hide comment
@timurt

timurt Aug 6, 2017

Collaborator

@MEZk

Traverse the tree from the root to the target nodes.

We can make column number and line number attributes for our nodes, so xpath query will like //*[@line=2 and @column=4]

Write Xpath starting from the root node to the target nodes.

After receiving list of nodes we can try to extract full path from each node using 'saxon:path()' query from saxon documentation , it says

The function returns a string whose value is an XPath expression identifying the selected node in the source tree

I will try to test this function

Collaborator

timurt commented Aug 6, 2017

@MEZk

Traverse the tree from the root to the target nodes.

We can make column number and line number attributes for our nodes, so xpath query will like //*[@line=2 and @column=4]

Write Xpath starting from the root node to the target nodes.

After receiving list of nodes we can try to extract full path from each node using 'saxon:path()' query from saxon documentation , it says

The function returns a string whose value is an XPath expression identifying the selected node in the source tree

I will try to test this function

@MEZk

This comment has been minimized.

Show comment
Hide comment
@MEZk

MEZk Aug 6, 2017

Contributor

We can make column number and line number attributes for our nodes, so xpath query will like //*[@line=2 and @column=4]

No need. Starting point is to generate XPath expressions for all nodes which match line and column number. First implementation should be simple. We need to allow the users to work with the new suppression model. All enhancements will be done later.

After receiving list of nodes we can try to extract full path from each node using 'saxon:path()' query from saxon documentation , it says

Ok, lets try, Send me the commit with the implementation.

Contributor

MEZk commented Aug 6, 2017

We can make column number and line number attributes for our nodes, so xpath query will like //*[@line=2 and @column=4]

No need. Starting point is to generate XPath expressions for all nodes which match line and column number. First implementation should be simple. We need to allow the users to work with the new suppression model. All enhancements will be done later.

After receiving list of nodes we can try to extract full path from each node using 'saxon:path()' query from saxon documentation , it says

Ok, lets try, Send me the commit with the implementation.

@timurt

This comment has been minimized.

Show comment
Hide comment
@timurt

timurt Aug 7, 2017

Collaborator

@MEZk

No need. Starting point is to generate XPath expressions for all nodes which match line and column number. First implementation should be simple. We need to allow the users to work with the new suppression model. All enhancements will be done later.

Do you assume to iterate all tree, visit all nodes and compare lineNumber and columnNumber variables?

After receiving list of nodes we can try to extract full path from each node using 'saxon:path()' query from saxon documentation , it says

It seems like saxon:path() function is the part of commercial release.

Write Xpath starting from the root node to the target nodes.

I think we can store xpathExpression as field inside node, value should be initialized when node is created using xpathExpression of the parent and attributes of the current node

Collaborator

timurt commented Aug 7, 2017

@MEZk

No need. Starting point is to generate XPath expressions for all nodes which match line and column number. First implementation should be simple. We need to allow the users to work with the new suppression model. All enhancements will be done later.

Do you assume to iterate all tree, visit all nodes and compare lineNumber and columnNumber variables?

After receiving list of nodes we can try to extract full path from each node using 'saxon:path()' query from saxon documentation , it says

It seems like saxon:path() function is the part of commercial release.

Write Xpath starting from the root node to the target nodes.

I think we can store xpathExpression as field inside node, value should be initialized when node is created using xpathExpression of the parent and attributes of the current node

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

@timurt

This comment has been minimized.

Show comment
Hide comment
@timurt

timurt Aug 7, 2017

Collaborator

@MEZk
My commit with two approaches.

First one uses plain DetailAst functionality, traverses the tree, finds nodes with same line and column number, after recursively generates xpath query for each node

Another one makes xpath queries to extract all nodes matching line and column (//*[@line=2 and @column=4]). For each node I additionally store xpathQuery variable, which initializes inside constructor

What do you think

Collaborator

timurt commented Aug 7, 2017

@MEZk
My commit with two approaches.

First one uses plain DetailAst functionality, traverses the tree, finds nodes with same line and column number, after recursively generates xpath query for each node

Another one makes xpath queries to extract all nodes matching line and column (//*[@line=2 and @column=4]). For each node I additionally store xpathQuery variable, which initializes inside constructor

What do you think

@MEZk

This comment has been minimized.

Show comment
Hide comment
@MEZk

MEZk Aug 7, 2017

Contributor

Another one makes xpath queries to extract all nodes matching line and column (//*[@line=2 and @column=4])

This is not correct. The main IDEA of xpath is to get rid of line and column number in suppressions. Second approach is a performance hit. It will recreate the tree on and on (getXpathItems). From my point of view, the first approach is better as it does not require additional changes to xpath-ast tree and easier to understand and support. If there are some problems with the approach, we will use the second one.

Contributor

MEZk commented Aug 7, 2017

Another one makes xpath queries to extract all nodes matching line and column (//*[@line=2 and @column=4])

This is not correct. The main IDEA of xpath is to get rid of line and column number in suppressions. Second approach is a performance hit. It will recreate the tree on and on (getXpathItems). From my point of view, the first approach is better as it does not require additional changes to xpath-ast tree and easier to understand and support. If there are some problems with the approach, we will use the second one.

@timurt

This comment has been minimized.

Show comment
Hide comment
@timurt

timurt Aug 8, 2017

Collaborator

@MEZk
what do you think about querying elements based on child element attribute, for example
if we have source code like this

import javax.swing.JToolBar;
import java.io.File;
public class InputXpathQueryGenerator {
}

We can query first import by two expressions
//IMPORT[1]
//IMPORT[/DOT/DOT[@text='javax']]
First one is simpler, but now we do not store sibling position
Second one uses attribute value of the child, but we should make sure /DOT/DOT[@text='javax'] is unique across all imports statements

Collaborator

timurt commented Aug 8, 2017

@MEZk
what do you think about querying elements based on child element attribute, for example
if we have source code like this

import javax.swing.JToolBar;
import java.io.File;
public class InputXpathQueryGenerator {
}

We can query first import by two expressions
//IMPORT[1]
//IMPORT[/DOT/DOT[@text='javax']]
First one is simpler, but now we do not store sibling position
Second one uses attribute value of the child, but we should make sure /DOT/DOT[@text='javax'] is unique across all imports statements

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

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

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

@MEZk

This comment has been minimized.

Show comment
Hide comment
@MEZk

MEZk Aug 9, 2017

Contributor

@timurt
Lets implement the second approach for now. It is easier and does not require changes in Xpath-Tree.

Second one uses attribute value of the child, but we should make sure /DOT/DOT[@text='javax'] is unique across all imports statements

We will solve this problem, when it really appears. Do not waste time on overengineering.

Contributor

MEZk commented Aug 9, 2017

@timurt
Lets implement the second approach for now. It is easier and does not require changes in Xpath-Tree.

Second one uses attribute value of the child, but we should make sure /DOT/DOT[@text='javax'] is unique across all imports statements

We will solve this problem, when it really appears. Do not waste time on overengineering.

timurt added a commit to timurt/checkstyle that referenced this issue Aug 10, 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

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 18, 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 21, 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

Marked as miscellaneous since it is not released to the users yet.

Member

rnveach commented Aug 23, 2017

Marked as miscellaneous since it is not released to the users yet.

@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