cayenne-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Evgeny Ryabitskiy <evgeny.ryabits...@gmail.com>
Subject 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/
Date Tue, 27 Apr 2010 17:40:25 GMT
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
>>
>
>

Mime
View raw message