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.

Sign up for GitHub

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

Fix "given type was invalid" error for saving filters #1428

Merged
merged 3 commits into from
May 28, 2019
Merged

Fix "given type was invalid" error for saving filters #1428

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
bb1f40e
Fix "given type was invalid" error for saving filters
sarahd93 May 28, 2019
e9efcde
update changelog
sarahd93 May 28, 2019
870f2d0
Merge branch 'gsa-8.0' into save_filter_gsa8
swaterkamp May 28, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
- Cleanup get_report function in gsad [#1263](https://github.com/greenbone/gsa/pull/1263)

### Fixed
- Fix "given type was invalid" error for saving filters [#1428](https://github.com/greenbone/gsa/pull/1428)
- Fix parsing CVSS authentication SINGLE_INSTANCE [#1427](https://github.com/greenbone/gsa/pull/1427)
- Fix loading data on login [#1426](https://github.com/greenbone/gsa/pull/1426)
- Fix result undefined error on result details [#1423](https://github.com/greenbone/gsa/pull/1423)
Expand Down
2 changes: 1 addition & 1 deletion gsa/src/web/entities/container.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -604,7 +604,7 @@ class EntitiesContainer extends React.Component {
<React.Fragment>
{children({
...other,
createFilterType: this.props.gmpname,
createFilterType: apiType(this.props.gmpname),
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

@swaterkamp swaterkamp May 28, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

In that case my suggestion is to change gmp/commands/filters.js line 46 from
resource_type: type,
to
resource_type: apiType(type),

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes but all calls of this API need to be checked carefully before. Changing it may brake other code.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@bjoernricks bjoernricks May 28, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am all for merging this PR. Just wanted to notice we need a better fix in the future

entities,
entitiesCounts,
entitiesSelected: selected,
Expand Down