Skip to content

Conversation

seantleonard
Copy link
Contributor

@seantleonard seantleonard commented Feb 16, 2024

Why make this change?

  "entities": {
    "SpUuidParam": {
      "source": {
        "object": "sp_uuid_param",
        "type": "stored-procedure",
        "parameters": {
          "param1": "f58b7b58-62c9-4b97-ab60-75de70793f66"
        }
      },
      "graphql": {
        "enabled": true,
        "operation": "Query",
        "type": {
          "singular": "SpUuidParam",
          "plural": "SpUuidParams"
        }
      },
      "rest": {
        "enabled": true
      },
      "permissions": [
        {
          "role": "anonymous",
          "actions": [ "*" ]
        },
        {
          "role": "authenticated",
          "actions": [ "*" ]
        }
      ]
    }

And stored proc in tsql

CREATE PROCEDURE [dbo].[sp_uuid_param] @param1 uniqueidentifier AS
      SELECT @param1 AS [ReturnPayload]

DAB was lacking the ability to handle the stored procedure having an input parameter with value type uniqueidentifier. When a default value was defined in the config, that value would fail conversion to the UUID type during GraphQL schema creation.

The call stack flows through:

                    if (entity.Source.Parameters is not null && entity.Source.Parameters.TryGetValue(param, out object? value))
                    {
                        Tuple<string, IValueNode> defaultGraphQLValue = ConvertValueToGraphQLType(value.ToString()!, parameterDefinition: spdef.Parameters[param]);
                        defaultValueNode = defaultGraphQLValue.Item2;
                    }

where ConvertValueToGraphQLType(...) would attempt to convert the config defined value to a GraphQL type based on the type inferred from the parameter's SystemType (System.Guid). The parameter object has the following properties when the parameter is passed to that function:

  • SystemType -> System.Guid
  • DbType -> Guid
  • ConfigDefaultValue -> f58b7b58-62c9-4b97-ab60-75de70793f66
  • HasConfigDefault -> true

ConvertValueToGraphQLType(...) did not have the conversion needed to create a UUID type.

What is this change?

  • In the function called to process config defined default values for stored procedure parameters, ConvertValueToGraphQLType(...) add the conversion:
UUID_TYPE => new(UUID_TYPE, new UuidType().ParseValue(Guid.Parse(defaultValueFromConfig))),
  • Moved ConvertValueToGraphQLType() from GraphQLUtils.cs to GraphQLStoredProcedureBuilder.cs to align with usage and purpose of function. Reduces size of MEGA UTIL class. Also adds comment for GraphQLUtils.BuiltInTypes hashset entry 'ID' to inform that it enables CosmosDB functionality as only cosmos tests failed when that entry was commented out.
  • Reorganizes the ConvertValueToGraphQLType() order of types in switch statements to align with GraphQLUtils.BuiltInTypes hashset. That way it is easier to notice discrepancies in the two lists.

How was this tested?

  • Integration Tests

Sample Request(s)

  • Use the db schema and entity config provided above. Startup will succeed without conversion errors when attempting to create the GraphQL schema.

…resolve dabconfig provided default parameter value to resolve as a GraphQL/HotChocolate UUID and SystemType.Guid, which will allow startup to finish without error. Add unit test checking the affirmative and test to ensure db type string with dabconfig value looking like a GUID gets resolved as string
@Aniruddh25
Copy link
Collaborator

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

Copy link
Contributor

@severussundar severussundar left a comment

Choose a reason for hiding this comment

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

Suggestion to include test cases with different UUID formats. Otherwise, looks good.

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

LGTM, thanks for the quick fix!

…dProcedureBuilder to align with usage and purpose of function. Reduces size of MEGA UTIL class. Also adds comment for BuiltInTypes hashset entry 'ID' to inform that it enables CosmosDB functionality as only cosmos tests failed when that entry was commented out. Also reorganizes the ConvertValueToGraphQLType() order of types in switch statements to align with GraphQLUtils.BuiltInTypes hashset.
@seantleonard
Copy link
Contributor Author

/azp run

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

Copy link
Contributor

@aaronburtle aaronburtle 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!

…that the message on console when an exception is caught is readable.
@seantleonard
Copy link
Contributor Author

/azp run

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@seantleonard seantleonard enabled auto-merge (squash) March 2, 2024 00:21
@seantleonard seantleonard merged commit ffd29b9 into main Mar 2, 2024
@seantleonard seantleonard deleted the dev/sean/2027_uuid branch March 2, 2024 00:42
seantleonard added a commit that referenced this pull request Mar 4, 2024
…ed stored proc parameter default values. (#2042)

## Why make this change?

- Closes #2027. DAB would not start up correctly when runtime config
took the following form:
```json
  "entities": {
    "SpUuidParam": {
      "source": {
        "object": "sp_uuid_param",
        "type": "stored-procedure",
        "parameters": {
          "param1": "f58b7b58-62c9-4b97-ab60-75de70793f66"
        }
      },
      "graphql": {
        "enabled": true,
        "operation": "Query",
        "type": {
          "singular": "SpUuidParam",
          "plural": "SpUuidParams"
        }
      },
      "rest": {
        "enabled": true
      },
      "permissions": [
        {
          "role": "anonymous",
          "actions": [ "*" ]
        },
        {
          "role": "authenticated",
          "actions": [ "*" ]
        }
      ]
    }
```

And stored proc in tsql
```tsql
CREATE PROCEDURE [dbo].[sp_uuid_param] @param1 uniqueidentifier AS
      SELECT @param1 AS [ReturnPayload]
```

DAB was lacking the ability to handle the stored procedure having an
input parameter with value type `uniqueidentifier`. When a default value
was defined in the config, that value would fail conversion to the UUID
type during GraphQL schema creation.

The call stack flows through:

```csharp
                    if (entity.Source.Parameters is not null && entity.Source.Parameters.TryGetValue(param, out object? value))
                    {
                        Tuple<string, IValueNode> defaultGraphQLValue = ConvertValueToGraphQLType(value.ToString()!, parameterDefinition: spdef.Parameters[param]);
                        defaultValueNode = defaultGraphQLValue.Item2;
                    }
```

where `ConvertValueToGraphQLType(...)` would attempt to convert the
config defined value to a GraphQL type based on the type inferred from
the parameter's SystemType (System.Guid). The parameter object has the
following properties when the parameter is passed to that function:

- SystemType -> `System.Guid`
- DbType -> `Guid`
- ConfigDefaultValue -> `f58b7b58-62c9-4b97-ab60-75de70793f66`
- HasConfigDefault -> `true`

`ConvertValueToGraphQLType(...)` did not have the conversion needed to
create a UUID type.

## What is this change?

- In the function called to process config defined default values for
stored procedure parameters, `ConvertValueToGraphQLType(...)` add the
conversion:

```csharp
UUID_TYPE => new(UUID_TYPE, new UuidType().ParseValue(Guid.Parse(defaultValueFromConfig))),
```

- Moved `ConvertValueToGraphQLType()` from `GraphQLUtils.cs` to
`GraphQLStoredProcedureBuilder.cs` to align with usage and purpose of
function. Reduces size of MEGA UTIL class. Also adds comment for
`GraphQLUtils.BuiltInTypes` hashset entry 'ID' to inform that it enables
CosmosDB functionality as only cosmos tests failed when that entry was
commented out.
- Reorganizes the ConvertValueToGraphQLType() order of types in switch
statements to align with GraphQLUtils.BuiltInTypes hashset. That way it
is easier to notice discrepancies in the two lists.

## How was this tested?

- [x] Integration Tests

## Sample Request(s)

- Use the db schema and entity config provided above. Startup will
succeed without conversion errors when attempting to create the GraphQL
schema.

---------

Co-authored-by: Aniruddh Munde <anmunde@microsoft.com>
seantleonard added a commit that referenced this pull request Mar 4, 2024
…ig.json` (#2075)

Full Title: Resolve `uniqueidentifier` database type from
`dab-config.json` defined stored proc parameter default values. (#2042)

## Why make this change?

- Closes #2027. DAB would not start up correctly when runtime config
took the following form:
```json
  "entities": {
    "SpUuidParam": {
      "source": {
        "object": "sp_uuid_param",
        "type": "stored-procedure",
        "parameters": {
          "param1": "f58b7b58-62c9-4b97-ab60-75de70793f66"
        }
      },
      "graphql": {
        "enabled": true,
        "operation": "Query",
        "type": {
          "singular": "SpUuidParam",
          "plural": "SpUuidParams"
        }
      },
      "rest": {
        "enabled": true
      },
      "permissions": [
        {
          "role": "anonymous",
          "actions": [ "*" ]
        },
        {
          "role": "authenticated",
          "actions": [ "*" ]
        }
      ]
    }
```

And stored proc in tsql
```tsql
CREATE PROCEDURE [dbo].[sp_uuid_param] @param1 uniqueidentifier AS
      SELECT @param1 AS [ReturnPayload]
```

DAB was lacking the ability to handle the stored procedure having an
input parameter with value type `uniqueidentifier`. When a default value
was defined in the config, that value would fail conversion to the UUID
type during GraphQL schema creation.

The call stack flows through:

```csharp
                    if (entity.Source.Parameters is not null && entity.Source.Parameters.TryGetValue(param, out object? value))
                    {
                        Tuple<string, IValueNode> defaultGraphQLValue = ConvertValueToGraphQLType(value.ToString()!, parameterDefinition: spdef.Parameters[param]);
                        defaultValueNode = defaultGraphQLValue.Item2;
                    }
```

where `ConvertValueToGraphQLType(...)` would attempt to convert the
config defined value to a GraphQL type based on the type inferred from
the parameter's SystemType (System.Guid). The parameter object has the
following properties when the parameter is passed to that function:

- SystemType -> `System.Guid`
- DbType -> `Guid`
- ConfigDefaultValue -> `f58b7b58-62c9-4b97-ab60-75de70793f66`
- HasConfigDefault -> `true`

`ConvertValueToGraphQLType(...)` did not have the conversion needed to
create a UUID type.

## What is this change?

- In the function called to process config defined default values for
stored procedure parameters, `ConvertValueToGraphQLType(...)` add the
conversion:

```csharp
UUID_TYPE => new(UUID_TYPE, new UuidType().ParseValue(Guid.Parse(defaultValueFromConfig))),
```

- Moved `ConvertValueToGraphQLType()` from `GraphQLUtils.cs` to
`GraphQLStoredProcedureBuilder.cs` to align with usage and purpose of
function. Reduces size of MEGA UTIL class. Also adds comment for
`GraphQLUtils.BuiltInTypes` hashset entry 'ID' to inform that it enables
CosmosDB functionality as only cosmos tests failed when that entry was
commented out.
- Reorganizes the ConvertValueToGraphQLType() order of types in switch
statements to align with GraphQLUtils.BuiltInTypes hashset. That way it
is easier to notice discrepancies in the two lists.

## How was this tested?

- [x] Integration Tests

## Sample Request(s)

- Use the db schema and entity config provided above. Startup will
succeed without conversion errors when attempting to create the GraphQL
schema.

Co-authored-by: Aniruddh Munde <anmunde@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Parse sp parameter default value from runtime config when target type is UUID.
4 participants