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

Bugfix for deserializing nullable enum properties. #670

Merged
merged 3 commits into from
May 11, 2023

Conversation

adambarath
Copy link
Contributor

Scenario of:


    enum SomethingEnum { A, B, C }

    class TestOfNullableEnums
    {
        public SomethingEnum? TestEnumNullable { get; set; } // Nullable<SomethingEnum>
        public SomethingEnum TestEnum { get; set; }

        public int Id { get; set; } // placeholder
    }

The table definition would be INTEGER for TestEnumNullable property and TEXT for TestEnum property. And table API data could not be deseralized on client side and stored in SQLite of:

 ..[ ... {  "testEnumNullable": null, "testEnum": "B", ...  } .. ].. 

@adrianhall adrianhall self-requested a review May 9, 2023 15:07
@adrianhall
Copy link
Member

Love it! A quick code review suggests it is solid code.

Can you please add a test for this. Best way is probably to add something into the Integration tests for KitchenSink.

@adrianhall adrianhall added Client blocked Issues that are blocked for some reason. labels May 9, 2023
@adambarath
Copy link
Contributor Author

Tried my best.

After adding nullable enums to models and removing my fix some test were break. It was great to see that my fix works pretty weel on unit test side as well. :)

@adrianhall
Copy link
Member

Thanks - I'll take a look at the tests on Friday and see what we can do.

Copy link
Member

@adrianhall adrianhall left a comment

Choose a reason for hiding this comment

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

Made one change - swapping Type instead of Type?, since the #nullable context is disabled in StdLibExtensions.cs.

@adrianhall adrianhall merged commit f95e68a into Azure:main May 11, 2023
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked Issues that are blocked for some reason. Client
Projects
None yet
2 participants