FAQ

On Apr 18, 2010, at 12:46 AM, evgeny@apache.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

Search Discussions

  • Evgeny Ryabitskiy at Apr 18, 2010 at 11:32 am
    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 <andrus@objectstyle.org>:
    On Apr 18, 2010, at 12:46 AM, evgeny@apache.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
  • Andrus Adamchik at Apr 27, 2010 at 11:13 am
    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 <andrus@objectstyle.org>:
    On Apr 18, 2010, at 12:46 AM, evgeny@apache.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
  • Evgeny Ryabitskiy at Apr 27, 2010 at 5:40 pm
    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 <andrus@objectstyle.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 <andrus@objectstyle.org>:
    On Apr 18, 2010, at 12:46 AM, evgeny@apache.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

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
groupdev @
categoriescayenne
postedApr 18, '10 at 4:56a
activeApr 27, '10 at 5:40p
posts4
users2
websitecayenne.apache.org

People

Translate

site design / logo © 2022 Grokbase