Skip to content

Fixes the bug #1680 that necessitated adding the 'name' parameter to the @model directive, regardless of the name of the GraphQL object type's name. - #1706

Merged
Aniruddh Munde (Aniruddh25) merged 6 commits into
release/0.8from
dev/neeraj-sharma2592/fixNameDirectiveInvalidArgException
Sep 14, 2023
Merged

Fixes the bug #1680 that necessitated adding the 'name' parameter to the @model directive, regardless of the name of the GraphQL object type's name.#1706
Aniruddh Munde (Aniruddh25) merged 6 commits into
release/0.8from
dev/neeraj-sharma2592/fixNameDirectiveInvalidArgException

Conversation

@neeraj-sharma2592

@neeraj-sharma2592 neeraj-sharma2592 commented Sep 11, 2023 •
edited by Aniruddh25
Loading

Copy link
Copy Markdown
Contributor

Why make this change?

   string typeName = GraphQLUtils.TryExtractGraphQLFieldModelName(underlyingType.Directives, out string? modelName) ?
                    modelName :
                    underlyingType.Name;

What is this change?

Checking Directive "name" exists before accessing its value.

How was this tested?

 Tested locally for now. Will be checking Test cases for it.
  • Integration Tests
  • Unit Tests

seantleonard
Sean Leonard (seantleonard) reviewed Sep 11, 2023
View reviewed changes

@seantleonard Sean Leonard (seantleonard) left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for addressing and looks good so far. Looking forward to seeing a test before merging.

Aniruddh25
Aniruddh Munde (Aniruddh25) reviewed Sep 11, 2023
View reviewed changes
Comment thread src/Service.GraphQLBuilder/GraphQLUtils.cs Outdated
Aniruddh25
Aniruddh Munde (Aniruddh25) reviewed Sep 11, 2023
View reviewed changes
Comment thread src/Service.GraphQLBuilder/GraphQLUtils.cs
@neeraj-sharma2592 neeraj-sharma2592 changed the title Dev/neeraj sharma2592/fix name directive invalid arg exception Fixes the bug that necessitated adding the 'name' parameter to the @model directive, regardless of the name of the GraphQL object type's name. Sep 12, 2023
@neeraj-sharma2592 neeraj-sharma2592 changed the title Fixes the bug that necessitated adding the 'name' parameter to the @model directive, regardless of the name of the GraphQL object type's name. Fixes the bug#1680 that necessitated adding the 'name' parameter to the @model directive, regardless of the name of the GraphQL object type's name. Sep 12, 2023
@neeraj-sharma2592 neeraj-sharma2592 changed the title Fixes the bug#1680 that necessitated adding the 'name' parameter to the @model directive, regardless of the name of the GraphQL object type's name. Fixes the bug #1680 that necessitated adding the 'name' parameter to the @model directive, regardless of the name of the GraphQL object type's name. Sep 12, 2023
Neeraj Sharma added 2 commits September 13, 2023 16:02
Neeraj Sharma
removed the 'name' attribute from the Planet model to test scenarios …
6eeba3f 
…where the 'name' attribute is not explicitly added in the schema
Neeraj Sharma
added comment
99f1910
@neeraj-sharma2592

neeraj-sharma2592 commented Sep 13, 2023

Copy link
Copy Markdown
Contributor Author

Thanks for addressing and looks good so far. Looking forward to seeing a test before merging.

I have modified the schema to test this scenario. We had a 'name' attribute for each model, and we couldn't identify this regression through our integration tests. Please let me know if this seems correct.

data-api-builder/src/Service.Tests/CosmosTests/TestBase.cs

Line 44 in 99f1910

type Planet @model {

Aniruddh25
Aniruddh Munde (Aniruddh25) approved these changes Sep 13, 2023
View reviewed changes

@Aniruddh25 Aniruddh Munde (Aniruddh25) left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM, thanks alot for the quick fix and the test!!

seantleonard
Sean Leonard (seantleonard) approved these changes Sep 14, 2023
View reviewed changes

@seantleonard Sean Leonard (seantleonard) left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for adding a test and pushing this fix.

@Aniruddh25
Aniruddh Munde (Aniruddh25) merged commit 9b0304e into release/0.8 Sep 14, 2023
@Aniruddh25
Aniruddh Munde (Aniruddh25) deleted the dev/neeraj-sharma2592/fixNameDirectiveInvalidArgException branch September 14, 2023 00:52
@neeraj-sharma2592
neeraj-sharma2592 restored the dev/neeraj-sharma2592/fixNameDirectiveInvalidArgException branch September 14, 2023 07:16
@neeraj-sharma2592
neeraj-sharma2592 deleted the dev/neeraj-sharma2592/fixNameDirectiveInvalidArgException branch September 14, 2023 07:19
neeraj-sharma2592 added a commit that referenced this pull request Sep 14, 2023
@neeraj-sharma2592 Neeraj Sharma
Fixes the bug #1680 that necessitated adding the 'name' parameter to …
6aece0a 
…the @model directive, regardless of the name of the GraphQL object type's name. (#1706)

## Why make this change?
- Closes #1680 
- Regression introduced in 0.8.49 caused required explicitly adding
"name" directive for all the model/entity irrespective of whether we are
using a different name than the one it originally has.
- Regression introduced in #1402 when we started using
TryExtractGraphQLName as follows:
```
   string typeName = GraphQLUtils.TryExtractGraphQLFieldModelName(underlyingType.Directives, out string? modelName) ?
                    modelName :
                    underlyingType.Name;
```
      
## What is this change?
    Checking Directive "name" exists before accessing its value. 

## How was this tested?
     Tested locally for now. Will be checking Test cases for it.
- [x] Integration Tests
- [ ] Unit Tests

---------

Co-authored-by: Neeraj Sharma <neesharma@microsoft.com>
neeraj-sharma2592 added a commit that referenced this pull request Sep 18, 2023
@neeraj-sharma2592 Neeraj Sharma
Fixes the bug #1680 that necessitated adding the 'name' parameter to …
eff9b1e 
…the @model directive, regardless of the name of the GraphQL object type's name. (#1706)

## Why make this change?
- Closes #1680 
- Regression introduced in 0.8.49 caused required explicitly adding
"name" directive for all the model/entity irrespective of whether we are
using a different name than the one it originally has.
- Regression introduced in #1402 when we started using
TryExtractGraphQLName as follows:
```
   string typeName = GraphQLUtils.TryExtractGraphQLFieldModelName(underlyingType.Directives, out string? modelName) ?
                    modelName :
                    underlyingType.Name;
```
      
## What is this change?
    Checking Directive "name" exists before accessing its value. 

## How was this tested?
     Tested locally for now. Will be checking Test cases for it.
- [x] Integration Tests
- [ ] Unit Tests

---------

Co-authored-by: Neeraj Sharma <neesharma@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Aniruddh25 Aniruddh Munde (Aniruddh25) Aniruddh25 approved these changes
@seantleonard Sean Leonard (seantleonard) seantleonard approved these changes
@mbhaskar Bhaskar Mallapragada (mbhaskar) Awaiting requested review from mbhaskar mbhaskar is a code owner
@JelteF Jelte Fennema-Nio (JelteF) Awaiting requested review from JelteF
@Mathos1432 Mathieu Tremblay (Mathos1432) Awaiting requested review from Mathos1432 Mathos1432 is a code owner
@gledis69 Gledis Zeneli (gledis69) Awaiting requested review from gledis69
@aaronburtle aaronburtle Awaiting requested review from aaronburtle aaronburtle is a code owner
@milismsft Milis (milismsft) Awaiting requested review from milismsft milismsft is a code owner
@tarazou9 tarazou9 Awaiting requested review from tarazou9 tarazou9 is a code owner
@ayush3797 Ayush Agarwal (ayush3797) Awaiting requested review from ayush3797
@abhishekkumams Abhishek Kumar (abhishekkumams) Awaiting requested review from abhishekkumams
@aaronpowell Aaron Powell (aaronpowell) Awaiting requested review from aaronpowell aaronpowell is a code owner
@severussundar Shyam Sundar J (severussundar) Awaiting requested review from severussundar severussundar is a code owner
@ravishetye Ravi Shetye (ravishetye) Awaiting requested review from ravishetye

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@neeraj-sharma2592 @Aniruddh25 @seantleonard