cxf-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Sergey Beryozkin (JIRA)" <j...@apache.org>
Subject [jira] Issue Comment Edited: (CXF-3145) Refactor toSQL method as visitor pattern
Date Wed, 24 Nov 2010 09:34:15 GMT

    [ 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.


Mime
View raw message