[Bug]: UPSERT doesn't work if there is a calculated column - MsSql

[yorek]

What happened?

If there is a calculated column in a table, for example:

create table [dbo].[todos]
(
  [id] [uniqueidentifier] not null,
  [title] [nvarchar](1000) not null,
  [completed] [bit] not null,
  [custom] [nvarchar](max) null,	
  [owner_id] [varchar](128) not null,
  [created_on] datetime2(7) not null,
) 
go
alter table dbo.todos
add created_on_date as cast(created_on as date)
go

then when doing an UPSERT the operation will fail with the following error:

{
  "error": {
    "code": "DatabaseOperationFailed",
    "message": "The column \u0022created_on_date\u0022 cannot be modified because it is either a computed column or is the result of a UNION operator.",
    "status": 500
  }
}

the generated SQL, in fact, should not do any operation on the calculated colum, but instead it tries to update/insert it:

UPDATE [dbo].[todos] WITH(UPDLOCK) SET [dbo].[todos].[title] = @param2, [dbo].[todos].[completed] = @param3, [dbo].[todos].[owner_id] = @param4, [dbo].[todos].[custom] = @param5, [dbo].[todos].[created_on] = @param6, [dbo].[todos].[position] = @param7, [dbo].[todos].[created_on_date] = @param8 OUTPUT Inserted.[id] AS [tid], Inserted.[title] AS [title], Inserted.[completed] AS [isDone], Inserted.[custom] AS [custom], Inserted.[owner_id] AS [visibility], Inserted.[created_on] AS [created_on], Inserted.[position] AS [position], Inserted.[created_on_date] AS [created_on_date] WHERE [dbo].[todos].[id] = @param1 AND ([owner_id] = @param0) 

IF @@ROWCOUNT = 0 BEGIN; 
INSERT INTO [dbo].[todos] ([id], [title], [completed], [owner_id]) OUTPUT Inserted.[id] AS [tid], Inserted.[title] AS [title], Inserted.[completed] AS [isDone], Inserted.[custom] AS [custom], Inserted.[owner_id] AS [visibility], Inserted.[created_on] AS [created_on], Inserted.[position] AS [position], Inserted.[created_on_date] AS [created_on_date] VALUES (@param1, @param2, @param3, @param4)END

Version

0.6.13

What database are you using?

Azure SQL

What hosting model are you using?

Local (including CLI)

Which API approach are you accessing DAB through?

REST

Relevant log output

info: Microsoft.AspNetCore.Hosting.Diagnostics[1]
      Request starting HTTP/1.1 PUT https://localhost:5001/api/todo/tid/3a67cc45-1234-4e66-9fa2-549c37d56eab application/json 80
info: Microsoft.AspNetCore.Routing.EndpointMiddleware[0]
      Executing endpoint 'Azure.DataApiBuilder.Service.Controllers.RestController.Upsert (Azure.DataApiBuilder.Service)'
info: Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker[3]
      Route matched with {action = "Upsert", controller = "Rest"}. Executing controller action with signature System.Threading.Tasks.Task`1[Microsoft.AspNetCore.Mvc.IActionResult] Upsert(System.String) on controller Azure.DataApiBuilder.Service.Controllers.RestController (Azure.DataApiBuilder.Service).
dbug: Azure.DataApiBuilder.Service.AuthenticationHelpers.ClientRoleHeaderAuthenticationMiddleware[0]
      d40453b8-c3e5-4c30-ac2f-1c101588330e: Request authentication state: Anonymous.
dbug: Azure.DataApiBuilder.Service.AuthenticationHelpers.ClientRoleHeaderAuthenticationMiddleware[0]
      d40453b8-c3e5-4c30-ac2f-1c101588330e: The request will be executed in the context of Anonymous role
dbug: Azure.DataApiBuilder.Service.Resolvers.IQueryExecutor[0]
      d40453b8-c3e5-4c30-ac2f-1c101588330e: Executing query:
SET TRANSACTION ISOLATION LEVEL SERIALIZABLE;BEGIN TRANSACTION; UPDATE [dbo].[todos] WITH(UPDLOCK) SET [dbo].[todos].[title] = @param2, [dbo].[todos].[completed] = @param3, [dbo].[todos].[owner_id] = @param4, [dbo].[todos].[custom] = @param5, [dbo].[todos].[created_on] = @param6, [dbo].[todos].[position] = @param7, [dbo].[todos].[created_on_date] = @param8 OUTPUT Inserted.[id] AS [tid], Inserted.[title] AS [title], Inserted.[completed] AS [isDone], Inserted.[custom] AS [custom], Inserted.[owner_id] AS [visibility], Inserted.[created_on] AS [created_on], Inserted.[position] AS [position], Inserted.[created_on_date] AS [created_on_date] WHERE [dbo].[todos].[id] = @param1 AND ([owner_id] = @param0) IF @@ROWCOUNT = 0 BEGIN; INSERT INTO [dbo].[todos] ([id], [title], [completed], [owner_id]) OUTPUT Inserted.[id] AS [tid], Inserted.[title] AS [title], Inserted.[completed] AS [isDone], Inserted.[custom] AS [custom], Inserted.[owner_id] AS [visibility], Inserted.[created_on] AS [created_on], Inserted.[position] AS [position], Inserted.[created_on_date] AS [created_on_date] VALUES (@param1, @param2, @param3, @param4) END; COMMIT TRANSACTION
fail: Azure.DataApiBuilder.Service.Resolvers.IQueryExecutor[0]
      d40453b8-c3e5-4c30-ac2f-1c101588330e: The column "created_on_date" cannot be modified because it is either a computed column or is the result of a UNION operator.
fail: Azure.DataApiBuilder.Service.Resolvers.IQueryExecutor[0]
      d40453b8-c3e5-4c30-ac2f-1c101588330e:    at Microsoft.Data.SqlClient.SqlCommand.<>c.<ExecuteDbDataReaderAsync>b__208_0(Task`1 result)        
         at System.Threading.Tasks.ContinuationResultTaskFromResultTask`2.InnerInvoke()
         at System.Threading.ExecutionContext.RunInternal(ExecutionContext executionContext, ContextCallback callback, Object state)
      --- End of stack trace from previous location ---
         at System.Threading.Tasks.Task.ExecuteWithThreadLocal(Task& currentTaskSlot, Thread threadPoolThread)
      --- End of stack trace from previous location ---
         at Azure.DataApiBuilder.Service.Resolvers.QueryExecutor`1.ExecuteQueryAgainstDbAsync[TResult](TConnection conn, String sqltext, IDictionary`2 parameters, Func`3 dataReaderHandler, HttpContext httpContext, List`1 args)
fail: Azure.DataApiBuilder.Service.Controllers.RestController[0]
      d40453b8-c3e5-4c30-ac2f-1c101588330e: The column "created_on_date" cannot be modified because it is either a computed column or is the result of a UNION operator.
fail: Azure.DataApiBuilder.Service.Controllers.RestController[0]
      d40453b8-c3e5-4c30-ac2f-1c101588330e:    at Azure.DataApiBuilder.Service.Resolvers.QueryExecutor`1.ExecuteQueryAgainstDbAsync[TResult](TConnection conn, String sqltext, IDictionary`2 parameters, Func`3 dataReaderHandler, HttpContext httpContext, List`1 args)
         at Azure.DataApiBuilder.Service.Resolvers.QueryExecutor`1.<>c__DisplayClass19_0`1.<<ExecuteQueryAsync>b__0>d.MoveNext()
      --- End of stack trace from previous location ---
         at Polly.Retry.AsyncRetryEngine.ImplementationAsync[TResult](Func`3 action, Context context, CancellationToken cancellationToken, ExceptionPredicates shouldRetryExceptionPredicates, ResultPredicates`1 shouldRetryResultPredicates, Func`5 onRetryAsync, Int32 permittedRetryCount, IEnumerable`1 sleepDurationsEnumerable, Func`4 sleepDurationProvider, Boolean continueOnCapturedContext)
         at Polly.AsyncPolicy.ExecuteAsync[TResult](Func`3 action, Context context, CancellationToken cancellationToken, Boolean continueOnCapturedContext)
         at Azure.DataApiBuilder.Service.Resolvers.QueryExecutor`1.ExecuteQueryAsync[TResult](String sqltext, IDictionary`2 parameters, Func`3 dataReaderHandler, HttpContext httpContext, List`1 args)
         at Azure.DataApiBuilder.Service.Resolvers.SqlMutationEngine.PerformUpsertOperation(IDictionary`2 parameters, RestRequestContext context)  
         at Azure.DataApiBuilder.Service.Resolvers.SqlMutationEngine.ExecuteAsync(RestRequestContext context)
         at Azure.DataApiBuilder.Service.Services.RestService.ExecuteAsync(String entityName, Operation operationType, String primaryKeyRoute)     
         at Azure.DataApiBuilder.Service.Controllers.RestController.HandleOperation(String route, Operation operationType)
info: Microsoft.AspNetCore.Mvc.Infrastructure.SystemTextJsonResultExecutor[1]
      Executing JsonResult, writing value of type '<>f__AnonymousType0`1[[<>f__AnonymousType1`3[[System.String, System.Private.CoreLib, Version=6.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e],[System.String, System.Private.CoreLib, Version=6.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e],[System.Int32, System.Private.CoreLib, Version=6.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]], Azure.DataApiBuilder.Service, Version=0.6.13.0, Culture=neutral, PublicKeyToken=null]]'.
info: Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker[2]
      Executed action Azure.DataApiBuilder.Service.Controllers.RestController.Upsert (Azure.DataApiBuilder.Service) in 22.5888ms
info: Microsoft.AspNetCore.Routing.EndpointMiddleware[1]
      Executed endpoint 'Azure.DataApiBuilder.Service.Controllers.RestController.Upsert (Azure.DataApiBuilder.Service)'
info: Microsoft.AspNetCore.Hosting.Diagnostics[2]
      Request finished HTTP/1.1 PUT https://localhost:5001/api/todo/tid/3a67cc45-1234-4e66-9fa2-549c37d56eab application/json 80 - 500 - application/json;+charset=utf-8 23.3078ms

Code of Conduct

• I agree to follow this project's Code of Conduct

[ayush3797]

@yorek, I agree, we should fix this behavior - not try to update a calculated column. The workaround for the time being would be to use PATCH operation instead of PUT.
PUT (upsert) operation assigns NULL value to all the columns missing in the request body. This is the expected behavior and follows PUT semantics. However if you would have tried PATCH (upsert incremental) operation, the request would have succeeded, as PATCH request would not NULL out the missing column created_on_date. I hope this makes sense. I tried out executing the below request and it ran fine, successfully inserting a record in the todos table.

drop table if exists todos;
create table [dbo].[todos]
(
  [id] [uniqueidentifier] PRIMARY KEY not null,
  [title] [nvarchar](1000) not null,
  [completed] [bit] not null,
  [custom] [nvarchar](max) null,	
  [owner_id] [varchar](128) not null,  
  [created_on] datetime2(7) not null,
) 
alter table dbo.todos
add created_on_date as cast(created_on as date)

Request type: PATCH
API type: REST
URL: https://localhost:5001/api/todo/id/F86D815C-5016-4929-BD78-2FE96A0132FD
Request body:

{
    "title": "def",
    "completed": true,
    "custom": "xyz",
    "owner_id": "abc",
    "created_on": "1999-01-07"
}

Response:

{
    "value": [
        {
            "id": "f86d815c-5016-4929-bd78-2fe96a0132fd",
            "title": "def",
            "completed": true,
            "custom": "xyz",
            "owner_id": "abc",
            "created_on": "1999-01-07T00:00:00",
            "created_on_date": "1999-01-07T00:00:00"
        }
    ]
}

[ayush3797]

Talking about the options suggested by @JerryNixon:

1. Option 1: This seems to be very straightforward solution and will need the developer inform DAB about what all columns are to be excluded from the update/insert. The use case pointed for something like rowversion also makes sense to me. Since the developer should be well aware of the underlying db configuration, it makes sense for them to provide the read-only columns.
2. Option 2: Fetching the metadata for the database object is definitely another way to go about this. For instance, for MsSql we can use a query like this to fetch the metadata for computed columns:

We would have to look for something similar for MySql, PgSql as well. We'll need to see what all different types of columns should be categorized as read-only.

[JerryNixon]

See #1446

[Aniruddh25]

For Option 1: The include, exclude properties are to be considered from the eyes of whether the role has permissions for an action on them or not. Adding a read-only property alongside, doesn't align with permissions - nor does it provide any meaning when action is update and we provide read-only list of fields.

For e.g. what does it mean for specifying fields in the read-only property below when provided under the action update and how is it different than if the same list of fields were to be provided for the action read? There is potential for redundant information.

            {
              "action": "update",
              "fields": {
                "include": [
                  "*"
                ],
                "exclude": [],
                "read-only": ["rowversion", "calculatedcolumn"]
              }
            }

I prefer fetching the metadata about the column is computed or not from the database, and based on that ignore/mandate the field in the request. We don't need to add additional config ensureMetadata - we should do this by default.

Also, for any extraneous fields in a REST request we have today been erroring out. When a computed column is provided in the request, once we detect it, we can continue to error out by default. But allow the capability for DAB to ignore such extraneous fields when the request provides a header x-ms-match-mode value to ignore. See #158

[ayush3797]

I misinterpreted Jerry's comment a bit earlier. If we decide to add the read-only-fields section to the config, then rather than adding it to every role's permission adjacent to include/exclude fields, it makes sense to add the read-only-fields section to the table/view source properties since fields being read-only is a behavior that comes in because of the field's definition in the table/view.

Eg.

"Book": {
      "source": {
        "type": "table",
        "object": "books",
        "key-fields": [
          "id"
        ],
        "read-only-fields": ["abc", "xyz"]
      },
...

[ayush3797]

I had a more thorough look into this issue. So, the problem here in front of us is that when we execute an update (a simple update or an update via PUT/PATCH), we are also trying to update a field which is not updateable (say a rowversion field, or a calculated field). Now the solutions are:

1. Option A: Determine metadata about whether the fields should be included in the update clause or not. If a field is not updateable (lets say it is a rowversion field), we don't include it in the update clause. This doesn't appear to be full-proof.
2. Option B: Have a read-only-fields section in the table/view's source properties in the config.

For option A, I delved deeper to investigate if this is a full-proof solution and we are not taking away any of the abilities from the user. And by the look of it, it does appear to take away capabilities from the user. Let's see how..

Suppose I have a table with the following definition:

create table [dbo].[testTable]
(
  [id] int not null,
  RV rowversion,
  [created_on] datetime2(7) not null
)

insert into testTable(id,created_on) values(1,GETDATE())

Obviously the column RV is not updateable directly because it is a timestamp column. It maybe the case that there is an INSTEAD OF trigger configured on the table which looks something like:

create trigger triggerOnTestTable on dbo.testTable
instead of update
as
update dbo.testTable set created_on = GETDATE()-3
from INSERTED INNER JOIN testTable on INSERTED.id = testTable.id and INSERTED.RV = CONVERT(binary,'0x00000000000399E1');

So the above trigger is executed whenever an update is executed on the testTable - and the action of the actual update query is totally overridden. We have access to two tables - INSERTED and DELETED inside triggers (see here) where the INSERTED table holds the new values (updated values) and the DELETED table holds the old values (deleted values).
If we carefully observe the trigger definition, we'll see the condition:

and INSERTED.RV = CONVERT(binary,'0x00000000000399E1')

so the INSERTED.RV holds the new value which will be given to this column in the actual update statement executed. Now you would argue that the update statement cannot assign a value to the RV column right? But that's true when the update statement is executed. In our case, the INSTEAD OF trigger will be executed instead of the update statement.

Suppose if we execute an update on this table like:

update testTable set RV = CONVERT(binary,'0x00000000000399E1') where id=1;

Instead of this update statement, the INSTEAD OF trigger will get triggered which will use the new value that is supposed to be allotted to the RV column i.e. the value CONVERT(binary,'0x00000000000399E1') will be allotted to INSERTED.RV and the above mentioned condition of INSERTED.RV = CONVERT(binary,'0x00000000000399E1') will be satisfied.

Conclusion: Even though RV was a non-updateable column by definition, the table can have an INSTEAD OF trigger defined. The trigger might use the new value being allotted to the RV column (by the actual update statement). We need to pass on this value to the database via the query that DAB generates. We cannot just skip the column in the query at DAB level. So, Option A is not full-proof.

[ayush3797]

As discussed in the planning for 0.9, I will be going ahead with option A. I did explain a case where option A would fail in the previous comment but in that case, I was assigning a value to a timestamp column RV in the update statement. Although I did not get any exceptions while doing so, the specs here: Use the inserted and deleted tables in INSTEAD OF triggers mention that Values cannot be specified for computed columns or columns with a timestamp data type. So the above case gets out of the picture.