[feature] Change mutation API to remove requirement to return all values except ID

[lumjjb]

Is your feature request related to a problem? Please describe.
Change mutation APIs to remove requirement to return the entire struct, but instead just IDs. This will make it so that mutations will have better execution time as usually results in INSERTs being used instead of UPSERTs with a followup retrieval if necessary.

A field is still required since GraphQL requires a return statement.

This stems from efforts in the #910, as well as some optimization efforts for implementation in ArangoDB.

Describe the solution you'd like

For each mutation like the following example for HasSBOM, go from

extend type Mutation {
  "Certifies that a package or artifact has an SBOM."
  ingestHasSBOM(subject: PackageOrArtifactInput!, hasSBOM: HasSBOMInputSpec!): HasSBOM!
  "Bulk ingest that package or artifact has an SBOM."
  ingestHasSBOMs(subjects: PackageOrArtifactInputs!, hasSBOMs: [HasSBOMInputSpec!]!): [HasSBOM!]!
}

to

extend type Mutation {
  "Certifies that a package or artifact has an SBOM."
  ingestHasSBOM(subject: PackageOrArtifactInput!, hasSBOM: HasSBOMInputSpec!): string!
  "Bulk ingest that package or artifact has an SBOM."
  ingestHasSBOMs(subjects: PackageOrArtifactInputs!, hasSBOMs: [HasSBOMInputSpec!]!): [string!]!
}

Describe alternatives you've considered

Return a boolean instead. However IDs are still usable. If the ID is not returnable, returning an empty string is also fine.

Additional context
Add any other context or screenshots about the feature request here.

[lumjjb]

FYI. @pxp928 @ivanvanderbyl @stevemenezes

[ivanvanderbyl]

Oh this would be neat for Postgres since upserts can only return IDs and currently require a separate query to fetch the actual record. However, bulk upserts still require a separate query.

[mihaimaruseac]

I'd say it should be ID! instead of string!.

[arorasoham9]

I can take this.

[mihaimaruseac]

Happy to help you, answer any question you have to solve this issue. Looking forward for a PR for this!

[arorasoham9]

@mihaimaruseac Could I please know what files I should look at to get a better understanding of what needs changes?

[arorasoham9]

I took a look at this issue and have reached this understanding. The GraphQL mutations are to be edited to return a String! or a [String!]! and instead of returning the struct array or struct array in the mutationresolvers, we return the string IDs. Please correct me if my understanding is off here. Thanks!

[pxp928]

yes that is correct so all the changes need to happen here on the server side: https://github.com/guacsec/guac/tree/main/pkg/assembler/graphql/schema (for all that contain a mutation) and on the client side: https://github.com/guacsec/guac/tree/main/pkg/assembler/clients/operations

For the Server side, all mutations have to change to:

For Example:

extend type Mutation {
  "Certifies that a package or artifact has an SBOM."
  ingestHasSBOM(subject: PackageOrArtifactInput!, hasSBOM: HasSBOMInputSpec!): ID!
  "Bulk ingest that package or artifact has an SBOM."
  ingestHasSBOMs(subjects: PackageOrArtifactInputs!, hasSBOMs: [HasSBOMInputSpec!]!): [ID!]!
}

For Client side, all the mutations will change to expect a return of an ID:

For example:

mutation IngestArtifact($artifact: ArtifactInputSpec!) {
  ingestArtifact(artifact: $artifact) {
    ID
  }
}

# Bulk Ingest Artifacts

mutation IngestArtifacts($artifacts: [ArtifactInputSpec!]!) {
  ingestArtifacts(artifacts: $artifacts) {
    ID
  }
}

After that you need to regenerate the code: make generate. This will most likely break a lot of the assembler, CLI and other...so that will have to be fixed to accept only IDs.

[arorasoham9]

That answered my queries as well. Thank you, @pxp928 . I'll try to send a PR soon.

[mihaimaruseac]

Sorry, been travelling. @pxp928 's answer is complete

[arorasoham9]

Could I get some insight on this error, please?
operations/vulnEqual.graphql:22: query-spec does not match schema: Cannot query field "ID" on type "ID".
It appears when I run go generate

[pxp928]

What have you changed so far @arorasoham9? Based on that we can figure out where the error is coming from. Are you working on a branch that we can look at the diff?

[arorasoham9]

@pxp928 This is what I have for now arorasoham9@7a7c2c6

[pxp928]

hmmm it's a strange error. @mihaimaruseac any thoughts why this would happen? I am looking into it more...

[arorasoham9]

https://stackoverflow.com/questions/56345757/cannot-query-field-id-on-type-createuser-with-official-graphql-tutorial
This could be it.

[arorasoham9]

I am guessing the fix is to use the spread operator like so.

mutation IngestBuilder($builder: BuilderInputSpec!) { ingestBuilder(builder: $builder) { ...ID } }

[pxp928]

I am guessing the fix is to use the spread operator like so.
mutation IngestBuilder($builder: BuilderInputSpec!) { ingestBuilder(builder: $builder) { ...ID } }

No this does not work. I just tried it

[mihaimaruseac]

I think because we return a scalar we don't need anything in {}. So mutation IngestBuilder($builder: BuilderInputSpec!) { ingestBuilder(builder: $builder) {} (or maybe even without the {}?

[pxp928]

@arorasoham9 looks like just doing:

mutation IngestBuilder($builder: BuilderInputSpec!) {
  ingestBuilder(builder: $builder)
}

works

Thanks @mihaimaruseac!

[mihaimaruseac]

We'll need to update examples too, in case we look at the values generated from the ingestion

[arorasoham9]

Can I safely ignore these errors that appear after I run make generate?

go generate ./... validation failed: packages.Load: /home/vagrant/guac/pkg/assembler/graphql/resolvers/artifact.resolvers.go:16:9: cannot use r.Backend.IngestArtifact(ctx, artifact) (value of type *model.Artifact) as string value in return statement /home/vagrant/guac/pkg/assembler/graphql/resolvers/artifact.resolvers.go:21:9: cannot use r.Backend.IngestArtifacts(ctx, artifacts) (value of type []*model.Artifact) as []string value in return statement /home/vagrant/guac/pkg/assembler/graphql/resolvers/builder.resolvers.go:15:9: cannot use r.Backend.IngestBuilder(ctx, builder) (value of type *model.Builder) as string value in return statement /home/vagrant/guac/pkg/assembler/graphql/resolvers/builder.resolvers.go:20:9: cannot use r.Backend.IngestBuilders(ctx, builders) (value of type []*model.Builder) as []string value in return statement /home/vagrant/guac/pkg/assembler/graphql/resolvers/certifyBad.resolvers.go:15:9: cannot use r.Backend.IngestCertifyBad(ctx, subject, &pkgMatchType, certifyBad) (value of type *model.CertifyBad) as string value in return statement /home/vagrant/guac/pkg/assembler/graphql/resolvers/certifyBad.resolvers.go:20:9: cannot use r.Backend.IngestCertifyBads(ctx, subjects, &pkgMatchType, certifyBads) (value of type []*model.CertifyBad) as []string value in return statement

I believe they are coming from the resolvers that require the return value changed.

[mihaimaruseac]

I think so too.

[pxp928]

Yes you can update the resolver based on updating the backend interface.

[pxp928]

Re-opening issue as updates need to be made to the backends to only return the IDs (currently returning the actual model object)

[pxp928]

Marking this issue as complete and follow-up work will be done in the issues referenced above.

[mrizzi]

Worth having a branch in the repo (e.g. 1116-return-ID) with the changes to Backend interface in backends.go and against which every backend can open PRs?
This should simplify the gathering of changes from all the backends.
Once done, 1116-return-ID could be merged into main with no effort/breaking changes.

WDYT?

[pxp928]

sure that sounds good, here is the branch - https://github.com/guacsec/guac/tree/1116-return-ID

[mrizzi]

sure that sounds good, here is the branch - https://github.com/guacsec/guac/tree/1116-return-ID

created #1285 for changing the Backend interface so that, once merged, all the backends can start with the implementations