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: Evgeny Ryabitskiy (evgeny.ryabitski..mail.com)
Date: Tue Apr 27 2010 - 13:40:25 EDT

  • Next message: Andrey Razumovsky: "Re: [VOTE] Cayenne 3.0-final"

    Yes.
    It makes sense.
    In addition, I would suggest to change constructor signature. Also to
    prevent user confusing.

    ublic SQLTemplate(DataMap rootMap, String defaultTemplate, boolean
    isFetchingDataRows) {
           setDefaultTemplate(defaultTemplate);
           setRoot(rootMap);
    + setFetchingDataRows(isFetchingDataRows);
       }

    And make previous not changed but deprecated.

    Evgeny.

    2010/4/27 Andrus Adamchik <andru..bjectstyle.org>:
    > 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 - 13:41:07 EDT