Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixing nested Boundaries #54

Merged
merged 1 commit into from
Nov 7, 2019
Merged

Fixing nested Boundaries #54

merged 1 commit into from
Nov 7, 2019

Conversation

jdcallet
Copy link

@jdcallet jdcallet commented Nov 6, 2019

No description provided.

@jdcallet jdcallet requested a review from izar as a code owner November 6, 2019 22:14
@ghost
Copy link

ghost commented Nov 6, 2019

DeepCode Report (#87e1ec)

DeepCode analyzed this pull request.
There are no new issues.

@izar
Copy link
Collaborator

izar commented Nov 6, 2019

Thanks for the PR! Could you provide before/after examples of how the issue appears and is resolved by your patch?

if type(e) == Boundary:
if not e._is_drawn:
_debug(result, "Now drawing boundary " + e.name)
e.dfd()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch - your approach to resolve makes sense. However I think this change can lead to elements being drawn twice due to the fact that TM._BagOfElements can contain Boundary objects. That is, due to the recursive calling of Boundary.dfd in the nested boundary case, we'll end up drawing any elements that are present inside nested boundaries multiple times - each element being drawn once for each boundary it's inside.

To handle this, we could do something like setting an attribute _is_drawn = False on the Element object also and set it to true when Element.dfd() is called (and we'll need to do the same for any objects that inherit from Element and override Element.dfd()). Then, when we iterate through TM._BagOfElements in the for loop, we check if not e._is_drawn and only call e.dfd if we've not drawn the element already. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jdcallet I explored this approach in e65af20

@izar izar merged commit 07eaa41 into OWASP:master Nov 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants