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

Feature Flag for Nested Mutations: CLI changes #1983

Merged

Conversation

severussundar
Copy link
Contributor

@severussundar severussundar commented Jan 21, 2024

Why make this change?

     "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?

  • Integration Tests
  • Unit Tests
  • Manual Tests

Sample Commands

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

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

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

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

@severussundar
Copy link
Contributor Author

/azp run

Copy link

Azure Pipelines successfully started running 6 pipeline(s).

@severussundar severussundar added cli Multiple mutations Fixes/enhancements related to nested mutations. labels Jan 22, 2024
@severussundar severussundar added this to the 0.11rc milestone Jan 22, 2024
@abhishekkumams
Copy link
Contributor

/azp run

Copy link
Contributor

@Aniruddh25 Aniruddh25 left a comment

Choose a reason for hiding this comment

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

Need to fix a typo, LGTM otherwise.

Copy link
Contributor

@Aniruddh25 Aniruddh25 left a comment

Choose a reason for hiding this comment

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

Seems like this open issue in the graphql spec: graphql/graphql-spec#252 contradicts with what we are calling as nested mutations. I think we should rename the feature to avoid confusion in its usage to something like multiple-mutations + multiple-create, since our feature specifically means performing multiple mutations on related entities in a serial manner.

Nested-mutations as per the open issue in the community refers to having a mutation operation under another query/mutation node - which is not what we plan to do here.

Thanks to @michaelstaib for bringing this up.

@michaelstaib
Copy link
Collaborator

michaelstaib commented Mar 3, 2024

@severussundar what you are doing is correct and spec compliant. It looks very similar to what is referred to as batch mutation where the input represents not a single mutation but potentially multiple.

mutation {
  updateCart(input: { productToAdd: [  ...], productsToRemove: [  ...] }) {   ...}
}

Nested Mutations in the GraphQL world would be something aking to:

mutation {
   product { create(input .... }
}

Copy link
Contributor

@seantleonard seantleonard 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 about conditions and valid config representations of nested mutations section. Should be good to go after addressing.

schemas/dab.draft.schema.json Outdated Show resolved Hide resolved
src/Cli.Tests/EndToEndTests.cs Outdated Show resolved Hide resolved
src/Cli.Tests/EndToEndTests.cs Outdated Show resolved Hide resolved
src/Cli.Tests/EndToEndTests.cs Outdated Show resolved Hide resolved
src/Cli.Tests/EndToEndTests.cs Show resolved Hide resolved
src/Service.Tests/Configuration/ConfigurationTests.cs Outdated Show resolved Hide resolved
@severussundar
Copy link
Contributor Author

/azp run

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@severussundar
Copy link
Contributor Author

severussundar commented Mar 11, 2024

Rename of the CLI options, JSON fields etc. (replacing nested mutations/nested create with a different name)
will be taken up in a follow-up PR. Created this issue for tracking the same.

@Aniruddh25 Aniruddh25 dismissed their stale review March 11, 2024 17:48

Comments addressed

Copy link
Contributor

@Aniruddh25 Aniruddh25 left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for your patience in the review.

Copy link
Contributor

@seantleonard seantleonard left a comment

Choose a reason for hiding this comment

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

Lgtm! thanks for addressing the various iterations of feedback.

@severussundar severussundar merged commit f7bc95d into dev/NestedMutations Mar 12, 2024
9 checks passed
@severussundar severussundar deleted the dev/shyamsundarj/adds-feature-flag-cli-option branch March 12, 2024 09:09
severussundar added a commit that referenced this pull request Mar 20, 2024
…ultiple-create respectively (#2103)

## Why make this change?

-  Closes #2090
- Renames `nested-mutations` and `nested-create` to `multiple-mutations`
and `multiple-create` respectively.
- Nested Mutations has a significantly different meaning in the graphql
specs (as explained in issue
graphql/graphql-spec#252) compared to what we
are trying to achieve. Hence, the decision to rename.

## What is this change?

- All references to nested mutations/nested create - option name, field
names in config JSON, class names, comments in code have been renamed.

## How was this tested?

- Unit and Integration tests added as part of
#1983 has been updated to
validate the rename of CLI option, property in the config file, etc.

## Sample Commands


![image](https://github.com/Azure/data-api-builder/assets/11196553/7815bdb8-b0ca-409c-9b28-1101d5639130)
severussundar added a commit that referenced this pull request Mar 26, 2024
…ue (#2116)

## Why make this change?

- Closes #1951
- PR #1983,
#2103 add CLI options to
enable or disable multiple mutation/multiple create operation through
CLI. With the changes introduced in the mentioned PRs, the configuration
properties successfully gets written to the config file. Also, during
deserialization, the properties are read and the
`MultipleMutationOptions`, `MultipleCreateOptions`,
`GraphQLRuntimeOptions` objects are created accordingly.
- The above-mentioned PRs do not introduce any change in DAB engine
behavior depending on the configuration property values.
- This PR introduces changes to read these fields and enable/disable
multiple create operation depending on whether the feature is
enabled/disabled through the config file. This is achieved by
introducing behavior changes in the schema generation.

## What is this change?
- This PR builds on top of a) Schema generation PR
#1902 b) Rename
nested-create to multiple create PR
#2103
- When multiple create operation is disabled, 
> i) Fields belonging to the related entities are not created in the
input object type are not created.
> ii) Many type multiple create mutation nodes (ex: `createbooks`,
`createpeople_multiple` ) are not created.
> iii) ReferencingField directive is not applied on relationship fields,
so they continue to remain required fields for the create mutation
operation.
> iv) Entities for linking objects are not created as they are relevant
only in the context of multiple create operations.

## How was this tested?

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

**Note:** At the moment, multiple create operation is disabled in the
config file generated for integration tests. This is because of the plan
to merge in the Schema generation, AuthZ/N branches separately to the
main branch. With just these 2 PRs, a multiple create operation will
fail, hence, the disabling multiple create operation. At the moment,
tests that perform validations specific to multiple create feature
enable it by i) updating the runtime object (or) ii) creating a custom
config in which the operation is enabled.

## Sample Request(s)

### When Multiple Create operation is enabled - MsSQL

#### Related entity fields are created in the input object type

![image](https://github.com/Azure/data-api-builder/assets/11196553/7a3a8bbe-2742-43e0-98d7-9412ed05db33)

#### Multiple type create operation is created in addition to point
create operation

![image](https://github.com/Azure/data-api-builder/assets/11196553/c6513d9a-5b49-44cc-8fcc-1ed1f44f5f58)


#### Querying related entities continue to work successfully

![image](https://github.com/Azure/data-api-builder/assets/11196553/4c1a61b8-0cbb-4a1e-afaa-1849d710be27)



### When Multiple Create operation is disabled - MsSQL

#### Only fields belonging to the given entity are created in the input
object type

![image](https://github.com/Azure/data-api-builder/assets/11196553/a3b6beb2-7245-4345-ba13-29d8905d859e)

#### Multiple type create operation is not created


### When Multiple Create operation is enabled - Other relational
database types

#### Only fields belonging to the given entity are created in the input
object type

![image](https://github.com/Azure/data-api-builder/assets/11196553/b2a4f7f6-b121-410d-806d-8c5772253080)

#### Multiple type create operation is not created

---------

Co-authored-by: Ayush Agarwal <ayusha083@gmail.com>
@severussundar severussundar mentioned this pull request Mar 26, 2024
1 task
severussundar added a commit that referenced this pull request Mar 29, 2024
## Why make this change?

- All code changes for **Multiple Create** feature was being merged into
`dev/NestedMutations` branch.
- This PR attempts to merge all these changes to the `main` branch in
preparation for the `0.12.* rc1` release

## What is this change?

- Right now, `dev/NestedMutations` branch contains the code changes for
the following components of Multiple Create feature
- Schema Generation -
#1902
  - AuthZ - #1943
- Feature flag - CLI changes
#1983
- Feature flag - Re-naming changes
#2103
- Feature flag - Engine changes
#2116
- Each specified PR was reviewed before merging into
`dev/NestedMutations` branch.
- This PR aims to merge all the changes into `main` branch
  
## How was this tested?

- [x] Unit, Integration and Manual tests were performed on each PR
before merging into `dev/NestedMutations`

---------

Co-authored-by: Shyam Sundar J <shyamsundarj@microsoft.com>
Co-authored-by: Sean Leonard <sean.leonard@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cli Multiple mutations Fixes/enhancements related to nested mutations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for enabling nested mutation from CLI
7 participants