[jira] Issue Comment Edited: (CXF-3145) Refactor toSQL method as visitor pattern

["Sergey Beryozkin (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/CXF-3145?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12935089#action_12935089 ] 

Sergey Beryozkin edited comment on CXF-3145 at 11/24/10 4:34 AM:
-----------------------------------------------------------------

Hi Brian, sorry for a delay, I'm a bit overwhelmed at the moment and at times like these I usually focus on issues I can deal with fast :-)

thanks for a patch. I have to admit I'm slightly hesitant at the moment as to whether I want to apply it or not.

First, please do run 'mvn -Psetup.eclipse' which should ensure you get all the formatting right and will make it easier for me to deal with the patch. We don't usually have the authors tags but we do acknowledge the authors in the commit emails.

As far as the actual idea about introducing a visitor is concerned.  

I do agree that SearchCondition.toSQL will not work in all the cases even when some complex enough SQL expressions have to be produced. I'd like to keep toSQL as not deprecated because I feel it will work in 80% well (80% will want SQL on the output and 80% will work with simple enough queries for toSQL to work properly) and it is simpler for a user then worry about initiating a visitor class.

Now, I'm open to changing the SeachCondition interface to support a visitor pattern, may be for 2.4, as it was difficult to come with the right interface without having practitioners like yourself trying it.

However, I did expect toSQL() won't work in 100% cases so that is why SearchCondition#getSearchConditions() exists.
Giving this method and SearchCondition.getType(), one can 'pull' all the conditions and easily build the custom expression.

Can you please let me know why do you think applying a Visitor pattern is better ? The 'pull' style kind of appeals better to me given that it is obvious when the top level condition is checked and thus the start of the expression like 'SELECT' can be added. As a side note I'd prefer to have 2 methods only  in the Visitor interface, if we were to go for it (one method accepting the base SearchCondition interface with implementations relying on getType() and an explicit toExpression())

Thoughts ?
thanks, Sergey

 

      was (Author: sergey_beryozkin):
    Hi Brian, sorry for a delay, I'm a bit overwhelmed at the moment and at times like these I usually focus on issues I can deal with fast :-)

thanks for a patch. I have to admit I'm slightly hesitant at the moment as to whether I want to apply it or not.

First, please do run 'mvn -Psetup.eclipse' which should ensure you get all the formatting right and will make it easier for me to deal with the patch. We don't usually have the authors tags but we do acknowledge the authors in the commit emails.

As far as the actual idea about introducing a visitor is concerned.  

I do agree that SearchCondition.toSQL will not work in all the cases even when some complex enough SQL expressions have to be produced. I'd like to keep toSQL as not deprecated because I feel it will work in 80% well (80% will want SQL on the output and 80% will work with simple enough queries for toSQL to work properly) and it is simpler for a user then worry about initiating a visitor class.

Now, I'm open to changing the SeachCondition interface to support a visitor pattern, may be for 2.4, as it was difficult to come with the right interface without having practitioners like yourself trying it.

However, I did expect toSQL() won't work in 100% cases so that is why SearchCondition#getSearchConditions() exists.
Giving this method and SearchCondition.getType(), one can 'pull' all the conditions and easily build the custom expression.

Can you please let me know why do you think applying a Visitor pattern is better ? The 'pull' style kind of appeals better to me given that it is obvious when the top level condition is checked and thus the start of the expression like 'SELECT' can be added. As a side note I'd prefer to have 2 methods only  in the Visitor interface, if we were to go for it (one method accepting the base SearchCondition interface with impls telying getType() and an explicit toExpression())

Thoughts ?

thanks, Sergey

 
  
> Refactor toSQL method as visitor pattern
> ----------------------------------------
>
>                 Key: CXF-3145
>                 URL: https://issues.apache.org/jira/browse/CXF-3145
>             Project: CXF
>          Issue Type: Improvement
>          Components: JAX-RS
>    Affects Versions: 2.3.0
>            Reporter: Brian Topping
>            Priority: Minor
>         Attachments: cxf3145.patch
>
>
> In doing some work with the FIQL parser, I needed to generate a different output than SQL.  By using a visitor pattern for rendering the SQL, any visitor can be applied to the SearchCondition object graph.
> The attached patch provides that refactoring. 
> As a part of that, SearchCondition.toSql() is deprecated, but I maintained the interface for maximum compatibility.  
> There was also a change to SearchCondition.getSearchConditions().  There were no callers to that code except test code, and it was set up to return null if there was only one condition.  That wasn't clear to me, hopefully that change is not unreasonable.
> This may not be formatted properly, please adjust to suit.

-- 
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.