Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ActiveRecord ignore nullable types with null value (on update) #485

Closed
sf-spb opened this issue Feb 17, 2021 · 8 comments
Closed

ActiveRecord ignore nullable types with null value (on update) #485

sf-spb opened this issue Feb 17, 2021 · 8 comments
Assignees
Labels
accepted Issue has been accepted and inserted in a future milestone bug

Comments

@sf-spb
Copy link

sf-spb commented Feb 17, 2021

Solution (MVCFramework.Serializer.JsonDataObjects.pas, method TMVCJsonDataObjectsSerializer.JsonDataValueToAttribute).

`jdtObject:

begin
    if (AValue.TypeInfo = System.TypeInfo(TValue)) then
      AValue := TValue.FromVariant(AJsonObject[AName].O['value'].VariantValue)
    else
    begin
      // dt: if a key is null, jsondataobjects assign it the type jdtObject
      if AJsonObject[AName].ObjectValue <> nil then
      begin
        [ ... skip ... ]
      end
      else begin
        // Nullable types
        case AValue.Kind of
          tkRecord:
            begin
              if AValue.TypeInfo = TypeInfo(NullableString) then
              begin
                AValue := TValue.From<NullableString>(Default(NullableString));
              end
              else if AValue.TypeInfo = TypeInfo(NullableTDate) then
              begin
                AValue := TValue.From<NullableTDate>(Default(NullableTDate));
              end
              else if AValue.TypeInfo = TypeInfo(NullableTDateTime) then
              begin
                AValue := TValue.From<NullableTDateTime>(Default(NullableTDateTime));
              end
              else if AValue.TypeInfo = TypeInfo(NullableTTime) then
              begin
                AValue := TValue.From<NullableTTime>(Default(NullableTTime));
              end
              else if AValue.TypeInfo = TypeInfo(NullableCurrency) then
              begin
                AValue := TValue.From<NullableCurrency>(Default(NullableCurrency));
              end
              else if AValue.TypeInfo = TypeInfo(NullableBoolean) then
              begin
                AValue := TValue.From<NullableBoolean>(Default(NullableBoolean));
              end
              else if AValue.TypeInfo = TypeInfo(NullableSingle) then
              begin
                AValue := TValue.From<NullableSingle>(Default(NullableSingle));
              end
              else if AValue.TypeInfo = TypeInfo(NullableDouble) then
              begin
                AValue := TValue.From<NullableDouble>(Default(NullableDouble));
              end
              else if AValue.TypeInfo = TypeInfo(NullableExtended) then
              begin
                AValue := TValue.From<NullableExtended>(Default(NullableExtended));
              end
              else if AValue.TypeInfo = TypeInfo(NullableInt16) then
              begin
                AValue := TValue.From<NullableInt16>(Default(NullableInt16));
              end
              else if AValue.TypeInfo = TypeInfo(NullableUInt16) then
              begin
                AValue := TValue.From<NullableUInt16>(Default(NullableUInt16));
              end
              else if AValue.TypeInfo = TypeInfo(NullableInt32) then
              begin
                AValue := TValue.From<NullableInt32>(Default(NullableInt32));
              end
              else if AValue.TypeInfo = TypeInfo(NullableUInt32) then
              begin
                AValue := TValue.From<NullableUInt32>(Default(NullableUInt32));
              end
              else if AValue.TypeInfo = TypeInfo(NullableInt64) then
              begin
                AValue := TValue.From<NullableInt64>(Default(NullableInt64));
              end
              else if AValue.TypeInfo = TypeInfo(NullableUInt64) then
              begin
                AValue := TValue.From<NullableUInt64>(Default(NullableUInt64));
              end
            end;
        end;

`

@danieleteti
Copy link
Owner

Can you give a reproducible test case?

@danieleteti danieleteti self-assigned this Aug 10, 2021
@danieleteti danieleteti added the cannot-reproduce The issue is not reproducible label Aug 10, 2021
@danieleteti danieleteti added this to the 3.2.2-nitrogen milestone Aug 10, 2021
@danieleteti
Copy link
Owner

Cannot reproduce. However, we created a specific test for this issue, just in case (procedure TTestActiveRecordBase.Test_ISSUE485 in ActiveRecordTestsU);

@azapater
Copy link
Contributor

azapater commented Jun 24, 2022

I was about to open an issue about this topic but I found this one, so I'm re-opening it.

Tested in latest release 3.2.1 as well as current main branch.
Database: Firebird (Although I'm sure it will happen with any db)

Bug:

Given an entity with a field of type "Nullable" (I've tried with NullableInt32 and NullableString), if a PUT request is done with a JSON body, the value in the entity is kept as it was in the database, ignoring the new null value.

I'm using the entity mapping it straight away to ActiveRecordMappingRegistry.

Example:

entity customer
id: integer
name: string
address: NullableString
countryId: NullableInt32

Initial value

 {
 	id: 1,
 	name: 'Peter Parker',
 	address: 'Baker St 1'	
 	countryId: 720
 }

now I try to update address and countryId to null values through a PUT request.

{
	id: 1,
	name: 'Peter Parker',
	address: null,
	countryId: null
}

In this case, the values are kept as the previous ones and the new null values are ignored.

I've spent a few hours trying to track down the problem myself, but I'm not 100% confident about where the problem is, although I agree with @sf-spb because I ended up in the same method:

File:
MVCFramework.Serializer.JsonDataObjects

Method:
procedure TMVCJsonDataObjectsSerializer.JsonDataValueToAttribute

Line:

// dt: if a key is null, jsondataobjects assign it the type jdtObject
if AJsonObject[AName].ObjectValue <> nil then

When the serializer is parsing the JsonObject, the AName variable has the right value of 'countryId' or 'address', but the objectValue always returns "nil", which makes avoiding the if clause all together.

If this info is not enough to replicate, I can try to provide a project with a practical example of the bug.

@danieleteti
Copy link
Owner

Please, provide a small and self contained example. So that we can put it in the test cases too.

@danieleteti danieleteti reopened this Jun 24, 2022
@azapater
Copy link
Contributor

Demo project:

activerecord_restful_crud_null_bug.zip

I've tweaked a bit the sample activerecord_restful_crud included in the library to adapt it to this scenario.

To reproduce the bug these steps need to be followed:

1º Open the project and connect to your database of choice. I've used the firebird activerecord.fdb bd provided with the samples in the folder "samples\data". Verify in the FDConnectionConfigU.pas unit that the connection params are correct.

2º Compile and execute the project.

3º Using postman or rest debugger send the GET request:

localhost:8080/api/entities/customers/5960

It should return:

{
    "data": {
        "id": 5960,
        "description": "GAS Srl",
        "city": "New York",
        "code": "00039",
        "note": "GAS",
        "rating": 2
    }
}

4º Send a PUT request to the same endpoint with the JSON body:

{
    "id": 5960,
    "description": null,
    "city": null,
    "code": null,
    "note": null,
    "rating": null
}

5º Send a GET request again and you'll see that none of the fields have been updated.

I added as well the entity object to the log in the event AfterUpdate, which you'll see hasn't updated any of the values.

You can see in the EntitiesU.pas unit that all the fields but the ID are defined as NullableString or NullableInt16. The fields in the table CUSTOMERS in the database aren't marked as NOT NULL apart from the ID, so this scenario should have worked.

I hope this helps to reproduce the problem.

@danieleteti
Copy link
Owner

Please, let me know if this commit fixes your problem

@danieleteti danieleteti added bug accepted Issue has been accepted and inserted in a future milestone and removed cannot-reproduce The issue is not reproducible labels Aug 2, 2022
@danieleteti
Copy link
Owner

Fixed, all tests pass

@azapater
Copy link
Contributor

azapater commented Aug 4, 2022

Tested this fix and it works as expected

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted Issue has been accepted and inserted in a future milestone bug
Projects
None yet
Development

No branches or pull requests

3 participants