groovy
  1. groovy
  2. GROOVY-5340

AbstractQueryCommand constructor should be protected for subclassing

    Details

    • Type: Bug Bug
    • Status: Closed Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 1.8.4, 2.0-beta-2
    • Fix Version/s: 2.0-beta-3, 1.8.7
    • Component/s: SQL processing
    • Labels:
      None
    • Number of attachments :
      0

      Description

      To customize the behavior of groovy.sql.Sql we override "protected groovy.sql.Sql.createQueryCommand(String)". Next, we attempt to implement the inner class groovy.sql.Sql.AbstractQueryCommand. Its only constructor "AbstractQueryCommand(String sql)" is package-protected. therefore subclassing is impossible unless we put our code into package "groovy.sql" which is against best practice.

      As a side note, AbstractQueryCommand is non-static, therefore requires an instance of groovy.sql.Sql for instantiation.
      I guess one would only want to subclass AbstractQueryCommand after subclassing Sql first, so this won't be an issue. Still, an instance of Sql could be provided on the constructor explicitly and let the inner class be static.

        Activity

        Hide
        Paul King added a comment -

        The constructor for AbstractQueryCommand now has protected visibility allowing for extension in combination with extending groovy.sql.Sql.

        It would also be possible to make AbstractQueryCommand static - or indeed a top-level class - but that seems like a lower priority change to me and we currently don't have a strong example/use case/justification driving that change. I am at this stage inclined to close this issue and leave such changes to be done if/when other refactoring takes place in the future. Do you agree or instead have further use cases/examples to be considered?

        Show
        Paul King added a comment - The constructor for AbstractQueryCommand now has protected visibility allowing for extension in combination with extending groovy.sql.Sql. It would also be possible to make AbstractQueryCommand static - or indeed a top-level class - but that seems like a lower priority change to me and we currently don't have a strong example/use case/justification driving that change. I am at this stage inclined to close this issue and leave such changes to be done if/when other refactoring takes place in the future. Do you agree or instead have further use cases/examples to be considered?
        Hide
        Markus Spann added a comment -

        Thanks for the quick action – I agree there's no need to make AbstractQueryCommand static or top-level, as the class would offer little or no benefit outside the context of an instance of groovy.sql.Sql.

        Show
        Markus Spann added a comment - Thanks for the quick action – I agree there's no need to make AbstractQueryCommand static or top-level, as the class would offer little or no benefit outside the context of an instance of groovy.sql.Sql.
        Hide
        Paul King added a comment -

        OK, we'll ignore the static inner class suggestion for now. Marking resolved, but please reopen if upon any further testing you spot any issues.

        Show
        Paul King added a comment - OK, we'll ignore the static inner class suggestion for now. Marking resolved, but please reopen if upon any further testing you spot any issues.

          People

          • Assignee:
            Paul King
            Reporter:
            Markus Spann
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: