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.

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

Id or inputspec #1708

Merged
merged 24 commits into from Feb 18, 2024
Merged

Id or inputspec #1708

merged 24 commits into from Feb 18, 2024

Conversation

pxp928
Copy link
Collaborator

@pxp928 pxp928 commented Feb 16, 2024

Description of the PR

Fixes issue #1261 and updates the keyvalue backend to use noun IDs else use the inputspec for Verb ingestion.

Updates the assembler (bulk and single ingestion) to update the IDorInputSpec for each noun with its IDs before passing them to the verbs to be used.

NOTE: The backends must ensure that the IDs are returned in the same order as the array of InputSpecs, added comments on noun ingestion to call this out.

This PR also deletes all the old unit tests for each backend, in favor of the centralized test suite.

Updates still need to be made on the arango and ent backend. Backend test suite needs to be updated to include ingested noun IDs in verb ingestion.

PR Checklist

  • All commits have a Developer Certificate of Origin (DCO) -- they are generated using -s flag to git commit.
  • All new changes are covered by tests
  • If GraphQL schema is changed, make generate has been run
  • If OpenAPI spec is changed, make generate has been run
  • If collectsub protobuf has been changed, make proto has been run
  • All CI checks are passing (tests and formatting)
  • All dependent PRs have already been merged

Signed-off-by: pxp928 <parth.psu@gmail.com>
Signed-off-by: pxp928 <parth.psu@gmail.com>
Signed-off-by: pxp928 <parth.psu@gmail.com>
Signed-off-by: pxp928 <parth.psu@gmail.com>
Signed-off-by: pxp928 <parth.psu@gmail.com>
Signed-off-by: pxp928 <parth.psu@gmail.com>
Signed-off-by: pxp928 <parth.psu@gmail.com>
Signed-off-by: pxp928 <parth.psu@gmail.com>
Signed-off-by: pxp928 <parth.psu@gmail.com>
Signed-off-by: pxp928 <parth.psu@gmail.com>
Signed-off-by: pxp928 <parth.psu@gmail.com>
Signed-off-by: pxp928 <parth.psu@gmail.com>
Signed-off-by: pxp928 <parth.psu@gmail.com>
Signed-off-by: pxp928 <parth.psu@gmail.com>
Signed-off-by: pxp928 <parth.psu@gmail.com>
Signed-off-by: pxp928 <parth.psu@gmail.com>
Signed-off-by: pxp928 <parth.psu@gmail.com>
Signed-off-by: pxp928 <parth.psu@gmail.com>
@pxp928
Copy link
Collaborator Author

pxp928 commented Feb 16, 2024

Working to fix up the failing tests

Signed-off-by: pxp928 <parth.psu@gmail.com>
Signed-off-by: pxp928 <parth.psu@gmail.com>
Signed-off-by: pxp928 <parth.psu@gmail.com>
Signed-off-by: pxp928 <parth.psu@gmail.com>
@pxp928
Copy link
Collaborator Author

pxp928 commented Feb 16, 2024

Ready for review

@pxp928
Copy link
Collaborator Author

pxp928 commented Feb 16, 2024

@mrizzi FYI. Will need to make some updates on the ENT backend for this to be fully utilized. I will start some of the work today and we can sync up on it.

Copy link
Collaborator

@mihaimaruseac mihaimaruseac left a comment

Choose a reason for hiding this comment

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

I'm wondering, wouldn't it be better to make the input specs have an ID field and remove the !s and move the requirement for the fields to be specified and not null in the root resolvers instead? It would at least remove remove all code changes for the verbs, I think.

cmd/guacone/cmd/gcs_test.go Outdated Show resolved Hide resolved
Makefile Show resolved Hide resolved
@pxp928
Copy link
Collaborator Author

pxp928 commented Feb 16, 2024

I'm wondering, wouldn't it be better to make the input specs have an ID field and remove the !s and move the requirement for the fields to be specified and not null in the root resolvers instead? It would at least remove remove all code changes for the verbs, I think.

We chatted about this and this would be the cleanest approach without removing which fields are required.

Copy link
Collaborator

@jeffmendoza jeffmendoza left a comment

Choose a reason for hiding this comment

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

Not sure about the schema change for noun ingests. Maybe ask the other maintainers.

Otherwise looks good with a small nit.

pkg/assembler/clients/helpers/bulk.go Show resolved Hide resolved
pkg/assembler/graphql/schema/artifact.graphql Show resolved Hide resolved
@mihaimaruseac
Copy link
Collaborator

Approving in this case, though I still think that it adds one extra indirection in the query. The generated code now does double verification that the fields are not null, if we remove the ! the generated code would do none and we'd add it manually in the top-level resolvers. But I think this also changes the types of some arguments, so let's go with this at least for now.

@jeffmendoza
Copy link
Collaborator

I like the "union" approach as it is more clear to a reader of the schema what is and isn't required for an ingest, otherwise it would just be documentation or reading code to tell you that you can either supply ID or the rest of the fields in the object.

Signed-off-by: pxp928 <parth.psu@gmail.com>
Signed-off-by: pxp928 <parth.psu@gmail.com>
@kodiakhq kodiakhq bot merged commit 5fbba0d into guacsec:main Feb 18, 2024
8 checks passed
@pxp928 pxp928 deleted the id-or-inputspec branch February 18, 2024 19:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants