Hi,
I've been struggling with a Magritte validation problem. I find that a condition I add (using #addCondition:labelled:) to a MABooleanDescription is only intermittently called. I've simplified the conditions to reproduce the problem to a component description comprising of two MABooleanDescription descriptions. The second description includes the condition which is intermittently called.
If the value of the first description is true (ie the checkbox is checked) the second description's condition will be executed, however if it is false it will never be executed. I suspect the problems lies in the code in MAValidatorVisitor>>#visitContainer: which calls MAGraphVisitor>>#use:during: which is as follows: MAGraphVisitor >>use: anObject during: aBlock
| previous | (seen includes: anObject) ifTrue: [ ^ self ].
anObject isNil ifFalse: [ seen add: anObject ]. previous := object. object := anObject.
aBlock ensure: [ object := previous ] If the first check box is false, and the second is also false then (seen includes: anObject) will return true and method returns early resulting in block associated with second description (self visit: description) never being executed.
I've attached a file out of a simple component TestMagritteValidation illustrating the problem that I tested in the latest build from Hudson. Thanks
Nick
_______________________________________________ Magritte, Pier and Related Tools ... https://www.iam.unibe.ch/mailman/listinfo/smallwiki TestMagritteValidation.st (2K) Download Attachment |
Wow, that's a very nasty bug. Amazing that this hasn't hit anybody
before. Looks like this could happen quite common in systems like Pier, not only with MABooleanDescription instances. http://code.google.com/p/magritte-metamodel/issues/detail?id=7 The validation code is really ugly. And likely this bug was introduced when trying to support the validation for recursive structures. I would be happy if someone could rewrite that code? (also get rid of these nested, resumable exceptions). Lukas -- Lukas Renggli www.lukas-renggli.ch _______________________________________________ Magritte, Pier and Related Tools ... https://www.iam.unibe.ch/mailman/listinfo/smallwiki |
HI Lukas,
On 11 October 2010 08:32, Lukas Renggli <[hidden email]> wrote:
Wow, that's a very nasty bug. Amazing that this hasn't hit anybody I'm happy to help, especially as I'd like a fix for the code I'm developing. However I'm struggling to understand the intent of the some of the code for example I don't understand the intent behind the use of object and previous in MAGraphVisitor>>#use:during
MAGraphVisitor>>#use: anObject during: aBlock | previous | (seen includes: anObject)
ifTrue: [ ^ self ]. anObject isNil ifFalse: [ seen add: anObject ].
previous := object. object := anObject. aBlock ensure: [ object := previous ]
_______________________________________________ Magritte, Pier and Related Tools ... https://www.iam.unibe.ch/mailman/listinfo/smallwiki |
> (seen includes: anObject)
> ifTrue: [ ^ self ]. > anObject isNil > ifFalse: [ seen add: anObject ]. That part just tries to avoid to go into recursion if a described object refers to another object that descriptions refer back to the original object. That was requested at some point in the past, but I never dependent on that myself. Lukas On 11 October 2010 11:49, Nick Ager <[hidden email]> wrote: > HI Lukas, > > On 11 October 2010 08:32, Lukas Renggli <[hidden email]> wrote: >> >> Wow, that's a very nasty bug. Amazing that this hasn't hit anybody >> before. Looks like this could happen quite common in systems like >> Pier, not only with MABooleanDescription instances. >> >> http://code.google.com/p/magritte-metamodel/issues/detail?id=7 >> >> The validation code is really ugly. And likely this bug was introduced >> when trying to support the validation for recursive structures. I >> would be happy if someone could rewrite that code? (also get rid of >> these nested, resumable exceptions). > > I'm happy to help, especially as I'd like a fix for the code I'm > developing. > However I'm struggling to understand the intent of the some of the code for > example I don't understand the intent behind the use of object and previous > in MAGraphVisitor>>#use:during > MAGraphVisitor>>#use: anObject during: aBlock > | previous | > (seen includes: anObject) > ifTrue: [ ^ self ]. > anObject isNil > ifFalse: [ seen add: anObject ]. > previous := object. object := anObject. > aBlock ensure: [ object := previous ] > _______________________________________________ > Magritte, Pier and Related Tools ... > https://www.iam.unibe.ch/mailman/listinfo/smallwiki > -- Lukas Renggli www.lukas-renggli.ch _______________________________________________ Magritte, Pier and Related Tools ... https://www.iam.unibe.ch/mailman/listinfo/smallwiki |
On 11 October 2010 10:53, Lukas Renggli <[hidden email]> wrote:
I understood that part but don't understand the intent of:
previous := object. object := anObject. aBlock ensure: [ object := previous ]
_______________________________________________ Magritte, Pier and Related Tools ... https://www.iam.unibe.ch/mailman/listinfo/smallwiki |
That just remembers (in 'previous') and restores the currently
validated object while (in 'object') recursively walking through the graph (which is a tree in 99% of the cases). Lukas On 11 October 2010 12:05, Nick Ager <[hidden email]> wrote: > > > On 11 October 2010 10:53, Lukas Renggli <[hidden email]> wrote: >> >> > (seen includes: anObject) >> > ifTrue: [ ^ self ]. >> > anObject isNil >> > ifFalse: [ seen add: anObject ]. >> >> That part just tries to avoid to go into recursion if a described >> object refers to another object that descriptions refer back to the >> original object. That was requested at some point in the past, but I >> never dependent on that myself. > > I understood that part but don't understand the intent of: > previous := object. object := anObject. > aBlock ensure: [ object := previous ] > _______________________________________________ > Magritte, Pier and Related Tools ... > https://www.iam.unibe.ch/mailman/listinfo/smallwiki > -- Lukas Renggli www.lukas-renggli.ch _______________________________________________ Magritte, Pier and Related Tools ... https://www.iam.unibe.ch/mailman/listinfo/smallwiki |
I've posted a mcz to http://code.google.com/p/magritte-metamodel/issues/detail?id=7
It uses the description instead of the object in the collection of descriptions already visited
John
On Mon, Oct 11, 2010 at 6:11 AM, Lukas Renggli <[hidden email]> wrote: That just remembers (in 'previous') and restores the currently -- http://john-mckeon.us _______________________________________________ Magritte, Pier and Related Tools ... https://www.iam.unibe.ch/mailman/listinfo/smallwiki |
I thought about that too, but this is not a solution: It can be that
different objects are described with the same description instances. Lukas On 12 October 2010 02:46, John McKeon <[hidden email]> wrote: > I've posted a mcz > to http://code.google.com/p/magritte-metamodel/issues/detail?id=7 > It uses the description instead of the object in the collection of > descriptions already visited > John > > On Mon, Oct 11, 2010 at 6:11 AM, Lukas Renggli <[hidden email]> wrote: >> >> That just remembers (in 'previous') and restores the currently >> validated object while (in 'object') recursively walking through the >> graph (which is a tree in 99% of the cases). >> >> Lukas >> >> On 11 October 2010 12:05, Nick Ager <[hidden email]> wrote: >> > >> > >> > On 11 October 2010 10:53, Lukas Renggli <[hidden email]> wrote: >> >> >> >> > (seen includes: anObject) >> >> > ifTrue: [ ^ self ]. >> >> > anObject isNil >> >> > ifFalse: [ seen add: anObject ]. >> >> >> >> That part just tries to avoid to go into recursion if a described >> >> object refers to another object that descriptions refer back to the >> >> original object. That was requested at some point in the past, but I >> >> never dependent on that myself. >> > >> > I understood that part but don't understand the intent of: >> > previous := object. object := anObject. >> > aBlock ensure: [ object := previous ] >> > _______________________________________________ >> > Magritte, Pier and Related Tools ... >> > https://www.iam.unibe.ch/mailman/listinfo/smallwiki >> > >> >> >> >> -- >> Lukas Renggli >> www.lukas-renggli.ch >> _______________________________________________ >> Magritte, Pier and Related Tools ... >> https://www.iam.unibe.ch/mailman/listinfo/smallwiki > > > > -- > http://john-mckeon.us > > _______________________________________________ > Magritte, Pier and Related Tools ... > https://www.iam.unibe.ch/mailman/listinfo/smallwiki > -- Lukas Renggli www.lukas-renggli.ch _______________________________________________ Magritte, Pier and Related Tools ... https://www.iam.unibe.ch/mailman/listinfo/smallwiki |
Wow, that's a hairy problem. I just read the thread a few minutes ago. I agree that using a description does not solve the problem. But if I'm not wrong then the problem exists for every object where
anObject copy == anObject right? (Btw. is there a selector in Smalltalk that can test this?) On the other hand we are talking about preventing infinite loops, right? Is there a chance that any of the objects that meet the condition above can trigger an infinite recursion? I don't think so. So it should be sufficient to manage the seen list only for the object that do no meet the condition. Although the above is neither nice nor fast. Norbert On 12.10.2010, at 08:14, Lukas Renggli wrote: > I thought about that too, but this is not a solution: It can be that > different objects are described with the same description instances. > > Lukas > > On 12 October 2010 02:46, John McKeon <[hidden email]> wrote: >> I've posted a mcz >> to http://code.google.com/p/magritte-metamodel/issues/detail?id=7 >> It uses the description instead of the object in the collection of >> descriptions already visited >> John >> >> On Mon, Oct 11, 2010 at 6:11 AM, Lukas Renggli <[hidden email]> wrote: >>> >>> That just remembers (in 'previous') and restores the currently >>> validated object while (in 'object') recursively walking through the >>> graph (which is a tree in 99% of the cases). >>> >>> Lukas >>> >>> On 11 October 2010 12:05, Nick Ager <[hidden email]> wrote: >>>> >>>> >>>> On 11 October 2010 10:53, Lukas Renggli <[hidden email]> wrote: >>>>> >>>>>> (seen includes: anObject) >>>>>> ifTrue: [ ^ self ]. >>>>>> anObject isNil >>>>>> ifFalse: [ seen add: anObject ]. >>>>> >>>>> That part just tries to avoid to go into recursion if a described >>>>> object refers to another object that descriptions refer back to the >>>>> original object. That was requested at some point in the past, but I >>>>> never dependent on that myself. >>>> >>>> I understood that part but don't understand the intent of: >>>> previous := object. object := anObject. >>>> aBlock ensure: [ object := previous ] >>>> _______________________________________________ >>>> Magritte, Pier and Related Tools ... >>>> https://www.iam.unibe.ch/mailman/listinfo/smallwiki >>>> >>> >>> >>> >>> -- >>> Lukas Renggli >>> www.lukas-renggli.ch >>> _______________________________________________ >>> Magritte, Pier and Related Tools ... >>> https://www.iam.unibe.ch/mailman/listinfo/smallwiki >> >> >> >> -- >> http://john-mckeon.us >> >> _______________________________________________ >> Magritte, Pier and Related Tools ... >> https://www.iam.unibe.ch/mailman/listinfo/smallwiki >> > > > > -- > Lukas Renggli > www.lukas-renggli.ch > > _______________________________________________ > Magritte, Pier and Related Tools ... > https://www.iam.unibe.ch/mailman/listinfo/smallwiki _______________________________________________ Magritte, Pier and Related Tools ... https://www.iam.unibe.ch/mailman/listinfo/smallwiki |
> Wow, that's a hairy problem. I just read the thread a few minutes ago. I agree that using a description does not solve the problem. But if I'm not wrong then the problem exists for every object where
> > anObject copy == anObject > > right? Yeah, the problem is complex. And I am tempted to avoid it by not going deeply into the graph, but by only validating what would be visible (if for example rendered by Seaside). This was the case a few years back, but then somebody complained about incomplete validation of 1:1 and 1:n relationships. Now I do not remember exactly why we changed this, but it seems to me the cause of all problems :-) > (Btw. is there a selector in Smalltalk that can test this?) I don't think so. The code you wrote is probably the only test. > On the other hand we are talking about preventing infinite loops, right? Is there a chance that any of the objects that meet the condition above can trigger an infinite recursion? I don't think so. Correct, but there can be other cases. Imagine multiple 1:1 relationships where the same some of them refer to the same object, but have different validation conditions. Similarly, as we saw before, we can have different objects with the same description instances. Lukas > So it should be sufficient to manage the seen list only for the object that do no meet the condition. Although the above is neither nice nor fast. > > Norbert > > On 12.10.2010, at 08:14, Lukas Renggli wrote: > >> I thought about that too, but this is not a solution: It can be that >> different objects are described with the same description instances. >> >> Lukas >> >> On 12 October 2010 02:46, John McKeon <[hidden email]> wrote: >>> I've posted a mcz >>> to http://code.google.com/p/magritte-metamodel/issues/detail?id=7 >>> It uses the description instead of the object in the collection of >>> descriptions already visited >>> John >>> >>> On Mon, Oct 11, 2010 at 6:11 AM, Lukas Renggli <[hidden email]> wrote: >>>> >>>> That just remembers (in 'previous') and restores the currently >>>> validated object while (in 'object') recursively walking through the >>>> graph (which is a tree in 99% of the cases). >>>> >>>> Lukas >>>> >>>> On 11 October 2010 12:05, Nick Ager <[hidden email]> wrote: >>>>> >>>>> >>>>> On 11 October 2010 10:53, Lukas Renggli <[hidden email]> wrote: >>>>>> >>>>>>> (seen includes: anObject) >>>>>>> ifTrue: [ ^ self ]. >>>>>>> anObject isNil >>>>>>> ifFalse: [ seen add: anObject ]. >>>>>> >>>>>> That part just tries to avoid to go into recursion if a described >>>>>> object refers to another object that descriptions refer back to the >>>>>> original object. That was requested at some point in the past, but I >>>>>> never dependent on that myself. >>>>> >>>>> I understood that part but don't understand the intent of: >>>>> previous := object. object := anObject. >>>>> aBlock ensure: [ object := previous ] >>>>> _______________________________________________ >>>>> Magritte, Pier and Related Tools ... >>>>> https://www.iam.unibe.ch/mailman/listinfo/smallwiki >>>>> >>>> >>>> >>>> >>>> -- >>>> Lukas Renggli >>>> www.lukas-renggli.ch >>>> _______________________________________________ >>>> Magritte, Pier and Related Tools ... >>>> https://www.iam.unibe.ch/mailman/listinfo/smallwiki >>> >>> >>> >>> -- >>> http://john-mckeon.us >>> >>> _______________________________________________ >>> Magritte, Pier and Related Tools ... >>> https://www.iam.unibe.ch/mailman/listinfo/smallwiki >>> >> >> >> >> -- >> Lukas Renggli >> www.lukas-renggli.ch >> >> _______________________________________________ >> Magritte, Pier and Related Tools ... >> https://www.iam.unibe.ch/mailman/listinfo/smallwiki > > > _______________________________________________ > Magritte, Pier and Related Tools ... > https://www.iam.unibe.ch/mailman/listinfo/smallwiki > -- Lukas Renggli www.lukas-renggli.ch _______________________________________________ Magritte, Pier and Related Tools ... https://www.iam.unibe.ch/mailman/listinfo/smallwiki |
Would a hash that combined the description's hash with the container description's hash work? _______________________________________________ Magritte, Pier and Related Tools ... https://www.iam.unibe.ch/mailman/listinfo/smallwiki |
I submitted an update to Magritte-Model that attempts to fix the
problem, by not walking through the complete graph but only through the immediate references. Let's see if this solves more problems than it introduces? Lukas On 12 October 2010 09:32, Nick Ager <[hidden email]> wrote: >> >> Imagine multiple 1:1 relationships where the same some of them refer >> to the same object, but have different validation conditions. >> Similarly, as we saw before, we can have different objects with the >> same description instances. >> > Would a hash that combined the description's hash with the container > description's hash work? > _______________________________________________ > Magritte, Pier and Related Tools ... > https://www.iam.unibe.ch/mailman/listinfo/smallwiki > -- Lukas Renggli www.lukas-renggli.ch _______________________________________________ Magritte, Pier and Related Tools ... https://www.iam.unibe.ch/mailman/listinfo/smallwiki |
Thanks Lukas it solves my problem.
On 12 October 2010 11:59, Lukas Renggli <[hidden email]> wrote: I submitted an update to Magritte-Model that attempts to fix the _______________________________________________ Magritte, Pier and Related Tools ... https://www.iam.unibe.ch/mailman/listinfo/smallwiki |
Free forum by Nabble | Edit this page |