Skip to content

Conversation

@severussundar
Copy link
Contributor

@severussundar severussundar commented Feb 2, 2023

Why make this change?

  1. Setup a stored procedure entity using add command
    dab add sptest --source "sptest" --source.type "stored-procedure" --permissions "anonymous:execute" --rest.methods "get,post" --graphql.operation "query"

Config after running this command:

 "rest": {
        "methods": [
          "get",
          "post"
        ]
      },
      "graphql": {
        "operation": "Query"
      }
  1. Update REST properties using update command
    dab update sptest --rest false
      "rest": false,

The graphql elements are lost. The correct config should've been as shown below

Expected config:

  "rest": false,
      "graphql": {
        "operation": "Query"
      }
  • When trying to fetch the operations configured for stored procedure, it was coming out as null since the type of GraphQL object was not resolved correctly. Due to this, during the execution of CLI update command, the operations property inside graphql was becoming null while it should've been what was configured earlier using add command.

What is this change?

  • Introduces a new method FetchGraphQLOperation which can be utilized when the type of the GraphQL is JsonElement. At the moment, CLI makes use of this.
  • Renamed existing method to fetch the graphQLOperation as FetchConfiguredGraphQLOperation. This is utilized when the GraphQL is completely deserialized to a concrete object. Currently, utilized by engine components such as QueryBuilder, MutationBuilder etc.
  • Includes a check for JsonValueKind.Arrary type for Rest. This is possible when --rest.methods is configured but nothing is specified for rest option.
  • Added more tests that check for the exact JSON generated for add/update commands.

How was this tested?

  • Existing Integration Tests
  • Unit Tests
  • Manually

Manual Testing

Config after running
dab add sptest --source "sptest" --source.type "stored-procedure" --permissions "anonymous:execute" --rest.methods "get,post" --graphql.operation "query"

After add command

Config after running
dab update sptest --rest false

After update command

@Aniruddh25
Copy link
Collaborator

Aniruddh25 commented Feb 2, 2023

The type of GraphQL is different before and after starting the engine.

Sorry, I am not clear with what the issue here is that requires this fix.

@severussundar
Copy link
Contributor Author

severussundar commented Feb 3, 2023

The type of GraphQL is different before and after starting the engine.

Sorry, I am not clear with what the issue here is that requires this fix.

I've updated the PR description and linked an issue outlining the problem

@severussundar severussundar changed the title Updating fetching of graphQL Operation Updating the logic of fetching the graphQL Operation for SP Feb 3, 2023
Copy link
Contributor

@ayush3797 ayush3797 left a comment

Choose a reason for hiding this comment

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

Some questions/suggestions. LGTM otherwise.

@ayush3797 ayush3797 merged commit 5565776 into main Feb 3, 2023
@ayush3797 ayush3797 deleted the dev/shyamsundarj/execute-cli-tests branch February 3, 2023 17:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

CLI - Update command resulting in loss of fields within graphql properties

4 participants