History | Log In     View a printable version of the current page.  
Issue Details (XML | Word | Printable)

Key: CASTOR-829
Type: Improvement Improvement
Status: Closed Closed
Resolution: Fixed
Priority: Minor Minor
Assignee: Bruce Snyder (bugzilla)
Reporter: Nick Stuart
Votes: 0
Watchers: 0
Operations

If you were logged in you would be able to see more operations.
castor

Boolean getter method not standardized..

Created: 20/Jul/04 06:43 AM   Updated: 19/Apr/05 10:51 AM
Component/s: JDO
Affects Version/s: 0.9.5.3
Fix Version/s: None

Time Tracking:
Not Specified

File Attachments: 1. Text File doc-patch.patch (1 kb)
2. Text File doc-patch2.patch (1 kb)
3. Text File mapping.patch (0.7 kb)
4. Text File outinfo.patch (12 kb)
5. Text File outinfo.patch (2 kb)
6. Text File patch.complete.2 (11 kb)
7. Text File patch.txt (2 kb)

Environment:
Operating System: Windows 2000
Platform: PC

Bugzilla Id: 1689


 Description  « Hide
The current get method for booleans is not using the standard isAttribute format
and is using the getAttribute format instead. I've made a quick patch for the
problem that I will attach shortly.

 All   Comments   Work Log   Change History      Sort Order: Ascending order - Click to sort in descending order
Nick Stuart - 20/Jul/04 07:12 AM
Created an attachment (id=583)
Patch file to replace get/is depending on type

Bruce Snyder (bugzilla) - 20/Jul/04 10:16 AM
I'm working on a small test for this. I'll post it as soon as I have it completed.

Werner Guttmann - 04/Aug/04 12:48 PM
Bruce, any chance of posting the 'small' test(s) ?

Bruce Snyder (bugzilla) - 15/Aug/04 09:04 PM
I just looked into this patch and I don't think this is ready to be tested. It
seems that the patch is assuming two things will always be true:

1) All boolean accessors will use the is<field-name>() nomenclature
2) All boolean fields will be of the primitive type boolean

The first assumption can be fixed by testing for both is<field-name() and
get<field-name() accessors, although this may be a bit tougher to accomplish
because as the patch currently stands, an assumption is made at the beginning of
the constructor.

The second assumption can be fixed by using Boolean.TYPE to represent the
primitive and "Boolean" to represent the object wrapper. Then each of these
types needs to be test in the condition at the beginning of the constructor.


Bruce Snyder (bugzilla) - 16/Aug/04 03:33 AM
> The second assumption can be fixed by using Boolean.TYPE to represent the
> primitive

Actually, in this case, just continue to use "boolean" to represent the primtive.


Werner Guttmann - 12/Sep/04 12:44 PM
Nick, Bruce, can you please point me to the place where this feature is
documented for the user ? I have just been browing http://castor.exolab.org for
some time now, but couldn't find anything related to this.

Nick Stuart - 13/Sep/04 05:10 AM
I'm assuming you are referring to the method signatures and such? If so the only
place I know if is #2 below this link http://castor.exolab.org/jdo-mapping.
html#The-<field>-element .
If this isn't what you are talking about could you be more specific?

Nick Stuart - 13/Sep/04 05:20 AM
Bruce, also in comment to your first argument. Isn't there an assumption anyways
that the methods are going to be <get><whatever> format, so why cant we use the
<is><whatever> for booleans?
Every API doc I've seen uses this standard signature so I dont see that we are
"assuming" anything more then we are with the <get> approach.

Werner Guttmann - 13/Sep/04 06:00 AM
> I'm assuming you are referring to the method signatures and such? If
> so the only place I know if is #2 below this link
> http://castor.exolab.org/jdo-mapping.html#The-<field>-element.
I have just been reading this page again, and I simply cannot find any reference
(s) to such a feature, where the getter method for a boolean property will be
set to is<CapitalizedName>().

Nick Stuart - 13/Sep/04 06:18 AM
Oh, well that's because its not there.
That's why I filed this bug, I cause it should be reclassified as an
"enhancment" instead of a bug.

Werner Guttmann - 13/Sep/04 07:43 AM
Oops, sorry for the misunderstanding. All the time whilst following this
thread, I had been under the impression that you are asking for an existing
implementation to be fixed or to be brought into coherence with what the docs
say.

Having said that, I think that the implementation should be flexible in a way
that if an

public boolean isXXX()

is not present, Castor should fall back onto

public boolean getXXX().


Nick Stuart - 13/Sep/04 08:26 AM
No problem Werner, I should have filed it correctly to begin with.

I was just thinking the same thing too, and am surprised I didnt do it that way
to begin with


Nick Stuart - 13/Sep/04 11:27 AM
Adding another patch for handling both get/is setup. Tell me what you think.
(Of course its not final as you'll see the comments, but is it ok to head down
this direction?)

Nick Stuart - 13/Sep/04 11:27 AM
Created an attachment (id=653)
New patch to handle get/is for booleans

Werner Guttmann - 13/Sep/04 12:22 PM
Nick, just looking at your amended patch now. As per our code submission
guidelines, would you be in a position to provide me with a JUnit test case that
successfully tests this new behaviour.

Nick Stuart - 13/Sep/04 01:11 PM
I suppose I could do that, sorry for not posting one earlier. It will be a
little bit before i get to it, but I'll see what I can come up with.

Bruce Snyder (bugzilla) - 13/Sep/04 09:52 PM
> Bruce, also in comment to your first argument. Isn't there an assumption anyways
> that the methods are going to be <get><whatever> format, so why cant we use the
> <is><whatever> for booleans?

I actually agree with you here, Nick. In fact, that's what I stated:

1) All boolean accessors will use the is<field-name>() nomenclature
2) All boolean fields will be of the primitive type boolean

The first assumption can be fixed by testing for both is<field-name() and
get<field-name() accessors, although this may be a bit tougher to accomplish
because as the patch currently stands, an assumption is made at the beginning of
the constructor.

> Every API doc I've seen uses this standard signature so I dont see that we are
> "assuming" anything more then we are with the <get> approach.

My point was that you need to test for both isSomething() and getSomething(),
not just one or the other.


Werner Guttmann - 14/Sep/04 01:31 AM
Alright, bearing the submission of a JUnit test case, your votes please !


Nick Stuart - 14/Sep/04 06:47 AM
Created an attachment (id=660)
Update patch for tests and other issues

Nick Stuart - 14/Sep/04 06:51 AM
Alright, take a look at the above patch. I integrated the test into the
TypeHandling tests as that seems like the most logical place to put it. I've
also update all the SQL Scripts accordingly.

One other issue I found was in the throwing exception part which is unrelated to
this bug, but for some reason a 'null' was being passed and the javaClass.
getName() method was always returning void, so I fixed those too. Should I open
up a new bug for this and post the changes there?

And on a final note. When I was trying to run the tests I couldn't because the
mapping for the TestNested is messed up and I kept getting a stack trace
envolving value2 get method. In the end I just commented that out in the mapping
file and everything but the nested test ran fine. Including my new addition.


Nick Stuart - 14/Sep/04 06:53 AM
And one more comment, Bruce I figured thats what you were talking about and the
new patches include the test for both get/is. Sorry for the mis-understanding
there, but I think its resolved now.

Bruce Snyder (bugzilla) - 14/Sep/04 12:28 PM
I haven't had a chance to test the patch, but it looks good. As for the test
failure, until I dig into it further, I'm not sure why it's occurring because
the method is there.

Werner Guttmann - 17/Sep/04 04:13 AM
> One other issue I found was in the throwing exception part which is
> unrelated to this bug, but for some reason a 'null' was being passed
> and the javaClass.getName() method was always returning void, so I fixed
> those too. Should I open up a new bug for this and post the changes there?
No, let's include this here. As we are already touching FieldMolder, we might
take the opportunity to clean it up a little bit .. .

Werner Guttmann - 17/Sep/04 04:46 AM
> And on a final note. When I was trying to run the tests I couldn't because
> the mapping for the TestNested is messed up and I kept getting a stack
> trace envolving value2 get method. In the end I just commented that out
> in the mapping file and everything but the nested test ran fine. Including
> my new addition.
Can you please have one more look at your patch. I've just checked out a fresh
copy from CVS and run 'test castor.mysql', and I do not get any of those errors
related to the value2 property.

It could be that your patch 'disables' Castor's ability to deal with nested
properties, as this is what the problem seems to be (have a look at
TestNestedFields).


Werner Guttmann - 17/Sep/04 07:10 AM
It actualla did .. I am just in the process of posting another patch
(FieldMolder only) that shows the difference, for completeness sake. Other than
this, I am ready to commit.

Werner Guttmann - 17/Sep/04 07:10 AM
Created an attachment (id=674)
Final patch (FieldMolder only).

Werner Guttmann - 17/Sep/04 07:31 AM
Committed as discussed above.

Btw, in src/tests/jdo/TypeHandling, I had to add testBooleanIsMethod() to
runTest() so that this method would be included during execution of this test case.

Please do not forget to close this bug once you've assessed the code's correctness.


Werner Guttmann - 17/Sep/04 07:35 AM
Btw, please think about what needs to be changed in the documentation at
src/docs to reflect the addition of this new feature.

Nick Stuart - 17/Sep/04 07:36 AM
Quick question, what was the cause for the nested attribute stuff? You said my
patch affected that? If so how? Just wondering cause I'm not sure what would
have done that.

Werner Guttmann - 17/Sep/04 07:39 AM
As you enclosed the old code to find a getter with a

if (method == null) {

one had to set

method = null

at the bottom of while (true) { ..} loop.


Werner Guttmann - 17/Sep/04 08:00 AM
Actually, Nick, it seems that your test case actually fails. Can you please
investigate ?

Nick Stuart - 17/Sep/04 09:36 AM
Yep, I can. Might be able to get to it today, no promises though

Werner Guttmann - 17/Sep/04 10:51 AM
Btw, there's no need to rush, as all test cases still work but the one you've
amended .. whereas they didn't work before I made the final change.

Nick Stuart - 20/Sep/04 08:02 AM
Created an attachment (id=681)
mapping file patch

Nick Stuart - 20/Sep/04 08:03 AM
Found the problem. It looks like the mapping file simply wasnt patched. Doh! Oh
well, attached the patch file for it. Should work now!

Werner Guttmann - 22/Sep/04 07:47 AM
. I'll take care of this as time permits.

Werner Guttmann - 22/Sep/04 07:48 AM
Reopened as per missing patch for mapping file.

Werner Guttmann - 23/Sep/04 12:15 PM
About to check this (last !!!) patch in. If you fancy, have a look at
src/tests/jdo/TypeHandling.java again, and its runTest() method in particular. I
tried to add testSimpleFloat() to it, but this produces an AssertionFailedError
.. .

Werner Guttmann - 23/Sep/04 12:29 PM
Nick, I really appreciate your efforts, but what about providing me with a patch
for the documentation, so that this new feature is properly documented.

Nick Stuart - 24/Sep/04 05:00 AM
Will do that as well werner. Should have it to you by this afternoon.

Nick Stuart - 24/Sep/04 05:19 AM
Created an attachment (id=690)
document patch

Nick Stuart - 24/Sep/04 05:20 AM
Attached the document patch. It seems a little clumsy to me, but not sure if
anything else is possible for this. Its a start and will work for now I think.

Nick Stuart - 24/Sep/04 05:23 AM
Created an attachment (id=691)
edited doc patch

Nick Stuart - 24/Sep/04 05:23 AM
Well just though of another way to word it. Pick which one sounds best to you

Werner Guttmann - 24/Sep/04 05:26 AM
Will do .. .

Werner Guttmann - 24/Sep/04 12:17 PM
Committed. Please do not forget to close this bug report once you've assessed
its correctness.

Bruce Snyder (bugzilla) - 14/Oct/04 08:53 PM
Nick, is this bug closed now?

Nick Stuart - 15/Oct/04 05:39 AM
Yes it is. Sorry about that.