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

Moved the model context from the TM class into the TM object #175

Closed
wants to merge 14 commits into from

Conversation

raphaelahrens
Copy link
Contributor

@raphaelahrens raphaelahrens commented Sep 23, 2021

To make it possible to import other models into the current model
without adding all elements the elements of the model are now stored in
the TM object and not the class.

I addition I added some test models to check how they differ between each other and between the current master branch.
But the result of the models have to be compared manual, because the UUID and the order often differ even though the resulting report and the diagrams are similar. Maybe it is possible to automate this process more.

Since I can only do manual checking it would be good if someone else checked if this PR is not changing the original behavior.

@raphaelahrens raphaelahrens requested a review from izar as a code owner September 23, 2021 09:01
@izar izar requested review from nineinchnick and removed request for izar September 23, 2021 12:56
@izar
Copy link
Collaborator

izar commented Sep 23, 2021

@nineinchnick you're WAY better at this stuff than I am, do you mind taking a look?

@nineinchnick
Copy link
Collaborator

I'll try tomorrow, I just rebased #75, and that took like 2 hours so I'm spent.

@ghost
Copy link

ghost commented Oct 2, 2021

CodeSee Review Map:

Review these changes using an interactive CodeSee Map

Review in an interactive map

View more CodeSee Maps

Legend

CodeSee Map Legend

@raphaelahrens
Copy link
Contributor Author

raphaelahrens commented Oct 5, 2021

@nineinchnick I wanted to ask what the opinion is about this PR?
Is it worth keeping it up to date with master?

@nineinchnick
Copy link
Collaborator

@nineinchnick I wanted to ask what the opinion is about this PR? Is it worth keeping it up to date with master?

Sorry, it's rather big and I'm struggling to find time to review it as a whole, not just nitpicking.

In the description you mentioned something about UUIDs being different and having to do manual tests. Can you try using random.seed(0)? We won't accept this without automated tests.

@raphaelahrens
Copy link
Contributor Author

Yes I do set the seed to zero with import norandom.
The issue is the order in which the elements are created.
For example the test files test0.py and test1.py produce the same model. But the order of the element creation differs. To then check if the models are the same I have to manually look at the diff to see that the uuids are switched.

Maybe you can just ignore the commit with my test cases 623d1f5
They might only be useful to me.

@raphaelahrens
Copy link
Contributor Author

I added some unit tests which are similar to the original tests.
While doing this I also found out that it is no longer necessary to set random.seed(0), see 3c0267a.

But still as soon as the creation order differs the uuids will change, which is rather annoying when comparing two threat models that are the same except for the import. See diff seq.plantuml import_data/seq.plantuml for example.

@raphaelahrens
Copy link
Contributor Author

Hi,

should I close the PR?

@izar
Copy link
Collaborator

izar commented Feb 18, 2022

Sorry, it looks like we are all busy with life I am looking for @nineinchnick 's review, but he seems to be unable at this time. Let's wait a bit more for his chiming in before we decide where this goes.

nozmore-vera and others added 12 commits April 28, 2022 21:44
To make it possible to import other models into the current model
without adding all elements the elements of the model are now stored in
the TM object and not the class.
To add the imported elements and data objects it is necessary to figure
out which elements an attributes are not part of the current TM.
For this `TM._add_imported_elements(self)` collects all elements and
data objects which are used by all `Dataflows` and then gets the
difference between the set of elements found in the flows and the set of
elements defined in the TM.

The result of `TM._add_imported_elements(self)` is merged by just adding the
imported elements to the lists

* `tm._actors`
* `tm._assets`
* `tm._boundaries`
* `tm._data`
* `tm._elements`

The behavior of `TM._add_imported_elements(self)` is very similar to
`_get_elements_and_boundaries(flows)`, only that
`_add_imported_elements` also needs to check if there are instances of
Data, Actor and Asset to add them to the specific list.
I hopes this would return more comparable results, but the oder of the
elements and there given uuid is affect by the creation time.
So the diffs need to be checked manually.

My make skills are a bit rusty so the make files might not be very
portable.
While keeping uptodaate with master some changes needed to be adjusted
to use the TM objects instead of the class
Added unit tests with models which import a test module
@izar
Copy link
Collaborator

izar commented Apr 28, 2022

Hi @raphaelahrens - I think this got superseded by #190 - please let me know if that's not the case..

@raphaelahrens
Copy link
Contributor Author

raphaelahrens commented Apr 29, 2022

Hi,

the problem is the commit is all over the place and keeping it in sync with upstream requires to much time.

It will be easier to start fresh.

@izar
Copy link
Collaborator

izar commented Oct 11, 2022 via email

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.

5 participants