[jira] Created: (JCR-1674) Provide means for exception handling for QueryNodeVisitor implementations

Provide means for exception handling for QueryNodeVisitor implementations
-------------------------------------------------------------------------

                 Key: JCR-1674
                 URL: https://issues.apache.org/jira/browse/JCR-1674
             Project: Jackrabbit
          Issue Type: Improvement
          Components: jackrabbit-spi-commons
            Reporter: Michael Dürig
            Priority: Minor


Currently the methods of QueryNodeVisitor do not declare any exceptions. Even though the query tree might be syntactically correct, an implementation might reach a point where it cannot continue (i.e. if it does not support one of the optional query features). For such cases there are currently two solution: 1. throw an unchecked exception or 2. communicate the error state through the visitor using the data object passed along.
While I don't like 2. it is still an option. For 1. I'm not sure if this is the right way to go. It might be better to actually throw a checked exception. I therefore created a patch which declares RepositoryException on all visit methods of QueryNodeVisitor. Although the necessary changes in classes using QueryNodeVisitor are trivial, there are quite many of them.

Any opinions on checked exception with probably breaking (trivially) existing code vs. using not checked exceptions?




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

     [ https://issues.apache.org/jira/browse/JCR-1674?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Michael Dürig updated JCR-1674:
-------------------------------

    Attachment: QueryNodeVisitor.patch

> Provide means for exception handling for QueryNodeVisitor implementations
> -------------------------------------------------------------------------
>
>                 Key: JCR-1674
>                 URL: https://issues.apache.org/jira/browse/JCR-1674
>             Project: Jackrabbit
>          Issue Type: Improvement
>          Components: jackrabbit-spi-commons
>            Reporter: Michael Dürig
>            Priority: Minor
>         Attachments: QueryNodeVisitor.patch
>
>
> Currently the methods of QueryNodeVisitor do not declare any exceptions. Even though the query tree might be syntactically correct, an implementation might reach a point where it cannot continue (i.e. if it does not support one of the optional query features). For such cases there are currently two solution: 1. throw an unchecked exception or 2. communicate the error state through the visitor using the data object passed along.
> While I don't like 2. it is still an option. For 1. I'm not sure if this is the right way to go. It might be better to actually throw a checked exception. I therefore created a patch which declares RepositoryException on all visit methods of QueryNodeVisitor. Although the necessary changes in classes using QueryNodeVisitor are trivial, there are quite many of them.
> Any opinions on checked exception with probably breaking (trivially) existing code vs. using not checked exceptions?

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

    [ https://issues.apache.org/jira/browse/JCR-1674?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12612146#action_12612146 ]

Thomas Mueller commented on JCR-1674:
-------------------------------------

+1 for this patch. I think using checked exceptions is a good solution.

I would use unchecked exceptions only if Jackrabbit as a whole also uses unchecked exceptions. It would be inconsistent to use unchecked exceptions in some places and checked exceptions in others. Communicating the error state using the data object doesn't sound like a good solution: you would have to check the error state everywhere.

> Provide means for exception handling for QueryNodeVisitor implementations
> -------------------------------------------------------------------------
>
>                 Key: JCR-1674
>                 URL: https://issues.apache.org/jira/browse/JCR-1674
>             Project: Jackrabbit
>          Issue Type: Improvement
>          Components: jackrabbit-spi-commons
>            Reporter: Michael Dürig
>            Priority: Minor
>         Attachments: QueryNodeVisitor.patch
>
>
> Currently the methods of QueryNodeVisitor do not declare any exceptions. Even though the query tree might be syntactically correct, an implementation might reach a point where it cannot continue (i.e. if it does not support one of the optional query features). For such cases there are currently two solution: 1. throw an unchecked exception or 2. communicate the error state through the visitor using the data object passed along.
> While I don't like 2. it is still an option. For 1. I'm not sure if this is the right way to go. It might be better to actually throw a checked exception. I therefore created a patch which declares RepositoryException on all visit methods of QueryNodeVisitor. Although the necessary changes in classes using QueryNodeVisitor are trivial, there are quite many of them.
> Any opinions on checked exception with probably breaking (trivially) existing code vs. using not checked exceptions?

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

     [ https://issues.apache.org/jira/browse/JCR-1674?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Marcel Reutegger resolved JCR-1674.
-----------------------------------

       Resolution: Fixed
    Fix Version/s: 1.5

Applied patch in revision: 677226

> Provide means for exception handling for QueryNodeVisitor implementations
> -------------------------------------------------------------------------
>
>                 Key: JCR-1674
>                 URL: https://issues.apache.org/jira/browse/JCR-1674
>             Project: Jackrabbit
>          Issue Type: Improvement
>          Components: jackrabbit-core, jackrabbit-spi-commons
>            Reporter: Michael Dürig
>            Priority: Minor
>             Fix For: 1.5
>
>         Attachments: QueryNodeVisitor.patch
>
>
> Currently the methods of QueryNodeVisitor do not declare any exceptions. Even though the query tree might be syntactically correct, an implementation might reach a point where it cannot continue (i.e. if it does not support one of the optional query features). For such cases there are currently two solution: 1. throw an unchecked exception or 2. communicate the error state through the visitor using the data object passed along.
> While I don't like 2. it is still an option. For 1. I'm not sure if this is the right way to go. It might be better to actually throw a checked exception. I therefore created a patch which declares RepositoryException on all visit methods of QueryNodeVisitor. Although the necessary changes in classes using QueryNodeVisitor are trivial, there are quite many of them.
> Any opinions on checked exception with probably breaking (trivially) existing code vs. using not checked exceptions?

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

     [ https://issues.apache.org/jira/browse/JCR-1674?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Marcel Reutegger updated JCR-1674:
----------------------------------

    Component/s: jackrabbit-core

Also affects core.

> Provide means for exception handling for QueryNodeVisitor implementations
> -------------------------------------------------------------------------
>
>                 Key: JCR-1674
>                 URL: https://issues.apache.org/jira/browse/JCR-1674
>             Project: Jackrabbit
>          Issue Type: Improvement
>          Components: jackrabbit-core, jackrabbit-spi-commons
>            Reporter: Michael Dürig
>            Priority: Minor
>             Fix For: 1.5
>
>         Attachments: QueryNodeVisitor.patch
>
>
> Currently the methods of QueryNodeVisitor do not declare any exceptions. Even though the query tree might be syntactically correct, an implementation might reach a point where it cannot continue (i.e. if it does not support one of the optional query features). For such cases there are currently two solution: 1. throw an unchecked exception or 2. communicate the error state through the visitor using the data object passed along.
> While I don't like 2. it is still an option. For 1. I'm not sure if this is the right way to go. It might be better to actually throw a checked exception. I therefore created a patch which declares RepositoryException on all visit methods of QueryNodeVisitor. Although the necessary changes in classes using QueryNodeVisitor are trivial, there are quite many of them.
> Any opinions on checked exception with probably breaking (trivially) existing code vs. using not checked exceptions?

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

     [ https://issues.apache.org/jira/browse/JCR-1674?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Michael Dürig closed JCR-1674.
------------------------------


Thanks for fixing

> Provide means for exception handling for QueryNodeVisitor implementations
> -------------------------------------------------------------------------
>
>                 Key: JCR-1674
>                 URL: https://issues.apache.org/jira/browse/JCR-1674
>             Project: Jackrabbit
>          Issue Type: Improvement
>          Components: jackrabbit-core, jackrabbit-spi-commons
>            Reporter: Michael Dürig
>            Priority: Minor
>             Fix For: 1.5
>
>         Attachments: QueryNodeVisitor.patch
>
>
> Currently the methods of QueryNodeVisitor do not declare any exceptions. Even though the query tree might be syntactically correct, an implementation might reach a point where it cannot continue (i.e. if it does not support one of the optional query features). For such cases there are currently two solution: 1. throw an unchecked exception or 2. communicate the error state through the visitor using the data object passed along.
> While I don't like 2. it is still an option. For 1. I'm not sure if this is the right way to go. It might be better to actually throw a checked exception. I therefore created a patch which declares RepositoryException on all visit methods of QueryNodeVisitor. Although the necessary changes in classes using QueryNodeVisitor are trivial, there are quite many of them.
> Any opinions on checked exception with probably breaking (trivially) existing code vs. using not checked exceptions?

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