Fix REST behavior for Stored Procedures when methods property is absent in the config file by severussundar · Pull Request #1548 · Azure/data-api-builder

severussundar · 2023-06-26T14:07:38Z

Why make this change?

Closes [Bug]: adding the rest object in an entity, remove the default value for the "method" property #1504
- For stored procedure entities, when the methods field of the rest section is absent in the config file, POST requests fail.

Sample config:

   "rest": {           
        "path": "bus-near-position"        
   }

Unless otherwise configured, by default, REST POST requests should be enabled for stored procedures.

What is this change?

EntityRestOptions: DEFAULT_SUPPORTED_VERBS_FOR_SP is added to represent the default HTTP methods supported for stored procedures
EntityRestOptionsConverter: Logic is modified to always populate the REST Methods as an empty array when absent in the config file. This is because, at this point, the converter cannot be aware of the entity type and hence, cannot populate different default values for tables/views vs stored procedures. The right values are populated later during the deserialization process.
RuntimeEntities: ProcessRestDefaults method is modified to populate the right set of supported HTTP methods based on the entity type.
TestSPRestDefaultsForManuallyConstructedConfigs: Integration tests for validating the REST API behavior for stored procedures for various scenarios are added.

How was this tested?

Integration Tests

Manual Tests:

REST section is completely absent in the config file.

POST requests should be supported on the default endpoint (/api/{entity's name})
REST section contains only enabled field: True

POST requests should be supported on the default endpoint (/api/{entity's name})
REST section contains only enabled field : False

REST APIs should be disabled for this entity.
REST section contains only path element

POST requests should be supported on the specified endpoint (/api/{path})
REST section contains only methods element

Specified list of HTTP actions should be supported on the default endpoint (/api/{entity's name})
REST section contains both methods and path element

Specified list of HTTP actions should be supported on the specified endpoint (/api/{path})

Finding missing types Refactoring some type names Simplifying the GraphQL naming by doing that when building the config, rather than on demand

Added implementation of JsonConverter.Write methods to generate an initial pass on the config file using CLI

Removed some tests that weren't testing the CLI but dependencies (such as CommandLineParser or ILogger)

…them simpler and clearer with the new object structure

Moved logic for Entities defaults to RuntimeEntities rather than deserialiser, as that is a more logical place. We always pass that type around, so we can assume the ctor ran, but we aren't always assuming the deserialiser ran (such as what happens with tests)

…ds-unspecified-scenario

abhishekkumams

Thanks for fixing it. just one nit.

…ds-unspecified-scenario

Aniruddh25

Thank you for the extensive manual testing, looking forward to more optimal way to have automated test.

…ds-unspecified-scenario

…ied-scenario' of https://github.com/Azure/data-api-builder into dev/shyamsundarj/bugfix-sp-handle-rest-methods-unspecified-scenario

seantleonard

Few questions. Would be worth identifying path forward based on Ani's comment: not using hard coded string config file values. Now that we have the new config system, you can create configs programmatically and in your case, only change the entity under test.

severussundar · 2023-07-08T08:59:25Z

Few questions. Would be worth identifying path forward based on Ani's comment: not using hard coded string config file values. Now that we have the new config system, you can create configs programmatically and in your case, only change the entity under test.

I considered using InitMinimalRuntimeConfig helper method in ConfigurationTests for generating the config files. There are few reasons why I decided not to use that and chose to go with hard coded config files.

a) This bug was identified by constructing the config file by hand and not through CLI.
b) This PR modifies the deserialization logic for the rest element in the config file. A direct and cleaner way to test this would be to pass the various hard coded strings that are possible and see if the deserialization works as expected. This ensures that the serialization logic does not interfere with the way we would like our test config files to be (programmatically it is not possible to generate all the cases that are possible when editing the config by hand).
For example: The scenario of not having methods in the config file at all.
Even if an Entity object (backed by a SP) is created by populating the rest methods as an empty string, finally, in the config file, it gets written out as an array with a POST element.

Considering the way this particular bug was identified; to have an exact representation of that scenario in the tests - hard coding the config file would be the way to go IMHO.

There was scope to remove some duplication by re-using the common parts of the hard coded string across all the scenarios - I have incorporated this.

Aniruddh25

LGTM, thanks for removing as much duplication as possible, yet preserving to test the bug scenario.

aaronpowell added 30 commits March 31, 2023 16:14

Removing the old config files and cleaning the project out

d12ad88

initial import of the new config types

def1dcf

accidentally deleted that type

3e7ccab

Some refactors in preperation of working on the tests

589ebcd

Started a new test class for the parsing of the config file

48c8ead

Porting the service startup to new config structure

5b0c22e

Adding readable static for default REST path prefix

a7baa6a

Implementing the config validator

a54c0c1

Finding missing types Refactoring some type names Simplifying the GraphQL naming by doing that when building the config, rather than on demand

Uplifing the AuthorizationResolver

db9be8d

Fixing converter after making field non-null

b69a4c6

Making exclude fields for an action default to empty hashset

da686f5

Working through compiler errors

f1e423a

last of the compile errors in the core service

8be02cb

Fixing loading the config based on the ASPNETCORE_ENVIRONMENT

816a1a9

Getting CLI project to compile

86d5e46

Created cli launch profile for init mssql config

6f25196

Added implementation of JsonConverter.Write methods to generate an initial pass on the config file using CLI

Fixed problem in how entities were serialized

e5e42f6

Entities is an object not an array

a6ca6fd

Testing if we can add entities with a new launch command

a35fcc9

Refactoring the init tests, introducing snapshot testing

3317e5a

Adding tests for AddEntity on the CLI

20d391c

Update entity tests refactored

4dfe671

Implementing e2e tests for the CLI

7e4b6bb

Removed some tests that weren't testing the CLI but dependencies (such as CommandLineParser or ILogger)

Implementing the utils tests with some refactors of the test to make …

6f132a9

…them simpler and clearer with the new object structure

Refactoring Service.Tests to compile with new config

fb6ca7a

Fixing tests for GraphQL Mutation builder

df3303d

Fixing SQL schema converter tests

71ba1e4

Initialising a config for db exception parser tests

9f23567

Proper serialization of the JSON config

279396c

severussundar requested review from aaronpowell, abhishekkumams, ayush3797 and ravishetye as code owners July 3, 2023 06:53

severussundar self-assigned this Jul 4, 2023

severussundar added this to the 0.8 milestone Jul 4, 2023

severussundar added 2 commits July 5, 2023 14:35

Merge branch 'main' into dev/shyamsundarj/bugfix-sp-handle-rest-metho…

e866849

…ds-unspecified-scenario

Merge branch 'main' into dev/shyamsundarj/bugfix-sp-handle-rest-metho…

57872dc

…ds-unspecified-scenario

abhishekkumams approved these changes Jul 6, 2023

View reviewed changes

abhishekkumams reviewed Jul 6, 2023

View reviewed changes

Comment thread src/Config/ObjectModel/RuntimeEntities.cs

improves readability

5856d50

ayush3797 reviewed Jul 6, 2023

View reviewed changes

Comment thread src/Config/Converters/EntityRestOptionsConverter.cs

Merge branch 'main' into dev/shyamsundarj/bugfix-sp-handle-rest-metho…

61dd558

…ds-unspecified-scenario

Aniruddh25 reviewed Jul 6, 2023

View reviewed changes

Comment thread src/Service.Tests/Configuration/ConfigurationTests.cs Outdated

Aniruddh25 requested changes Jul 6, 2023

View reviewed changes

severussundar added 3 commits July 7, 2023 13:41

Merge branch 'main' into dev/shyamsundarj/bugfix-sp-handle-rest-metho…

3bc7b65

…ds-unspecified-scenario

adds logic to generate config json dynamically

7ccbc3e

Merge branch 'dev/shyamsundarj/bugfix-sp-handle-rest-methods-unspecif…

6101ddf

…ied-scenario' of https://github.com/Azure/data-api-builder into dev/shyamsundarj/bugfix-sp-handle-rest-methods-unspecified-scenario

severussundar commented Jul 7, 2023

View reviewed changes

Comment thread src/Service.Tests/TestHelper.cs

severussundar requested a review from Aniruddh25 July 7, 2023 12:03

seantleonard reviewed Jul 7, 2023

View reviewed changes

Comment thread src/Config/ObjectModel/RuntimeEntities.cs Outdated

seantleonard reviewed Jul 7, 2023

View reviewed changes

Comment thread src/Config/ObjectModel/EntityRestOptions.cs Outdated

seantleonard reviewed Jul 7, 2023

View reviewed changes

updates comments, renames variable

4bb6349

severussundar requested review from ayush3797 and seantleonard July 8, 2023 09:00

Aniruddh25 approved these changes Jul 10, 2023

View reviewed changes

severussundar merged commit d840baf into main Jul 10, 2023

severussundar deleted the dev/shyamsundarj/bugfix-sp-handle-rest-methods-unspecified-scenario branch July 10, 2023 04:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix REST behavior for Stored Procedures when methods property is absent in the config file #1548

Fix REST behavior for Stored Procedures when methods property is absent in the config file #1548
severussundar merged 228 commits intomainfrom
dev/shyamsundarj/bugfix-sp-handle-rest-methods-unspecified-scenario

severussundar commented Jun 26, 2023 •

edited

Loading

Uh oh!

abhishekkumams left a comment •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Aniruddh25 left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

seantleonard left a comment

Uh oh!

severussundar commented Jul 8, 2023

Uh oh!

Aniruddh25 left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Conversation

severussundar commented Jun 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why make this change?

What is this change?

How was this tested?

Uh oh!

abhishekkumams left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Aniruddh25 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

seantleonard left a comment

Choose a reason for hiding this comment

Uh oh!

severussundar commented Jul 8, 2023

Uh oh!

Aniruddh25 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

severussundar commented Jun 26, 2023 •

edited

Loading

abhishekkumams left a comment •

edited

Loading