Fix "given type was invalid" error for saving filters #1428
Conversation
Codecov Report
@@ Coverage Diff @@
## gsa-8.0 #1428 +/- ##
=======================================
Coverage 37.1% 37.1%
=======================================
Files 961 961
Lines 21742 21742
Branches 6025 6047 +22
=======================================
Hits 8067 8067
Misses 12365 12365
Partials 1310 1310
Continue to review full report at Codecov.
|
| @@ -604,7 +604,7 @@ class EntitiesContainer extends React.Component { | |||
| <React.Fragment> | |||
| {children({ | |||
| ...other, | |||
| createFilterType: this.props.gmpname, | |||
| createFilterType: apiType(this.props.gmpname), | |||
There was a problem hiding this comment.
If I remember correctly we have done a similar fix already at another place. Would be better if the command class method to create the filter gets fixed instead.
I really would like to use our names at all places in GSA and convert the names to api types when doing an api call only.
There was a problem hiding this comment.
In that case my suggestion is to change gmp/commands/filters.js line 46 from
resource_type: type,
to
resource_type: apiType(type),
There was a problem hiding this comment.
Yes but all calls of this API need to be checked carefully before. Changing it may brake other code.
There was a problem hiding this comment.
Currently our code is full of special handling of the type at different places. We really should move all special handling to the commands (and models). We need to abstract the types away and be consistent in GSA.
But also this can be done in another PR.
There was a problem hiding this comment.
The other place that you mentioned, seems to be getTagsByType() in the container (line 530). But here we are stitching together a filter string, where the apiType is needed at that very place.
There was a problem hiding this comment.
The other place that you mentioned, seems to be getTagsByType() in the container (line 530).
Was't sure about that. Just had in mind we already did implement a similar fix.
There was a problem hiding this comment.
Yes, just found another place: it's for the EntityVerifyIcon where we use apiType again. The thing is, that in this place as well, we are stitching together a string for a request.
My suggestion for this PR and the current type problem: Merge PR as is and add "unifying types" to our to-do for the near future. I would really like the filter creation to work for 5.0.2, but with your concerns about breaking other API calls, I would refrain from changing the command directly. I believe that filter.create() is not used very often and we might be able to test it manually, but with the current time pressure for 5.0.2 we should not take any risks.
There was a problem hiding this comment.
I am all for merging this PR. Just wanted to notice we need a better fix in the future
Checklist: