Re: svn commit: r935207 - in /cayenne/main/trunk: docs/doc/src/main/resources/ framework/cayenne-jdk1.5-unpublished/src/main/java/org/apache/cayenne/query/ framework/cayenne-jdk1.5-unpublished/src/test/java/org/apache/cayenne/query/

From: Andrus Adamchik (andru..bjectstyle.org)
Date: Tue Apr 27 2010 - 07:12:51 EDT

  • Next message: Michael Gentry: "Re: [VOTE] Cayenne 3.0-final"

    Hi Evgeny,

    The failure was a result of the other part of that change:

    ..-105,6 +105,7 @@ public class SQLTemplate extends Abstrac
         public SQLTemplate(DataMap rootMap, String defaultTemplate) {
             setDefaultTemplate(defaultTemplate);
             setRoot(rootMap);
    + setFetchingDataRows(true); // ObjEntity not passed, so it's
    DataRow query
         }

    So, SQLTemplate property defaults... I actually agree with you that
    the proposed defaults per r935207 make more sense when viewed in
    isolation. But I'd still like to keep the old ones for consistency
    reasons:

    1. All SQLTemplate constructors create a query that fetches objects,
    so users will have to manually change to DataRows if they need to,
    without having to think about the query creation history.
    2. When there are 2 properties in an object "abc" and "xyz", calling
    setAbc should not change the value of xyz implicitly. Different
    behavior may be confusing to many users.

    So back to the original issue CAY-1328... Adding
    "q2.setFetchingDataRows(true)" to the test fixes it. So the Jira IMO
    is about better error reporting, not changing the defaults. E.g.
    consider this:

    SQLTemplate q2 = new SQLTemplate(testDataMap, "SELECT * FROM ARTIST");
    q2.setFetchingDataRows(false); // the user might override the new
    default by mistake,
            // causing that same NPE as the Jira mentions

    If instead of an NPE Cayenne would throw CayenneRuntimeException
    saying "SQLTemplate is not mapped to ObjEntity and no SQLResult is
    set, so 'fetchingDataRows' property must be set to 'false'", I think
    we'll solve the problem in the least confusing way for the end user.
    One possible place to perform this validation is in the
    Query.route(..) method.

    Does this make sense?

    Andrus

    On Apr 18, 2010, at 7:31 PM, Evgeny Ryabitskiy wrote:

    > This was add to fix another JUnit test (that was failing).
    > JUnit test was expecting that this flag is false while use
    > setFatchingDataRows.
    > So I add it here, to didn't change previous behavior
    > Maybe I didn't understand the meaning of SQLResult....
    >
    > Evgeny.
    >
    >
    > 2010/4/18 Andrus Adamchik <andru..bjectstyle.org>:
    >>
    >> On Apr 18, 2010, at 12:46 AM, evgen..pache.org wrote:
    >>
    >>> public void setResult(SQLResult resultSet) {
    >>> + setFetchingDataRows(false); // turn off mapping to
    >>> DataRows, use
    >>> explicit
    >>> this.result = resultSet;
    >>> }
    >>
    >> Implicit flipping of the DataRows flag outside constructor seems
    >> counterintuitive. Also SQLResult is not equal to an object fetch.
    >> So I think
    >> this is wrong.
    >>
    >> Andrus
    >



    This archive was generated by hypermail 2.0.0 : Tue Apr 27 2010 - 07:13:48 EDT