jsr-283
  1. jsr-283
  2. JSR_283-355

Clarify InvalidQueryException on QueryObjectModelFactory methods

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: current
    • Fix Version/s: milestone 1
    • Component/s: javadoc/api
    • Labels:
      None
    • Environment:

      Operating System: All
      Platform: All

    • Issuezilla Id:
      355

      Description

      All methods on QueryObjectModelFactory may currently throw an
      InvalidQueryException. IMO this exception only makes sense for the two
      createQuery() methods. All other methods only create node tree fragments that
      cannot be checked for validity.

        Issue Links

          Activity

          Hide
          dpitfiel added a comment -

          Although full validation cannot be performed in creating each tree fragment, an
          implementation can elect to perform certain validation at that time. Validating
          path syntax and the value of the "operand" parameter to the comparison(...)
          method are examples.

          Since we don't require validation to be performed until the query is
          execute()d, there's no implementor burden by allowing validation to be
          performed earlier.

          From an API caller's standpoint, I think it's better to report problems sooner,
          because it's more clear where the problem lies.

          So I'd prefer we not make the proposed change.

          Show
          dpitfiel added a comment - Although full validation cannot be performed in creating each tree fragment, an implementation can elect to perform certain validation at that time. Validating path syntax and the value of the "operand" parameter to the comparison(...) method are examples. Since we don't require validation to be performed until the query is execute()d, there's no implementor burden by allowing validation to be performed earlier. From an API caller's standpoint, I think it's better to report problems sooner, because it's more clear where the problem lies. So I'd prefer we not make the proposed change.
          Hide
          mreutegg added a comment -

          > Validating path syntax and the value of the "operand" parameter to
          > the comparison(...) method are examples.

          I agree, but the API should be clearer about that. In various locations
          it declares an InvalidQueryException without saying under what
          circumstances it will (or actually may) throw the exception.

          In some places the API declares an InvalidQueryException for
          situations that are impossible to detected. This should be fixed as
          well. Example:

          /**

          • Tests whether a node in the default selector is a child of a node
          • reachable by a specified absolute path.
            *
          • @param path an absolute path; non-null
          • @return the constraint; non-null
          • @throws javax.jcr.query.InvalidQueryException
          • if the query has no default
          • selector or is otherwise invalid
          • @throws javax.jcr.RepositoryException if the operation otherwise fails
            */
            public ChildNode childNode(String path) throws InvalidQueryException,
            RepositoryException // CM

          For an implementation it is impossible to find out if there is a default
          selector, because that's a choice the caller will make independently of
          this call, or he might not even create a Selector at all.

          > Since we don't require validation to be performed until the
          > query is execute()d, there's no implementor burden by allowing
          > validation to be performed earlier.

          Agreed. But for simplicity of the API and usability we should be careful
          when declaring exceptions. If a client has to write code for an exception
          that is never thrown, what good is it?

          > From an API caller's standpoint, I think it's better to report problems
          > sooner, because it's more clear where the problem lies.

          Agreed. I'm OK with keeping the InvalidQueryExceptions. But let the API
          be clear in what situations the InvalidQueryException may be thrown by
          an implementation.

          The only situation I can think of is a malformed path or name. But as
          you mentioned an implementation is not required to perform that check
          before the query is executed.

          Show
          mreutegg added a comment - > Validating path syntax and the value of the "operand" parameter to > the comparison(...) method are examples. I agree, but the API should be clearer about that. In various locations it declares an InvalidQueryException without saying under what circumstances it will (or actually may) throw the exception. In some places the API declares an InvalidQueryException for situations that are impossible to detected. This should be fixed as well. Example: /** Tests whether a node in the default selector is a child of a node reachable by a specified absolute path. * @param path an absolute path; non-null @return the constraint; non-null @throws javax.jcr.query.InvalidQueryException if the query has no default selector or is otherwise invalid @throws javax.jcr.RepositoryException if the operation otherwise fails */ public ChildNode childNode(String path) throws InvalidQueryException, RepositoryException // CM For an implementation it is impossible to find out if there is a default selector, because that's a choice the caller will make independently of this call, or he might not even create a Selector at all. > Since we don't require validation to be performed until the > query is execute()d, there's no implementor burden by allowing > validation to be performed earlier. Agreed. But for simplicity of the API and usability we should be careful when declaring exceptions. If a client has to write code for an exception that is never thrown, what good is it? > From an API caller's standpoint, I think it's better to report problems > sooner, because it's more clear where the problem lies. Agreed. I'm OK with keeping the InvalidQueryExceptions. But let the API be clear in what situations the InvalidQueryException may be thrown by an implementation. The only situation I can think of is a malformed path or name. But as you mentioned an implementation is not required to perform that check before the query is executed.
          Hide
          mreutegg added a comment -

          Changed title

          Show
          mreutegg added a comment - Changed title
          Hide
          mreutegg added a comment -

          Changed title again

          Show
          mreutegg added a comment - Changed title again
          Hide
          dpitfiel added a comment -

          F2F consensus: move description of validation constraints from the class
          description of the constructed object to the corresponding factory method on
          QueryObjectModelFactory

          Show
          dpitfiel added a comment - F2F consensus: move description of validation constraints from the class description of the constructed object to the corresponding factory method on QueryObjectModelFactory
          Hide
          Peeter Piegaze added a comment -

          Status changed to STARTED

          Show
          Peeter Piegaze added a comment - Status changed to STARTED
          Hide
          Peeter Piegaze added a comment -

          Additionally, there is the case where the query contains an unbound variable. This can only be determined
          on Query.execute, which currently throws only RepositoryException.

          PROPOSAL: Add to Query.execute:

          @throws InvalidQueryException if the query contains a variable but that variable
          has not been bound to a value using

          {@link Query#bindValue}

          .
          See

          {@link javax.jcr.query.qom.QueryObjectModelFactory#bindVariable QueryObjectModelFactory.bindVariable}

          and

          {@link javax.jcr.query.qom.BindVariableValue}

          BindVariableValue)

          Show
          Peeter Piegaze added a comment - Additionally, there is the case where the query contains an unbound variable. This can only be determined on Query.execute, which currently throws only RepositoryException. PROPOSAL: Add to Query.execute: @throws InvalidQueryException if the query contains a variable but that variable has not been bound to a value using {@link Query#bindValue} . See {@link javax.jcr.query.qom.QueryObjectModelFactory#bindVariable QueryObjectModelFactory.bindVariable} and {@link javax.jcr.query.qom.BindVariableValue} BindVariableValue)
          Hide
          Peeter Piegaze added a comment -

          Fixed

          Show
          Peeter Piegaze added a comment - Fixed

            People

            • Assignee:
              jsr-283-issues
              Reporter:
              mreutegg
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: