Server-Side Occurrence Names #160
Conversation
daniel-sanche
force-pushed
the
server_side_occurrence
branch
2 times, most recently
from
0f86732
to
ebc4f4e
Compare
|
It looks like some tests are failing because it assumes it can set and retrieve specific occurrence names, so I'll have to fix the tests. Also, right now I'm rejecting any occurrences that specify a name, so the user can get some feedback. This also breaks some tests. Let me know if you'd prefer it if the server just changed the name silently |
daniel-sanche
force-pushed
the
server_side_occurrence
branch
from
ebc4f4e
to
1ef01f9
Compare
|
It might be useful to have the name have some user-friendly part, e.g., the project name as a prefix, and/or allow some customizable prefix or suffix. |
|
@dansanche server should silently ignore any output only fields users set: @mikecico that's an interesting proposal - can you outline your use case for this? |
| log.Printf("Invalid occurrence name: %v", o.Name) | ||
| return nil, status.Error(codes.InvalidArgument, "Invalid occurrence name") | ||
| } | ||
|
|
||
| if o.NoteName == "" { |
There was a problem hiding this comment.
This check might not be necessary, the parse note below should error on empty note name.
| return nil, status.Error(codes.InvalidArgument, "Invalid note name") | ||
| } | ||
| if _, err = g.S.GetProject(pID); err != nil { | ||
| log.Printf("Unable to get project %v, err: %v", pID, err) | ||
| return nil, status.Error(codes.NotFound, fmt.Sprintf("Project %v not found", pID)) |
There was a problem hiding this comment.
I think there is a status.Errorf you can use instead.
Also, for Go style, errors should start with lowercase, you don't have to update all of these, just this one.
| } | ||
| } | ||
| if o.Name != "" { |
There was a problem hiding this comment.
We shouldn't validate output only fields as per:
https://cloud.google.com/apis/design/design_patterns#output_fields
| log.Printf("Invalid Argument. Name field is read-only") | ||
| return nil, status.Error(codes.InvalidArgument, "Name field is read-only") | ||
| } | ||
| // assign a random name |
There was a problem hiding this comment.
I think we can remove this comment, the code should make this obvious.
| randID, err := uuid.NewRandom() | ||
| if err != nil { | ||
| log.Printf("Error Assiging Occurrence Name: %v", err) | ||
| return nil, status.Error(codes.Internal, "Internal Error: Could not generate Occurrence Name") |
There was a problem hiding this comment.
Probably don't need to say "Internal Error" since the status code is already internal.
daniel-sanche
force-pushed
the
server_side_occurrence
branch
17 times, most recently
from
57764be
to
00aef77
Compare
daniel-sanche
force-pushed
the
server_side_occurrence
branch
4 times, most recently
from
ed82c21
to
f7c259c
Compare
daniel-sanche
force-pushed
the
server_side_occurrence
branch
from
f7c259c
to
ed896d6
Compare
|
@vtsao I've addressed your comments, and the tests are now passing |
|
@vtsao we have a use case where we might be storing occurrences from multiple scanners within the same project (the other option of course is to have separate projects for each one, but we're trying to consolidate scanning steps within a pipeline job/step). Since prior to this the naming field was set by the client, we were thinking of creating a shallow hierarchy through naming conventions, which would also make the occurrences more readable, and allow us to be able to categorize occurrences within the project that we can create searches on, etc. I think perhaps allowing clients influence on the naming field might not be necessary, and its probably better to leave it under the control of Grafeas if it is intended to be a generated ID. But perhaps if there were something akin to an optional "display-name" field, or something under client control, that might be useful. Unless there's something we can already leverage for this purpose? |
|
To second @mikecico 's comment above, it will be useful to be able to add labels/tags to occurrences so they can be filtered/queried effectively. |
|
@mikecico and @raj-inamdar If you're storing occurrences from two scanners, the correct thing to do here would be to use separate not projects for each scanner, and then you could differentiate the occurrences by note name. Is there a reason that wouldn't work? |
|
@R2wenD2 In general, we expect to create separate notes projects for each scanner and use the note-name in the occurrences to differentiate, just as you suggest. However, we think that there may be situations where multiple scanners may have some overlap and may identify the same underlying condition. Instead of duplicating notes in respective projects for such scanners, we may want to create a "shared" notes project. In that case, the note-name in occurrences may not be sufficient to differentiate across different scanners. Besides, we think that a user-customizable labels/tags field may be useful in general to add flexibility for filtering, querying, etc. |
|
@mikecico @raj-inamdar thanks for the use case description. Can either of you open an issue so we can track/discuss it there? Thanks. |
| } | ||
| } | ||
| randID, err := uuid.NewRandom() |
There was a problem hiding this comment.
You can do this in a follow-up PR, but we should move the UUID generation to the storage layer, since it's a storage concept. Depending on the DB being used, some people may choose to let the DB create this ID for instance.
|
I have a comment, hope it is not too late. In general it might be useful to have a human readable name. We can keep the original name field which is provided by the user in the CreateOccurrenceRequest and introduce a new ID field which is generated by the Server and returned to user after a successful create. It will keep the API same but existing queries will break, we can address it with a new method to ListOccurrencesByName. |
|
@raj-inamdar thanks. @sandeeps2 can you add your comment on #163. |
|
@vtsao While a general purpose labeling facility will be useful, it is good to have Name as a first class field which can be indexed and easily searched for an Occurrence. My comment was more for the in-flight change for this Issue to retain the Name field of the Occurrence as a user-supplied value and define a new field for the generated ID. I will add my related thoughts on #163 as well. |
|
Is this ready to be merged now Dan? |
|
Yes, I believe it should be. It looks like I don't have merge permissions for the repository |
Addressing the following issue: #157
Now, the server will automatically assign an name to new occurrences, and reject any that try to set an explicit one
I made use of a UUID library to generate the names, so they will look something like "projects/best-vuln-scanner/occurrences/fadad2f2-6899-446d-9428-8876c75c7962"