cayenne-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Andrus Adamchik <and...@objectstyle.org>
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 11:12:51 GMT
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