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

Add support for enabling nested mutation from CLI #1950

Closed
Tracked by #1576
ayush3797 opened this issue Jan 3, 2024 · 9 comments · Fixed by #1983
Closed
Tracked by #1576

Add support for enabling nested mutation from CLI #1950

ayush3797 opened this issue Jan 3, 2024 · 9 comments · Fixed by #1983
Assignees
Labels
cli graphql Multiple mutations Fixes/enhancements related to nested mutations.
Milestone

Comments

@ayush3797
Copy link
Contributor

ayush3797 commented Jan 3, 2024

This issue tracks the efforts needed for exposing the feature flag options through CLI commands and subsequently, serializing the same in the config file.

@severussundar
Copy link
Contributor

severussundar commented Jan 4, 2024

Proposal:

Property Name: nested-inserts-enabled
Position in the config file: This will be a property within runtime.graphql. Placing the element here implies that when the field is set to true/false, the nested inserts feature is enabled/disabled for all the entities.

Deafult behavior: Nested Inserts will be enabled be default.

One more thing to note: When the property is updated to true/false to enable/disable the feature, the engine will have to be restarted for the change to take effect.

In the first iteration, hot reloading of this flag will not be supported. This can be added iteratively in the subsequent versions (if needed).

@JerryNixon @Aniruddh25 @seantleonard @ayush3797 Please kindly share your thoughts.

@ayush3797
Copy link
Contributor Author

ayush3797 commented Jan 4, 2024

In future, we might also add support for nested updates. Having a stand-alone property directly in the config for nested inserts won't scale well. Instead, we should add an object (something like nested-mutations) and then inside that object we can have objects storing properties for inserts/updates.

Also we should be creating objects for inserts/updates because we don't know what all inputs we might need from the user. In future, we might also allow them to specify the nesting limit (due to obvious payload size considerations). Then such a property can also go in the already existing json object. This is just one use case I can think of right now but more can come up as we move forward.

"nested-mutations":{
    "inserts":{
        "enabled": true,
        "nesting-limit": 10
    },
    "updates":{
        "enabled": false,
        "nesting-limit": 10
    },
}

Obviously, to start with, we would not add the section for updates.

@seantleonard seantleonard added this to the 0.11rc milestone Jan 10, 2024
@JerryNixon
Copy link
Contributor

Would this also be a possibility to reduce syntax?

"nested-mutations":{
    "*":{
        "enabled": true,
        "nesting-limit": 10
    },
    "inserts":{
        "enabled": true,
        "nesting-limit": 10
    },
    "updates":{
        "enabled": false,
        "nesting-limit": 10
    },
}

@severussundar
Copy link
Contributor

severussundar commented Jan 12, 2024

Yes, * would help eliminate redundant entries when the same settings are to be applied for all the operations.

I am inclined to start off with just the insert section. The reason being that, nested update operation is still under evaluation.
When adding the support for update operation, the logic for interpreting * as insert + update can be introduced.

Adding it now is pre-mature for the following reasons:

  1. * would expand to only insert
  2. When adding support for update, a logic change would definitely be required in both CLI and engine to interpret * to be insert, update. So, we can introduce * when implementing the update operation.

Also, I want to call out nesting-limit will not be added right now (In the first version, there will be no restrictions on the depth of operations) . In the example, the field nesting-limit was added to emphasize that creating an object type will make it easier to add more fields in the future.

For the first version, the exact format of the feature flag would be

"runtime":{
      ...
      "graphql": {
           ...
           "nested-mutations": {
               "insert": {
                     "enabled": true/false
               }
           }
      } 
}

@JerryNixon
Copy link
Contributor

100% agree.

@severussundar
Copy link
Contributor

severussundar commented Jan 22, 2024

CLI Option in the init command: --graphql.nested-insert.enabled. This would be an optional field.

Sample commands:

  1. dab init --database-type mssql --connection-string connString --graphql.nested-insert.enabled true
  2. dab init --database-type mssql --connection-string connString --graphql.nested-insert.enabled false

Right now, Nested Mutation operations are applicable only for MsSQL database type. So, when the option --graphql.nested-insert.enabled is used along with other database types, it is not honored and will always be written to the config file as enabled: false. In addition, a warning will be logged to let users know that the option is inapplicable.

Warning message: The option --graphql.nested-insert.enabled is not supported for {database type} database type and will not be honored.

@JerryNixon
Copy link
Contributor

Thanks for keeping the CLI in sync. Looks great.

@Aniruddh25 Aniruddh25 modified the milestones: 0.11rc, 0.12rc Feb 12, 2024
severussundar added a commit that referenced this issue Mar 12, 2024
## Why make this change?

-  Closes #1950 
- Introduces a feature flag in the config for nested mutation
operations.
    - Feature Flag format:
 
```json
     "runtime":{
      ...
       "graphql": {
           ...
           "nested-mutations": {
               "create": {
                     "enabled": true/false
                }
           }
       } 
     }
```
- CLI Option: `--graphql.nested-create.enabled`. This option can be used
along with `init` command to enable/disable nested insert operation.
 - By default, the nested mutation operations will be **disabled**.
- Nested Mutation operations are applicable only for MsSQL database
type. So, when the option `--graphql.nested-create.enabled` is used
along with other database types, it is not honored and nested mutation
operations will be disabled. Nested Mutation section will not be written
to the config file. In addition, a warning will be logged to let users
know that the option is inapplicable.

## What is this change?

- `dab.draft.schema.json` - The schema file is updated to contain
details about the new fields
- `InitOptions` - A new option `--graphql.nested-create.enabled` is
introduced for the `init` command.
- `NestedCreateOptionsConverter` - Custom converter to read & write the
options for nested insert operation from/to the config file
respectively.
- `NestedMutationOptionsConverter` - Custom converter to read & write
the options for nested mutation operations from/to the config file
respectively.
- `GraphQLRuntimeOptionsConverterFactory` - Updates the logic for
reading and writing the graphQL runtime section of the config file.
Incorporates logic for reading and writing the nested mutation operation
options.
- `dab-config.*.json`/`Multidab-config.*.json` - All the reference
config files are updated to include the new Nested Mutation options

## How was this tested?

- [x] Integration Tests
- [x] Unit Tests
- [x] Manual Tests

## Sample Commands

- **Nested Create Operations are enabled**:
`dab init --database-type mssql --connection-string connString
--graphql.nested-create.enabled true`

![image](https://github.com/Azure/data-api-builder/assets/11196553/c1821897-1553-46d7-97d2-bf31b7f6178d)



- **Nested Create Operations are disabled**:
`dab init --database-type mssql --connection-string connString
--graphql.nested-create.enabled false`

![image](https://github.com/Azure/data-api-builder/assets/11196553/ea421080-beb8-4f01-a2c9-99916b8b83cc)



- **When --graphql.nested-create.graphql option is not used in the init
command**:
`dab init --database-type mssql --connection-string connString`

![image](https://github.com/Azure/data-api-builder/assets/11196553/d6f1d56c-a553-4dbf-8ad1-e813edc4274d)



- **When --graphql.nested-create.graphql option is used with a database
type other than MsSQL**:

![image](https://github.com/Azure/data-api-builder/assets/11196553/f9cdda69-f0bd-4e9d-8f65-dd1f0df48402)
@severussundar
Copy link
Contributor

severussundar commented Mar 13, 2024

Based on inputs, nested-insert was renamed to multiple-create.

CLI Option: --graphql.multiple-create.enabled true

JSON property

     "runtime":{
      ...
       "graphql": {
           ...
           "multiple-mutations": {
               "create": {
                     "enabled": true/false
                }
           }
       } 
     }

Further the feature (and consequentially, the CLI option name and JSON properties) is going to be renamed since multiple-mutations has a different meaning (details in the graphql specs graphql/graphql-spec#252 ).

The renaming of the feature, CLI option, fields in the config files are tracked under the issue: #2090

@severussundar
Copy link
Contributor

Closing this issue as PR #1983 is merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cli graphql Multiple mutations Fixes/enhancements related to nested mutations.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants