-
Notifications
You must be signed in to change notification settings - Fork 557
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
Update Metadata type in capa main #1502
Update Metadata type in capa main #1502
Conversation
I have made some temporary changes here, Will add from_capa apis to other Models and update it soon. |
Lines 243 to 255 in 8d016de
Here should I change feature_counts & library_functions dict to their Respective models mentioned in ResultDocument. Then we won't have to recast meta in dict and then to Metadata. It will be easy to update any changes. |
would you mark this PR as a draft until you're ready for review? |
7fa74d9
to
445214b
Compare
I have looked into meta types used in main, direct partial update to meta data with outputs from |
Would it make sense to not make these types Frozen then? I think I generally prefer frozen when it’s a choice, but since we’re not hashing the metadata object then I don’t think there’s a good reason for it to be frozen.
|
capa/main.py
Outdated
meta["analysis"].update(counts) | ||
meta["analysis"]["layout"] = compute_layout(rules, extractor, capabilities) | ||
|
||
meta.analysis.__dict__.update(counts) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don’t think there’s a good reason for it to be frozen.
If Frozen = True, we can't do direct item assignment, here, for example using:
meta.analysis.features_counts = counts["feature_counts"]
for the changes I made in this PR shows error:
File "pydantic/main.py", line 359, in pydantic.main.BaseModel.__setattr__
TypeError: "Analysis" is immutable and does not support item assignment
thus, it is updated via __dict__.update(counts)
.
@williballenthin If frozen is removed we won't need to update it via dict.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lets remove frozen so that we can do the updates more easily
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then we won't need from_capa functionality for most of the classes in result_document since we can easily update meta.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@williballenthin if we are removing frozen, I think we should change name FrozenModel class, as it can be misleading. Can you suggest what I should name it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also can you look into script capa_as_library.py
, since we have set meta format as MetaData everywhere. Should the output format in script as Dictionary be removed.
capa/main.py
Outdated
return rdoc.Metadata.from_capa( | ||
{ | ||
"timestamp": datetime.datetime.now().isoformat(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ideally we can just use the rdoc.Metadata
constructor directly here.
capa/ida/helpers.py
Outdated
return capa.render.result_document.Metadata.from_capa( | ||
{ | ||
"timestamp": datetime.datetime.now().isoformat(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ideally we can just use the Metadata
constructor directly here.
you're doing good work @Aayush-Goel-04, keep it up! thank you! |
capa/main.py
Outdated
@@ -1198,7 +1209,7 @@ def main(argv=None): | |||
return E_FILE_LIMITATION | |||
|
|||
# TODO: #1411 use a real type, not a dict here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# TODO: #1411 use a real type, not a dict here. |
capa/render/result_document.py
Outdated
class FrozenModel(BaseModel): | ||
class Config: | ||
frozen = True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lets not call this Frozen
if its not frozen.
for things that can still stay frozen, lets use the existing FrozenModel
. but lets also introduce a Model
class for things that aren't frozen.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these changes look awesome! the only thing i'd like to see is the new Model class, then lets merge and get on to the next thing! thanks @Aayush-Goel-04!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
🎉
Hey @williballenthin @mr-tz , Thanks |
Awesome, some issues that come to mind:
I think these may be a good fit for your interests and skill set! Happy to hear your thoughts on those or any other open issue. We really appreciate your interest and all the great contributions! |
Closes Issue #1411
Closes PR #1444
Checklist