CXF
  1. CXF
  2. CXF-3145

Refactor toSQL method as visitor pattern

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.3
    • Fix Version/s: 2.4
    • Component/s: JAX-RS
    • Labels:
      None
    • Estimated Complexity:
      Moderate

      Description

      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.

      1. cxf3145.patch
        17 kB
        Brian Topping

        Activity

        Hide
        Brian Topping added a comment -

        Oh, uh, the standard file template in my IDE has my name in it. I don't think that's standard formatting for CXF. Let me know if I need to generate another patch...

        Show
        Brian Topping added a comment - Oh, uh, the standard file template in my IDE has my name in it. I don't think that's standard formatting for CXF. Let me know if I need to generate another patch...
        Hide
        Sergey Beryozkin added a comment - - edited

        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

        Show
        Sergey Beryozkin added a comment - - edited 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
        Hide
        Sergey Beryozkin added a comment -

        Regarding getSearchConditions() returning 'null'. As far as I recall some not-Primitive SearchConditions can have no child expressions so 'null' was meant to indicate it somehow - I'll have to look into the code...

        Show
        Sergey Beryozkin added a comment - Regarding getSearchConditions() returning 'null'. As far as I recall some not-Primitive SearchConditions can have no child expressions so 'null' was meant to indicate it somehow - I'll have to look into the code...
        Hide
        Sergey Beryozkin added a comment -

        Hi Brian - I've finally managed to look at this issue. I've committed the patch with few minor changes.

        http://svn.apache.org/viewvc?rev=1059923&view=rev

        This is a breaking change so it will be limited to 2.4.0.

        Thanks

        Show
        Sergey Beryozkin added a comment - Hi Brian - I've finally managed to look at this issue. I've committed the patch with few minor changes. http://svn.apache.org/viewvc?rev=1059923&view=rev This is a breaking change so it will be limited to 2.4.0. Thanks

          People

          • Assignee:
            Sergey Beryozkin
            Reporter:
            Brian Topping
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development