Fix, GDL2, EORelationship (-validateName:)

Hello,

here is a patch for EORelationship, proposing a fix for
awakening of  
attributes that have a definition.

When relationships are constructed they are validated
against  
attributes. That in turn awakens the attributes, being
dependent on  
relationships that are not there at this time. The
definitions will  
fail to be set.
Here is the part of the stack of that problematic
dependency:

#0	0x002237aa in -[EOEntity(EOEntityPrivateXX)
_parsePropertyName:]  
at EOEntity.m:3940
#1	0x002223c5 in -[EOEntity(EOEntityPrivateXX)  
_parseDescription:isFormat:arguments:] at EOEntity.m:3721
#2	0x0022813a in -[EOAttribute(EOAttributeEditing)  
_setDefinitionWithoutFlushingCaches:] at EOAttribute.m:832
#3	0x0022821c in -[EOAttribute(EOAttributeEditing)
setDefinition:] at  
EOAttribute.m:878
#4	0x002264b9 in -[EOAttribute awakeWithPropertyList:] at  
EOAttribute.m:249
#5	0x00214630 in -[EOEntity attributes] at EOEntity.m:839
#6	0x0021cba9 in -[EOEntity(EOEntityPrivate)
attributesByName] at  
EOEntity.m:2506
#7	0x00214907 in -[EOEntity attributeNamed:] at
EOEntity.m:878
#8	0x00214ba9 in -[EOEntity anyAttributeNamed:] at
EOEntity.m:903
#9	0x0022ff67 in -[EORelationship(EORelationshipEditing)  
validateName:] at EORelationship.m:1181
#10	0x0023034b in -[EORelationship(EORelationshipEditing)
setName:]  
at EORelationship.m:1245
#11	0x0022b236 in -[EORelationship
initWithPropertyList:owner:] at  
EORelationship.m:151
#12	0x0022ae1f in +[EORelationship  
relationshipWithPropertyList:owner:] at EORelationship.m:89
#13	0x00214ffb in -[EOEntity relationships] at
EOEntity.m:964


The attached patch (-c) is fixing the issue by simply not
validating  
the relationships against attributes.
This seems to be correct behaviour, since the class
documentation  
doesn't mention a test against attributes, only against
relationships  
and stored procedures. Quote from the EOAccess docu:

validateName:
"Validates name and returns nil if its a valid name, or
an exception  
if it isn’t. A name is invalid if it has zero
length; starts with a character other than a letter, a
number, or  
“”,
“#”, or “_”; or contains a character other
than a letter, a number, “”, “#”, “_”, or “$”. A name
is also  
invalid if the receiver’s EOEntity already has
an EORelationship with the same name, or if the model has a
stored  
procedure that has an argument with
the same name."

Best wishes,
Georg Fleischmann



*** EOAccess/EORelationship.m.old	Wed Mar 12 14:30:36 2008
--- EOAccess/EORelationship.m	Mon Apr 28 16:27:05 2008
***************
*** 1178,1186 ****
   					 *p]
                           userInfo: nil];

!       if ([[self entity] anyAttributeNamed: name])
           exc++;
!       else if ([[self entity] anyRelationshipNamed:
name])
           exc++;
         else if ((storedProcedures = [[[self entity] model]
 
storedProcedures]))
           {
--- 1178,1189 ----
   					 *p]
                           userInfo: nil];

!       /* validating with attributes creates the problem,
that on  
awakening the relationships,
!          the attributes are not there yet, and awakening
them will  
require the relationships.
!          The EOAccess class documentation does not mention
a test  
against attributes! */
!       /*if ([[self entity] anyAttributeNamed: name])
           exc++;
!       else*/ if ([[self entity] anyRelationshipNamed:
name])
           exc++;
         else if ((storedProcedures = [[[self entity] model]
 
storedProcedures]))
           {



_______________________________________________
Bug-gnustep mailing list
Bug-gnustepgnu.org
htt
p://lists.gnu.org/mailman/listinfo/bug-gnustep

Hello Georg,

Thanks for the report!

Georg Fleischmann schrieb:

> here is a patch for EORelationship, proposing a fix for
awakening of
> attributes that have a definition.
> 
> When relationships are constructed they are validated
against
> attributes. That in turn awakens the attributes, being
dependent on
> relationships that are not there at this time. The
definitions will fail
> to be set.
> Here is the part of the stack of that problematic
dependency:

OK, we have a bug.  We are working out a test case for the
test suite.

> The attached patch (-c) is fixing the issue by simply
not validating the
> relationships against attributes.
> This seems to be correct behaviour, since the class
documentation
> doesn't mention a test against attributes, only against
relationships
> and stored procedures. Quote from the EOAccess docu:
> 
> validateName:
> "Validates name and returns nil if its a valid
name, or an exception if
> it isn’t. A name is invalid if it has zero
> length; starts with a character other than a letter, a
number, or β€œβ€,
> β€œ#”, or β€œ_”; or contains a character other
> than a letter, a number, β€œβ€, β€œ#”, β€œ_”, or
β€œ$”. A name is also invalid
> if the receiver’s EOEntity already has
> an EORelationship with the same name, or if the model
has a stored
> procedure that has an argument with
> the same name."

I'm not sure.  Matt has been doing a lot of work with
respect to
validation, especially in conjunction with DBModeler.  I'd
rather like
to give him a chance to handle it.

Thanks!
David


_______________________________________________
Bug-gnustep mailing list
Bug-gnustepgnu.org
htt
p://lists.gnu.org/mailman/listinfo/bug-gnustep

On Mon, Apr 28, 2008 at 11:56 PM, David Ayers <ayersfsfe.org> wrote:
> Hello Georg,
>
>  Thanks for the report!
>
>  Georg Fleischmann schrieb:
>
>  > here is a patch for EORelationship, proposing a
fix for awakening of
>  > attributes that have a definition.
>  >
>  > When relationships are constructed they are
validated against
>  > attributes. That in turn awakens the attributes,
being dependent on
>  > relationships that are not there at this time.
The definitions will fail
>  > to be set.
>  > Here is the part of the stack of that problematic
dependency:
>
>  OK, we have a bug.  We are working out a test case for
the test suite.

i haven't got a test case for it yet, but i see from the
backtrace
that things would go awry after
frame #5,

>
>  > The attached patch (-c) is fixing the issue by
simply not validating the
>  > relationships against attributes.
>  > This seems to be correct behaviour, since the
class documentation
>  > doesn't mention a test against attributes, only
against relationships
>  > and stored procedures. Quote from the EOAccess
docu:
>  >
>  > validateName:
>  > "Validates name and returns nil if its a
valid name, or an exception if
>  > it isn't. A name is invalid if it has zero
>  > length; starts with a character other than a
letter, a number, or "",
>  > "#", or "_"; or contains a
character other
>  > than a letter, a number, "",
"#", "_", or "$". A name is
also invalid
>  > if the receiver's EOEntity already has
>  > an EORelationship with the same name, or if the
model has a stored
>  > procedure that has an argument with
>  > the same name."
>

that documentation is actually incomplete

however, IIRC it is a gdl2 extension that we do any
validation of
model objects loaded from plists.
I only recently changed this area to using validateName:

previously it was manually doing duplicate name checks, but
lacked any
checks for invalid names.
and I thought it was safe but appears not to be in the case
of definitions.

though, we can get a list of names for dupe checking
without
triggering the loading code,
attached is a patch which implements this, we should
probably do a
similar thing for relationships/etc
if it becomes an issue there as well...

seems to pass the testsuite,
let me know if this fixes your bug; and if you have time to
come up
with a test for it, that'd be helpful

i haven't tried it yet but
it might perform better if we use _attributesByName if it
exists, and
then falling back on the [_attributes valueForKey:] stuff

though we can't currently use -attributesByName: to create
_attributesByName without triggering loading...
making it possible is another idea, but a bit more
complicated,
(-_attributesByName:loadFlag?

for loadFlag:
NO would potentially return objects with which are not yet
awakened
(dictionaries or EOAttributes depending on if loading has
occured
previously) and YES would cause the loading code to occur if
it hasn't
already and initialize/awaken attributes and all objects
would be
EOAttribute objects.

anyhow sounds like quite a bit of work i'm not sure how
critical
performance is for the methods involved 

matt

_______________________________________________
Bug-gnustep mailing list
Bug-gnustepgnu.org
htt
p://lists.gnu.org/mailman/listinfo/bug-gnustep

Hello Matt,

On 30.04.2008, at 22:27, Matt Rice wrote:

> though, we can get a list of names for dupe checking
without
> triggering the loading code,
> attached is a patch which implements this

> let me know if this fixes your bug; and if you have
time to come up
> with a test for it, that'd be helpful

Yes your patch works fine for me, thanks!
Test? Well, I see myself unable to provide one, sorry.

I get this warning, when compiling with your patch:
  Compiling file EOEntity.m ...
EOEntity.m:3280: warning: incomplete implementation of
category  
'EOEntityPrivate'
EOEntity.m:3280: warning: method definition for '- 
_hasAnyAttributeNamed:' not found


Best wishes,
Georg Fleischmann


On 30.04.2008, at 22:27, Matt Rice wrote:

> On Mon, Apr 28, 2008 at 11:56 PM, David Ayers
<ayersfsfe.org> wrote:
>> Hello Georg,
>>
>>  Thanks for the report!
>>
>>  Georg Fleischmann schrieb:
>>
>>> here is a patch for EORelationship, proposing a
fix for awakening of
>>> attributes that have a definition.
>>>
>>> When relationships are constructed they are
validated against
>>> attributes. That in turn awakens the
attributes, being dependent on
>>> relationships that are not there at this time.
The definitions  
>>> will fail
>>> to be set.
>>> Here is the part of the stack of that
problematic dependency:
>>
>>  OK, we have a bug.  We are working out a test case
for the test  
>> suite.
>
> i haven't got a test case for it yet, but i see from
the backtrace
> that things would go awry after
> frame #5,
>
>>
>>> The attached patch (-c) is fixing the issue by
simply not  
>>> validating the
>>> relationships against attributes.
>>> This seems to be correct behaviour, since the
class documentation
>>> doesn't mention a test against attributes, only
against  
>>> relationships
>>> and stored procedures. Quote from the EOAccess
docu:
>>>
>>> validateName:
>>> "Validates name and returns nil if its a
valid name, or an  
>>> exception if
>>> it isn't. A name is invalid if it has zero
>>> length; starts with a character other than a
letter, a number, or  
>>> "",
>>> "#", or "_"; or contains a
character other
>>> than a letter, a number, "",
"#", "_", or "$". A name is
also  
>>> invalid
>>> if the receiver's EOEntity already has
>>> an EORelationship with the same name, or if the
model has a stored
>>> procedure that has an argument with
>>> the same name."
>>
>
> that documentation is actually incomplete
>
> however, IIRC it is a gdl2 extension that we do any
validation of
> model objects loaded from plists.
> I only recently changed this area to using
validateName:
>
> previously it was manually doing duplicate name checks,
but lacked any
> checks for invalid names.
> and I thought it was safe but appears not to be in the
case of  
> definitions.
>
> though, we can get a list of names for dupe checking
without
> triggering the loading code,
> attached is a patch which implements this, we should
probably do a
> similar thing for relationships/etc
> if it becomes an issue there as well...
>
> seems to pass the testsuite,
> let me know if this fixes your bug; and if you have
time to come up
> with a test for it, that'd be helpful
>
> i haven't tried it yet but
> it might perform better if we use _attributesByName if
it exists, and
> then falling back on the [_attributes valueForKey:]
stuff
>
> though we can't currently use -attributesByName: to
create
> _attributesByName without triggering loading...
> making it possible is another idea, but a bit more
complicated,
> (-_attributesByName:loadFlag?
>
> for loadFlag:
> NO would potentially return objects with which are not
yet awakened
> (dictionaries or EOAttributes depending on if loading
has occured
> previously) and YES would cause the loading code to
occur if it hasn't
> already and initialize/awaken attributes and all
objects would be
> EOAttribute objects.
>
> anyhow sounds like quite a bit of work i'm not sure how
critical
> performance is for the methods involved 
>
> matt<foo.diff>



_______________________________________________
Bug-gnustep mailing list
Bug-gnustepgnu.org
htt
p://lists.gnu.org/mailman/listinfo/bug-gnustep

On Wed, Apr 30, 2008 at 11:05 PM, Georg Fleischmann
<G.Fleischmannvhf.de> wrote:
> Hello Matt,
>
>
>  On 30.04.2008, at 22:27, Matt Rice wrote:
>
>
> > though, we can get a list of names for dupe
checking without
> > triggering the loading code,
> > attached is a patch which implements this
> >
>
>
> > let me know if this fixes your bug; and if you
have time to come up
> > with a test for it, that'd be helpful
> >
>
>  Yes your patch works fine for me, thanks!

OK i commited something similar,

>  Test? Well, I see myself unable to provide one,
sorry.
>

that's fine i'll try to come up with one here shortly,
FYI if you aren't aware of the testsuite:

svn://svn.gna.org/svn/gnustep/tests/testsuite/trunk

there is some documentation in
README and gdl2/README

>  I get this warning, when compiling with your patch:
>   Compiling file EOEntity.m ...
>  EOEntity.m:3280: warning: incomplete implementation of
category
> 'EOEntityPrivate'
>  EOEntity.m:3280: warning: method definition for
'-_hasAnyAttributeNamed:'
> not found
>

Thanks, i moved the implementation to the correct category.
this appears to be something we don't warn about in FSF gcc


_______________________________________________
Bug-gnustep mailing list
Bug-gnustepgnu.org
htt
p://lists.gnu.org/mailman/listinfo/bug-gnustep