Skip to content

Skip writing the HTTP actions to config file and log warning when REST Methods are configured for tables/views#1607

Merged
severussundar merged 15 commits intomainfrom
dev/shyamsundarj/make-rest-methods-nullable
Aug 17, 2023
Merged

Skip writing the HTTP actions to config file and log warning when REST Methods are configured for tables/views#1607
severussundar merged 15 commits intomainfrom
dev/shyamsundarj/make-rest-methods-nullable

Conversation

@severussundar
Copy link
Copy Markdown
Contributor

@severussundar severussundar commented Jul 25, 2023

Why make this change?

What is this change?

  • EntityRestOptions: Methods property is made nullable. References where it was initialized with an empty array is updated to null. Appropriate null checks are added.
  • EntityRestOptionsConverter: Serialization logic is updated to write to the config file only when the Methods within REST element is not empty
  • RuntimeConfigValidator.ValidatePresenceOfExtraRestProperties(): Helper method to log a warning message when Methods property is added manually to an entity backed by table/view.
  • HTTP Actions are removed from the snapshot files.
  • dab-config.* files - Methods property is removed from the reference config files

How was this tested?

  • Integration Tests
  • Unit Tests
  • Manual Tests

Sample Request(s)

  • CLI: REST actions are not written to the config file
    dab add books --source books --permissions "anonymous:*"

Config Json created:

"entities": {
    "books": {
  "source": {
    "object": "books",
    "type": "table"
  },
  "graphql": {
    "enabled": true,
    "type": {
      "singular": "books",
      "plural": "books"
    }
  },
  "rest": {
    "enabled": true
  },
  "permissions": [
    {
      "role": "anonymous",
      "actions": [
        {
          "action": "*"
        }
      ]
    }
  ]
}
  }
  • When the config file is hand-edited to add methods property to a table/view, engine logs a warning message during start-up

Config:

"Book": {
      "source": {
        "object": "books",
        "type": "table"
      },
      "rest": {
        "enabled": true,
        "methods": [
          "get",
          "post"
        ]
      },
      "graphql": {
        "enabled": true,
        "type": {
          "singular": "book",
          "plural": "books"
        }
      },

Warning Logged:

image

Copy link
Copy Markdown
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.

Comment thread src/Config/Converters/EntityRestOptionsConverter.cs Outdated
Comment thread src/Core/Authorization/AuthorizationResolver.cs Outdated
Comment thread src/Core/Configurations/RuntimeConfigValidator.cs Outdated
Comment thread src/Core/Configurations/RuntimeConfigValidator.cs Outdated
Comment thread src/Core/Configurations/RuntimeConfigValidator.cs
Comment thread src/Service.Tests/GraphQLBuilder/Sql/SchemaConverterTests.cs Outdated
Comment thread src/Service.Tests/Unittests/ConfigValidationUnitTests.cs Outdated
Comment thread src/Service.Tests/dab-config.MsSql.json Outdated
Comment thread src/Core/Services/OpenAPI/OpenApiDocumentor.cs Outdated
Comment thread src/Core/Services/RestService.cs Outdated
Comment thread src/Service.Tests/Authorization/AuthorizationResolverUnitTests.cs
Comment thread src/Service.Tests/Configuration/ConfigurationTests.cs
Comment thread src/Core/Configurations/RuntimeConfigValidator.cs Outdated
Comment thread src/Service.Tests/Unittests/ConfigValidationUnitTests.cs Outdated
Comment thread src/Service.Tests/Unittests/ConfigValidationUnitTests.cs Outdated
Comment thread src/Service.Tests/Unittests/ConfigValidationUnitTests.cs Outdated
Comment thread src/Service.Tests/TestHelper.cs
Comment thread src/Core/Authorization/AuthorizationResolver.cs Outdated
Comment thread src/Config/ObjectModel/EntityRestOptions.cs Outdated
@ayush3797
Copy link
Copy Markdown
Contributor

Some questions and suggestions around the same. Looks good so far!

Copy link
Copy Markdown
Collaborator

@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.

Looks good so far, left few comments.

Comment thread src/Core/Services/RestService.cs Outdated
Comment thread src/Core/Services/OpenAPI/OpenApiDocumentor.cs Outdated
Comment thread src/Core/Authorization/AuthorizationResolver.cs Outdated
Comment thread src/Service.Tests/Unittests/ConfigValidationUnitTests.cs
Comment thread src/Config/Converters/EntityRestOptionsConverter.cs Outdated
Comment thread src/Core/Authorization/AuthorizationResolver.cs
Comment thread src/Core/Configurations/RuntimeConfigValidator.cs Outdated
Comment thread src/Core/Services/OpenAPI/OpenApiDocumentor.cs Outdated
Copy link
Copy Markdown
Collaborator

@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.

Thanks for noticing this bug and fixing it for 0.8!

Comment thread src/Core/Services/OpenAPI/OpenApiDocumentor.cs
Copy link
Copy Markdown
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.

Just one question. LGTM! Thanks for fixing this!

@severussundar severussundar merged commit 33774c2 into main Aug 17, 2023
@severussundar severussundar deleted the dev/shyamsundarj/make-rest-methods-nullable branch August 17, 2023 06:11
severussundar added a commit that referenced this pull request Aug 23, 2023
…T Methods are configured for tables/views (#1607)

- Closes #1571 #1570
- At the moment, CLI writes out all the 5 HTTP methods to the config
file when adding an entity backed by table/view.

![image](https://github.com/Azure/data-api-builder/assets/11196553/206ddffb-a2d5-42c5-94dd-50165ef96f24)

- However, for tables/views, the `Methods` property does not play any
role and can be skipped writing to the config file.

- `EntityRestOptions`: `Methods` property is made nullable. References
where it was initialized with an empty array is updated to `null`.
Appropriate `null` checks are added.
- `EntityRestOptionsConverter`: Serialization logic is updated to write
to the config file only when the `Methods` within REST element is not
empty
- `RuntimeConfigValidator.ValidatePresenceOfExtraRestProperties()`:
Helper method to log a warning message when `Methods` property is added
manually to an entity backed by table/view.
- HTTP Actions are removed from the snapshot files.
- `dab-config.*` files - `Methods` property is removed from the
reference config files

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

- CLI: REST actions are not written to the config file
`dab add books --source books --permissions "anonymous:*"`

Config Json created:
```json
"entities": {
    "books": {
  "source": {
    "object": "books",
    "type": "table"
  },
  "graphql": {
    "enabled": true,
    "type": {
      "singular": "books",
      "plural": "books"
    }
  },
  "rest": {
    "enabled": true
  },
  "permissions": [
    {
      "role": "anonymous",
      "actions": [
        {
          "action": "*"
        }
      ]
    }
  ]
}
  }
```
- When the config file is hand-edited to add `methods` property to a
table/view, engine logs a warning message during start-up

Config:
```json
"Book": {
      "source": {
        "object": "books",
        "type": "table"
      },
      "rest": {
        "enabled": true,
        "methods": [
          "get",
          "post"
        ]
      },
      "graphql": {
        "enabled": true,
        "type": {
          "singular": "book",
          "plural": "books"
        }
      },
```
Warning Logged:

![image](https://github.com/Azure/data-api-builder/assets/11196553/287efcc2-e088-48b1-81f1-f762e18c2b51)
severussundar added a commit that referenced this pull request Aug 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Make the REST Methods property nullable in EntityRestOptions class Need to unignore test once behavior of stored procedures is fixed

4 participants