guac: [bug] nil pointer when filter is not specified
Describe the bug By default, tests were run by specifying an empty filter for queries similar to below:
query HasSBOMQ1 {
HasSBOM(hasSBOMSpec: {}) {
...allHasSBOMTree
}
}
But it is possible to run the query without specifying the filter (not specifying hasSBOMSpec: {}).
query HasSBOM {
HasSBOM {
uri
}
When hasSBOMSpec: {} is not specified, it causes the filter itself to be nil
Also related to the issue: https://github.com/guacsec/guac/issues/1001 and https://github.com/guacsec/guac/issues/1105
Updated need to be made in the graphQL API filter for:
- artifact
- builder
- certifyBad
- certifyGood
- certifyScorecard
- certifyVex
- certifyVuln
- cve (will be deprecated)
- ghsa (will be deprecated)
- osv (will be deprecated)
- hashEqual
- hasSBOM
- hasSLSA
- hasSourceAt
- isDependency
- isOccurrence
- isVulnerability
- pkg
- pkEqual
- src
~~Fix and tests have to be added similar to the changes made in: https://github.com/guacsec/guac/pull/1106~~
EDIT:
Based on the discussion below, updating the graphQL API to ensure that the filter is not null would ensure consistency across the backends and allow for other features (such as pagination) to be added in the future.
For example:
This should be changed to
HasSBOM(hasSBOMSpec: HasSBOMSpec!): [HasSBOM!]!
This will still allow an empty filter but will guarantee that it is not null. Users will be prompted by the error message to add the filter.
To Reproduce Run a query without the filter specified in the graphQL playground
Expected behavior Return a graphQL error message that the filter is required for the query to function (even an empty filter)
Screenshots If applicable, add screenshots to help explain your problem.
GUAC version main (febfb54)
About this issue
- Original URL
- State: closed
- Created a year ago
- Comments: 20
Thank you @mrizzi for testing it! Agree to make pagination have default values.
I think this shouldn’t be a problem, since right now if you use spec-less queries you’d get a segfault (excluding the fix you’ve done in the PR, thank you for that).
We’re also working on ways to ensure some versioning scheme for GraphQL, such that clients would know when new features are available, vs when a breaking change has occured.
I’ve given it a try following @mihaimaruseac’s suggestion and this is how it looks from GraphQL playground and the response which looks quite clear for the user in terms of understanding what is missing.
I think that it definitely possible. We could put a table in release notes with the schema support.
Totally makes sense, thanks.
I was wondering if and how the versioning could also help towards backends implementations.
I mean, let’s assume a new GraphQL file schema is defined for the new schemas version (eg
1.1.0) This implies all the backends have to implement the methods for managing the newly added schema. But the implementations won’t take all the same time so it could be that backend X is “Graphql schemas v1.1.0 compatible” because it already implemented the new schema and, at the same time, backend Y is “Graphql schemas v1.0.0 compatible” because it has no implementation for the v1.1.0 changes yet. Could this be a valid approach for “decoupling” the GraphQL schema changes and the multiple backends implementations?Could you guys assign this issue to me please? I tried locally to apply the changes in order for me to better understand guac and how it works/builds.
Well, it might be out of scope for this issue but I’m very interested in the versioning scheme because, as @mihaimaruseac pointed out, if some changes have to be implemented in all the backends (e.g. pagination), it could be that not all the backends will be ready at the same time so having a way to state which backend supports which GraphQL scheme version would be awesome.
@mihaimaruseac for the pagination, if mandatory, maybe it would be good to evaluate having default values so that if not provided, then everything will work anyway
https://github.com/guacsec/guac/blob/febfb5407256bc8eefde042777a1ed1805673106/pkg/assembler/graphql/schema/hasSBOM.graphql#L66
This should be
Will still allow empty filter, but will guarantee that it is not null. Users will be prompted by the error message to add the filter and this will be consistent with pagination support, etc. that will be implemented